[release/9.0] Acquire the migrations database lock in a transaction. #34489

AndriySvyryd · 2024-08-21T05:45:49Z

Description

The migrations database lock introduced in #34115 doesn't work with providers that use a transaction-based lock implementation.

With this the lock can be acquired inside the transaction and all migrations are executed in a single transaction (when possible) to allow them to be covered by a single lock.

Additionally, the non-transaction based locks will be reacquired when retrying on a transient failure.

Customer impact

Currently, the migrations database lock does not work on some providers. And they aren't reacquired when retrying on a transient failure for the other providers.

How found

Reported by a provider maintainer.

Regression

No, new feature.

Testing

Tests added.

Risk

Medium. It's a breaking change for providers from rc1, but it's unlikely that the existing implementations were working correctly.

AndriySvyryd · 2024-08-21T05:46:58Z

@roji Let me know if this is an acceptable design

roji

@AndriySvyryd thanks, overall looks to me... There are some slight things I don't understand about flowing the state across Migrator/MigrationCommandExecutor, but that may just be the draft state of this PR. Otherwise see comments below.

I'm personally not seeing the value of the separation of Migrator vs. MigrationCommandExecutor - the separation may actually add more complexity/cognitive load rather than simplifying (especially now that transaction management and locking are also managed in Migrator...). Something to consider for the future...

src/EFCore.Relational/Migrations/Internal/Migrator.cs

src/EFCore.Relational/Migrations/Internal/MigrationCommandExecutor.cs

src/EFCore.Relational/Migrations/Internal/Migrator.cs

src/EFCore.Relational/Migrations/Internal/MigrationCommandExecutor.cs

AndriySvyryd · 2024-08-21T20:56:01Z

I'm personally not seeing the value of the separation of Migrator vs. MigrationCommandExecutor - the separation may actually add more complexity/cognitive load rather than simplifying (especially now that transaction management and locking are also managed in Migrator...). Something to consider for the future...

This is mainly to reduce code duplication as it's also called from RelationalDatabaseCreator.CreateTables. However, the lock isn't taken there, since the migrations history table shouldn't be created.

roji · 2024-08-21T20:59:22Z

I'm personally not seeing the value of the separation of Migrator vs. MigrationCommandExecutor - the separation may actually add more complexity/cognitive load rather than simplifying (especially now that transaction management and locking are also managed in Migrator...). Something to consider for the future...

This is mainly to reduce code duplication as it's also called from RelationalDatabaseCreator.CreateTables. However, the lock isn't taken there, since the migrations history table shouldn't be created.

OK. This is academic at the moment in any case, I'm not actually proposing we do anything for 9.0...

roji

I think it's getting closer @AndriySvyryd but I can still see some trouble (or stuff I don't understand), take a look.

src/EFCore.Relational/Migrations/Internal/Migrator.cs

roji · 2024-08-29T11:36:04Z

src/EFCore.Relational/Migrations/HistoryRepository.cs

+    ///     any release. You should only use it directly in your code with extreme caution and knowing that
+    ///     doing so can result in application failures when updating to a new Entity Framework Core release.
+    /// </summary>
+    public abstract LockReleaseBehavior LockReleaseBehavior { get; }


Naming: I'd love to just call this LockingScope, but it's true that for Explicit we still take it ourselves, so it seems to indeed apply only to the release part...

@dotnet/efteam API review question

Am also OK with the current naming if we don't get around to discussing this (same with the other naming proposals below).

Ok. We won't have any meetings for this, so if anyone else in @dotnet/efteam has an opinion - speak up

src/EFCore.Relational/Migrations/Internal/Migrator.cs

roji · 2024-08-29T11:44:06Z

src/EFCore.Relational/Migrations/IMigrationsDatabaseLock.cs

+    ///     <see langword="null"/> if there's no current transaction.
+    /// </param>
+    /// <returns>An object that can be disposed to release the lock.</returns>
+    IMigrationsDatabaseLock Refresh(bool connectionReopened, bool? transactionRestarted)


This feels like it might belong in the callsites, rather than as part of the lock implementation - but I'm not sure... The behavior here seems universal for the various LockReleaseBehavior values, i.e. I don't see an implementation of IMigrationsDatabaseLock ever needing to change it - do you have a specific case in mind?

If we're not sure, it may be better to not prematurely expose this hook before we have a request, or at least flag it as pubternal...

Otherwise, we may just want to call this Reacquire() instead of Refresh(), for consistency (or ReacquireIfNeeded())

The main reason it's here is because I don't want to expose HistoryRepository to the callsites as that would allow them to acquire new locks.

The secondary benefit is having an escape hatch in case there's a behavior that can't be currently expressed in LockReleaseBehavior. Any request for this would be too late to incorporate into 9.0.0

@dotnet/efteam API Review

The main reason it's here is because I don't want to expose HistoryRepository to the callsites as that would allow them to acquire new locks.

OK, that may be a bit over-defensive for very low-level code that we maintain (or possibly a provider).

The secondary benefit is having an escape hatch in case there's a behavior that can't be currently expressed in LockReleaseBehavior. Any request for this would be too late to incorporate into 9.0.0

I'm a bit doubtful about the extent to which actual custom logic could be implemented in the current design... Simply because it's not really possible to override Migrator and MigrationCommandExecutor behavior which determines exactly where the lock gets taken/released...

Anyway, I'm OK with the design as-is. I'd maybe slightly prefer putting pubternal/experimental on this as if feels like we may want to refactor/improve the design later, but I'll defer to you.

src/EFCore.Sqlite.Core/Migrations/Internal/SqliteHistoryRepository.cs

src/EFCore.Relational/Migrations/Internal/MigrationCommandExecutor.cs

roji · 2024-08-29T12:24:54Z

src/EFCore.Relational/Migrations/IMigrationsDatabaseLock.cs

+    ///     <see langword="null"/> if there's no current transaction.
+    /// </param>
+    /// <returns>An object that can be disposed to release the lock.</returns>
+    IMigrationsDatabaseLock Refresh(bool connectionReopened, bool? transactionRestarted)


nit: transactionStarted rather than restarted?

I don't think this name accurately conveys the tri-value aspect.

@dotnet/efteam API Review

I don't think this name accurately conveys the tri-value aspect.

Maybe, but transactionRestarted is also inaccurate for the case where we're just starting out and there nothing to re-start.

But never mind, this isn't important and it's a bit late for minor naming discussions...

src/EFCore.Relational/Migrations/Internal/MigrationCommandExecutor.cs

src/EFCore.Relational/Migrations/Internal/Migrator.cs

Reacquire the lock when retrying. Execute all migrations in a single transaction. Fixes #34439

Fixes #34569

AndriySvyryd requested a review from roji August 21, 2024 05:45

roji reviewed Aug 21, 2024

View reviewed changes

AndriySvyryd force-pushed the Issue34439 branch 2 times, most recently from e41cba4 to 444fb34 Compare August 23, 2024 01:44

AndriySvyryd marked this pull request as ready for review August 23, 2024 01:44

roji self-assigned this Aug 23, 2024

AndriySvyryd added providers-beware tell-mode labels Aug 23, 2024

AndriySvyryd requested a review from artl93 August 23, 2024 19:38

AndriySvyryd force-pushed the Issue34439 branch from 444fb34 to 49f3b71 Compare August 23, 2024 19:40

artl93 approved these changes Aug 23, 2024

View reviewed changes

AndriySvyryd changed the title ~~Acquire the migrations database lock in a transaction.~~ [release/9.0] Acquire the migrations database lock in a transaction. Aug 24, 2024

roji requested changes Aug 29, 2024

View reviewed changes

AndriySvyryd force-pushed the Issue34439 branch 2 times, most recently from 378c56a to e27f07a Compare August 30, 2024 04:54

roji approved these changes Aug 31, 2024

View reviewed changes

AndriySvyryd added 4 commits September 2, 2024 17:14

Acquire the migrations database lock in a transaction.

953a4d9

Reacquire the lock when retrying. Execute all migrations in a single transaction. Fixes #34439

Review feedback

15dbb8c

More review feedback

af0968b

Fixes #34569

Add a warning for user transactions.

3b884d9

AndriySvyryd force-pushed the Issue34439 branch from 5c7c4c5 to 3b884d9 Compare September 3, 2024 00:14

AndriySvyryd enabled auto-merge (squash) September 3, 2024 00:15

AndriySvyryd merged commit 972a50a into release/9.0 Sep 3, 2024
7 checks passed

AndriySvyryd deleted the Issue34439 branch September 3, 2024 01:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[release/9.0] Acquire the migrations database lock in a transaction. #34489

[release/9.0] Acquire the migrations database lock in a transaction. #34489

AndriySvyryd commented Aug 21, 2024 •

edited

Loading

AndriySvyryd commented Aug 21, 2024

roji left a comment

AndriySvyryd commented Aug 21, 2024

roji commented Aug 21, 2024

roji left a comment

roji Aug 29, 2024

AndriySvyryd Aug 29, 2024

roji Aug 30, 2024 •

edited

Loading

AndriySvyryd Aug 30, 2024 •

edited

Loading

roji Aug 29, 2024

AndriySvyryd Aug 29, 2024 •

edited

Loading

AndriySvyryd Aug 29, 2024

roji Aug 30, 2024

roji Aug 29, 2024

AndriySvyryd Aug 29, 2024

AndriySvyryd Aug 29, 2024

roji Aug 30, 2024

[release/9.0] Acquire the migrations database lock in a transaction. #34489

[release/9.0] Acquire the migrations database lock in a transaction. #34489

Conversation

AndriySvyryd commented Aug 21, 2024 • edited Loading

Description

Customer impact

How found

Regression

Testing

Risk

AndriySvyryd commented Aug 21, 2024

roji left a comment

Choose a reason for hiding this comment

AndriySvyryd commented Aug 21, 2024

roji commented Aug 21, 2024

roji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roji Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

AndriySvyryd Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndriySvyryd Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndriySvyryd commented Aug 21, 2024 •

edited

Loading

roji Aug 30, 2024 •

edited

Loading

AndriySvyryd Aug 30, 2024 •

edited

Loading

AndriySvyryd Aug 29, 2024 •

edited

Loading