-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JOIN instead of CROSS APPLY in generated query in SQL Server #17936
Comments
@roji - I believe you investigated some perf work around rownumber. Can you answer why generating RowNumber is more efficient than CROSS APPLY? |
I'll take a look at this soon. |
Some very basic research I did back in June on window functions vs. PostgreSQL lateral join (which is equivalent to SQL Server cross apply in this context), showing window functions to be superior. I'll investigate the scenario above more deeply to understand the exact differences etc. See also this article referenced by @divega at the time: https://blog.jooq.org/2017/09/22/how-to-write-efficient-top-n-queries-in-sql/ Here is a comparison between lateral join and window function, for getting the first row. Window functions win hands down (look at the second cost number on the outermost plan node). In theory the plan could vary due to table size (I only had very few rows), but I have no time to go deeper. For info on reading PostgreSQL EXPLAIN results: https://www.postgresql.org/docs/current/using-explain.html Lateral join
Window function
|
In our case with MS SQL the latter query (select with joins from select) could not complete at all. Either PostgreSQL and MS SQL use very different ways to optimize such queries, or the optimizations heavily depend on other database specifics, such as indexes, etc. |
Thanks @dmitry-slabko, I'll do a more in-depth investigation and comparison of the two databases in the next few days. |
@roji - I can send to you the two complete queries, as they are a bit larger then I initially posted with their execution plans. |
That could definitely help - along with the actual schema please. The actual data could also be relevant, or at least the number of rows - plans can sometimes be different based on table size etc. |
This is the full linq query: from navObject in Context.NavObjects
join vessel in Context.Vessels on navObject.VesselId equals vessel.VesselId
from passage in Context.Passages
.Where(x => x.VesselId == navObject.VesselId && x.ActualDepartureTime.HasValue && x.ActualDepartureTime.Value <= fromTime)
.OrderByDescending(x => x.ActualDepartureTime)
.Take(1)
.DefaultIfEmpty()
from simulationPoint in Context.SimulationDetails
.Where(s => !isPresent && s.PassageId == passage.PassageId && s.Time <= fromTime)
.OrderByDescending(s => s.Time)
.Take(1)
.DefaultIfEmpty()
from position in Context.Positions
.Where(p => p.VesselId == navObject.VesselId && p.Time <= fromTime)
.OrderByDescending(p => p.Time)
.Take(1)
.DefaultIfEmpty()
from trackPoint in Context.TrackPoints
.Where(t => t.VesselId == navObject.VesselId && t.Time <= fromTime)
.OrderByDescending(t => t.Time)
.Take(1)
.DefaultIfEmpty()
where simulationPoint != null || position != null || trackPoint != null I attached both generated queries, their execution plans, and schema files for involved tables. |
Thanks. As that query uses the same construct multiple times, if there's a perf issue it would indeed be magnified here. I'll dive into this soon. |
@dmitry-slabko am exploring these two options and seeing some interesting phenomena with window functions and cross apply. However, to complete the picture, could you please also submit your EF Core model? A simple, self-contained console program would be ideal, including the model and the problematic query. |
Note also that the tables.sql you provided contains some issues (e.g. references to |
@ajcvickers - I propose punting this issue. |
I'd like to at least complete the investigation, even though I agree there's little chance we'll actually change anything for 3.1. |
Ok, some more input on this problem. Here is the linq: from vessel in Context.Vessels.Where(...)
from position in Context.Positions
.Where(t => t.VesselId == vessel.VesselId && t.Time <= fromTime)
.OrderByDescending(s => s.Time)
.Take(1)
.DefaultIfEmpty()
select new LocationPoint ... The meaning is to get the latest point for each vessel id. SELECT ... FROM [Vessel] AS [v]
CROSS APPLY (
SELECT [t3].*
FROM (
SELECT NULL AS [empty]
) AS [empty1]
LEFT JOIN (
SELECT TOP(1) [p0].*
FROM [Position] AS [p0]
WHERE ([p0].[ObjectId] = [v].[ObjectId]) AND ([p0].[Time] <= @__fromTime_4)
ORDER BY [p0].[Time] DESC
) AS [t3] ON 1 = 1
) AS [t4] The subquery to retrieve data from Position is effectively filtered. Now, since Preview 5 and until 3.1 release, the query is such: SELECT ... FROM [Vessel] AS [v]
LEFT JOIN (
SELECT ...
FROM (
SELECT ..., ROW_NUMBER() OVER(PARTITION BY [p].[ObjectId] ORDER BY [p].[Time] DESC) AS [row]
FROM [Position] AS [p]
WHERE [p].[Time] <= @__fromTime_1
) AS [t]
WHERE [t].[row] <= 1
) AS [t0] ON [v].[ObjectId] = [t0].[ObjectId] And this is the problem - the inner subquery retrieves all rows from Position table, and in our case it is 16+ million rows, which may even be much more for some other customers. However, the subquery is executed for each row in the master query. So, it appears that the use of partitioned queries for MS SQL was based on wrong assumptions, as this pattern generates queries that will not perform quite well even on small data sets, while on large data sets they simply kill the reader. I cannot say how this pattern behaves on other servers, such as PosgreSQL and Oracle, but for MS SQL it is not applicable. I would highly recommend to change the query generation pattern for such linq expressions back to what it was up until 3.0 Preview 5. |
@smitpatel just a quick guess, but shouldn't the In any case I'll try to find time to look into this, although it won't be right away. |
I tried to execute the innermost query, and its performance issue comes from this part: |
One more note: the table even has an index on VesselId + EventDateTimeUTC columns, yet it does not help. |
Nope. The innermost subquery computes value of |
Ok, yet another note :) |
@roji sure. 3.1 SQL: SELECT [t].[Id], [t0].[Timestamp], [t0].[VariableId]
FROM (
SELECT TOP(2) [v].[Id]
FROM [Variables] AS [v]
WHERE [v].[Id] = @__variableId_0
) AS [t]
OUTER APPLY (
SELECT TOP(2) [v0].[Timestamp], [v0].[VariableId]
FROM [Values] AS [v0]
WHERE [t].[Id] = [v0].[VariableId]
ORDER BY [v0].[Timestamp] DESC
) AS [t0]
ORDER BY [t].[Id], [t0].[Timestamp] DESC, [t0].[VariableId] 6.0 SQL: SELECT [t].[Id], [t0].[Timestamp], [t0].[VariableId]
FROM (
SELECT TOP(2) [v].[Id]
FROM [Variables] AS [v]
WHERE [v].[Id] = @__variableId_0
) AS [t]
LEFT JOIN (
SELECT [t1].[Timestamp], [t1].[VariableId]
FROM (
SELECT [v0].[Timestamp], [v0].[VariableId], ROW_NUMBER() OVER(PARTITION BY [v0].[VariableId] ORDER BY [v0].[Timestamp] DESC) AS [row]
FROM [Values] AS [v0]
) AS [t1]
WHERE [t1].[row] <= 2
) AS [t0] ON [t].[Id] = [t0].[VariableId]
ORDER BY [t].[Id], [t0].[VariableId], [t0].[Timestamp] DESC Models: public class Variable
{
public int Id { get; private set; }
public string Name { get; set; }
public ICollection<VariableValue> Values { get; private set; }
}
public void Configure(EntityTypeBuilder<Variable> builder)
{
builder.Property(v => v.Id)
.ValueGeneratedOnAdd();
builder.HasKey(v => v.Id)
.IsClustered();
}
public class VariableValue
{
public int VariableId { get; set; }
public DateTimeOffset Timestamp { get; set; }
public double? Value { get; set; }
public Variable Variable { get; private set; }
}
public void Configure(EntityTypeBuilder<VariableValue> builder)
{
builder.HasKey(w => new { w.VariableId, w.Timestamp })
.IsClustered();
builder.HasOne(w => w.Variable)
.WithMany(v => v.Values)
.HasForeignKey(w => w.VariableId);
} |
Thanks @palhal and @mdawood1991; we'll still need to do a perf investigation to see if there are scenarios where OUTER APPLY performs worse than the ROW_NUMBER approach. Clearing milestone to consider for 7.0. |
I'd like to share another workaround that I've found in case others have the same problem. The good news is that it's simpler and it also works with the more advanced queries I had problems with. Adding a second OrderBy (after Take): var _ = _db.Variables
.Where(v => v.Id == variableId)
.Select(v => v.Values
.OrderByDescending(w => w.Timestamp)
.Take(2)
.OrderByDescending(w => w.Timestamp)
.Select(w => w.Timestamp)
.ToList())
.SingleOrDefault(); produces the following SQL: SELECT [t].[Id], [t0].[Timestamp], [t0].[VariableId]
FROM (
SELECT TOP(2) [v].[Id]
FROM [Variables] AS [v]
WHERE [v].[Id] = @__variableId_0
) AS [t]
OUTER APPLY (
SELECT [t1].[Timestamp], [t1].[VariableId]
FROM (
SELECT TOP(2) [v0].[VariableId], [v0].[Timestamp]
FROM [Values] AS [v0]
WHERE [t].[Id] = [v0].[VariableId]
ORDER BY [v0].[Timestamp] DESC
) AS [t1]
) AS [t0]
ORDER BY [t].[Id], [t0].[Timestamp] DESC Maybe not as clean as 3.1, but the result is returned instantly which means I can now actually use EF Core 6 :) |
@iRandell removing the first OrderBy makes the result non-deterministic - you're effectively asking to get any 2 values, and only then order those two values by timestamp. Assuming you want the first two values by timestamp, this is unsafe and could lead to incorrect results. |
You're right. I didn't notice that. Thank you! Also you can use Skip(0) instead of the second OrderBy() AFTER the Take(). Even though it looks better, it adds one more ORDER BY clause Provider: Npgsql.EntityFrameworkCore.PostgreSQL (6.0.1) The use case with Skip(0): var result = context.Chats
.Include(chat => chat.Messages
.OrderByDescending(message => message.CreatedAt)
.Take(1)
.Skip(0))
.Where(chat => chat.Id == 1)
.ToList(); SELECT c."Id",
t0."Id",
t0."ChatId",
t0."CreatedAt"
FROM "Chats" AS c
LEFT JOIN LATERAL
(SELECT t."Id",
t."ChatId",
t."CreatedAt"
FROM
(SELECT m."Id",
m."ChatId",
m."CreatedAt"
FROM "Messages" AS m
WHERE c."Id" = m."ChatId"
ORDER BY m."CreatedAt" DESC
LIMIT 1) AS t
ORDER BY t."CreatedAt" DESC
OFFSET 0) AS t0 ON TRUE
WHERE c."Id" = 1
ORDER BY c."Id",
t0."CreatedAt" DESC The use case with two OrderBy(): var result = context.Chats
.Include(chat => chat.Messages
.OrderByDescending(message => message.CreatedAt)
.Take(1)
.OrderByDescending(message => message.CreatedAt))
.Where(chat => chat.Id == 1)
.ToList(); SELECT c."Id",
t0."Id",
t0."ChatId",
t0."CreatedAt"
FROM "Chats" AS c
LEFT JOIN LATERAL
(SELECT t."Id",
t."ChatId",
t."CreatedAt"
FROM
(SELECT m."Id",
m."ChatId",
m."CreatedAt"
FROM "Messages" AS m
WHERE c."Id" = m."ChatId"
ORDER BY m."CreatedAt" DESC
LIMIT 1) AS t) AS t0 ON TRUE
WHERE c."Id" = 1
ORDER BY c."Id",
t0."CreatedAt" DESC |
Just to add to the list of relevant reporters/data on this, I wanted to report my use case which results in an extreme difference under the different generated queries. With this query in EF Core 6.x against PostgreSQL, I'm getting an EF Core generated query with the row number partition strategy that takes over 2 minutes to run - versus the LATERAL JOIN which is instant:
Note that each query just returns 1 result, but the I do have a workaround to 'fix' this in one of a few ways:
|
Thanks @dhedey and others - we're indeed aware that there are some scenarios where lateral join/cross apply is superior. The problem here is to work out exactly when (and possibly in which databases) it's preferred over row number. I hope to investigate this for 7.0. |
Any news if this update will make it into 7.0? Seeing a lot of cases where this is causing increased computing time. |
This issue is in the Backlog milestone. This means that it is not planned for the next release (EF Core 7.0). We will re-assess the backlog following the this release and consider this item at that time. However, keep in mind that there are many other high priority features with which it will be competing for resources. Make sure to vote (👍) for this issue if it is important to you. |
Personally I think a bug that has been open for three years, and was a regression, should be prioritized higher especially since there is no obvious reason for the queries failing from the POV of an EF Core user. A simple test for me on a small database shows at least 16 times slower queries between using windowing and CROSS APPLY just by adding |
@NetMage technically this isn't a bug, in the sense that results are correct; it's a perf issue. Could you please post the small repro and queries showing the x16 perf difference? That would help get this prioritized. |
Any news on this performance update? |
This issue is in the Backlog milestone. This means that it is not planned for the next release (EF Core 8.0). We will re-assess the backlog following the this release and consider this item at that time. However, keep in mind that there are many other high priority features with which it will be competing for resources. Make sure to vote (👍) for this issue if it is important to you. One thing that would help prioritize this is a perf comparison that shows this to be significant. |
@MoMack20 thanks for sharing - this is definitely an issue I intend to explore (thought it probably won't happen for 8.0). With this issue (and other query performance issues), what would be really helpful would be a simple, minimal repro comparing the two SQLs, showing both query plans and clear benchmark results. We get reports from time to time but without this information, making it harder to properly investigate etc. If you could help with that, that could go a long way to bump up the priority of this issue. |
@roji Is there any specific template I can follow for setting up the database for that repo? Would I need to include the data in a SqlPackage file if the case is determined by number of rows in the tables? |
@MoMack20 no template really - as long as you deliver a minimal, clear repro that's fine. In most cases, a simple, minimal SQL script for creating the database schema and some data is ideal; in this case, since lots of data may be needed, you can either generate it programmatically in the SQL script, or send e.g. a bacpac for SQL Server for a dump of the database (if needed, my email is listed on my github profile). Basically any way in which we can easily get your database up and running and clearly see the plans and perf differences between the two queries. |
@roji hi, I am not a sql expert, but I did a little research
Indexes:
Тest requests:
Window functions were faster in scenarios with both the bp.Id scalar filter and the range from the example above. In case of scalar filter the query is optimized and window function doesn't perform aggregation of the whole table, you can see it in the screenshot below: |
@Gagarin23 thanks for the testing and the in-depth analysis, this is exactly the kind of thing that can help. Unfortunately, it may take some time before we're able to properly look into this and improve the situation - but it definitely is high up on my priority list. We also have to verify what happens on other databases, but if the behavior is consistently similar, we can definitely consider changing what EF does here. Note to self: I've forked the repro above to https://github.com/roji/EF17936 just in case it disappears |
@roji I finally got around to making a repo for this The DACPAC file should have everything necessary to standup the database. Query plans and EF generated sql are all included in the repo as well. Results as of my last run: |
Thank you @MoMack20. I do plan to take a deep look at this and at other query performance issues, but unfortunately there's a good chance this won't yet happen in 8.0 due to many other competing priorities. There's a good chance we'll do a significant push in this area for 9.0. FYI I've forked your repo to https://github.com/roji/EFCore-Issue-17936 to make sure it's still there when I get to it. |
EF Core Preview 5 would generate CROSS APPLY from a linq query like this:
The generated query would be:
In RC1 the query contains JOINs from SELECTs from SELECTs which cause where bad performance and timeouts:
As you can clearly see, the Preview 5 generated query is clear and effective while the RC1 generated query is off. Please fix this query generation pattern.
Further technical details
EF Core version: 3.0 RC1 (versus 3.0 Preview 5)
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: .NET Core 3.0
Operating system: Windows 10
IDE: Visual Studio 2019 16.2.5
The text was updated successfully, but these errors were encountered: