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

SQL Server: Allow users to explicitly specify the target SQL Server version/type #30163

Closed
3 tasks
roji opened this issue Jan 27, 2023 · 14 comments · Fixed by #30738
Closed
3 tasks

SQL Server: Allow users to explicitly specify the target SQL Server version/type #30163

roji opened this issue Jan 27, 2023 · 14 comments · Fixed by #30738
Labels
area-dbcontext area-sqlserver closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement
Milestone

Comments

@roji
Copy link
Member

roji commented Jan 27, 2023

We have cases where we should vary our SQL based on the target SQL Server version (e.g. #30161); to support that, we should introduce a way for users to tell us the target version, in the DbContext options. We should default to "latest", allowing users to specify older versions. We should also allow users to explicitly tell us that they're using Azure SQL, as that could have an impact on some things (e.g. execution strategy).

@ErikEJ
Copy link
Contributor

ErikEJ commented Apr 4, 2023

Oldest extended supported box version today is 2014 (12.x)

  • STRING_AGG - (ServerProperty('ProductVersion') > 13 OR ServerProperty('EngineEdition') IN (5,6,8)) and Compatibility_level > 100 if using WITHIN GROUP)
  • CONCAT_WS -(ServerProperty('ProductVersion') > 13 OR ServerProperty('EngineEdition') IN (5,6,8))
  • OPENJSON - Compatibility_level > 120

roji added a commit to roji/efcore that referenced this issue Apr 4, 2023
@roji
Copy link
Member Author

roji commented Apr 4, 2023

Oldest supported box version today is 2017 (14.x)

I'm assuming you mean "mainstream end date"; for "extended end date" the older versions are still supported no? (info).

OPENJSON - Compatibility_level > 120

According to the docs I'm seeing, OPENJSON is supported on SQL Server 2016 (13.x) and later (so compat level 130), and STRING_AGG and CONCAT_WS on SQL Server 2017 (14.x) and later (compat level 140).

@ErikEJ
Copy link
Contributor

ErikEJ commented Apr 5, 2023

I'm assuming you mean "mainstream end date";

Yes, SQL Server 2014 and later is currently in extended support

Compatibility level is not the same a SKU (product version) - for example Azure SQL supports compatibility level 100 to 160 currently.

OPENJSON requires compatibility level 130 or newer, so not be running on a database with that. That can only happen with SQL Server 2016+ or Azure SQL

STRING_AGG and CONCAT_WS runs on "any" compatibility level, but only on SQL Server 2017+ or Azure SQL. So compat level 140 is not required in this case.

@ajcvickers
Copy link
Member

@ErikEJ So, would you recommend that we use compatibility level as the fundamental way to decide on behaviors? And, if we do that, are the compatibility levels consistent between SQL Server and Azure SQL?

We could then potential add sugar to the API to set the default compatibility level given a product version.

@ErikEJ
Copy link
Contributor

ErikEJ commented Apr 5, 2023

STRING_AGG and CONCAT_WS will actually work on Azure SQL even on a database with a compat level below 140. But maybe it is too theoretical.

are the compatibility levels consistent between SQL Server and Azure SQL?

Yes, they are.

@ErikEJ
Copy link
Contributor

ErikEJ commented Apr 5, 2023

would you recommend that we use compatibility level as the fundamental way to decide on behaviors?

Yes.

@roji
Copy link
Member Author

roji commented Apr 5, 2023

I looked at all this recently for #29427 (OPENJSON)... I think it should be sufficient to simply allow the user to determine the compatibility level (see this table). This sidesteps the complexity of SQL Server vs. Azure SQL, and also accounts for the fact that newer SQL Server versions can still be configured for an older compatibility level (with ALTER DATABASE). The downside is that users need to find out which compatibility level they want (i.e. instead just saying "I'm using SQL Server X, they need to look up the correct compat level value).

What do you guys think?

As long as we're discussing this, here are some additional design questions:

  • What should be the default level be? I'm hesitating between 14 (SQL Server 2017) and 15 (SQL Server 2019), though note that I'm note aware of a place where EF would distinguish between the two.
    • Note that we'll presumably bump up the default from time to time in major versions.
  • This will first and foremost be a context option (and a singleton option), affecting the SQL Server type mapping source and some method tranlators. However, as I wrote above, SQL Server also allows setting the compat level on the database.
    • Should we also add model metadata to allow configuring the compat level on the database?
    • If so, should it default to the context option?

@ajcvickers
Copy link
Member

What should be the default level be? I'm hesitating between 14 (SQL Server 2017) and 15 (SQL Server 2019), though note that I'm note aware of a place where EF would distinguish between the two.

From past experience, I'd argue the default level should be "latest". That is, we assume you're using the latest version and all features are available unless you explicitly opt in to an earlier version.

Should we also add model metadata to allow configuring the compat level on the database?

No. I think it's reasonable to expect all databases used by the application are at the same compat level.

@roji
Copy link
Member Author

roji commented Apr 5, 2023

What should be the default level be? I'm hesitating between 14 (SQL Server 2017) and 15 (SQL Server 2019), though note that I'm note aware of a place where EF would distinguish between the two.

From past experience, I'd argue the default level should be "latest". That is, we assume you're using the latest version and all features are available unless you explicitly opt in to an earlier version.

FWIW with Npgsql the default version lags ~2 years behind the latest - it's a balancing act between having most things just work well/efficiently by default, and not breaking users not on the absolute latest version. For example, I think there are very few users already on SQL Server 2022 - note that even Azure SQL doesn't support compatibility level 2016 yet (i.e. 2022 features).

Should we also add model metadata to allow configuring the compat level on the database?

No. I think it's reasonable to expect all databases used by the application are at the same compat level

This is more about whether we should allow users to set the compat level in migrations with ALTER DATABASE (just like any other ALTER DATABASE thing), and whether that should be related to the context option or not.

@ajcvickers
Copy link
Member

FWIW with Npgsql the default version lags ~2 years behind the latest - it's a balancing act between having most things just work well/efficiently by default, and not breaking users not on the absolute latest version. For example, I think there are very few users already on SQL Server 2022 - note that even Azure SQL doesn't support compatibility level 2016 yet (i.e. 2022 features).

This could work. The main thing is that the default is something that automatically changes, and a specific value set by the application will override it and not automatically change when the application is updated.

This is more about whether we should allow users to set the compat level in migrations with ALTER DATABASE (just like any other ALTER DATABASE thing), and whether that should be related to the context option or not.

We can discuss.

@roji
Copy link
Member Author

roji commented Apr 5, 2023

This could work. The main thing is that the default is something that automatically changes, and a specific value set by the application will override it and not automatically change when the application is updated.

Yep, agreed. The high-order bit for me is that new users get reasonably new features (and better perf relying on newer database features, e.g. better Contains via OPENJSON) without having to explicitly opt in or even think abuot anything. If you do need to explicitly set the version because you're using an old version, it becomes your responsibility to bump that when you upgrade your SQL Server version etc.

@ErikEJ
Copy link
Contributor

ErikEJ commented Apr 5, 2023

Azure SQL certainly supports 160 - it is just not the default for new databases (yet)

I think default should be 160 (latest), currently only EF Core users on 130 (2017) or lower will be affected.

@roji
Copy link
Member Author

roji commented Apr 5, 2023

Azure SQL certainly supports 160 - it is just not the default for new databases (yet)

@ErikEJ you're right, I misread.

roji added a commit to roji/efcore that referenced this issue Apr 5, 2023
@roji
Copy link
Member Author

roji commented Apr 7, 2023

Executive architect decision: we'll default to the latest compatibility level (currently 160, SQL Server 2022). In any case, we don't expect to use any super-new feature which would block users with slightly older databases.

roji added a commit to roji/efcore that referenced this issue Apr 14, 2023
roji added a commit to roji/efcore that referenced this issue Apr 14, 2023
roji added a commit to roji/efcore that referenced this issue Apr 19, 2023
roji added a commit to roji/efcore that referenced this issue Apr 20, 2023
roji added a commit to roji/efcore that referenced this issue Apr 20, 2023
roji added a commit to roji/efcore that referenced this issue Apr 20, 2023
roji added a commit to roji/efcore that referenced this issue Apr 26, 2023
@roji roji closed this as completed in 309d8f0 Apr 26, 2023
roji added a commit to roji/efcore that referenced this issue Apr 27, 2023
roji added a commit that referenced this issue Apr 27, 2023
@ajcvickers ajcvickers added closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. and removed needs-design labels Oct 11, 2023
@ajcvickers ajcvickers modified the milestones: Backlog, 8.0.0-preview4 Oct 11, 2023
@ajcvickers ajcvickers modified the milestones: 8.0.0-preview4, 8.0.0 Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-dbcontext area-sqlserver closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants