efcore Performance decrease (10 second delay) in database migration 8.0.0 vs 8.0.3

Hi,

After upgrading from 8.0.0 to 8.0.3, get a large performance decrease during database creation.

https://github.com/EvgenyMuryshkin/EF8PPerf

EF 9 also has the same issue

Include provider and version information

.NET 8/.NET 9 EF 8.0.0 => 8.0.3, 9.0.0-preview.2.24128.4 Windows 11 Pro VS 2022 17.9.4/17.10.0 Preview 2.0

Thank you

Mar 25 '24 22:03 EvgenyMuryshkin

@EvgenyMuryshkin looks like a duplicate of #33176, which has already been fixed for 8.0.4. Can you please try the workaround in this comment and confirm?

Mar 26 '24 07:03 roji

@roji nope, did not work. I think my issue is not model creation, but database creation. Model is already created

Mar 26 '24 07:03 EvgenyMuryshkin

This doesn't repro for me (MacOS Sonoma):

I'm noticing that EF upgraded patch versions of SqlClient between 8.0.0 (SqlClient 5.1.1) and 8.0.3 (SqlClient 5.1.5). Can you try to take a dependency on SqlClient 5.1.1 when using EF Core 8.0.0 to see if the problem comes form there? I'm thinking possibly of #7283 or similar.

Mar 26 '24 08:03 roji

@roji EF 8.0.1 fast, 8.0.2 slow.

with 8.0.0 5.1.1 - fast 5.1.2 - slow

Mar 26 '24 09:03 EvgenyMuryshkin

Thanks for confirming!

The 5.1.2 release notes are here, the culprit may be https://github.com/dotnet/SqlClient/pull/1983. This may very well be a dup of #7283.

/cc @David-Engel @cheenamalhotra

Mar 26 '24 09:03 roji

Have you tried setting ConnectRetryCount=0 on the connection string to disable transient fault handling?

Mar 27 '24 05:03 cheenamalhotra

@cheenamalhotra This should not be needed. Issue https://github.com/dotnet/efcore/issues/7283 goes into this in detail.

Mar 27 '24 09:03 ajcvickers

/cc @SamMonoRT

Mar 27 '24 09:03 ajcvickers

@cheenamalhotra in addition to #7283, see https://github.com/dotnet/SqlClient/issues/29 and https://github.com/dotnet/SqlClient/pull/463 on the SqlClient side where this was discussed at length (especially why the connection string-based approach is insufficient here).

Mar 27 '24 10:03 roji

Have you tried setting ConnectRetryCount=0 on the connection string to disable transient fault handling?

it actually worked, thanks :) for 8.0.3 and 9.0.0 as well

Mar 27 '24 10:03 EvgenyMuryshkin

Interestingly, this test is not failing: https://github.com/dotnet/SqlClient/blob/main/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectivityTests/ConnectivityTest.cs#L364

Mar 27 '24 11:03 ErikEJ

it actually worked, thanks :)

The problem with setting ConnectRetryCount=0 in your actual connection string is that is disables connection retrying everywhere, not just in the database existence check.

Mar 27 '24 11:03 roji

@cheenamalhotra in addition to #7283, see dotnet/SqlClient#29 and dotnet/SqlClient#463 on the SqlClient side where this was discussed at length (especially why the connection string-based approach is insufficient here).

Thanks for the context, I think we should also make sure FailFast is supported for OpenAsync() as well, now that Transient fault handling is enabled.. This probably skipped our minds and we didn't realize before.

The problem with setting ConnectRetryCount=0 in your actual connection string is that is disables connection retrying everywhere, not just in the database existence check.

Yes, that is true.

For @roji and @ajcvickers Can you confirm the reason why dotnet/SqlClient#29 regressed was due to the change we made on OpenAsync() API to enable Transient Fault Handling? And that Open() API works as expected with Fail fast option defined?

Mar 27 '24 19:03 cheenamalhotra

Interestingly, this test is not failing: https://github.com/dotnet/SqlClient/blob/main/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/ConnectivityTests/ConnectivityTest.cs#L364

Note that it's only testing Open() and not OpenAsync()..

Mar 27 '24 19:03 cheenamalhotra

But does OpenAsync support the new overload?

Mar 27 '24 19:03 ErikEJ

Ah, I found: https://github.com/dotnet/SqlClient/issues/615

Mar 27 '24 20:03 ErikEJ

Confirmed the regression is for async only.

Mar 31 '24 10:03 ajcvickers

Hi all,

8.0.4 looks good, will run our full test suite to confirm shortly.

Thanks,

Apr 11 '24 23:04 EvgenyMuryshkin

@EvgenyMuryshkin I don't think we've done anything on the EF side (unless I've missed something) - we're still waiting for support to be added to SqlClient to programmatically opt-out of the retrying behavior (that's https://github.com/dotnet/SqlClient/pull/2433).

Apr 12 '24 08:04 roji

@roji this is weird. I restarted visual studio and now it is back to 10 second delay... false alarm, sorry about.

Apr 12 '24 10:04 EvgenyMuryshkin

Note: the plan on the SqlClient side is to revert the retrying behavior for OpenAsync for 5.0 (comment), and re-introduce it in SqlClient 6.0.

Opened #33741 to track eventually using the opt-out when we switch to SqlClient 6.0. We can keep this issue open just to track SqlClient's reverting of the async retry logic in 5, so that people can find it.

May 17 '24 13:05 roji

In other words, this will be fixed in 5.2.1

May 17 '24 13:05 ErikEJ

Verified this is fixed with SqlClient 5.2.1.

Jun 04 '24 18:06 ajcvickers

@ajcvickers which version of EF will get this fix? 8.0.7?

Jun 05 '24 00:06 EvgenyMuryshkin

@EvgenyMuryshkin You can just add an explicit PackageReference to 5.2.1

Jun 05 '24 05:06 ErikEJ

@ErikEJ I tried but I got lots of failed tests due to transient connection failures, will try to create repro soon

Jun 05 '24 06:06 EvgenyMuryshkin

Re-opening to consider updating the 8.0 dependency, which is currently at 5.1.5.

Jun 05 '24 09:06 ajcvickers

@ajcvickers isn't SqlClient planning to revert their change in the 5.1 branch? I haven't been following closely...

Jun 05 '24 09:06 roji

@roji Not sure. Will try to pull together the relevant info for the team to discuss.

Jun 05 '24 09:06 ajcvickers

@ajcvickers @roji This will be fixed in 5.1.6 https://github.com/dotnet/SqlClient/projects/20

Jun 05 '24 10:06 ErikEJ