diff --git a/doc/releasenotes/14_0_0_summary.md b/doc/releasenotes/14_0_0_summary.md index 0e9fbdf02f9..8a89deb9280 100644 --- a/doc/releasenotes/14_0_0_summary.md +++ b/doc/releasenotes/14_0_0_summary.md @@ -64,9 +64,13 @@ Example: vtctlclient ApplySchema -skip_preflight -ddl_strategy='vitess' -sql "alter table my_table add column my_val int not null default 0" commerce ``` +### --singleton-context and REVERT migrations + +It is now possible to submit a migration with `--singleton-context` strategy flag, while there's a pending (queued or running) `REVERT` migration that does not have a `--singleton-context` flag. + #### Behavior changes -- `vtctl ApplySchema -uuid_list='...'` now rejects a migration if an existing migration has the same UUID but with different `migration_context`. +- `vtctl ApplySchema --uuid_list='...'` now rejects a migration if an existing migration has the same UUID but with different `migration_context`. ### Table lifecycle diff --git a/go/test/endtoend/onlineddl/singleton/onlineddl_singleton_test.go b/go/test/endtoend/onlineddl/singleton/onlineddl_singleton_test.go index 870b40a9347..2fd2da7d5b9 100644 --- a/go/test/endtoend/onlineddl/singleton/onlineddl_singleton_test.go +++ b/go/test/endtoend/onlineddl/singleton/onlineddl_singleton_test.go @@ -45,8 +45,8 @@ var ( cell = "zone1" schemaChangeDirectory = "" tableName = `stress_test` - onlineSingletonDDLStrategy = "online -singleton" - onlineSingletonContextDDLStrategy = "online -singleton-context" + onlineSingletonDDLStrategy = "online --singleton" + onlineSingletonContextDDLStrategy = "online --singleton-context" createStatement = ` CREATE TABLE stress_test ( id bigint(20) not null, @@ -75,6 +75,9 @@ var ( dropStatement = ` DROP TABLE stress_test ` + dropNonexistentTableStatement = ` + DROP TABLE IF EXISTS t_non_existent + ` multiDropStatements = `DROP TABLE IF EXISTS t1; DROP TABLE IF EXISTS t2; DROP TABLE IF EXISTS t3;` ) @@ -151,21 +154,21 @@ func TestSchemaChange(t *testing.T) { // CREATE t.Run("CREATE TABLE", func(t *testing.T) { // The table does not exist - uuid := testOnlineDDLStatement(t, createStatement, onlineSingletonDDLStrategy, "vtgate", "", "", false) + uuid := testOnlineDDLStatement(t, createStatement, onlineSingletonDDLStrategy, "vtgate", "", "", "", false) uuids = append(uuids, uuid) onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete) checkTable(t, tableName, true) }) t.Run("revert CREATE TABLE", func(t *testing.T) { // The table existed, so it will now be dropped (renamed) - uuid := testRevertMigration(t, uuids[len(uuids)-1], "vtgate", "", false) + uuid := testRevertMigration(t, uuids[len(uuids)-1], "vtgate", onlineSingletonDDLStrategy, "", "", false) uuids = append(uuids, uuid) onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete) checkTable(t, tableName, false) }) t.Run("revert revert CREATE TABLE", func(t *testing.T) { // Table was dropped (renamed) so it will now be restored - uuid := testRevertMigration(t, uuids[len(uuids)-1], "vtgate", "", false) + uuid := testRevertMigration(t, uuids[len(uuids)-1], "vtgate", onlineSingletonDDLStrategy, "", "", false) uuids = append(uuids, uuid) onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete) checkTable(t, tableName, true) @@ -173,19 +176,19 @@ func TestSchemaChange(t *testing.T) { var throttledUUID string t.Run("throttled migration", func(t *testing.T) { - throttledUUID = testOnlineDDLStatement(t, alterTableThrottlingStatement, "gh-ost -singleton --max-load=Threads_running=1", "vtgate", "hint_col", "", false) + throttledUUID = testOnlineDDLStatement(t, alterTableThrottlingStatement, "gh-ost --singleton --max-load=Threads_running=1", "vtgate", "", "hint_col", "", false) onlineddl.CheckMigrationStatus(t, &vtParams, shards, throttledUUID, schema.OnlineDDLStatusRunning) }) t.Run("failed singleton migration, vtgate", func(t *testing.T) { - uuid := testOnlineDDLStatement(t, alterTableThrottlingStatement, "gh-ost -singleton --max-load=Threads_running=1", "vtgate", "hint_col", "rejected", true) + uuid := testOnlineDDLStatement(t, alterTableThrottlingStatement, "gh-ost --singleton --max-load=Threads_running=1", "vtgate", "", "hint_col", "rejected", true) assert.Empty(t, uuid) }) t.Run("failed singleton migration, vtctl", func(t *testing.T) { - uuid := testOnlineDDLStatement(t, alterTableThrottlingStatement, "gh-ost -singleton --max-load=Threads_running=1", "vtctl", "hint_col", "rejected", true) + uuid := testOnlineDDLStatement(t, alterTableThrottlingStatement, "gh-ost --singleton --max-load=Threads_running=1", "vtctl", "", "hint_col", "rejected", true) assert.Empty(t, uuid) }) t.Run("failed revert migration", func(t *testing.T) { - uuid := testRevertMigration(t, throttledUUID, "vtgate", "rejected", true) + uuid := testRevertMigration(t, throttledUUID, "vtgate", onlineSingletonDDLStrategy, "", "rejected", true) assert.Empty(t, uuid) }) t.Run("terminate throttled migration", func(t *testing.T) { @@ -195,20 +198,20 @@ func TestSchemaChange(t *testing.T) { onlineddl.CheckMigrationStatus(t, &vtParams, shards, throttledUUID, schema.OnlineDDLStatusFailed) }) t.Run("successful gh-ost alter, vtctl", func(t *testing.T) { - uuid := testOnlineDDLStatement(t, alterTableTrivialStatement, "gh-ost -singleton", "vtctl", "hint_col", "", false) + uuid := testOnlineDDLStatement(t, alterTableTrivialStatement, "gh-ost --singleton", "vtctl", "", "hint_col", "", false) onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete) onlineddl.CheckCancelMigration(t, &vtParams, shards, uuid, false) onlineddl.CheckRetryMigration(t, &vtParams, shards, uuid, false) }) t.Run("successful gh-ost alter, vtgate", func(t *testing.T) { - uuid := testOnlineDDLStatement(t, alterTableTrivialStatement, "gh-ost -singleton", "vtgate", "hint_col", "", false) + uuid := testOnlineDDLStatement(t, alterTableTrivialStatement, "gh-ost --singleton", "vtgate", "", "hint_col", "", false) onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete) onlineddl.CheckCancelMigration(t, &vtParams, shards, uuid, false) onlineddl.CheckRetryMigration(t, &vtParams, shards, uuid, false) }) t.Run("successful online alter, vtgate", func(t *testing.T) { - uuid := testOnlineDDLStatement(t, alterTableTrivialStatement, onlineSingletonDDLStrategy, "vtgate", "hint_col", "", false) + uuid := testOnlineDDLStatement(t, alterTableTrivialStatement, onlineSingletonDDLStrategy, "vtgate", "", "hint_col", "", false) uuids = append(uuids, uuid) onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete) onlineddl.CheckCancelMigration(t, &vtParams, shards, uuid, false) @@ -217,7 +220,7 @@ func TestSchemaChange(t *testing.T) { }) t.Run("revert ALTER TABLE, vttablet", func(t *testing.T) { // The table existed, so it will now be dropped (renamed) - uuid := testRevertMigration(t, uuids[len(uuids)-1], "vttablet", "", false) + uuid := testRevertMigration(t, uuids[len(uuids)-1], "vtctl", onlineSingletonDDLStrategy, "", "", false) uuids = append(uuids, uuid) onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete) checkTable(t, tableName, true) @@ -226,7 +229,7 @@ func TestSchemaChange(t *testing.T) { var throttledUUIDs []string // singleton-context t.Run("throttled migrations, singleton-context", func(t *testing.T) { - uuidList := testOnlineDDLStatement(t, multiAlterTableThrottlingStatement, "gh-ost -singleton-context --max-load=Threads_running=1", "vtctl", "hint_col", "", false) + uuidList := testOnlineDDLStatement(t, multiAlterTableThrottlingStatement, "gh-ost --singleton-context --max-load=Threads_running=1", "vtctl", "", "hint_col", "", false) throttledUUIDs = strings.Split(uuidList, "\n") assert.Equal(t, 3, len(throttledUUIDs)) for _, uuid := range throttledUUIDs { @@ -234,7 +237,7 @@ func TestSchemaChange(t *testing.T) { } }) t.Run("failed migrations, singleton-context", func(t *testing.T) { - _ = testOnlineDDLStatement(t, multiAlterTableThrottlingStatement, "gh-ost -singleton-context --max-load=Threads_running=1", "vtctl", "hint_col", "rejected", false) + _ = testOnlineDDLStatement(t, multiAlterTableThrottlingStatement, "gh-ost --singleton-context --max-load=Threads_running=1", "vtctl", "", "hint_col", "rejected", false) }) t.Run("terminate throttled migrations", func(t *testing.T) { for _, uuid := range throttledUUIDs { @@ -249,7 +252,7 @@ func TestSchemaChange(t *testing.T) { }) t.Run("successful multiple statement, singleton-context, vtctl", func(t *testing.T) { - uuidList := testOnlineDDLStatement(t, multiDropStatements, onlineSingletonContextDDLStrategy, "vtctl", "", "", false) + uuidList := testOnlineDDLStatement(t, multiDropStatements, onlineSingletonContextDDLStrategy, "vtctl", "", "", "", false) uuidSlice := strings.Split(uuidList, "\n") assert.Equal(t, 3, len(uuidSlice)) for _, uuid := range uuidSlice { @@ -261,34 +264,59 @@ func TestSchemaChange(t *testing.T) { //DROP t.Run("online DROP TABLE", func(t *testing.T) { - uuid := testOnlineDDLStatement(t, dropStatement, onlineSingletonDDLStrategy, "vtgate", "", "", false) + uuid := testOnlineDDLStatement(t, dropStatement, onlineSingletonDDLStrategy, "vtgate", "", "", "", false) uuids = append(uuids, uuid) onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete) checkTable(t, tableName, false) }) t.Run("revert DROP TABLE", func(t *testing.T) { // This will recreate the table (well, actually, rename it back into place) - uuid := testRevertMigration(t, uuids[len(uuids)-1], "vttablet", "", false) + uuid := testRevertMigration(t, uuids[len(uuids)-1], "vttablet", onlineSingletonDDLStrategy, "", "", false) uuids = append(uuids, uuid) onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete) checkTable(t, tableName, true) }) - // Last two tests (we run an incomplete migration) - t.Run("submit successful migration, no wait, vtgate", func(t *testing.T) { - _ = testOnlineDDLStatement(t, alterTableTrivialStatement, "gh-ost -singleton", "vtgate", "hint_col", "", true) + t.Run("fail concurrent singleton, vtgate", func(t *testing.T) { + uuid := testOnlineDDLStatement(t, alterTableTrivialStatement, "vitess --postpone-completion --singleton", "vtgate", "", "hint_col", "", true) + uuids = append(uuids, uuid) + _ = testOnlineDDLStatement(t, dropNonexistentTableStatement, "vitess --singleton", "vtgate", "", "hint_col", "rejected", true) + onlineddl.CheckCompleteMigration(t, &vtParams, shards, uuid, true) + status := onlineddl.WaitForMigrationStatus(t, &vtParams, shards, uuid, 20*time.Second, schema.OnlineDDLStatusComplete, schema.OnlineDDLStatusFailed) + fmt.Printf("# Migration status (for debug purposes): <%s>\n", status) + onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete) }) - t.Run("fail submit migration, no wait, vtgate", func(t *testing.T) { - _ = testOnlineDDLStatement(t, alterTableTrivialStatement, "gh-ost -singleton", "vtgate", "hint_col", "rejected", true) + t.Run("fail concurrent singleton-context with revert", func(t *testing.T) { + revertUUID := testRevertMigration(t, uuids[len(uuids)-1], "vtctl", "vitess --allow-concurrent --postpone-completion --singleton-context", "rev:ctx", "", false) + onlineddl.WaitForMigrationStatus(t, &vtParams, shards, revertUUID, 20*time.Second, schema.OnlineDDLStatusRunning) + // revert is running + _ = testOnlineDDLStatement(t, dropNonexistentTableStatement, "vitess --allow-concurrent --singleton-context", "vtctl", "migrate:ctx", "", "rejected", true) + onlineddl.CheckCancelMigration(t, &vtParams, shards, revertUUID, true) + status := onlineddl.WaitForMigrationStatus(t, &vtParams, shards, revertUUID, 20*time.Second, schema.OnlineDDLStatusComplete, schema.OnlineDDLStatusFailed) + fmt.Printf("# Migration status (for debug purposes): <%s>\n", status) + onlineddl.CheckMigrationStatus(t, &vtParams, shards, revertUUID, schema.OnlineDDLStatusFailed) + }) + t.Run("success concurrent singleton-context with no-context revert", func(t *testing.T) { + revertUUID := testRevertMigration(t, uuids[len(uuids)-1], "vtctl", "vitess --allow-concurrent --postpone-completion", "rev:ctx", "", false) + onlineddl.WaitForMigrationStatus(t, &vtParams, shards, revertUUID, 20*time.Second, schema.OnlineDDLStatusRunning) + // revert is running but has no --singleton-context. Our next migration should be able to run. + uuid := testOnlineDDLStatement(t, dropNonexistentTableStatement, "vitess --allow-concurrent --singleton-context", "vtctl", "migrate:ctx", "", "", false) + uuids = append(uuids, uuid) + onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete) + onlineddl.CheckCancelMigration(t, &vtParams, shards, revertUUID, true) + status := onlineddl.WaitForMigrationStatus(t, &vtParams, shards, revertUUID, 20*time.Second, schema.OnlineDDLStatusComplete, schema.OnlineDDLStatusFailed) + fmt.Printf("# Migration status (for debug purposes): <%s>\n", status) + onlineddl.CheckMigrationStatus(t, &vtParams, shards, revertUUID, schema.OnlineDDLStatusFailed) }) } // testOnlineDDLStatement runs an online DDL, ALTER statement -func testOnlineDDLStatement(t *testing.T, alterStatement string, ddlStrategy string, executeStrategy string, expectHint string, expectError string, skipWait bool) (uuid string) { +func testOnlineDDLStatement(t *testing.T, alterStatement string, ddlStrategy string, executeStrategy string, migrationContext string, expectHint string, expectError string, skipWait bool) (uuid string) { strategySetting, err := schema.ParseDDLStrategy(ddlStrategy) require.NoError(t, err) if executeStrategy == "vtgate" { + assert.Empty(t, migrationContext, "explicit migration context not supported in vtgate. Test via vtctl") result := onlineddl.VtgateExecDDL(t, &vtParams, ddlStrategy, alterStatement, expectError) if result != nil { row := result.Named().Row() @@ -297,7 +325,7 @@ func testOnlineDDLStatement(t *testing.T, alterStatement string, ddlStrategy str } } } else { - output, err := clusterInstance.VtctlclientProcess.ApplySchemaWithOutput(keyspaceName, alterStatement, cluster.VtctlClientParams{DDLStrategy: ddlStrategy, SkipPreflight: true}) + output, err := clusterInstance.VtctlclientProcess.ApplySchemaWithOutput(keyspaceName, alterStatement, cluster.VtctlClientParams{DDLStrategy: ddlStrategy, SkipPreflight: true, MigrationContext: migrationContext}) if expectError == "" { assert.NoError(t, err) uuid = output @@ -322,10 +350,11 @@ func testOnlineDDLStatement(t *testing.T, alterStatement string, ddlStrategy str } // testRevertMigration reverts a given migration -func testRevertMigration(t *testing.T, revertUUID string, executeStrategy string, expectError string, skipWait bool) (uuid string) { +func testRevertMigration(t *testing.T, revertUUID string, executeStrategy string, ddlStrategy string, migrationContext string, expectError string, skipWait bool) (uuid string) { revertQuery := fmt.Sprintf("revert vitess_migration '%s'", revertUUID) if executeStrategy == "vtgate" { - result := onlineddl.VtgateExecDDL(t, &vtParams, onlineSingletonDDLStrategy, revertQuery, expectError) + assert.Empty(t, migrationContext, "explicit migration context not supported in vtgate. Test via vtctl") + result := onlineddl.VtgateExecDDL(t, &vtParams, ddlStrategy, revertQuery, expectError) if result != nil { row := result.Named().Row() if row != nil { @@ -333,7 +362,7 @@ func testRevertMigration(t *testing.T, revertUUID string, executeStrategy string } } } else { - output, err := clusterInstance.VtctlclientProcess.ApplySchemaWithOutput(keyspaceName, revertQuery, cluster.VtctlClientParams{DDLStrategy: onlineSingletonDDLStrategy, SkipPreflight: true}) + output, err := clusterInstance.VtctlclientProcess.ApplySchemaWithOutput(keyspaceName, revertQuery, cluster.VtctlClientParams{DDLStrategy: ddlStrategy, SkipPreflight: true, MigrationContext: migrationContext}) if expectError == "" { assert.NoError(t, err) uuid = output diff --git a/go/vt/vttablet/onlineddl/executor.go b/go/vt/vttablet/onlineddl/executor.go index fa21b3ee8c2..8df745379f7 100644 --- a/go/vt/vttablet/onlineddl/executor.go +++ b/go/vt/vttablet/onlineddl/executor.go @@ -3586,6 +3586,28 @@ func (e *Executor) CompleteMigration(ctx context.Context, uuid string) (result * return e.execQuery(ctx, query) } +func (e *Executor) submittedMigrationConflictsWithPendingMigrationInSingletonContext( + ctx context.Context, submittedMigration, pendingOnlineDDL *schema.OnlineDDL, +) bool { + if pendingOnlineDDL.MigrationContext == submittedMigration.MigrationContext { + // same migration context. this is obviously allowed + return false + } + // Let's see if the pending migration is a revert: + if _, err := pendingOnlineDDL.GetRevertUUID(); err != nil { + // Not a revert. So the pending migration definitely conflicts with our migration. + return true + } + + // The pending migration is a revert + if !pendingOnlineDDL.StrategySetting().IsSingletonContext() { + // Aha! So, our "conflict" is with a REVERT migration, which does _not_ have a -singleton-context + // flag. Because we want to allow REVERT migrations to run as concurrently as possible, we allow this scenario. + return false + } + return true +} + // SubmitMigration inserts a new migration request func (e *Executor) SubmitMigration( ctx context.Context, @@ -3667,9 +3689,10 @@ func (e *Executor) SubmitMigration( if err != nil { return err } - if pendingOnlineDDL.MigrationContext != onlineDDL.MigrationContext { - return vterrors.Errorf(vtrpcpb.Code_FAILED_PRECONDITION, "singleton migration rejected: found pending migration: %s in different context: %s", pendingUUID, pendingOnlineDDL.MigrationContext) + if e.submittedMigrationConflictsWithPendingMigrationInSingletonContext(ctx, onlineDDL, pendingOnlineDDL) { + return vterrors.Errorf(vtrpcpb.Code_FAILED_PRECONDITION, "singleton-context migration rejected: found pending migration: %s in different context: %s", pendingUUID, pendingOnlineDDL.MigrationContext) } + // no conflict? continue looking for other pending migrations } } // OK to go! We can submit the migration: