Add infrastructure for too many Includes warning. #20140

ajcvickers · 2020-03-03T17:01:52Z

Part of #19933

ajcvickers · 2020-03-03T17:02:28Z

@smitpatel My part of #19933. Warning is not wired up yet--need you for that.

smitpatel · 2020-03-03T17:09:08Z

What is our cut-off to generate warning?

ajcvickers · 2020-03-03T17:09:18Z

@smitpatel I'll just let you take over this PR in whatever way you want, since I can't get approval to merge until the complete change is done anyway.

AndriySvyryd · 2020-03-03T18:09:44Z

@smitpatel If the query has two or more collection includes or collection nav expansions then we should generate the warning

smitpatel · 2020-03-06T06:11:10Z

src/EFCore.Relational/Diagnostics/RelationalLoggerExtensions.cs

+        /// <param name="diagnostics"> The diagnostics logger to use. </param>
+        /// <param name="expressionPrinter"> Used to create a human-readable representation of the expression tree. </param>
+        /// <param name="queryExpression"> The query expression tree. </param>
+        public static void TooManyIncludesWarning(


The place we know that there too many includes in relational, we don't have actual expression tree.

@smitpatel I can update it to take whatever you have available.

Let's discuss in triage today.

smitpatel · 2020-03-06T23:26:50Z

I looked at this further, the place where we accurately know that there is multiple collection projection, we only have SelectExpression. Further, in order to print SelectExpression in most readable format, we need sql generator which we don't have. Any other place to identify this requires a public surface on places where we don't want to add public surface. (even if we add in patch, it would be throw away work).

At this point, warning seems pretty low value as if query (with or without multiple collection include), either it works just fine or perf is pretty bad and customers would know.

ajcvickers · 2020-03-06T23:29:11Z

@smitpatel Can we get any type of string from the SelectExpression? If so, can you post examples of what it looks like?

smitpatel · 2020-03-06T23:53:52Z

Can strip out shaper if needed

(ShapedQueryExpression: 
    QueryExpression: 
        (Projection Mapping:
        SELECT ((c.CustomerID)), ((c.Address)), ((c.City)), ((c.CompanyName)), ((c.ContactName)), ((c.ContactTitle)), ((c.Country)), ((c.Fax)), ((c.Phone)), ((c.PostalCode)), ((c.Region)), ((t.OrderID)), ((t.CustomerID)), ((t.EmployeeID)), ((t.OrderDate)), ((t.OrderID0)), ((t.ProductID)), ((t.Discount)), ((t.Quantity)), ((t.UnitPrice))
        FROM (Customers AS c)
        (LEFT JOIN (Projection Mapping:
        (
            SELECT ((o.OrderID)), ((o.CustomerID)), ((o.EmployeeID)), ((o.OrderDate)), ((o0.OrderID) AS OrderID0), ((o0.ProductID)), ((o0.Discount)), ((o0.Quantity)), ((o0.UnitPrice))
            FROM (Orders AS o)
            (LEFT JOIN (Order Details AS o0) ON ((o.OrderID) == (o0.OrderID)))
        ) AS t) ON ((c.CustomerID) == (t.CustomerID)))
        ORDER BY ((c.CustomerID) ASC), ((t.OrderID) ASC), ((t.OrderID0) ASC), ((t.ProductID) ASC))
    ShaperExpression: (IncludeExpression(
            (EntityShaperExpression: 
                EntityType: Customer
                ValueBufferExpression: 
                    (ProjectionBindingExpression: CustomerID:0,
                        Address:1,
                        City:2,
                        CompanyName:3,
                        ContactName:4,
                        ContactTitle:5,
                        Country:6,
                        Fax:7,
                        Phone:8,
                        PostalCode:9,
                        Region:10,
                    )
                IsNullable: False
            ), 
            (RelationalCollectionShaper:
                CollectionId: 0
                ParentIdentifier:new object[]{ (ProjectionBindingExpression: 0) }
                OuterIdentifier:new object[]{ (ProjectionBindingExpression: 0) }
                SelfIdentifier:new object[]{ (object)(ProjectionBindingExpression: 11) }
                InnerShaper:(IncludeExpression(
                    (EntityShaperExpression: 
                        EntityType: Order
                        ValueBufferExpression: 
                            (ProjectionBindingExpression: OrderID:11,
                                CustomerID:12,
                                EmployeeID:13,
                                OrderDate:14,
                            )
                        IsNullable: True
                    ), 
                    (RelationalCollectionShaper:
                        CollectionId: 1
                        ParentIdentifier:new object[]{ (object)(ProjectionBindingExpression: 11) }
                        OuterIdentifier:new object[]{ (object)(ProjectionBindingExpression: 11) }
                        SelfIdentifier:new object[]
                        { 
                            (object)(ProjectionBindingExpression: 15), 
                            (object)(ProjectionBindingExpression: 16) 
                        }
                        InnerShaper:(EntityShaperExpression: 
                            EntityType: OrderDetail
                            ValueBufferExpression: 
                                (ProjectionBindingExpression: OrderID:15,
                                    ProductID:16,
                                    Discount:17,
                                    Quantity:18,
                                    UnitPrice:19,
                                )
                            IsNullable: True
                        )
                        Navigation: OrderDetails
                    ), OrderDetails)
                )
                Navigation: Orders
            ), Orders)
        )
)

ajcvickers · 2020-03-06T23:56:00Z

@smitpatel Thanks. Let's discuss again in triage. :-)

AndriySvyryd · 2020-03-07T01:35:59Z

This still has value. If one needs to identify the query that triggers the warning they can just set it to throw.

smitpatel · 2020-03-07T01:37:26Z

This still has value.

You mean warning or exception message? Because in today's triage while discussing, people said, if we cannot give good indication of which query then it is not useful.

AndriySvyryd · 2020-03-07T01:46:41Z

You mean warning or exception message?

It's one and the same, depends on user configuration.

Because in today's triage while discussing, people said, if we cannot give good indication of which query then it is not useful.

If it throws then it's clear

AndriySvyryd · 2020-03-07T01:51:10Z

src/EFCore.Relational/Properties/RelationalStrings.resx

@@ -250,6 +250,10 @@
    <value>Possible unintended use of a potentially throwing aggregate method (Min, Max, Average) in a subquery. Client evaluation will be used and operator will throw if no data exists. Changing the subquery result type to a nullable type will allow full translation.</value>
    <comment>Warning RelationalEventId.QueryPossibleExceptionWithAggregateOperatorWarning</comment>
  </data>
+  <data name="LogTooManyIncludesWarning" xml:space="preserve">
+    <value>The query '{query}' uses 'Include' for more than one collection navigation property. Including multiple collections can result in slow query performance. See ... for more information.</value>


Don't forget to replace the ...

And add "To identify the query that's triggering this warning call .ConfigureWarnings(w => w.Throw(RelationalEventId.LogTooManyIncludesWarning))"

ajcvickers · 2020-03-07T15:08:32Z

@smitpatel What I intended to say in triage was, "if we cannot give any indication of which query then it is not useful."

ajcvickers · 2020-03-26T15:53:26Z

Blocked for now until we have the new code in place.

ajcvickers · 2020-06-12T23:22:11Z

@smitpatel Is this PR now obsolete?

AndriySvyryd · 2020-06-12T23:23:41Z

How else will the users find out that they need to call the new API?

ajcvickers · 2020-06-12T23:25:23Z

@AndriySvyryd I meant specifically this PR, not the concept of having a warning.

smitpatel · 2020-06-15T18:41:09Z

So should I put this thing to master and generate warning when not split query and more than one collection is being projected/included?

ajcvickers · 2020-06-15T18:45:43Z

@smitpatel Yes. :-D

smitpatel · 2020-06-26T00:10:16Z

Superseded by #21419

Add infrastructure for too many Includes warning.

83704d1

Part of #19933

ajcvickers requested a review from smitpatel March 3, 2020 17:01

smitpatel approved these changes Mar 3, 2020

View reviewed changes

AndriySvyryd assigned smitpatel Mar 6, 2020

smitpatel reviewed Mar 6, 2020

View reviewed changes

smitpatel added the propose-close label Mar 6, 2020

AndriySvyryd reviewed Mar 7, 2020

View reviewed changes

ajcvickers removed the propose-close label Mar 16, 2020

ajcvickers added the blocked label Mar 26, 2020

smitpatel closed this Jun 26, 2020

smitpatel deleted the WarningMyToesByTheRyzen0303 branch June 26, 2020 00:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add infrastructure for too many Includes warning. #20140

Add infrastructure for too many Includes warning. #20140

ajcvickers commented Mar 3, 2020

ajcvickers commented Mar 3, 2020

smitpatel commented Mar 3, 2020

ajcvickers commented Mar 3, 2020

AndriySvyryd commented Mar 3, 2020

smitpatel Mar 6, 2020

ajcvickers Mar 6, 2020

smitpatel Mar 6, 2020

smitpatel commented Mar 6, 2020

ajcvickers commented Mar 6, 2020

smitpatel commented Mar 6, 2020

ajcvickers commented Mar 6, 2020

AndriySvyryd commented Mar 7, 2020

smitpatel commented Mar 7, 2020

AndriySvyryd commented Mar 7, 2020

AndriySvyryd Mar 7, 2020 •

edited

Loading

ajcvickers commented Mar 7, 2020

ajcvickers commented Mar 26, 2020

ajcvickers commented Jun 12, 2020

AndriySvyryd commented Jun 12, 2020

ajcvickers commented Jun 12, 2020

smitpatel commented Jun 15, 2020

ajcvickers commented Jun 15, 2020

smitpatel commented Jun 26, 2020

Add infrastructure for too many Includes warning. #20140

Add infrastructure for too many Includes warning. #20140

Conversation

ajcvickers commented Mar 3, 2020

ajcvickers commented Mar 3, 2020

smitpatel commented Mar 3, 2020

ajcvickers commented Mar 3, 2020

AndriySvyryd commented Mar 3, 2020

smitpatel Mar 6, 2020

Choose a reason for hiding this comment

ajcvickers Mar 6, 2020

Choose a reason for hiding this comment

smitpatel Mar 6, 2020

Choose a reason for hiding this comment

smitpatel commented Mar 6, 2020

ajcvickers commented Mar 6, 2020

smitpatel commented Mar 6, 2020

ajcvickers commented Mar 6, 2020

AndriySvyryd commented Mar 7, 2020

smitpatel commented Mar 7, 2020

AndriySvyryd commented Mar 7, 2020

AndriySvyryd Mar 7, 2020 • edited Loading

Choose a reason for hiding this comment

ajcvickers commented Mar 7, 2020

ajcvickers commented Mar 26, 2020

ajcvickers commented Jun 12, 2020

AndriySvyryd commented Jun 12, 2020

ajcvickers commented Jun 12, 2020

smitpatel commented Jun 15, 2020

ajcvickers commented Jun 15, 2020

smitpatel commented Jun 26, 2020

AndriySvyryd Mar 7, 2020 •

edited

Loading