Skip to content

Online DDL: edit CONSTRAINT names in CREATE TABLE #14517

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions go/mysql/flavor.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down
2 changes: 2 additions & 0 deletions go/mysql/flavor_mysql.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
10 changes: 10 additions & 0 deletions go/mysql/flavor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
18 changes: 16 additions & 2 deletions go/test/endtoend/onlineddl/revert/onlineddl_revert_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -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))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 <table-name> 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)
Expand Down Expand Up @@ -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 {
Expand Down
11 changes: 11 additions & 0 deletions go/vt/vttablet/onlineddl/executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 <tablename> prefix.
if _, err := e.validateAndEditCreateTableStatement(ctx, onlineDDL, newCreateTable); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated to this PR, but why do we pass a context to this function? Functions that don't perform RPCs do not need to take in a context.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This specific function indeed doesn't use the given context. There's many other non-gRPC functions that do use a context because they either use a Connection (context cancellation ensures connection terminates) or loops/waits.

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())
Expand Down