Skip to content
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

Adding EF.Constant to prevent parameterization in query #31552

Closed
Liero opened this issue Aug 25, 2023 · 15 comments · Fixed by #32412 or #32434
Closed

Adding EF.Constant to prevent parameterization in query #31552

Liero opened this issue Aug 25, 2023 · 15 comments · Fixed by #32412 or #32434
Assignees
Labels
area-primitive-collections area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported Servicing-approved type-enhancement
Milestone

Comments

@Liero
Copy link

Liero commented Aug 25, 2023

What problem are you trying to solve?

"Query is fast in SSMS but slow in production" kind of performance issues, where query is parametrized by EF and SQL Server chooses query execution plan suboptimal for concrete parameter value.

For example:

SELECT TOP(1000) * FROM dbo.MyView  -- fast
int pageSize = 60
if (loadMore) pageSize = 1000;
ctx.MyView.Take(pageSize).ToArray();  //slow when pageSize is 1000

Describe the solution you'd like

Introduce an EF.Constant() mechanism, which is identified by EF Core and prevents parameterization.
This goes hand in hand with #28151

ctx.MyView.Take(() => EF.Contant(pageSize)).ToArray();
ctx.MyView.Where(b => b.Name == EF.Constant("bar"))

Alternatively, add support for query hints like RECOMPILE or OPTIMIZE FOR
/cc @dsyme @NinoFloris @roji

@roji
Copy link
Member

roji commented Aug 25, 2023

Duplicate of #28632

This actually doesn't help with top-level Skip/Take because these don't accept a lambda.

@roji
Copy link
Member

roji commented Aug 25, 2023

ctx.MyView.Take(() => EF.Contant(pageSize)).ToArray();

Note that if we indeed introduce an overload of Take that accepts a lambda expression, there wouldn't be any need for EF.Constant, since the expression tree itself would contain a constant node vs. a parameter. I've updated #28632 with a comment about this.

@Liero
Copy link
Author

Liero commented Aug 30, 2023

Just to make sure it is not forgotten, Constant vs Parameter can make a performance difference not only in Offset/Fetch but also in Where clause, so EF.Constant would still make sense

@roji
Copy link
Member

roji commented Aug 30, 2023

@Liero Where doesn't have this problem since you can already control whether a constant or parameter is used, see #28632 (comment).

@roji roji marked this as not a duplicate of #28632 Aug 31, 2023
@roji
Copy link
Member

roji commented Aug 31, 2023

Proposing to keep this open to track EF.Constant as sugar to make it easy to inject Constant nodes into queries (where that makes sense for perf) without using Expression builder APIs.

Note #28151 which is the same but in the other direction, i.e. an EF.Parameter API to force parameterization.

@KLuuKer
Copy link

KLuuKer commented Sep 22, 2023

I would actually need something like this on a whole array of int's.
Pull #13617 MIGHT introduce a performance regression when used with global query filters.

I would like for it to NOT optimize my tenant id global query filter into a openjson call.
x => enabledTenantIds.Contains(x.TenantId)
and just keep that list of id's hardcoded.

The list itself doesn't actually change all that much, and it wont generate that many extra query plans in my case.
Having this Constant call work on a Array of int would be helpful in tuning the queries just in case the planner can't handle the extra parameter.

@roji
Copy link
Member

roji commented Sep 22, 2023

@KLuuKer I understand, though I'd highly recommend first ensuring that there's an actual, meaningful performance regression before going into something like this (people sometimes assume perf issues where there are no actual ones, without measuring etc.).

Note also that this issue would only add sugar - you can already create an expression tree yourself and inject Constant nodes via Expression.Constant. Note also that if the size of enabled tenant IDs is fixed, you can parameterize the list contents instead of the entire list (x => new[] { ids[0], ids[1] }.Contains(x.TenantId)).

@KLuuKer

This comment was marked as off-topic.

@roji

This comment was marked as off-topic.

@ajcvickers ajcvickers added this to the Backlog milestone Sep 25, 2023
@KLuuKer

This comment was marked as off-topic.

@roji

This comment was marked as off-topic.

@KLuuKer

This comment was marked as off-topic.

@roji

This comment was marked as off-topic.

@roji
Copy link
Member

roji commented Nov 26, 2023

Note that this will unfortunately not help with Skip/Take parameterization, since these operators accept a non-lambda parameter, and therefore calling EF.Constant within their argument will simply evaluate EF.Constant (which will throw). EF.Constant isn't needed for non-top-level Skip/Take, because there we already detect the difference between parameters and constants.

However, EF.Constant would still be helpful for various other scenarios.

roji added a commit to roji/efcore that referenced this issue Nov 26, 2023
roji added a commit to roji/efcore that referenced this issue Nov 26, 2023
@roji roji removed this from the Backlog milestone Nov 27, 2023
@roji roji self-assigned this Nov 27, 2023
roji added a commit that referenced this issue Nov 27, 2023
@roji
Copy link
Member

roji commented Nov 27, 2023

Reopening to consider patching.

@roji roji reopened this Nov 27, 2023
@roji roji added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Nov 27, 2023
@roji roji changed the title Consider adding EF.Constant to prevent parameterization in query Adding EF.Constant to prevent parameterization in query Nov 27, 2023
roji added a commit to roji/efcore that referenced this issue Nov 28, 2023
Closes dotnet#31552

(cherry picked from commit 484a3ed)
@ajcvickers ajcvickers added this to the 8.0.2 milestone Nov 29, 2023
roji added a commit to roji/efcore that referenced this issue Nov 30, 2023
Closes dotnet#31552

(cherry picked from commit 484a3ed)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-primitive-collections area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported Servicing-approved type-enhancement
Projects
None yet
4 participants