From 024af9375d36e083b05684452dcbc51e2ab83fd0 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Wed, 15 Nov 2023 06:38:32 +0200 Subject: [PATCH] Online DDL: edit CONSTRAINT names in CREATE TABLE (#14517) Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/mysql/flavor.go | 1 + go/mysql/flavor_mysql.go | 2 ++ go/mysql/flavor_test.go | 10 +++++++++ .../onlineddl/revert/onlineddl_revert_test.go | 18 ++++++++++++++-- .../scheduler/onlineddl_scheduler_test.go | 21 ++++++++++++++++++- go/vt/vttablet/onlineddl/executor.go | 11 ++++++++++ 6 files changed, 60 insertions(+), 3 deletions(-) diff --git a/go/mysql/flavor.go b/go/mysql/flavor.go index edb64913c31..a8c2de2e114 100644 --- a/go/mysql/flavor.go +++ b/go/mysql/flavor.go @@ -55,6 +55,7 @@ const ( MySQLUpgradeInServerFlavorCapability DynamicRedoLogCapacityFlavorCapability // supported in MySQL 8.0.30 and above: https://dev.mysql.com/doc/relnotes/mysql/8.0/en/news-8-0-30.html DisableRedoLogFlavorCapability // supported in MySQL 8.0.21 and above: https://dev.mysql.com/doc/relnotes/mysql/8.0/en/news-8-0-21.html + CheckConstraintsCapability // supported in MySQL 8.0.16 and above: https://dev.mysql.com/doc/relnotes/mysql/8.0/en/news-8-0-16.html ) const ( diff --git a/go/mysql/flavor_mysql.go b/go/mysql/flavor_mysql.go index bc5f31006e5..3cc2e08e489 100644 --- a/go/mysql/flavor_mysql.go +++ b/go/mysql/flavor_mysql.go @@ -397,6 +397,8 @@ func (mysqlFlavor80) supportsCapability(serverVersion string, capability FlavorC return ServerVersionAtLeast(serverVersion, 8, 0, 30) case DisableRedoLogFlavorCapability: return ServerVersionAtLeast(serverVersion, 8, 0, 21) + case CheckConstraintsCapability: + return ServerVersionAtLeast(serverVersion, 8, 0, 16) default: return false, nil } diff --git a/go/mysql/flavor_test.go b/go/mysql/flavor_test.go index 891725b5afc..925504722b7 100644 --- a/go/mysql/flavor_test.go +++ b/go/mysql/flavor_test.go @@ -160,6 +160,16 @@ func TestGetFlavor(t *testing.T) { capability: DisableRedoLogFlavorCapability, isCapable: false, }, + { + version: "8.0.15", + capability: CheckConstraintsCapability, + isCapable: false, + }, + { + version: "8.0.20", + capability: CheckConstraintsCapability, + isCapable: true, + }, } for _, tc := range testcases { name := fmt.Sprintf("%s %v", tc.version, tc.capability) diff --git a/go/test/endtoend/onlineddl/revert/onlineddl_revert_test.go b/go/test/endtoend/onlineddl/revert/onlineddl_revert_test.go index 41cd5b5a1be..3220465269e 100644 --- a/go/test/endtoend/onlineddl/revert/onlineddl_revert_test.go +++ b/go/test/endtoend/onlineddl/revert/onlineddl_revert_test.go @@ -428,7 +428,20 @@ func testRevertible(t *testing.T) { droppedNoDefaultColumnNames := row.AsString("dropped_no_default_column_names", "") expandedColumnNames := row.AsString("expanded_column_names", "") - assert.Equal(t, testcase.removedForeignKeyNames, removeBackticks(removedForeignKeyNames)) + // Online DDL renames constraint names, and keeps the original name as a prefix. + // The name of e.g. "some_fk_2_" might turn into "some_fk_2_518ubnm034rel35l1m0u1dc7m" + expectRemovedForeignKeyNames := strings.Split(testcase.removedForeignKeyNames, ",") + actualRemovedForeignKeyNames := strings.Split(removeBackticks(removedForeignKeyNames), ",") + assert.Equal(t, len(expectRemovedForeignKeyNames), len(actualRemovedForeignKeyNames)) + for _, actualRemovedForeignKeyName := range actualRemovedForeignKeyNames { + found := false + for _, expectRemovedForeignKeyName := range expectRemovedForeignKeyNames { + if strings.HasPrefix(actualRemovedForeignKeyName, expectRemovedForeignKeyName) { + found = true + } + } + assert.Truef(t, found, "unexpected FK name", "%s", actualRemovedForeignKeyName) + } assert.Equal(t, testcase.removedUniqueKeyNames, removeBackticks(removedUniqueKeyNames)) assert.Equal(t, testcase.droppedNoDefaultColumnNames, removeBackticks(droppedNoDefaultColumnNames)) assert.Equal(t, testcase.expandedColumnNames, removeBackticks(expandedColumnNames)) @@ -466,7 +479,8 @@ func testRevertible(t *testing.T) { droppedNoDefaultColumnNames := row.AsString("dropped_no_default_column_names", "") expandedColumnNames := row.AsString("expanded_column_names", "") - assert.Equal(t, "some_fk_2", removeBackticks(removedForeignKeyNames)) + // Online DDL renames constraint names, and keeps the original name as a prefix. The name will be e.g. some_fk_2_518ubnm034rel35l1m0u1dc7m + assert.Contains(t, removeBackticks(removedForeignKeyNames), "some_fk_2") assert.Equal(t, "", removeBackticks(removedUniqueKeyNames)) assert.Equal(t, "", removeBackticks(droppedNoDefaultColumnNames)) assert.Equal(t, "", removeBackticks(expandedColumnNames)) diff --git a/go/test/endtoend/onlineddl/scheduler/onlineddl_scheduler_test.go b/go/test/endtoend/onlineddl/scheduler/onlineddl_scheduler_test.go index fbe6377c1fe..627567441b1 100644 --- a/go/test/endtoend/onlineddl/scheduler/onlineddl_scheduler_test.go +++ b/go/test/endtoend/onlineddl/scheduler/onlineddl_scheduler_test.go @@ -941,6 +941,25 @@ func testScheduler(t *testing.T) { }) }) + checkConstraintCapable, err := capableOf(mysql.CheckConstraintsCapability) // 8.0.16 and above + require.NoError(t, err) + if checkConstraintCapable { + // Constraints + t.Run("CREATE TABLE with CHECK constraint", func(t *testing.T) { + query := `create table with_constraint (id int primary key, check ((id >= 0)))` + uuid := testOnlineDDLStatement(t, createParams(query, ddlStrategy, "vtgate", "chk_", "", false)) + onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete) + t.Run("ensure constraint name is rewritten", func(t *testing.T) { + // Since we did not provide a name for the CHECK constraint, MySQL will + // name it `with_constraint_chk_1`. But we expect Online DDL to explicitly + // modify the constraint name, specifically to get rid of the prefix, + // so that we don't get into https://bugs.mysql.com/bug.php?id=107772 situation. + createStatement := getCreateTableStatement(t, shards[0].Vttablets[0], "with_constraint") + assert.NotContains(t, createStatement, "with_constraint_chk") + }) + }) + } + // INSTANT DDL instantDDLCapable, err := capableOf(mysql.InstantAddLastColumnFlavorCapability) require.NoError(t, err) @@ -2328,7 +2347,7 @@ func testRevertMigration(t *testing.T, params *testRevertMigrationParams) (uuid return uuid } -// checkTable checks the number of tables in the first two shards. +// checkTable checks the number of tables in all shards func checkTable(t *testing.T, showTableName string, expectExists bool) bool { expectCount := 0 if expectExists { diff --git a/go/vt/vttablet/onlineddl/executor.go b/go/vt/vttablet/onlineddl/executor.go index 8a3cf61348b..771a9000435 100644 --- a/go/vt/vttablet/onlineddl/executor.go +++ b/go/vt/vttablet/onlineddl/executor.go @@ -2863,6 +2863,17 @@ func (e *Executor) executeCreateDDLActionMigration(ctx context.Context, onlineDD } } } + if originalCreateTable, ok := ddlStmt.(*sqlparser.CreateTable); ok { + newCreateTable := sqlparser.CloneRefOfCreateTable(originalCreateTable) + // Rewrite this CREATE TABLE statement such that CONSTRAINT names are edited, + // specifically removing any prefix. + if _, err := e.validateAndEditCreateTableStatement(ctx, onlineDDL, newCreateTable); err != nil { + return failMigration(err) + } + ddlStmt = newCreateTable + onlineDDL.SQL = sqlparser.String(newCreateTable) + } + // from now on, whether a VIEW or a TABLE, they get the same treatment sentryArtifactTableName, err := schema.GenerateGCTableName(schema.HoldTableGCState, newGCTableRetainTime())