Efcore: Inefficient idempotent script generation in Migrator.GenerateScript()

[MaxG117]

The HistoryRepository class contains the GetBeginIf[Not]ExistsScript and GetEndIfScript methods. I think the purpose of these methods is to allow multiple statements to execute between the BEGIN and END blocks of the script. However, the Migrator.GenerateScript method ignores this purpose, and idempotent scripts end up placing each command inside its own BEGIN...END block instead of placing all of the commands from a single migration inside a single BEGIN...END block.

This is because the loop that walks through each command in a migration is in the wrong place. The following code:

```C#
foreach (var command in GenerateUpSql(migration))
{
if (idempotent)
{
builder.AppendLine(_historyRepository.GetBeginIfNotExistsScript(migration.GetId()));
using (builder.Indent())
{
builder.AppendLines(command.CommandText);
}
builder.AppendLine(_historyRepository.GetEndIfScript());
}
...
}

should have probably been:

```C#
if (idempotent)
{
    builder.AppendLine(_historyRepository.GetBeginIfNotExistsScript(migration.GetId()));
    using (builder.Indent())
    {
        foreach (var command in GenerateUpSql(migration))
        {
            builder.AppendLines(command.CommandText);
        }
    }
    builder.AppendLine(_historyRepository.GetEndIfScript());
}
...

Same thing for the loop around GenerateDownSql.

Additionally, GenerateScript assumes that only a single Indent is needed to make the script pretty, but my Begin block requires more than one indent. It would be nice if GetBeginIf[Not]ExistsScript and GetEndIfScript could access the IndentedStringBuilder.

[ajcvickers]

/cc @bricelam

[bricelam]

This is by design. BEGIN and END statements can't be separated by GO, so we re-issue the IF..BEGIN..END for each GO block. Issue #3928 would make it a lot prettier.

[MaxG117]

I'd think "GO" should be executed after END, not between BEGIN and END. I think you've done that in your prototype by suppressing the transaction. It's not immediately clear to me how your changes affect other providers though.

[ajcvickers]

@MaxG117 GO is executed after END. If this isn't the case for you, can you explain more or provide a repro that demonstrates this.

[MaxG117]

@ajcvickers I was just replying to Brice's comment, I did not say that GO is executed in the wrong place for me.

Here's what I'm getting at. The Can_generate_idempotent_up_scripts() unit test in EFCore.SqlServer.FunctionalTests project expects the following SQL to be generated for the migration whose MigrationId = 00000000000001_Migration1:

IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'00000000000001_Migration1')
BEGIN
    CREATE TABLE [Table1] (
        [Id] int NOT NULL,
        CONSTRAINT [PK_Table1] PRIMARY KEY ([Id])
    );
END;

GO

IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'00000000000001_Migration1')
BEGIN
    INSERT INTO [__EFMigrationsHistory] ([MigrationId], [ProductVersion])
    VALUES (N'00000000000001_Migration1', N'7.0.0-test');
END;

GO

My understanding is that multiple statements may be executed between BEGIN and END, so I don't understand why a single migration is split into two separate "IF NOT EXISTS ... BEGIN ... END; GO" blocks. Could the above SQL not be replaced with something like this instead?

IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'00000000000001_Migration1')
BEGIN
    CREATE TABLE [Table1] (
        [Id] int NOT NULL,
        CONSTRAINT [PK_Table1] PRIMARY KEY ([Id])
    );
    INSERT INTO [__EFMigrationsHistory] ([MigrationId], [ProductVersion])
    VALUES (N'00000000000001_Migration1', N'7.0.0-test');
END;

GO

?

[bricelam]

EF6 assumed that every statement could be combined into a single batch (batches are separated by GO) and put everything for a migration inside the BEGIN..END block, but there are several Transact-SQL statements which are required to be at the beginning of a batch and even some that are required to be the only statement in a batch.

EF Core generates a script that represents exactly how the migrations are executed at runtime (e.g. via dbContext.Database.Migrate()) with one statement per batch. This yields greater confidence that the script will work if dotnet ef database update worked. But even with this approach, there are still some issues we need to fix. (like #12911 and #7681)

Issue #3928 is about updating Migrations to understand when Transact-SQL statements can be batched together. This would put more statements inside the BEGIN..END block and result in fewer IF statements (possibly just one) per migration.

[MaxG117]

Ok, this makes sense now.

The documentation for the IHistoryRepository GetBeginIf[Not]ExistsScript and GetEndIfScript does not make it clear that even though these methods are used to BEGIN and END a "block", which to me implies multiple statements, the Migrator.GenerateScript method does not treat them as wrappers around a "block" of statements, but rather as wrappers around a single statement. That's why I thought there was a bug in the Migrator.GenerateScript method.

I'm closing this, and you can reopen or open another if you think the documentation for those methods should be improved.