[5.0.1] Disable savepoints on SQL Server when MARS is enabled

[roji]

Fixes #23269

Additional context for Tactics

This is a tricky one, because it is a regression in 5.0 resulting in an exception, the root cause of which is new feature added in 5.0. We don't want to break this new feature in 5.0.1 for people who are using it successfully in 5.0.0. Therefore, we are:

• Reverting the behavior here to that of 3.1, but only in the specific case that is broken. (This kind of pivot isn't ideal, but this is a tricky case.)
• Rather than doing this silently, we also generate a warning in the logs when we do this.

Description

EF Core 5.0 adds transaction savepoint support, and automatically uses savepoints to make SaveChanges better when an external transaction is used. Unfortunately savepoints do not work in SQL Server when the Multiple Active Result Set (MARS) feature is enabled.

Customer Impact

Regression for users who have turned on MARS and are using external transactions receive an error when attempting to save changes.

How found

User-reported on 5.0

Test coverage

Added in this PR. We will also revisit coverage for MARS in general.

Regression?

Yes, from 3.1.

Risk

Low. There was already infrastructure in place for checking whether savepoints are supported by the database provider or not; this PR modifies that check for SQL Server to check if MARS is enabled. When savepoints aren't supported, we revert back to the previous, 3.1 behavior.

[roji]

Can we get an fwlink for the doc link? See https://github.com/dotnet/EntityFramework.Docs/pull/2869ץ

/cc @JeremyLikness

[JeremyLikness]

I assume this? https://go.microsoft.com/fwlink/?linkid=2149338 @roji

[roji]

Great, thanks @JeremyLikness!

[roji]

One more thing @JeremyLikness, can you please make it point specifically to the savepoints section of that page (https://docs.microsoft.com/en-us/ef/core/saving/transactions#savepoints)?

[roji]

@AndriySvyryd as discussed offline, TransactionSqlServerTest no longer disables MARS for the entire test suite. I looked at an attribute to not run a test if MARS is enabled/disabled, but ran into difficulties for retrieving the actual test class instance (in order to get the TestStore and check the connection string). However, there's currently exactly one inherited test which relies on MARS being disabled, so I just overrode and added that as a condition in the test for now. Let me know what you think.

[smitpatel]

Are we adding a warning log in patch?

[roji]

Are we adding a warning log in patch?

IIRC we agreed to do this when discussing, as the warning would be emitted in cases where 5.0.0 currently fails.

[ajcvickers]

I missed that the warning happens only when asking is savepoints are enabled, which we only do when we're about to use one.

@roji When we discussed this, it was my understanding that we would only generate the warning if you actually hit the case where we would now fail. Otherwise if people are treating warnings as errors, then this becomes a break for applications that are not currently broken. So I think we need to do this at the time that we're going to create a savepoint when MARS is on. We can't do it just because MARS is on, since if they never create their own transaction, or indeed never call SaveChanges, then there should be no issue, right?

I think we might need to discuss this again.

[ajcvickers]

Approved by Tactics for 5.0.1. Let's get the forward link done and agree on any wording before merging.