efcore: Significant Query Slowdown When Using Multiple Joins Due To Changes In 3.0
Note:
- Issue #20892 has been created to track actual work on split queries in EF Core 5.0. The new issue is locked so that it can be subscribed to for updates on the implementation without noise.
- This older issue has been re-purposed to be discussion about different approaches, etc. It will not be locked because it is important to us that we don’t shut down channels of communication with the community.
Preview6 introduces AsSplitQuery API to load collections using multiple queries. Details of the API has been posted https://github.com/dotnet/efcore/issues/20892#issuecomment-644456029
A way to rewrite the query to avoid large result set & mimick previous version behavior is posted https://gist.github.com/smitpatel/d4cb3619e5b33e8d9ea24d3f2a88333a
_Note: Per @roji’s request, I am opening this in response to the comments on https://github.com/aspnet/EntityFrameworkCore/issues/17455_
I have a very large query that contains about 57 includes that is being used to copy down a large entity so that it can be modified and cloned.
With EF 2.2.6, this large query ran successfully in about 1-3 seconds (variable). With the changes in 3.0 (all includes create one entire SQL statement with joins), the query takes significantly longer and always times out with the default execution timeout settings.
Steps to reproduce
Use a (IMO nasty) Linq query similar to the one as follows:
dbContext.Projects
.Include(z => z.Financial)
.Include(z => z.ProjectProtocol)
.Include(z => z.ReportCategories)
.Include(z => z.ReportSubCategories)
.Include(x => x.SubProjects).ThenInclude(y => y.Address)
.Include(x => x.SubProjects).ThenInclude(y => y.LegacyAddress)
.Include(x => x.SubProjects).ThenInclude(z => z.BuildingTypes)
.Include(x => x.SubProjects).ThenInclude(z => z.Buildings).ThenInclude(b => b.BuildingType)
.Include(x => x.SubProjects).ThenInclude(z => z.Buildings).ThenInclude(b => b.Site)
.Include(x => x.SubProjects).ThenInclude(z => z.Sites).ThenInclude(s => s.Address)
.Include(x => x.SubProjects).ThenInclude(z => z.Participants).ThenInclude(p => p.Address)
.Include(x => x.SubProjects).ThenInclude(z => z.ExcelFileJson)
.Include(x => x.SubProjects).ThenInclude(z => z.CompanionAddress)
.Include(x => x.SubProjects).ThenInclude(z => z.UtilityTypes)
.Include(x => x.SubProjects).ThenInclude(z => z.InspectedUnits).ThenInclude(i => i.Building)
.Include(x => x.SubProjects).ThenInclude(z => z.InspectedUnits).ThenInclude(i => i.UnitType)
.Include(x => x.SubProjects).ThenInclude(z => z.Utilities).ThenInclude(i => i.UtilityType)
.Include(x => x.SubProjects).ThenInclude(z => z.Units).ThenInclude(i => i.Building)
.Include(x => x.SubProjects).ThenInclude(z => z.UnitTypes)
.Include(x => x.SubProjects).ThenInclude(z => z.CommonAreas).ThenInclude(ca => ca.Building)
.Include(x => x.SubProjects).ThenInclude(z => z.FixtureAreas).ThenInclude(w => w.Fixtures)
.Include(x => x.SubProjects).ThenInclude(y => y.LineItems).ThenInclude(z => z.EnergyOneItems)
.Include(x => x.SubProjects).ThenInclude(y => y.LineItems).ThenInclude(z => z.EnergyTwoItems)
.Include(x => x.SubProjects).ThenInclude(y => y.LineItems)
.ThenInclude(z => z.TraditionalItem).AsNoTracking().FirstOrDefault(x => x.Id == project.Id);
Results:
With EF Core 2.2.6 - I can see in the output via the SQL Server Profiler that EF is breaking up the LINQ statement into smaller queries. The overall process takes about 1-3 seconds.
With EF Core 3.0 - I can see in the output via the SQL Server Profiler that EF is emitting one massive query with lots and lots of joins. The overall process always times out with the default execution timeout setting.
At this point, I am open to the notion that this query needs to be either re-written or the process needs to be changed for handling the cloning. I would still like to hear if there are any workarounds, findings that this is a bug or other suggestions to avoid having to devote a significant amount of time on rewriting.
Edit For now we worked around this by splitting the query up manually using EF Plus’ “Includes Optimized” method and then looping through the change tracker to set all of the entities as untracked so we can then reset their keys so that the graph can be comitted as a clone (this gave me a flashback to my EF 6 days).
Note: The model changed somewhat between the time this issue was first encountered and now due to user requests and other factors. I should also note that the system is now in production and users are pretty happy with the performance.
var clone = dbContext.Projects
.IncludeOptimized(z => z.Financial)
.IncludeOptimized(z => z.ProjectProtocol)
.IncludeOptimized(z => z.ReportCategories)
.IncludeOptimized(z => z.ReportSubCategories)
.IncludeOptimized(z => z.SubProject)
.IncludeOptimized(z => z.SubProject.ValidationFlag)
.IncludeOptimized(z => z.SubProject.ExcelFileJson)
.IncludeOptimized(z => z.SubProject.EnergyAuditUtilities)
.IncludeOptimized(z => z.SubProject.EnergyAuditData)
.IncludeOptimized(z => z.SubProject.EnergyAuditData.EnergyAuditAreas)
.IncludeOptimized(z => z.SubProject.Address)
.IncludeOptimized(z => z.SubProject.Utilities)
.IncludeOptimized(z => z.SubProject.UtilityTypes)
.IncludeOptimized(z => z.SubProject.Units)
.IncludeOptimized(z => z.SubProject.Units.Select(y => y.Building))
.IncludeOptimized(z => z.SubProject.BuildingTypes)
.IncludeOptimized(z => z.SubProject.Buildings)
.IncludeOptimized(z => z.SubProject.Buildings.Select(y => y.BuildingType))
.IncludeOptimized(z => z.SubProject.Buildings.Select(y => y.Site))
.IncludeOptimized(z => z.SubProject.Sites)
.IncludeOptimized(z => z.SubProject.Sites.Select(y => y.Address))
.IncludeOptimized(z => z.SubProject.Participants)
.IncludeOptimized(z => z.SubProject.Participants.Select(y => y.Address))
.IncludeOptimized(z => z.SubProject.InspectedUnits)
.IncludeOptimized(z => z.SubProject.InspectedUnits.Select(y => y.Building))
.IncludeOptimized(z => z.SubProject.InspectedUnits.Select(y => y.UnitType))
.IncludeOptimized(z => z.SubProject.UnitTypes)
.IncludeOptimized(z => z.SubProject.CommonAreas)
.IncludeOptimized(z => z.SubProject.CommonAreas.Select(y => y.Building))
.IncludeOptimized(z => z.SubProject.CommonAreas.Select(y => y.Building).Select(z => z.Units))
.IncludeOptimized(z => z.SubProject.FixtureAreas)
.IncludeOptimized(z => z.SubProject.FixtureAreas.Select(y => y.Fixtures))
.IncludeOptimized(x => x.SubProject.LineItems)
.IncludeOptimized(x => x.SubProject.LineItems.Select(y => y.EnergyOneItem))
.IncludeOptimized(x => x.SubProject.LineItems.Select(y => y.EnergyTwoTwoItem))
.IncludeOptimized(x => x.SubProject.LineItems.Select(y => y.TraditionalItem))
.FirstOrDefault(x => x.Id == project.Id);
if (clone != null)
{
foreach (var entityEntry in dbContext.ChangeTracker.Entries())
{
if (entityEntry.Entity != null)
{
entityEntry.State = EntityState.Detached;
}
}
return clone;
}
My team was struggling with this at first due to the fact that EF was at first wiping out the entities when we detatched them due to an issue on https://github.com/dotnet/efcore/issues/18982. Using the work around that was posted there allowed for things to work. The overall performance is actually better due to the fact there isn’t any client side evaluation. However, I would still prefer if the behavior of Includes Optimized (which is pretty much what EF 2.x did, splitting the query) was something that came out of the box with EF Core. I also do not like it how this entire scenario can no longer be done with a no tracking query (or so it seems), as it was possible to do it before with EF Core 2.x.
Further technical details
EF Core version: 3.0.0 Database provider: Microsoft.EntityFramework.SqlServer Target framework: .Net Core 3.0 Operating system: Windows 10 x64 / Windows Server 2016 IDE: Visual Studio 2019 Pro (16.3)
About this issue
- Original URL
- State: closed
- Created 5 years ago
- Reactions: 84
- Comments: 242 (69 by maintainers)
Triage decision:
We (the team) have been discussing this issue a lot recently. We have a tentative plan for both 5.0 and 3.1.x.
In 5.0 we plan to:
For 3.1.x we plan to:
@roji ,
I’ve been monitoring this situation closely since 3.0 came out and our application’s performance fell apart after upgrading.
I want to point out that, at the time of my comment, this is the 5th most commented on open Issue in the EF Core repository. Positions 1-4 are occupied by Issues which were opened in 2014 and 2015. This one was only opened in late 2019. In just a few months this Issue has managed to garner more attention than many complex Issues as old as EF Core itself. Most Commented Open Issues The reason I want to bring this up is that I want to establish that, for many users, this may well be the most pressing issue EF Core faces today.
I do acknowledge that you likely wouldn’t have backlash if EF Core was like this from the start. However, I am concerned that you may have used that axiom as a pretext for devaluing the feedback you’re receiving.
Seriously, this is fantastic news. And I do think this is a sufficient to make everyone happy. But my understanding, which could be wrong, is that the EF Core team is stretched fairly thin. Where is it on the priority list? As I hopefully established earlier this is a critical problem with EF Core 3.X and I hope it is prioritized as such.
I also want to give some design feedback. Query splitting is the more performant solution in the overwhelming majority of situations; often by hundreds or thousands of times. So the default should likely be reverted to query splitting. Then syntactic sugar should be added to use the cartesian product approach as needed to address the problems you initially set out to solve.
Here’s a breakdown of the benefits of this approach from my perspective as a user.
I hope I’ve made a productive contribution to the discussion of this Issue. While I believe your decision was incorrect I also acknowledge that: I am not a member of your team; I do not see the situation from the more informed position of someone who works on the project daily; I was not at the design meetings; I was not at the meetings discussing the feedback on this problem; and I have not even looked at the code in any detail. I am frustrated but have made my absolute best attempt to not let that affect my feedback in any way. Please forgive me if any emotion has leaked into my comments as I attempt to make my business case.
We’ll investigate a way to execute a query in split query mode for common scenarios.
Hey folks. I must confess I am having a moment of cognitive dissonance with regards to the new Single Query behavior. I understand why we want low roundtrips and how we could argue it improves performance… but the attached zip has a pretty stark example of why EF 3.0 isn’t going to be particularly useful for dealing with object graphs.
Salient Query
Mileage is going to vary incredibly and exponentially based upon the number of Includes. But look at that payload difference… holy null values resulting in a hot mess batman. (the nulls causing a giant amount of not null data tagging along just so we can say ‘yup… that recordset is empty’)
I get it. Some people are OCD about roundtrips, and for small apps, it’s probably fine… possibly better.
Certainly for our use case, which I would characterize as somewhat unique (read-only, highly mathematical with thousands of discrete and variable graphs), this recent change is a deal breaker. Even with clever stitching. In fact, if we have to tap ‘off the beaten path’ EF/Linq functionality to defeat the “Single Query” feature, are we truly better off? I’d have to say at that point, the likes of Dapper and hand-rolled serialization and fixup would the more responsible and efficient course of action.
Our API on EF2.2, from a total cold start and unprimed caches can turn the request out in ~2.5 seconds for 38 seed graphs. With EF 3.0, we can’t complete 1 before the SqlConnection timeout kicks in and bombs the lot of it (what’s that? 30s? 60s?)
Please give serious consideration to at least adding hooks to disable “Sinqle Query” dispatch and using the old approach. (or something that you can inject between Includes to allow manual tuning of the roundtrips. Think .Include().Go().Include().ThenInclude().Go().Include().ToList() )
-joel
PS> I’m happy to give anyone a personal demo if the zip is in doubt.
2.2v3.0_ScriptsAndResults.zip
I think this is quickly going to become the most frequent issue/question with EF Core 3.x once people to start to transition and have the WTF moment, and indeed whilst my EAV model is mandated by a legacy system and is a questionable design choice, this new restriction affects even the most simple scenarios - the canonical Blog/Tags/Comments/Upvotes. If we are really restricted to realistically 2 or 3 to .Include 1-to-many relations and/or carry the cognitive burden of computing the avg number of related rows to power of number of .Include so we don’t stream too much redundant information then I can’t see how EF Core is any better than the micro ORMs for the query side.
I understand the argument that splitting the queries can potentially carry consistency risks that aren’t clear when not used with the right transaction modes and so wasn’t perfect fit, however it was pragmatic and this strive for the right choice seems like a strive for purity rather than what the consumers of EF Core 3.0 desired.
I would like to chime in and say that we are also having this issue, and is very problematic for us. An
.AsMultipleSqlStatements()extension method would suffice for those of us who need faster execution.I’ve been trying to refactor one of our cases and it turned out to be even more frustrating than I’d expected it too be. I understand that there’s many scenarios where executing one query makes sense, but there should really be a way to keep the old behavior. Even opt-in would be totally fine IMO. This is a huge step back otherwise in my opinion!
I’m therefore asking to reopen this issue.
Here’s the issues I ran into so far:
Loading data for a single entity requires storing the entity and the query
If I want to load just a single object, I can no longer use
.FirstOrDefaultAsync()directly as I (AFAIK) don’t have a way to load the related data on the resulting entity. I therefore have to keep the query in a separate variable.A manual check before calling LoadAsync is necessary to prevent empty queries
If I’d use the previous code, any calls to non-existing userIds would result in 2 useless SQL queries for the
LoadAsynccalls, so to do this properly I have to add a check and end up with the following code:AsNoTracking is not usable anymore
I can no longer use
AsNoTracking()as this would require me to use manual enumerators and populate the navigation properties myself in the loading code. If I had to load 7 child objects I’d have to write about 10 lines of code per relation instead of 7 in total just to get my object hierarchy back and I’m not willing to do that.Relying on
nullfor non-loaded collection properties is no longer possible.We have never initialized our one-to-many navigation properties in our entities because this way, we knew that the relation had not been loaded if the property was null. This was great to differentiate between “relation has not been loaded” and “there’s no related objects” and was a great safeguard against forgetting to call
Include(). With the newLoadAsync()the navigation property staysnullif there are no related objects.Generic paging / sorting methods are more complex
We have a generic
ToPagedListAsync(...)methd that takes a IQueryable variable, runs it once with Count() to get the total item count, applies paging & sorting, executes the query again and returns the paged data. Since the query that goes in doesn’t have the paging/sorting filters, I can’t callLoadAsync()on it - this would load too much data. I therefore have to return the final IQueryable as well so that I can applyLoadAsync()on it.Of course, I also have to check if there even were any results before I do my calls to LoadAsync() (see above).
This rewrite might also be necessary later when performance problems arrise
Since the simple
.Include()statements “just work” and since development databases often are small with little data, people will most likely use the old code and not realize their mistake until performance problems occur in the production database. Switching to the manual process can easily lead to other bugs/issues if one of the above things is forgotten / done wrong. If there were a simple.RunInSeparateQueries()switch, this would be an easy & safe change.??? Switching to a document DB like Cosmos requires reverting to the old code
Don’t know about this one but I’m just wondering if using Cosmos would require me to switch back to the “old” code again if the child objects are part of the parent document?
This also happens with me: #18017
It seems it’s happening to quite a few people. I know my query sucks, as do you in OP, but it would be a shame to have to revert an upgrade due this.
don’t blame the tools
there are different grads of craftsmanship, noob, junior, junior-fuzy, all the way to expert**
What i think ppl forget is that EF isn’t just magic, which can take also sorts of thinking out of the equation and magically make things work without side effects or meaning.
On the one hand you have lazy dev and on the other you have critical dev, sure its about finding a balance… but if you don’t want EF to write SQL for you, then structure LINQ which generate the SQL you want.
with great power come great responsibility.
Even with the older implementation (altho may be “faster” in alot of situations) it masks what is being done. This help you identity where query are really poor to begin with, when if the older was “faster”* its probably not optimal (lazy dev)
It allow you the opportunity to rethink how you are calling your data.
altho would like an exploration of what it was(older) doing like was it using CTE/Temp tables in the DB or multiple db calls and loading into memory.
I am very glad of the new behavior. Many times I had written a query expecting it to run in a single call, when it makes multiple roundtrips to the database to retrieve what I know to be very small amounts of data. Now it is in my hands as a developer whether to make one call to SQL for all the joins, or make separate requests to the database to retrieve all the pertinent information. Rewriting a few complex queries seems a small price to pay for the benefits gained in the majority of queries, at least certainly going forwards.
Ideally, we could provide SQL-compilation hints to the query, where it would run those queries separately. Or maybe opt-in behavior for the query as a whole. However, I never found EF 2.2 to produce very efficient queries in this scenario, so I would hope for more optimized behavior.
I’m with @cwe1ss on this one.
EFc 3.0 has already started the timer for us to switch away from it’s use all together. Our concern being, someone is going to see ‘oh, this is an old package, i’ll just update it’ and boom… tanked performance.
It wouldn’t be a big deal if it was a minor code change to ‘stay within the fold which is ef goodness’, but it’s not… it’s mid-level surgery to stay on the right side of EFc 3.0. As I stated before, at THAT point, the friction that EF was removing for us (moving relational data into a cache), has in fact increased the friction and we might as well fall back on Dapper or ADO.Net (arguably the most responsible choice to begin with… but we’re lazy).
@smitpatel included a reference to #12098. I’ve read the thread twice now, and I’m still not seeing a black and white argument in favor of single query dispatch. I must be missing something because I really don’t get why this change is considered an obvious choice.
The one qualifier being, I always assume the ORM is lying to me, so I double check I haven’t hit any of the edge cases where it might have messed up on the query generation. Is this why the choice was made to change behaviors?
If that’s the case, why not have single-query-dispatch be the default “will never lie” mode, with an option a “i think all ORM’s are liars mode” for those of us that are ok with that.
To be clear, I love what you guys are doing, and want to stay within a sweet spot that means I get to use your thoughtfully crafted code and STILL be lazy… but your making the sweet spot awfully small (imo).
I have create a sample app which shows a way to rewrite a multiple level collection include in LINQ to do split SQL queries and stitch that up. You can access the app as gist here https://gist.github.com/smitpatel/d4cb3619e5b33e8d9ea24d3f2a88333a
I have added code comments for better understanding. Feel free to ask if any questions. The sample covers tracking/non-tracking queries with buffering/streaming both cases. The sample is written for sync query but same can be done with async queries easily. You can also compose the base query to apply filters/orderings/paging.
Just realize that there are a lot of devs, like me, that felt that EF Core 1.x / 2.x was a big step backwards from EF6 where in EF Core 2.x there is no way to issue certain queries without them being broken up into multiple queries. I can provide samples of seemingly simple queries on EF Core 2.x that would run a subquery individually for EVERY result in the primary table, totally breaking performance! I’ve had to write and rewrite queries many times to get EF 2.x to produce the desired SQL. For us, EF 3.0 is a pleasant relief, and while it would be nice if there was a ‘multi-query mode’ for EF 3.0, I’d rather take the 3.0 behavior over the 2.x behavior any day.
Ideally, I’d like to see a parameter added to Include to indicate multi-query behavior (e.g.
.Include(x => x.Images, separateQuery: true)), plus an.AsSeparateQuery()method for splitting custom joins, but I understand that this might be a great deal of work. Hopefully an enhancement will make it into EF Core 5 to help with these scenarios.Please, provide an API because this behavior change is very problematic. We need to rewrite a lot of our queries in EF Core 3. I can understand this change but the problem is that I don’t see how I can easily go back to the previous behavior. I can enable tracking but I must write a lot of queries to do a ThenInclude(…).ThenInclude(…).ThenInclude(…).ThenInclude(…).
My 2 cents doesn’t matter if the old behavior is default and opt in to the new, or vice versa - the most important thing is that there be a way to do the old style queries. Many people have working applications using 2.2 that are blocked from upgrading to 3.1 because of this change. The reason we are blocked is we don’t have the time to rewrite the code and retest it - the effort involved is significant for many apps. So we need a way to keep up with EF, without major rewrites. It’s urgent. It there a time frame for the fix?
I’ve seen this issue as well. My version is much more reasonable than the OP’s:
Ef 2.2 run’s this as 3 queries. The entire operation takes less than 1 sec and returns 8 rows. Running on 3.1, 1 query, takes 8 minutes and returns 110863 rows that then have to be mapped down into 8 entities
We were planning on flatting our queries anyway, but this difference is remarkable.
I’ve been using EF since 1.0. EF6 was good and 2.x felt like a step forward. This honestly feels like a step backwards, especially for a new engineer (not most of us in this thread). I like the idea of adding an extension method to split the queries. Unfortunately it looks like 3.1 isn’t the version for our needs, we will be ‘downgrading’ back to 2.1.
@sitepodmatt Indeed, it was a pretty nasty surprise to see things like this with EF Core 3.0. As I implied, I am in a position where I can go either way and see both sides. And also for what it is worth, me and my team did notice certain areas perform better with 3.0 versus 2.6. Unfortunately, with the number of breaking changes (such as this) and the other issues that have cropped up on here over the past couple of days, we have decided to table upgrading to EF 3.0 for the time being.
Also, I think for the sake of backwards compatibility, they should have left in a hook or some type of flag on the context that you could toggle to use multi-query mode vs single query mode. I believe that would have been more pragmatic.
I almost wonder if @roji was being semi-prophetic when he said this on #12098:
I doubt “everyone” lost with this design change. But at this stage I think it’s too early to say definitively what was the net gain/loss. I am under the impression that most devs are not rushing to upgrade to Core 3.0, so it will take some time.
Non-sense. You can use EFCore 2.2 (or 2.1 is still LTS if you’re that bothered) on .NET Core 3.1 as explained above.
You own your application, not Microsoft. If you’re paying them directly for EFCore support, then this wouldn’t be the channel to ask for paid help. It seems like you should pay for support to a vendor or switch to an ORM that has paid support.
Like everyone else, we’ve had the same problem with query performance when trying to upgrade from EFCore 2.2 to 3.X. Going through all the code and re-writing/splitting all the queries and re-testing them all to ensure they come back with the correct data is not really an option. So upgrading is effectively blocked.
I’m pretty shocked the decision was made to completely change the underlying behavior. Sure it fixes a problem that has been there since day one, that affected certain applications that have many writers/readers of the same data rows, but breaks all of the most common use cases! The use-case you were trying to fix is easier fixed through use of a view or SP; or as suggested, by adding an extension method .AsSingleQuery() rather than changing the behavior.
I’m incredibly disappointed by this, as many of the other features in EFCore are well thought out and designed.
A work-around of some kind has been talked about for a rather long time since this issue was introduced, but, to the best of my knowledge, nothing is available, 7 months later. I do not understand the great reluctance to revert this behavior that breaks so many applications, and go about implementing the single query/Cartesian product methodology a different way.
I would like to second @stevestokes sentiment. I have been an ardent supporter of EF code, and was very excited to port our product from dotnetcore 2.2 to 3.1. Man was I surprised to see EF slow down so much the whole product became complete unusable! I was assuming I would get a perf boost…but now I have to roll back and can’t go to 3.1. Those cartesian product joins of entities with multiple child collections are a complete killer - all queries timing out. Many of the objects my business logic fetches include several collections of several nested children, each with nested collections - your typical object graph. There needs to be a way to opt in to the old behavior, maybe even a global default to opt in to it. I am not convinced it’s in the general case to do one query - data access patterns vary so much. We need choice.
@NetTecture can you please make an effort to be more respectful with your comments? Regardless of the content of you comment, it’s possible to express it politely and without implying that the other party hasn’t thought things through. Just trying to maintain a good and open environment for everyone to express themselves.
So, I’ve had this discussion with a developer friend of mine who a few weeks ago “tried to migrate” to .Net Core 3 and EF Core 3 who said: "We stopped the migration because of major performance issues, pages taking even 20 seconds to load, while before they were sub 0.5 sec ". Well, apparently the cause is the new single query model. Even funnier, is that one year ago he was heavily arguing against me saying EF core 2.0 splitting of queries is bad for performance and a single query should be generated, while I was arguing the opposite.
Leaving anecdotal evidence aside. The single query will always perform worse once the join level reaches a certain threshold. That’s why ever since EF 1.0 thee fix for this performance issue was to split the queries with the most common optimal pattern being 1 query per collection with related entities included.
I wish multiple queries would become an option again as it’s better for cases where large graphs with many includes are loaded. So I hope EF team really (re)considers this.
Yes, there’s no need to switch back the default. There just should be an easy, integrated way to do split queries because it was an extremely powerful and easy-to-use feature. And in my opinion, this is not an edge case but a very common one, especially with big databases.
I think my favorite way to use this would be an additional parameter on an individual
.Include()method - something like.Include(x => x.PreviousLogins, useSeparateQuery: true). This way, one could combine directly joined relations with separately loaded relations.This way, switching from one way to the other would be very easy!
PS: I understand that building a LINQ provider is extremely complicated so any work you do here is very much appreciated!
For anyone interested in the new split query functionality, some new docs are now live: https://docs.microsoft.com/en-us/ef/core/querying/related-data#single-and-split-queries
@mguinness After much analysis on what we could do as an add-on for 3.1 verses implementing this properly in 5.0, we decided to direct our efforts into the 5.0 feature. This was for two main reasons:
Therefore, we believe that people who need something to work with EF Core 3.1 should look into using EF Plus, which has a free version available. At the same time, the working feature for EF Core 5.0 will be merged soon.
Sorry for not providing this update sooner.
I also hit this issue recently when trying to migrate from 2.2 to 3.1 Indeed this change requires really alot of time to go trough all business logic and find all queries that need to be updated. (And the proposed solutions doesn’t look pretty)
I think that https://github.com/dotnet/efcore/issues/18022#issuecomment-576193879 explains well difference between single and multiple queries impact. And my idea of solutions is similar to https://github.com/dotnet/efcore/issues/18022#issuecomment-558574926 and can be even smarter.
When we have Include that is 1-1, the best solution is to have the join. The call would ask all the columns related to includes from multiple tables in one call, then split the data on client side to related objects. When we have Includes that are 1 to many, the best solutions would be to have separate call. If we include those related tables into the same call with joins, it will create columns that will be filled with same data over and over and increase size of the payload. (And probably calculation time on the server to consolidate the response, and client side to manage the split of the data)
This should be the default behavior, and then we should have different Include methods if we want to override it:
Because it should be simple for the novice user who doesn’t know about those subtile things. They should simply use Include(). Now for those who need advanced uses, different types of Includes can exists. In v1 the split queries were announced as a feature and I think in most cases it is helping simple users and we’re used to it since few years now. I agree it’s better in some cases to load includes it single query, but it looks like that in majority of cases it is just generating overcomplicated queries that kills database servers. (That I guess you tried to avoid when designing EF Core after EF6)
Glad that something will be done about it https://github.com/dotnet/efcore/issues/18022#issuecomment-586521749 and hopefully for 3.1.x to save all those who plan on migrating from 2.2
Perhaps this is obvious but I just wanted to say that while a lot of discussion here seems to centre around Include, this also affects Select/projections with navigation properties too, which don’t need explicit Includes to trigger joins.
AutoMapper’s ProjectTo extension produces a translatable projection and is quite handy for generic queries. A mapping between an entity and DTO can be configured, referring to multiple, perhaps nested, navigation properties, and the code at the point of querying becomes something as simple as
Set<TEntity>().ProjectTo<TDto>(config).ToList(). I’m not sure if this use of AutoMapper is considered bad practice, but the behaviour in 2.2 obscured that - as you want in an abstraction I would argue?I mention this scenario because the advice to simply rewrite queries to manually split the query doesn’t work in quite the same way here as it does for a lot of the other examples here. Many other examples are quite long, explicit queries that would need to be converted into a similar length query. In the scenario I describe, it can mean introducing many more lines of code, it’s much harder to use in any generic helper methods, might require more fundamental refactoring of the app.
I’d rather say the attention was created by EF 3.0 performance regressions that were kind of a surprise for users.
The query rewrite is a bandaid. It’s not the ideal scenario. Graph Loading is potentially a performance time bomb now. If your data doubles in DB you mays see a 10x performance drop in Production due to cartesian joins potentially being quadratic, and the performance behavior is not evident with small amounts of data but explodes on usage.
The query split that, users affected by performance problem need to do now, can also affect correctness/consistency. In general, almost anything in Db world can be affected by concurrent modifications, unless you’re always in a serializable transaction. Load with joins has in no way solved the concurrency issue, it just moved the problem into the user’s yard (Which is the place where it always was. The “user” is responsible with proper isolation and consistency). But I understand the reasoning, an error in user’s code vs library code is more telling to the user where the fix should be added.
I don’t think this should be a tradeoff that the EF team needs to make. This is by definition a tradeoff the library user needs to make. The user is the one controlling isolation levels and therefore correctness in the face of concurrency anyway.,
For web site type applications where small amounts of data are loaded due to display limitations. The performance benefit from reducing roundtrips by using joins is undisputable. This is where milliseconds are shaved from a page due to reduced roundtrips.
For enterprise-type or data-intensive applications where large graphs are loaded for thousands of entities, this is most certainly a strategy that can add from seconds to minutes to load times if the graphs are large.
I’m in both groups, and I don’t think the pain in the second case can be reduced by the milliseconds gained in the first case.
The impact on memory in a web application cannot be that great because typically web applications do not load big amounts of data per request. Also, it’s fairly rare that applications stream the materialized entities instead of placing them in a List, projecting and manipulating them. If a user is worried about the memory he usually does not use EF’s include to load entity graphs anyway.
Yes, I think this is the correct fix for users on both sides of the spectrum.
As long as the default behavior can also be set at context level, this should pose no issues.
@popcatalin81 the main point for me is that although we’ve changed the default behavior, you can still produce split queries by writing your queries in another way. The opposite was not true - you could not produce a single query with JOINs previously. At the end of the day we did make this as a reaction to issues and bug reports from actual users.
I totally get and understand what you’re expressing. I do believe that a lot of the frustration comes from the fact that the behavior changed here from 2.x to 3.x - and that’s completely legitimate and understandable. As written above we also do recognize the difficulty here and are looking into sugar for expressing query splitting and making that easier.
Same here, seeing how the multiple queries returned their own subset of results then were consolidated in the framework that resulted in 100s of results. My query is fairly basic, it’s really just: Get me products by brand, their images, their options and those option’s values.
Example:
await _context.Products.Include("Images").Include("Options.Values").ToListAsync();To my surprise, when I opened SQL Inspector when I started getting timeouts, I found out it was returning 150,000+ results when running the command manually.
@KevinMallinson I’m not surprised, it’s a pretty significant change. I was hoping when I first read about it, that there would be some way to opt-out of this new behaviour, but unfortunately, that doesn’t seem to be the case.
@ChristopherHaws - See https://github.com/dotnet/efcore/issues/21355 We have already added methods. They should be available in preview8
Now that #21189 has been merged in, a new
AsSplitQueryoperator has been introduced and I was thinking that it would be nice to introduce it’s counterpart e.g.AsSingleQueryand the ability to configure the default behavior at the context level e.g.context.QueryOptions.DefaultQueryLoadingBehavior = QueryLoadingBehavior.SplitQuery;in order to be able to opt-in/opt-out easily without the need to add aAsSplitQueryto every query that eagerly loads related entities because it’s a bit tedious and prone to errors IMOFiled #20892 to track first iteration of the solution we are going to provide in 5.0 release.
@roji I think the frustration comes from having completely rewrite and rework a large number of queries that were working and providing business value.
Given, I knew there would be some items we would need to change in order to use 3.0+, since it’s a major version update, but I did not expect that we would need to completely rewrite all our linq statements. As @popcatalin81 pointed out, for a large or complex application this can be quite an undertaking. It’s hard to justify to product owners/business owners, that we now need to “refactor” and re-regression test all applications (many microservices).
It’s an effort just to go thru and make everything compatible to work 3.0, but what is so disappointing is when you get there you realize that your application no longer works, I feel a better approach could have been provided than the “all or nothing”. Just provide a feature flag (like you did for other behaviours) so we have time to refactor this in phases. And not the response of “just rewrite all your linq/queries”.
I understand you feel your decision was ‘right’, But it’s the delivery and execution that were done poorly.
All this ideological talk is just noise.
The thing is the EF Core 2.X Load model is significantly better than EF Core 3.X for certain scenarios, and we would like it back. That’s the core of the issue.
I will be posting a detailed work-around later. For easier work-around in tracking queries, you need to rewrite in following way to get exactly same queries as before. Tracking is necessary part to do fix-up. Doing fix-up manually is not impossible but slightly more involved.
Support we have following entities & navigations in the model
You would need to rewrite only for collection navigations, reference navigations can be part of same query.
This will generate 3 queries to server. It would be slightly more optimized SQL then what EF Core 2.2 generated. And StateManager will fix up navigations. It also avoids duplicating any records coming from server to client.
Generated SQL:
Edit: If you do not have CLR navigation to use in SelectMany then you can use EF.Property to reference the collection navigation.
@Shane32 - Your work-around avoids large result set upto an extent but still doing Customer include with Orders will repeat customers in the records. By using SelectMany & selecting Customer separately you remove all duplication.
I have also linked this post from top.
Some observations:
The differences in my project are extreme. I ran the 2.2 sql query and it ran in <1 second. I ran the 3.1 sql query and it ran in about an hour and a half. The amount of rows being returned was astronomical as well. 19 compared to 12 million. This is really screwing over the transition to 3.1
I felt weird this one got closed with its resolution, but I also did not feel sure about what to say since my circumstances felt weird (trying to clone an object graph because we didn’t want to write manual clone code). I still feel that this issue is a big one since it can cause a significant amount of time having to rewrite queries and test app performance. It isn’t as painless as what @smitpatel was saying, if you ask me.
Do not take this the wrong way @smitpatel , I respect you and the rest of the EF team immensely. But at the end of the day, the “work around” suggestion, IMO, doesn’t feel like a satisfactory solution to the problem. I understand you guys are short staffed, but this isn’t something that I think can be easily dealt with in all cases with the suggested work around.
Thus I agree that this issue be reopened and there needs to be considered some type of API or other way to toggle the default behavior instead of having to be forced to rewrite code.
@jostapotcf When we ran into this problem in our environment, we were using a test database running on a separate server that was not on the same machine. As you probably saw in my reply up above, it took too long for it to run in even SSMS (I think it takes like 2-3 minutes for the whole result to finish). The situation was pretty similar to yours, judging from the sample query you provided.
And like you said, I am concerned about the entirety of this change when it comes to dealing with large object graphs. I was under the impression that switching from single query to multi-query with EF Core initially was to get away from the Cartesian explosion problem… and now we’re back to where we were before with classic EF. I find this odd.
Please correct me if I’m wrong. All suggested workarounds are not working together with
AsNoTracking(), so, even if we rewrite our queries, we are not able to achieve the performance of EF Core 2.1.Here is my personal opinion on it. The tried and tested behavior of EF Core 2.1 should be restored. This emerges from a simple risk analysis. With the logic of EF Core 3.1, it is much more likely to accidentally write a query that is not noticeable, but still significantly slower than the one that is executed in several split database queries.
Rather, I’m in favor of a function like
AsSingleQuery()that allows any database query to be executed as a single query. In this case it would also be more consistent (from my point of view), since theAsNoTracking()function also gains more performance, if it really matters and tracking is irrelevant.Since EF Core 3.0 you get an exception, if the query cannot be translated. However, you don’t even get a warning, if the query, which took less than five seconds to execute, now takes about half an hour. That is also not understandable for me.
I’d like to to respectfully disagree, with your assessment if you don’t mind. The dreaded Cartesian join was the No. 1 problem why EF 6 was perceived as slow for over a decade. And this behavior is brought back to EF Core.
I think there a relatively small numbers of users who see current behavior as a benefit when doing only small queries where milliseconds are gained, but the vast number of users working on medium to complex applications as a design and performance regression for complex graph loads going from 1 to 2 or more orders of magnitude.
What I’m basing the above statement on? On working for over a decade fixing hundreds and hundreds of EF6 performance issues, while some of which were overly complex Linq queries, most of the issues were Loads with too many included collections (not including DB specific performance issues here)
blogsQuery.Include(x => x.Posts).Load will return all blogs inner joined with posts. This means that if there are 5 columns in the blogs table, and 5 in the posts table, it will pull 10 columns back from SQL. If there was 10 blogs and each had 20 posts (200 records), that is 2000 columns of data.
blogsQuery.SelectMany(x => x.Posts).Load will perform the inner join, but will only return the posts. So for the previous example, it’s only pulling 1000 columns of data back (5 columns * 10 blogs * 20 posts).
Depending on your dataset, this could be significant – for example, let’s say there was a binary image column on the blogs table containing 20KB images. This might not normally appear to be an issue when you only have 10 blogs. But the original query already returned these images, and using SelectMany for this one query alone would reduce the data returned from SQL by at least 4MB.
SQL can of course execute the queries faster if it is returning less data, and make better use of its indexes and available I/O resources.
Split-Query behavior was a big improvement over EF6, at least in our scenarios. The new single-join behavior is a step backwards and in my experience leads to unnecessary slow and complex cartesian result queries. (Especially when the number of includes increases it gets really slow).
As already commented here, the recommended workaround of manually fetching related collections via
Set<>().Load()is cumbersome and increases code-size + complexity.So a big +1 from my side, for getting the old behavior back. I dont believe this should be enabled by adding additional
IncludeSplit(or whatever its name would be) overloads, as this is too much provider specific detail on this level (E.x. when executing in-memory, this does not make sense IMO). Furthermore, this does not work when using implicitly included collections (owned type mapping).Instead, I think this needs to be enabled globally, or at least at the Entity-level. This is especially important when using Owned-Collection mapping to model aggregate roots (canonical example:
Order->OrderPositions), as related collection gets included automatically when mapping collections as owned.Maybe a relational overload in OnModelCreating could be the solution:
Another option could be to enable it globally (e.x. if this per-entity level configuration is too complex or with too little value).
Same thing here. Today we started the migration to EF Core 3.1. 6 hours later we are running screaming from it. Some queries that ran instantaneously now take almost a minute to run.
@smitpatel I think the last point would probably provide the best flexibility, but I’ll understand if your team decides against it. In the least, the documentation definitely needs to be updated so others can avoid running into this situation.
@divega Having an API or extension to use the previous behavior would be good for those who don’t have the bandwidth to rewrite large queries like this one. I personally would like to rewrite mine since it’s inefficient and cumbersome to read, but as it stands I’ll have to remain on 2.2.6 for a while longer. I’m sure others are in the same boat.
As far as refactoring it goes, I seldom use explicit loading, so excuse my unfamiliarity with it, but would I be able to use that in tandem with No Tracking to be able to obtain a copy of the entity that can have its keys changed before saving it again?
Preview6 introduces
AsSplitQueryAPI to load collections using multiple queries. Details of the API has been posted https://github.com/dotnet/efcore/issues/20892#issuecomment-644456029 The API is available in nightly builds.Yes, welcome to dealing with Microsoft, where they do what they want regardless of what the community wants.
That is not the case. Old versions of EF Core can be used with all newer versions of .NET Core, since .NET Core is backwards-compatible. For example, you can use .NET Core 3.1 with EF Core 2.1.
In general, you are not forced to upgrade to newer versions of EF Core, but have the option of doing so. If you do upgrade to a new major version, then you typically have to deal with some breaking changes - the number and impact of these in 3.0 was indeed quite large, and future major versions are supposed to be much less extreme.
@TylerDM I definitely see your comment as a productive contribution, and there’s no need to apologize for anything at all.
First, it’s true that this issue has attracted a lot of attention, but I’d be a bit more careful inferring from this that the community as a whole is asking for a return to the previous behavior; scrolling up shows people arguing the opposite and wanting single-query to remain the default (as well simply people requesting help rewriting their queries). This isn’t an argument against anything you said really, just a small note I wanted to make about the difficulty of gauging “what the community wants”.
More to the point, one thing that’s somehow gotten lost in this discussion, is that single-query vs. split-query isn’t just about performance - it’s also about correctness/consistency. Split-query is problematic since the multiple queries can actually see different states of the database as it is being modified concurrently, resulting in inconsistent results; note that this isn’t a theoretical problem - we got actual, concrete complaints from users about this happening (https://github.com/dotnet/efcore/issues/9014 is an example). I do feel strongly that our default behavior here should tend towards correctness and consistency, and for any perf trade-off that could compromise correctness to be a user opt-in.
Aside from that, after discussions both internally and with users, I don’t really agree there’s evidence that’s split query is “the more performant solution in the overwhelming majority of situations” - this really is one of those cases where the answer purely depends on the query (how many includes), the actual cardinality of the data in your particular database, and many other factors. I’ve personally seen various cases where additional round trips can be devastating to performance, especially as latency to the the database increases with the cloud, and while single-query can be streamed, split-query cannot (without MARS), affecting memory usage. However, I do recognize that single-query cartesian explosion may cause a more dramatic degradation than the above, and will obviously cause frustration to anyone porting an application from 2.x to 3.x which uses many Includes per query.
I do hope the above adds a bit more information on why the decision was made to switch to single-query. Please understand we do recognize the problem and intend to provide an easy way to opt into split-query behavior (this is what most people seem to be asking for), but at this point I don’t think we’re considering changing the default behavior back (doing that in a 3.1 patch release is probably not possible in any case). We’re also aware that 3.0 was a release with a lot of user-impacting changes - I’m notably thinking about client evaluation which also caused a lot of upgrade pains. Our intention is to not do such a user-breaking release again.
Finally, keep in mind that all of the above is just the opinion of one engineer on the team…!
Me too 127,000+ rows for a query with 4 Include and 3 ThenInclude statements. and 182 rows when i did split the query for 3 pieces.
I AM blaming the tools. I have been using dotnet, EF, dotnetcore and EF core since they each respectively were available, quite a while now, I know the ins and outs of them. I love the frameworks for the most part, and was very excited to see this iteration. I have always been an early adopter and as such willing to update my code to take advantage of new features or follow better patterns. I understand what is happening here and what the workarounds are. The problem is that the tools have changed drastically with no opt out, and the amount of work to rewrite the product it too high of a bar to justify it.
Yes, my 2 key is faulty.
@jnm2 all this has already been addressed above. For example, ensuring consistency would mean using the serializable (or snapshot) isolation level, which comes with its own price and complications - which also vary across databases. I really believe it should be up to the user to make that decision, rather than it being done magically by EF. The plan is already to provide a way for you to opt into ~single query~ multiple queries for 5.0 - but not to make that the default.
Yeah, I think we’re completely aligned now in understanding the situation…
It’s a fair point about this being a question of definition. I think we already agree that both single and split query modes should be made available, so this is more a question of what the default should be. I do lean towards guaranteeing that a single LINQ query return consistent results by default (even if it may not be on SQL Server by default), because I believe the default should be as safe as possible… I prefer the potential perf issues of a single query with JOINs (which are hopefully more easily visible/discoverable) to subtle concurrency-related consistency issues.
A related question is whether when split queries are done (whether opt-in or by default), an ORM should wrap them in a serializable/snapshot transaction to again guarantee consistency even in that scenario. I’m pretty wary of implicitly starting a transaction with such a high isolation level, so it may be better left to users to do themselves - though again there’s value in providing consistency by default without forcing users to think about all this complicated stuff.
Thanks again for this great conversation @FransBouma.
Yes and no. The best rule of the thumb for generally best performance across the board is to load related entities with a join (N:1 and 1:1) and collections separately (1:N). This should be de default. Any performance gains from loading collections with a join are specific and the user needs to opt-in because he knows that:
Show stopper for us upgrading. looks like 2.2 for a while longer, very unfortunate.
@mguinness We will provide something to help with this issue for 5.0. If it’s deemed useful and low risk we’ll try to port it back to 3.1.x, but the bar for adding new API in patch releases is very high. Currently we are focusing on new features so we can ship something meaningful in 5.0 preview 1 so no work has been done for this issue yet, but it is high on our list.
@Shane32 - Nice explanation.
@flipdoubt - If you want to split into multiple queries then you are going to be having each SQL query have an associated data reader open. When you use buffering, the data reader is fully consumed and closed. When you use enumerating, you are keeping data readers open and only reading what you need. You need to enable MARS in SqlServer to have multiple data readers open at same time else you will hit that exception. On side note, previous versions of EF Core did buffering behind the scene when running multiple queries and not using MARS, so if you cannot have MARS enabled then buffering is nearly equivalent of what EF Core 2.x did.
The team has stated they won’t revert back to split queries so single query mode is staying.
Then this discussion is not relevant to you, this is about navigation properties.
I think what people are looking for is a way to optionally split queries into more manageable/efficient pieces and then stitch the results together. Easier said then done I’m sure, so I welcome any effort to get this done for .NET 5.0 before 2.1 LTS ends in 2021.
@mguinness That’s the plan as long as this issue is in the 5.0.0 milestone
@shane32 I agree it’s better that it adhire to what is expected but at the time it was the seemingly recommended approach based on the documentation provided and what other resources recommended for the cloning scenario. Im fine with rewriting it, but as I said in the OP, I’d rather explore all options first before committing to a rewrite. I’m not sure what others will say, for me it’s a pain, but not a super huge impact on my team since my code base uses pretty simple queries for all other operations.
@dragnilar you just made me realize there are two interesting points I didn’t think of:
Any workaround that relies on explicit loading or executing multiple queries would require fixup behavior that only happens for tracked queries.
It is possible that some of the negative performance impact of 3.0 comes from the fact that we no longer perform identity resolution in non-tracked queries, which should result in many more objects being created. There are two things you could measure that may give us a hint here:
@ajcvickers @smitpatel
FYI, we had a chance to test this today with the original query and 5.0 did resolve the performance problem (we had to enable Split Query mode obviously). I am noticing side effects of #22283, but we have our work around with EF Plus for now.
Thanks for addressing this and keeping the 🦄 magic going! 👍
I’m surprised to see so few mentions of the increased memory usage caused by the “single query / no automatic splitting” feature in EF Core 3. I’ve just hunted down a tenfold memory increase after moving to EF Core 3.1.10 (from 2.2.6) and the cause was that the query was no longer automatically split into 5 simpler queries.
EF Core 2 - ~120 MB used
EF Core 3 - ~1.3 GB used
This has a huge impact.
FYI, the EF Core 2 query looked like:
@popcatalin81 and others, one of the point that constantly gets missed/ignored in this discussion, is that multiple queries suffer from a severe consistency issue. The two queries can see a different state of the database as it is being updated in between the two queries. This can result in extremely problematic discrepancies, as dependents in the 2nd query reference a principal that no longer exists, or worse, whose state has changed since in an incompatible way. This can be worked around by enclosing the split queries in a serializable (or snapshot) transaction, but that also creates its own problems and should not be done implicitly by EF.
In my personal opinion, this makes split queries unsuitable as a default loading strategy - safety and consistency should be the most important goal here, especially as long as alternatives do exist. In other words, you should definitely be able to opt into split queries if you’re willing to assume the responsibility for possible consistency issues (by enclosing in a serializable transaction, or just because you know there’s no concurrency).
Having said that, the team is currently actively discussing a more 1st-class split query option to simplify the workarounds that @smitpatel has already shown above. Expect more details soon.
This is a straw man argument.
No one was saying the change should be reverted because we don’t want changes or breaking changes for that matter. Breaking changes for the right cause, are good.
This change, in particular, was bad because it affects a large class of consumers in a dramatic way, with no clear benefits in other places.
Having lived in OSS and other ecosystems for a while, I’m used to major versions introducing breaking changes.
I was also disappointed in migrating from .NET Core 2.2 to 3.1 that I didn’t realize how EFCore 3.1 broke this. However, the workaround of using EFCore 2.2 with .NET Core 3.1 is working for me. I’ve been updating queries in a branch (changing huge Includes to multiple statements) and it’s not so bad. It was supposedly what was happening before anyway!
I understand this is painful and you may disagree with the decision but asking for rollbacks and refusing to accept any breaking changes is what kept .NET Framework down. There is a path forward, use it but also be mindful that all code needs maintainance and regular revisiting.
Just ‘upgraded’ to core 3.1 from 4.7. After working out the ridiculous amount of Include()/ThenInclude() we have an unworkable solution with minutes to compile the expression followed by SQL timeouts. The old solution was clean and fast, please revert.
I am having the same issue as @snappyfoo my SQLite integration tests now time out because it cant handle all the LEFT JOINs. I also use generic
Set<TEntity>()calls and I use AutoMapper’sProjectToso refactoring in not an option for me.FWIW, Django’s ORM solved this problem with select_related and prefetch_related functions many years ago.
I really don’t see why this is so difficult for the EF team to solve in the same or similar fashion.
Agreed - most of the slowdowns preventing me from moving forwards belong to this class of problems - relatively large object graphs that are projects to DTOs with ProjectTo and AutoMapper.
@NetTecture we’re not currently doing any of that as far as I’m aware, and have no plans to do so. If anything, these would likely be quite SQL Server-specific and wouldn’t work on other databases. I’m not an expert in related entity loading, but I don’t see a reason why batching wouldn’t be possible.
Note to anyone who is curious - I edited the OP with an example of a work around we used to get around this issue. I should note that we ended up using EF Plus for it due to the fact that it was the most pragmatic choice. What EF Plus is doing is IMO what EF Core should offer (a method that lets you fire off a split query within a big includes chain). Unfortunately it still requires you to use a tracking query. I imagine this may not still be the best for people who have queries like this that are more pervasive throughout their code base, but in our case this is the only query we have that is this huge. All of our other ones are very simple.
I hope it helps anyone else, although it does require using a third party library. 😕
I’m really disappointed by this very significant change as well. Something that was easy to use and worked really well is now broken and requires weird workarounds.
Suggesting people should use manual enumerators with
GetEnumerator()andMoveNext()is very error-prone, not intuitive and really shouldn’t be the “recommended” solution.This issue adds a significant amount of time to our migration to .NET Core 3, as we now have to closely look at every usage of
Include(), rewrite many queries and do a lot more testing than we anticipated.Closing this issue as a simple way to rewrite the query has been posted. To document it in our docs is being tracked at https://github.com/aspnet/EntityFramework.Docs/issues/1854 To improve overall perf of query is being tracked at https://github.com/aspnet/EntityFrameworkCore/issues/15892 Providing a method which would do fix-up needs to create new context behind the scene anyway as the issue with shadow FK to connect objects. Which users can do themselves right now too.
If we’re talking a lot of joins or anything complex I feel like EF shouldn’t be used with LINQ but with something like a SQL View or Stored Procedure, especially when performance is of concern.
With that said, imagine my scenario:
Nothing crazy, nothing complicated, just asking for 3 includes. In EF 2.2 that would return 50 accumulative results. In EF 3.0 that would return 10,000 results. In a real world example using actually data where not every product has 5 images, 5 options and 20 values. It went from approximately 1000 results (the EF 2.2 way) to approximately 3500 results for a random brand I just tested.
Now keep in mind, that’s someone using a statement like I posted above.
await _context.Products.Include(x => x.Images).Include("Options.Values").Where(x => x.BrandId = 'xxx').ToListAsync();I assume most people will be fine using this? And perhaps my use-case is more of an edge-case, however it seems like anything with an exponential join can be bad for performance. I came from using EF 6.0 and never ran into what I’m running into now, so maybe it is an edge-case.
In my situation I’m going to have to rewrite my entire Service/Repository layer. Because we pass in the includes to the repository so that each service request is only getting information relevant to what it needs, rather than wasteful includes. This will all have to be rewritten before going to EF 3.0, unless there’s a way to change how it creates the statements.
With all of that said, I’m sure you all had a reason to do this, especially if it’s proven to work well in EF 6.0 and if the frameworks are going to merge in the future that’s just another reason. I also realize that doing a single async call for the new Async Streams that’s new in core 3.0 is probably another good reason to go to a single query.
The problem you describe is exactly why split query was introduced, so you would be best served by upgrading to 5.0 instead of 3.1 (unless you have a blocker). Enabling split queries globally allows you to make that the default.
Usage of Union can also break index usage for join.
The following is quoted from https://github.com/dotnet/efcore/issues/18022#issuecomment-586521749
The propose-punt label was just added, does that mean the 3.1.x helper will no longer be offered?
Why is this fix postponed for the 5.0 release? We’re having problems right now with the 3.0 and 3.1 releases.
We’ve just upgraded to .net Core 3.1, including EF Core 3.1, in the last month. Everything is running in our acceptance environment now. Initially when we started migrating a lot queries with includes got really bad performance after the upgrade due to the behavioral change of EF Core 3.0 and 3.1 for include statements. We thought we had fixed these issues by using Entity Framework Plus. Everything seemed okay in our Development environment. But we’re now getting errors quite frequently in our Acceptance environment regarding the request limit on our azure databases (which is 90). Also specific azure functions are running into timeouts due to the default maximum time of 5 minutes. These issues all started exactly on the day that we upgraded our Acceptance environment.
It’s not acceptable that the release of a fix in which we get back the old behaviour will only happen in the 5.0 release. With this breaking change in EF Core 3.x many systems just can not be upgraded without major performance issues.
BTW EF Core (as well as EF6) sets READ_COMMITTED_SNAPSHOT when it creates the database (see https://github.com/dotnet/efcore/issues/4809#issuecomment-283330842).
@FransBouma the point here is not that data can change at any time and become stale - that’s indeed true. The problem is that when one LINQ query accessing navigations is executed via two SQL queries, EF needs to wire up the instances coming out of the first query to instances coming out of the second.
To give a concrete example, consider the following simple example with a Blog that has many Posts:
In EF Core 2.2, this would be executed via the following two queries (SQL is from PostgreSQL):
Now, if a certain Blog’s Name started with
Bwhen the first query runs, but was renamed in between, the query results would include that blog, but with an empty collection of posts - although at no point did the blog post have zero posts in the database. This isn’t stale data - it’s false data, and can lead to quite severe corruption issues depending on the application.In other words, again, the problem isn’t that users get “a reflection of a state that has passed”, but that they get a state that never existed. We still fully intend to provide the option to do this, but don’t think it’s right for the default behavior to contain such hidden, subtle concurrency issues that only manifest themselves under load etc.
@roji
The database can always be updated in between DML operations, and also in the time between the query is executed and the query is consumed. Heck, during a select on a set of rows in a database which uses MVCC (like oracle) it’s not going to include the updated data but when the query is finished the state of the table has changed. So you’ll have to work with stale data that might be out of date in any case, no matter what method you’re using.
When querying a database and consuming data outside the database in a graph, you’ll work with stale data, it doesn’t matter how you fetch it: one big query might take longer and updates might be applied because of the read locks after the fetch has been completed (so the db is different at the time of consumption), and multiple queries might result in a graph that’s different from the graph you’d get when you’d have fetched it in one go.
That last point is however not something that’s fixable with a client consuming data in a graph form outside the database: because at one point can you say ‘this is consistent with the database state at this time’ ? The answer to that is … never. So you have to accept data is stale when you consume it, and it’s a reflection of a state that has passed, no matter what you do.
(edit) I do recall an old debate about this same topic when ObjectSpaces was in beta and they introduced the ‘span’ api (include / prefetch / eager load), where some people were against it as it would lead to inconsistencies and Warren argued it was stale data anyway (I remember as I then realized it was undoable to avoid stale data and thus inconsistency between client and server side data, one of these aha moments in a novice ORM developer 😛 ). It was in a newsgroup back in 2002/3, not sure if it’s available still.
@mamazgut I wasn’t saying anything about single-query, or in fact about the direction EF Core should take. I was merely responding to @sdepouw’s statement that newer versions of .NET Core require newer versions of EF Core. While newer versions of EF Core may depend on newer versions of .NET Core, the opposite is not the case, and there’s nothing stopping you from using EF Core 2.1 on .NET Core 3.1, for example. It is also high unlikely that .NET 5.0 will introduce breaking changes to a degree that would prevent EF Core 2.1 from running on it.
I’m sorry, but I do not entirely agree with your statement. Although it is technically possible, the usefulness of such a solution remains very questionable, because in this case the problem is only postponed and not completely solved. As support for the current LTS version 2.1 ends on August 21, 2021, developers are faced with even greater challenges when migrating to the current .NET Core version (some breaking changes in .NET 5.0 are also to be expected).
Please don’t get me wrong. You are doing a fantastic job and I am happy to be able to use the results of your work. But I think your decision to put everything in one query was not correct. The advantage of EF Core was that it always worked out-of-the-box and delivered the best possible performance. If you need clinically clean SQL statements, you can write them by hand. The strength of EF had always been productivity.
Why does .NET Core so tightly couple itself to EFCore like this? Most upgrade paths involve “how did EF break everything this time” in my experience, and it leads to scenarios like this. Where you literally can’t upgrade to the latest version of a framework, because it obligates you to upgrade your database-layer package and there’s no real way around it.
Let’s sever this awful dependency! I understand you can’t always do that, but I find it hard to believe that EFCore has to be upgraded every time .NET Core is (does compiling against .NET Core 3 assemblies really necessitate such breaking changes under the hood that consumers can’t change, opt out of, or at least get notified of prior to getting stuck with it?)
Like others in this thread, we have show-stoppers on long-invested upgrade paths not because of anything in .NET Core, but because of this tightly-clung dependency that cannot be avoided. Bare minimum - give people a choice. Maintain old behavior. Upgrade older versions of EFCore to be workable with newer .NET Core. Don’t force upgrades that break things so hard that people can’t upgrade .NET Core. It’s one thing to upgrade so people use your tooling “better.” It’s another to break existing code for no reason.
@Mitch-Healy and others, we’re aware that queries with many includes will be much slower when executed as-is on 3.1 compared to 2.2. You can achieve the 2.2 performance again by rewriting your queries to split as detailed above, although we understand the frustration and extra work this entails. Although this was an intentional change in 3.0, we definitely don’t take it lightly but do believe we did the right thing for the product and many users; there’s a long discussion on the reasoning above (though this issue is definitely getting quite long…).
Whether to split or not to split will be a decision that the users will have to make. As we’ve learned that this and evaluating parts of the query on the client automatically is something not maintainable in the long term. When bugs are fixed or new features are added to query translation these behaviors could flip resulting in unintentional breaking changes, so we’ve front-loaded all the breaking changes in 3.0 by making all the queries behave in the same way by default.
As a reminder @smitpatel has posted workarounds: https://github.com/dotnet/efcore/issues/18022#issuecomment-537219137 https://github.com/dotnet/efcore/issues/18022#issuecomment-542397085
The additional API that we would add will likely be just some sugar to get the same behavior with less code.
@smitpatel, thanks for the tip to use the buffer strategy. Enumerating the result before loading the child properties (as you did on line 64) pulled the performance back to 2.2 speeds for me. That was the subtle detail I overlooked.
i think it should be 1 query, not even sure what multiple query’s means. does it apply the where multiple times… anyway, if you are concerned about performance, then i suggest that you measure and if you find inadequate sql is being generated, i suggest that the convert to Linq (from a in cx.Accounts), you can use Linq Pad to ensure the sql is being generate like you would like. I have been using EF6 and Core for years, and if 3 is the same as EF6 then i think this is the way someone would expect it to work. If it now starts making CTE tables or multiple query’s internally, then you should be explicit on that. havnt done much comparing generated SQL here with regards to 2 vs 3, but that’s why i write the Linq instead of relying on includes to do the joins efficiently.
@Shane32 I don’t think anyone wants to go “backwards”. I believe the problems that myself, @jostapotcf , @cwe1ss and others have run into are cases where we need “backwards compatibility” /more ability to control what EF does, because we wound up with edge cases where we still need the split query functionality and/or the flexibility it allotted.
I’d just like to say, while an opt-in approach for single-query mode would be great, please don’t change the default behavior. Split queries are immensely useful at times, but having a single LINQ statement run a single SQL statement is not only intuitive, but required for writing performance oriented queries. EF 3 is so much better at translating complex queries, it would be a shame to go backwards.
I’ll grant ORDER BY is likely the biggest culprit in the SQLConnection timeout. Let’s say the ORDER BY is instantaneous.
That’s has to then get serialized over the wire to the client into memory, unpacked, POCO’ified, and finally fixed up.
I should have recorded the row count difference. I’ll wager it is in the order of 10000:1 of useless data to useful.
Even if the SQLCmd and TDS Streaming happened instantly, we’d still end up wrecked by EF’s time spend wrapping the results in objects (by virtue of having to contemplate a lot of useless rows)
This is actually an important point. Data set size aside (1.22gb is what - 0.15 seconds on a modern backend network), the order by set is ridiculous and seems to speak from bad optimization. And that really puts all that data down into tempdb for a very heavy order by. THAT definitely must be fixed. It is broken SQL being WAY over protective on the required sorts, it seems.
@dragnilar Correct, my workaround only works with tracked entities. You should be able to write a snippet to untrack ALL entities from a dbcontext, if that works for you. Something like this: (not sure if this is a EF6 or EF Core snippet)
If that works for you, you can retrieve the data with tracking, then detach all tracking, then use your existing code to clone the data.
You could probably also write code to inspect any entity object, and utilize the IModel to recursively iterate through all of its navigation members, creating a list of all related entities, then detach them all. Should be relatively easy to do. With a little extra work, it could reset primary keys and foreign keys, which may be necessary as well.
For anyone wondering how to ‘hack’ your way around this to get 2.2 speed on 3.0, follow the idea on this comment. Basically, you define an initial query to filter your results to the entity you want, and then create dummy variables with .includes to include the related entities you need.
Important: I have only tested if this returns the correct data, nothing else. If you need to do something more than reading/displaying, make sure you test first.
For example, I went from this:
to this:
You should only need to do this for the entities that need a
.ThenInclude(). It’s not the best at all, but it allows me to still use .NET Core 3.0 without having to sacrifice load times.Glad that you’ve been able to make progress. Take a look at Keyless Entity Types for views. If you need further help, I’d recommend asking on Stack Overflow as you’ll get much better help there than on a unrelated closed issue.
@Tasteful - Filed #22483
This link https://gist.github.com/smitpatel/d4cb3619e5b33e8d9ea24d3f2a88333a
@CaiusJard your description of how EF Core loads related entities in 3.1 is pretty accurate - but we haven’t necessarily seen evidence that the ORDER BY clauses are the cause of significant perf degradation, as opposed to the cartesian explosion itself. See #19571 for a discussion specifically on this, and note that #19828 is about removing the last ORDER BY, which indeed isn’t necessary.
To keep this already huge thread focused, I propose that if you want to continue discussing this, please post on one of these two issues rather than here.
Hehe, I never said my example was “reasonable”. 😄
I haven’t had a chance to try it with the release preview since we’re pretty swamped at the moment due to some other circumstances, and frankly, the work around we found with EF Plus has been working great. If I do have a chance, I will try out the release preview of Net 5 to see if its any faster than EF Plus’ implementation.
Is it possible to configure the default query behavior of the DbContext to execute all queries as split queries? If not, I would love to see this added. Something like this:
and adding an overload to
AsSplitQueryto allow you to disable this behavior on demand via.AsSplitQuery(false)or adding another quarriable extensionAsSingleQuery?@roji I agree with that. The part I missed was where it was noted that multiple roundtrips are not required in order to do multiple queries.
Correct, that’s what I can make up from the docs too, it’s basically enforcing what postgresql and oracle and other mvcc using databases do by default 😃
And thank you for pointing out a situation with split queries. Luckily for me I define eager loading as a series of operations one can define together with a root query fetch (so not an atomic operation), but indeed if you define it as an atomic fetch then split queries might not be consistent unless RCSI is used (which is more costly).
It boils down a bit to a definition problem, perhaps? whether ‘include’ is seen as atomic or not? If you see it as ‘include in the resultset’ but don’t define it as an atomic operation (I’d be careful with these as it’s a client operation) you don’t need to take measures. That does require the ability to explicitly enforce a start of a transaction tho (like ctx.StartTransaction(…) which then also has to explicitly be committed).
In SQL Server, readers block writers and writers block readers, however in other databases this isn’t the case, (I think postgresql indeed does things differently, due to it uses MVCC like oracle ? ), but that said, what you quoted that’s about an update in progress, like a transaction T1 on connection C1 has updated a row R1 and a select of another connection C2 then reads R1 (or tries to) and sees the old results. In SQL Server, the C2 connection’s read of R1 waits till T1 is committed/rolled back as the x lock on r1 is then lifted, in the default transaction isolation read committed.
Rereading my action order in my example (Please be aware I find this discussion pretty academic 😛, as to me it’s impossible to guarantee a single linq query is an atomic operation without a transaction ), I do realize the update+delete, if ran in a transaction, won’t show the behavior as the select on blogs will stall as the transaction has an x lock on blogs. If the isolation level is readuncommitted, then the inconsistency can occur, but you asked for it.
I wonder if PostgreSQL effectively implements the same behavior for Read Committed as SQL Server does when READ_COMMITTED_SNAPSHOT is on?
Edit: This article seems to confirm this:
Generally very useful series about the isolation levels in SQL Server, and the two read committed modes in particular: https://sqlperformance.com/2014/07/t-sql-queries/isolation-levels
Using the mechanisms specifically designed to solve these types of problems isn’t acceptable, but using a hack that can bring applications to their knees is? To my (limited) knowledge snapshot transactions don’t even lock aside from recovery scenarios. What was wrong with that approach? @roji
I found the issue. For anyone reading this message in the future who is also trying to do query splitting with collections on derived entities using inheritance:
EFC2.2 was intelligent enough not to do the query if there was no element of that type in the collection. EFC3.1 mega query always join them, and even if you try to load them manually afterward it will do it even if there’s no need. What you need to do is check for Any() before launching relation loading call with Load(). Example:
This is a very disappointing change and discourage the use the EF Core 3.1. We have a large project and how MS can expect us to change entire code to in single shot which will involve regression as well. Ideally these should have been a additional feature and let the app teams decide if they want to leverage it. This is big hurdle to adoption of EF Core 3.1. What is more disappointing is the fact that EF team is not talking about rollback, no sign of consideration of this pain.
@dragnilar I think @Joehannus means this
.IncludeOptimized(z => z.SubProject.Units.Select(y => y.Building))Notice the Select statement after Units that is supposed to load the Building related data.
2.2 is end of life so you need to drop back to 2.1 to get security updates, see .NET Core release lifecycles.
@Cooksauce the only way Npgsql could “implement” MARS would be:
So at least in the foreseeable future I can’t see anything like this added to Npgsql…
@Cooksauce another possibility is to work with two database connections via two DbContext instances, this way you stream results on each connection separately. That doesn’t work well if you need to track entities coming back in a single context, or if you need transactionality.
At the end of the day MARS is a SQL Server feature that may also cause certain important perf degradation (as multiple roundtrips to the database are done to fetch result, instead of one). At the SQL level you can do something like MARS with cursors; you can have multiple cursors open at the same time for multiple queries, and draw X rows at a time from each cursors. That again is bad for perf because of the additional roundtrips, and also won’t be generated by EF Core.
You can not. A proper implementation of c may i.e. utilize the use of session level sql table variables to store primary keys from the first query and then reference this in additional followupqueries. It may also utilize using multiple parallel SQL statements (MARS - multiple active result sets) to get data out in parallel over one connection. There is no way to do both without significant changes to the pipeline - they can IIRC not be expressed by LINQ.
This would actually be a good area for an ORM to shine and get away from the simple and “stupid” (as in: mechanical expression.
The decisions of which subqueries to isolate, though, can not be effectively done by EF - it may require additional data and thus be an application elvel decision. I would love to be able to split a query with methods, though, and then use metadata on the objects to decide when an object hierarcy gets goo expensive 😉
For some queries it make more sense to repeat the parent query rather than perform multiple round trips to the database. You could query for parent, then query for parent-join-child returning only the children and then combine the two. We have some queries where we do this manually as the parent query is very fast, allowing us to pull back multiple results without the combinatorial explosion of a single query with joins.
Is there any chance of having this sort of thing optional? Picking one behaviour is always going to annoy somebody.
The issue is actually deeper. I would say soemthing is seriously wrong with Ef Core in general. Up to 3.1 release (and even now) EfCore is struggling to generate sql for anything not totally basic queries. Up before 3.0 you had features that where incompatible - global filters work, but do not expect to use global filters in all cases because it generates internal errors.
I have been repeatedly berated in errors (when saying I would move back or stay with Ef non core) how EF Core is better and the future and has more features (by people associated with the team). This is all nice and dandy, but if I get a sql error generated because Ef Core decides to create crap SQL or if I get internal errors because it has problems handling it’s own functions, I simply do not care that it is “better” and “the future”.
Even today (not tries it out NOW - preparing to make a 3.1 upgrade run next week) the toolset is off - there is no way to wokrk database first and generate proper stored procedure bindings, I have been told - which is fine with me, I use a third party tool.
But something is seriously broken if the “new and better tool” is not suitable for usage in the 4th generation or so and still sold as better than an older tool that acutally works.
If I run into ANY issue with EfCore, I will use dotnetcore 3.1 to jump to a third party implementation of Ef (non core) that supports global query filters in Entity Framework. Performance is secondary, imho - not saying it is not important, but if things like simple group by generates internal errors or bad sql, then I do not care about performance.
And do not get me talk about the bug fixing non-cascade. Right now it seems any “bad sql” issue again has to wait for - not sure. 5.0? 6.0? 10.0?
I would love a dynamic efficient query mechanism - but right now I am happy with “generate proper sql for all basic scenarios that I can throw at it via odata” - WITHOUT pulling all data into memory and THEN filtering, because this is my current workaround in dotnetcore 2.2. Next weke I try 3.1 - and as I said, if i run into any issues, it is classical EF again. I have to ship, not to wait another year for another major version.
While I somewhat agree with your point, particularly around “EF isn’t Magic”, at the same time I have to disagree with most of it. The greatest part about being a developer is creative freedom. Constricting that is never healthy for any engineer or even the framework creators. Becoming a great engineer comes from deep learning. If there are framework options that enable more learning and mistakes I welcome that vs boxing people in. A framework shouldn’t restrict engineers from doing what they need to do, it should enable them. So yes it does create the opportunity to have adverse affects, performance issues, ect but without making those mistakes, you do not learn. So with this, your Yoda quote applies as well.
You certainly could simply execute a query.Include(Posts).Include(LikedBy).Load and then run query.Include(Posts).Include(DislikedBy).Load - which will prevent a cross join from the LikedBy and DislikedBy tables.
But this type of coding will continually return the query and Post rows back from SQL. For optimal execution, you should/could use SelectMany to only load the related records required, which will execute more similarly to EF 2. Search this thread for SelectMany to find smitpatel’s post which describes the correct approach.
You can still get the same behavior in EF 3 that you got in EF 2 with a little refactoring.
This is based off @smitpatel comment (https://github.com/aspnet/EntityFrameworkCore/issues/18022#issuecomment-542397085).
I would like to see some way of changing the default behavior but this has been an acceptable workaround for me.
Specifically for the transactionality, see https://github.com/aspnet/EntityFrameworkCore/issues/9014 as well.
#12098 captures the discussion over the design which should satisfy anyone’s interest.
Can you outline some examples out of interest? I ponder this briefly and I think most of them can be mitigated with the correct transaction modes.
@NetTecture If it were in a format that was ready to be spooled over the network, sure it could be streamed that fast. But the SQL Server’s got to do a lot of work serializing that, and then the client has to de-serialize it, all before EF ever even sees it.
The SQLTimeout would indicate that the TDS stream was simply taking too long (ie, EF never even saw the results)
(This was all running on my local machine with ample specs. the situation would be much worse hosting up on azure when DTU throttles come in to play)
@doughtcom For simple queries like those, there’s a simple workaround shown here https://github.com/aspnet/EntityFrameworkCore/issues/18017#issuecomment-535763068 that should work well for you without rewriting much code. Just write the code like this:
This will split the SQL access into two queries, most importantly without returning duplicates of the images. No other code needs to change in your service layer (for this database call), as EF will stitch the models back together, and
retwill have all the results (including both images and options/values).