Efcore: Optimize Find/Single operations with includes

[joakimriedel]

In 5.0 RC1 a warning log message is thrown for .SingleAsync() queries:

warn: Microsoft.EntityFrameworkCore.Query[10102]
      Query uses a row limiting operation (Skip/Take) without OrderBy, which may lead to unpredictable results.

I guess this has something to do with the fact that the actual SQL generated includes an automatic TOP(2) clause to determine if there are more than 1 item matching the query.

In 3.1 there were no warnings logged. I prefer no warnings logged.

[smitpatel]

The warning is logged because doing Single on a query without using ~Where~ and OrderBy, which means that the query is going to get 1 record from server which can change in future iteration of query. This warning existed in 2.x release and was absent in 3.1. We have just reintroduced what was missing. To avoid warning, add ~Where/~OrderBy to the query before calling Single.

[joakimriedel]

This particular query just picks the current user by primary key, hence no Where/OrderBy.

.SingleAsync(u => u.Id == currentUserId)

Meaning I want the single user by Id, but I want exceptions thrown if:

1) the user is missing
or
2) for some reason the primary key is no longer unique and two users exist with same primary key in the database

I don't see why I should add Where/OrderBy in this case?

[smitpatel]

Error on my part that it should not logged with Where. It does get logged in the absence of OrderBy when translating Skip or Take.
SingleAsync does not use Skip/Take pipeline directly, can you share full query?

[joakimriedel]

Sure

// get a hold on the current user with metadata
                _user = await _dbContext.Users
                        .TagWith("CurrentRequest - GetUserAsync")
                        .Include(u => u.Currency)
                        .Include(u => u.Verifications)
                        .Include(u => u.NotificationSettings)
                        .Include(u => u.UserConnections)
                            .ThenInclude(uc => uc.BillingGroup)
                            .ThenInclude(uc => uc.Verifications)
                        .AsSplitQuery()
                        .SingleAsync(u => u.Id == currentUserId);

[smitpatel]

Additional queries for Include has to do Take(1) to only get correlated data for first result they don't have OrderBy so result could be non-deterministic. For Single we may be able to identify that the predicate is on PK so there would be only one match, it is hard to generalize it for any Where/Take combination. Queries can have multiple rows with same PK when doing joins and other complex scenarios. This was probably happening in 2.1 also.

[joakimriedel]

Still trying to wrap my head around this. From a user perspective, something feels wrong since I expect to get the single user with all included navigation properties without warnings or having to add OrderBy.

I also don't really understand why the Include have to do Take(1) ? I thought it would just match by the predicate given, joining any related items.

But technical reasons aside, what is the recommended way to retrieve a single tracked item from the database by primary key having additional navigation properties eagerly loaded?

[smitpatel]

I also don't really understand why the Include have to do Take(1) ? I thought it would just match by the predicate given, joining any related items.

It does predicate match but it needs to figure out which items from parent it needs to match in the predicate joining to Children. If only 1 (or limited number of parents) are selected then we have to restrict the set of parents to same amount in additional queries so that predicate only matches related data for that limited set rather than generating join for everything.
You can suppress the warning or add OrderBy before First.

We will discuss in the meeting "recommended way".

[joakimriedel]

Thanks. I'll try to suppress it for now, don't want any possible overhead of OrderBy since this is on a critical application path. How do I suppress a single warning? I do want warnings for other Single-query abuses, just not this particular one.

[smitpatel]

Actually there will be OrderBy always because of Include so that we get records in a deterministic order to create proper buckets. It can be optimized for Single case but currently that is not happening.

[joakimriedel]

Did a bit of debugging this morning and it adds an extra ORDER BY in the inner clause. But the sql query planner didn't seem to add much overhead from this, perhaps it's smart enough to optimize out the ORDER BY since the WHERE clause is on primary key?

query.OrderBy(u => u.Id).SingleAsync(u => u.Id == currentUserId)

(no warning)

SELECT [t].[Id], [t].[...]
FROM (
    SELECT TOP(2) [a].[Id], [a].[...], [c].[Id] AS [Id0], [c].[...]
    FROM [AspNetUsers] AS [a]
    INNER JOIN [Currencies] AS [c] ON [a].[CurrencyId] = [c].[Id]
    WHERE [a].[Id] = @__currentUserId_0
    ORDER BY [a].[Id]
) AS [t]
ORDER BY [t].[Id], [t].[Id0]

query.SingleAsync(u => u.Id == currentUserId)

(warning)

SELECT [t].[Id], [t].[...]
FROM (
    SELECT TOP(2) [a].[Id], [a].[...], [c].[Id] AS [Id0], [c].[...]
    FROM [AspNetUsers] AS [a]
    INNER JOIN [Currencies] AS [c] ON [a].[CurrencyId] = [c].[Id]
    WHERE [a].[Id] = @__currentUserId_0
) AS [t]
ORDER BY [t].[Id], [t].[Id0]

I'll keep the OrderBy for now until your meeting to see if there's a better way. At least I do not get any runtime warnings now anymore.

[ajcvickers]

Moving to the backlog to attempt to not generate this warning when it is not needed.

[joakimriedel]

For your info - I also did some basic StopWatch-benchmarking of the SingleAsync query with Include compared to FindAsync followed by LoadAsync, but the SingleAsync still performed slightly better (~38 ms compared to ~43 ms) even with the ORDER BY.

An optimized FindAsync with Include would be handy for querying out single items and related data, looking forward to that!