EFCore generate wrong sql when using UseRowNumberForPaging under concurrency requests

[hbwang518]

The SQL EFCore generated is as below.

exec sp_executesql N'SELECT [t0].[AutoID], [t0].[CreateTime], [t0].[HrefType], [t0].[HrefUrl], [t0].[IdentityID], [t0].[ImgUrl], [t0].[IsDel], [t0].[AutoID], [t0].[MenuType], [t0].[MerchID], [t0].[ModifyTime], [t0].[OrderAsc], [t0].[SchoolID], [t0].[Title], [t0].[CreateTime], [t0].[HrefType], [t0].[HrefUrl], [t0].[IdentityID], [t0].[ImgUrl], [t0].[IsDel], [t0].[MenuType], [t0].[MerchID], [t0].[ModifyTime], [t0].[OrderAsc], [t0].[SchoolID], [t0].[Title]
FROM (
    SELECT [t].[AutoID], [t].[CreateTime], [t].[HrefType], [t].[HrefUrl], [t].[IdentityID], [t].[ImgUrl], [t].[IsDel], [t].[MenuType], [t].[MerchID], [t].[ModifyTime], [t].[OrderAsc], [t].[SchoolID], [t].[Title], ROW_NUMBER() OVER(ORDER BY [t].[OrderAsc]) AS [__RowNumber__]
    FROM [T_Menu] AS [t]
    WHERE (([t].[IsDel] = 0) AND ([t].[SchoolID] = @__schoolid_0)) AND ([t].[MenuType] = @__req_TypeID_1)
) AS [t0]
(
    SELECT [t].[AutoID], [t].[CreateTime], [t].[HrefType], [t].[HrefUrl], [t].[IdentityID], [t].[ImgUrl], [t].[IsDel], [t].[MenuType], [t].[MerchID], [t].[ModifyTime], [t].[OrderAsc], [t].[SchoolID], [t].[Title], ROW_NUMBER() OVER(ORDER BY [t].[OrderAsc]) AS [__RowNumber__]
    FROM [T_Menu] AS [t]
    WHERE (([t].[IsDel] = 0) AND ([t].[SchoolID] = @__schoolid_0)) AND ([t].[MenuType] = @__req_TypeID_1)
) AS [t0]
WHERE (([t0].[__RowNumber__] > @__p_2) AND ([t0].[__RowNumber__] <= (@__p_2 + @__p_3))) AND ([t0].[__RowNumber__] <= (@__p_2 + @__p_3))',N'@__schoolid_0 uniqueidentifier,@__req_TypeID_1 int,@__p_2 int,@__p_3 int',@__schoolid_0='545E4B15-8427-41F8-95BD-421A9B1F2FCA',@__req_TypeID_1=1,@__p_2=0,@__p_3=20

In concurrency case, the issue can be reproduced with high change. The sql EFCore generated are different for same query and many of them are wrong.

Steps to reproduce

Create dbContext with codes below.

protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
 {
            if (!optionsBuilder.IsConfigured)
            {
                optionsBuilder.UseSqlServer("Server=.;Database=mydatabase;user id=sa;password=123456;", options => options.UseRowNumberForPaging());
            }
 }

Start two threads and generate new DbContext and call query in each thread. The query is as below.

dbContext.Set<TMenu>().Where(t => t.SchoolId == Guid.Parse("545E4B15-8427-41F8-95BD-421A9B1F2FCA") && t.MenuType == typeId) .OrderBy(t => t.OrderAsc).Skip(0).Take(20);

Further technical details

EF Core version: (2.1.1) (The issue can be reproduced under 2.1.4)
Database Provider: (e.g. Microsoft.EntityFrameworkCore.SqlServer)
Operating system: Windows 10 Pro
IDE: (e.g. Visual Studio 2017 15.7.4)

[ajcvickers]

@hbwang518 Could you please comment on issue #13959 and let us know why you are using RowNumberForPaging?

[OpenSpacesAndPlaces]

I believe I may be running into the same problem - in certain high volume cases we start getting errors like:
The multi-part identifier "t.__RowNumber__" could not be bound. The multi-part identifier "t.__RowNumber__" could not be bound.
They only get cleared by recycling the entire site.

SQL 2008 R2
EF Core version: 2.2.3

[aiscrim]

Do you have any news on this issue? I got the same error when testing my application with the UseRowNumberForPaging flag set to true. I need to use that flag because one of the installations of my application needs to target Sql Server 2008, and the OFFSET and FETCH keywords that EFCore otherwise generates are obviously not compatible with that version...

[mailivore]

I just discovered this issue after updating my apps to 2.2.3.
Any fix available?

[ajcvickers]

@mailivore This issue is in the Backlog milestone. This means that it is not going to happen for the 3.0 release. We will re-assess the backlog following the 3.0 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.

Also, I'm curious if you're planning to continue using SQL Server 2008 after it is out of support next month?

[aiscrim]

@ajcvickers Sorry for jumping in, but as I said last month I have the same issue and I'm following the discussion.

Also, I'm curious if you're planning to continue using SQL Server 2008 after it is out of support next month?

In my case that's not in my hands, my application will be deployed at my client's premise. They have plans to migrate to a newer version of SQL Server but it won't happen before this fall in the best case (and I'm pretty sure it will take much more time), and in the meantime they are not able to use my application because of this bug...

[mailivore]

Not in my hands also.
A migration will certainly occur but not before many months.
I wrote some ugly raw SQL to get rid of the Skip() and Take() functions.
I'll take a look later after SQL Server migration to change it back to a more readable form.

[ajcvickers]

@aiscrim @mailivore Thanks for the info.

[andreichuk]

@ajcvickers

RowNumberPagingExpressionVisitor in SqlServerQuerySqlGenerator's constructor leads to races in SelectExpression: if you have 2+ same "skip/take" queries executing in parallel at the same time, RowNumberPagingExpressionVisitor modifies the same SelectExpression object in 2+ threads which leads to an exception or invalid sql generated.

I do not know reasons why RowNumberPagingExpressionVisitor was put in SqlServerQuerySqlGenerator but probably it would be better to move it to the query compilation stage as everything (except parameters?) related to queries should be immutable at the sql generation stage.

As for now there is a simple fix for the issue: using a clone of SelectExpression to pass into a sql generator
3319cca

also RowNumberPagingExpressionVisitor in SqlServerQuerySqlGenerator leads to races in RelationalQueryCompilationContext (generation of unique table names) as it is unsafe to access SelectExpression's _queryCompilationContext field outside of the query compiler:
https://github.com/aspnet/EntityFrameworkCore/blob/260b7f4fe0e8de07339448b4a2d2e5a10b577946/src/EFCore.Relational/Query/Expressions/SelectExpression.cs#L41-L42

[ajcvickers]

@andreichuk If you have root-caused the issue, then would you consider submitting a pull request for the fix?

[andreichuk]

@ajcvickers any chance to have a new version with the fix released before 3.0? I have tested my code with 2.2.5 version only

[ajcvickers]

@andreichuk This isn't currently planned for 3.0, but if we get a PR that fixes the issue then that could still make it in.

[smitpatel]

This functionality hasn't been ported to 3.0 yet.
Cloning SelectExpression is not correct fix.
In 3.0 we are going to move this processing to compilation phase in ShapedQueryOptimizingExpressionVisitor.

[EzrealJ]

@OpenSpacesAndPlaces
After 4 months, I encountered the same problem as you. In version 2.2.6, did you find a solution to this problem, except restart the site?

[OpenSpacesAndPlaces]

@EzrealJ
If the problem occurs, restarting is the only fix.

The only real mitigation, while keeping the same code in play, would be to prevent duplicate simultaneous requests by custom memory caching or something along those lines.

IMO, probably the best fix if you don't have a heavy dependency on dynamics would be just to use like: raw SQL, Stored Procedures, or DB Functions.

For small datasets, paging in-memory after generating the full data might be an option.

TLDR: You're looking for a work-around that best fits your use-case.

Hope this helps!

[EzrealJ]

@OpenSpacesAndPlaces
I m very sad that there is no way to solve it, but I am very grateful to you for taking the time to answer my questions.

[ajcvickers]

Closing old issue as this is no longer something we intend to implement.