Efcore: Script-Migration command should begin/commit transactions

[Zysce]

Script-Migration command currently generate creation (or drop) script without checking objects existence.
If an error happens during script deployment (network, server error, ...), you can't redeploy generated script.

A check is done for table __EFMigrationsHistory :

IF OBJECT_ID(N'__EFMigrationsHistory') IS NULL
BEGIN
    CREATE TABLE [__EFMigrationsHistory] (
        [MigrationId] nvarchar(150) NOT NULL,
        [ProductVersion] nvarchar(32) NOT NULL,
        CONSTRAINT [PK___EFMigrationsHistory] PRIMARY KEY ([MigrationId])
    );
END;

GO

It should be the same for user tables, ex of generated creation script :

IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20170217083056_EFEntityInitialCreate')
BEGIN
    CREATE TABLE [SomeTable] (
        [Id] int NOT NULL IDENTITY,
        [Name] nvarchar(max) NOT NULL,
        CONSTRAINT [PK_SomeTable] PRIMARY KEY ([Id])
    );
END;

GO

The modification should be done in creation script (check if not exists) and drop script (check if exists).

[ErikEJ]

Did you use the

-Idempotent

command swith?

https://docs.microsoft.com/en-us/ef/core/miscellaneous/cli/powershell

[Zysce]

Yes,
IdemPotent check if __EFMigrationsHistory table contains migration row.
IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20170217083056_EFSomeTableInitialCreate')

However, the row with MigrationId = '20170217083056_EFSomeTableInitialCreate' is added at the end of the script.

If an error occurs on script deployment before the row is inserted, you cannot redeploy the script since the table __EFMigrationsHistory does not contains migration row.

[rowanmiller]

When we execute migrations from tools/code, we use transactions for most operations (though there are some DDL operations that can't be executed in a transaction). @bricelam should we be emitting BEGIN TRANSACTION etc. in the script we generate?

[bricelam]

@rowanmiller I think it depends on whether most tools (e.g. MSDeploy, SSMS, SSDT, etc.) give you the option to run it inside a transaction...

[ajcvickers]

We discussed this at length and have decided that at this time we will not start generating any transactions code in the script. We want to leave the scripts generated to be useful in a variety of scenarios, some of which don't involve use of transactions, and some of which will do transactions themselves separately from the script. We considered emitting a comment saying this at the start of the script, but for now we don't think there is enough value in doing that. We will re-consider this if there is more feedback and/or specific pit-of-failure scenarios that we see people falling into.

[bricelam]

Based on feedback, we're re-considering whether the script should include the BEGIN TRANSACTION and COMMIT statements corresponding to where transactions occur during Update-Database.

[jonsagara]

Hi Brice,

I see this is attached to milestone 3.0.0. Will this definitely make it into EF Core 3.0?

Thanks,

Jon

[bricelam]

@jonsagara Our 3.0 planning happend before I took on porting EF6 to .NET Core. I need to do another pass of punting my 3.0 issues. That said, I still hope I can get to this one during the 3.0 milestone.

[bricelam]

I recently discovered that this is important for suppressTransaction: true. Without the BEGIN and END statements, you don't know which parts of the script are supposed to run outside of a transaction. Today, Web Publish wraps the whole script inside a transaction causing these operations to fail.

[bricelam]

I have a rough draft of this in bricelam:commit. Unfortunately, I have a lot of other EF Core 3.0 and EF 6.3 work that I need to finish first.

[jonsagara]

Hi @bricelam, did this make it into EF Core 3.0?

Edit: Just saw the label "punted-for-3.0" - that means this will come post-3.0, correct?

[bricelam]

Correct. EF6 was a lot more work than I anticipated

[ChristopherHaws]

This is a hacked together solution that I am using for my project. It only works for idempotent sql server script migrations, but it seems to be working for me ok. Would be nice to have this be built in.

[System.Diagnostics.CodeAnalysis.SuppressMessage("Usage", "EF1001:Internal EF Core API usage.", Justification = "The built in migrator doesn't wrap the script in a transaction...")]
public class TransactionWrappedMigrator : Migrator
{
    public TransactionWrappedMigrator(
        IMigrationsAssembly migrationsAssembly,
        IHistoryRepository historyRepository,
        IDatabaseCreator databaseCreator,
        IMigrationsSqlGenerator migrationsSqlGenerator,
        IRawSqlCommandBuilder rawSqlCommandBuilder,
        IMigrationCommandExecutor migrationCommandExecutor,
        IRelationalConnection connection,
        ISqlGenerationHelper sqlGenerationHelper,
        ICurrentDbContext currentContext,
        IDiagnosticsLogger<DbLoggerCategory.Migrations> logger,
        IDiagnosticsLogger<DbLoggerCategory.Database.Command> commandLogger,
        IDatabaseProvider databaseProvider)
        : base(migrationsAssembly, historyRepository, databaseCreator, migrationsSqlGenerator, rawSqlCommandBuilder, migrationCommandExecutor, connection, sqlGenerationHelper, currentContext, logger, commandLogger, databaseProvider)
    {
    }

    public override String GenerateScript(String fromMigration = null, String toMigration = null, Boolean idempotent = false) => $@"
-- Since the migrations run in batches, we cant wrap the entire migration in a try/catch.
-- Instead, set the query to automaticly rollback on failure and make sure that the transaction state has no errors for every migration.
SET XACT_ABORT ON;
BEGIN TRANSACTION;
{base.GenerateScript(fromMigration, toMigration, idempotent).Replace(
    "IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] =",
    $"DECLARE @XACT_STATE SMALLINT = XACT_STATE();{Environment.NewLine}IF @XACT_STATE = 1 AND NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] =")}

IF XACT_STATE() = 1
BEGIN
    COMMIT TRANSACTION;
    PRINT 'Transaction committed.';
END;";
}

services.AddDbContext<ApplicationContext>(options =>
{
    options.UseSqlServer(this.configuration.GetConnectionString("DefaultConnection"));
    options.ReplaceService<IMigrator, TransactionWrappedMigrator>();
});

[esquino]

@ChristopherHaws
How do you get around each batch having a GO that resets the XACT_STATE()? I tried this approach but in order for it to work I had to remove the GO between statements.

[ChristopherHaws]

@esquino It seems to work for me. I just created a basic migration that inserts invalid data into a table and it rolled back the entire script with GO statements. Here is the script it generates. If I run this in a new database, no tables are created.

SET XACT_ABORT ON;
BEGIN TRANSACTION;

IF OBJECT_ID(N'[__EFMigrationsHistory]') IS NULL
BEGIN
    CREATE TABLE [__EFMigrationsHistory] (
        [MigrationId] nvarchar(150) NOT NULL,
        [ProductVersion] nvarchar(32) NOT NULL,
        CONSTRAINT [PK___EFMigrationsHistory] PRIMARY KEY ([MigrationId])
    );
END;

GO

IF XACT_STATE() = 1 AND NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20200409230545_Initial')
BEGIN
    CREATE TABLE dbo.Blog (
        BlogId INT NOT NULL PRIMARY KEY,
        [Name] NVARCHAR(250) NOT NULL
    );
END;

GO

IF XACT_STATE() = 1 AND NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20200409230545_Initial')
BEGIN
    INSERT INTO dbo.Blog
    (BlogId, Name) VALUES
    (1,N'Test Blog');
END;

GO

IF XACT_STATE() = 1 AND NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20200409230545_Initial')
BEGIN
    INSERT INTO [__EFMigrationsHistory] ([MigrationId], [ProductVersion])
    VALUES (N'20200409230545_Initial', N'3.1.2');
END;

GO

IF XACT_STATE() = 1 AND NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20200409230613_Second')
BEGIN
    INSERT INTO dbo.Blog
    (BlogId, Name) VALUES
    (1,N'Test Blog');
END;

GO

IF XACT_STATE() = 1 AND NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20200409230613_Second')
BEGIN
    INSERT INTO [__EFMigrationsHistory] ([MigrationId], [ProductVersion])
    VALUES (N'20200409230613_Second', N'3.1.2');
END;

GO

IF XACT_STATE() = 1
BEGIN
    COMMIT TRANSACTION;
    PRINT 'Transaction committed.';
END;

GO

[esquino]

I just tested the example you gave. If [__EFMigrationsHistory] exists. The transaction Inserts in to [__EFMigrationsHistory] still occur and are not rolled back.

Here are the results.

(1 row affected)

(1 row affected)
Msg 2627, Level 14, State 1, Line 44
Violation of PRIMARY KEY constraint 'PK__Blog__54379E30668D031E'. Cannot insert duplicate key in object 'dbo.Blog'. The duplicate key value is (1).

(1 row affected)

Completion time: 2020-04-10T13:29:39.8621100-05:00

[ChristopherHaws]

@esquino I honestly don't understand why, but it looks like evaluating XACT_STATE() with an AND in the IF statement doesn't work, however it does work if it is the only thing evaluated in the if statement or if you set it as a variable and evaluate that. I modified my original post's code so that it outputs like this now and it does appear to be working:

DECLARE @XACT_STATE SMALLINT = XACT_STATE();
IF @XACT_STATE = 1 AND NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20200409230613_Second')
BEGIN
    -- Migrate Data
END;

[bricelam]

I looked into MSDeploy (used by Web Publish) and it's safe to include the BEGIN/COMMIT statements in the script. I was worried we might run into nested transaction errors because it automatically wraps the entire script inside a transaction, but it simply ignores the statements. I filed dotnet/sdk#12676 to, once this is implemented, disable the automatic transaction and start using the script's transactions instead.

[bricelam]

Design meeting notes:

• Start doing this by default
• Add a --no-transactions option to revert to the previous behavior in the off chance that it breaks existing workflows

[woeterman94]

Any update on this?

[bricelam]

@woeterman94 Will be available in 5.0.0-rc.1