Skip to content

Commit

Permalink
Online DDL: revert considerations for migrations with foreign key con…
Browse files Browse the repository at this point in the history 
…straints (vitessio#14368)

Signed-off-by: Shlomi Noach <[email protected]>
  • Loading branch information
@shlomi-noach
shlomi-noach authored Nov 7, 2023
1 parent a7a44c6 commit 3aead9e
Show file tree
Hide file tree
Showing 15 changed files with 2,423 additions and 1,974 deletions.
88 changes: 85 additions & 3 deletions go/test/endtoend/onlineddl/revert/onlineddl_revert_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,9 +125,11 @@ type revertibleTestCase struct {
fromSchema string
toSchema string
// expectProblems bool
removedForeignKeyNames string
removedUniqueKeyNames string
droppedNoDefaultColumnNames string
expandedColumnNames string
onlyIfFKOnlineDDLPossible bool
}

func TestMain(m *testing.M) {
Expand Down Expand Up @@ -218,6 +220,25 @@ func TestSchemaChange(t *testing.T) {

func testRevertible(t *testing.T) {

fkOnlineDDLPossible := false
t.Run("check 'rename_table_preserve_foreign_key' variable", func(t *testing.T) {
// Online DDL is not possible on vanilla MySQL 8.0 for reasons described in https://vitess.io/blog/2021-06-15-online-ddl-why-no-fk/.
// However, Online DDL is made possible in via these changes: https://github.com/planetscale/mysql-server/commit/bb777e3e86387571c044fb4a2beb4f8c60462ced
// as part of https://github.com/planetscale/mysql-server/releases/tag/8.0.34-ps1.
// Said changes introduce a new global/session boolean variable named 'rename_table_preserve_foreign_key'. It defaults 'false'/0 for backwards compatibility.
// When enabled, a `RENAME TABLE` to a FK parent "pins" the children's foreign keys to the table name rather than the table pointer. Which means after the RENAME,
// the children will point to the newly instated table rather than the original, renamed table.
// (Note: this applies to a particular type of RENAME where we swap tables, see the above blog post).
// For FK children, the MySQL changes simply ignore any Vitess-internal table.
//
// In this stress test, we enable Online DDL if the variable 'rename_table_preserve_foreign_key' is present. The Online DDL mechanism will in turn
// query for this variable, and manipulate it, when starting the migration and when cutting over.
rs, err := shards[0].Vttablets[0].VttabletProcess.QueryTablet("show global variables like 'rename_table_preserve_foreign_key'", keyspaceName, false)
require.NoError(t, err)
fkOnlineDDLPossible = len(rs.Rows) > 0
t.Logf("MySQL support for 'rename_table_preserve_foreign_key': %v", fkOnlineDDLPossible)
})

var testCases = []revertibleTestCase{
{
name: "identical schemas",
Expand Down Expand Up @@ -253,6 +274,20 @@ func testRevertible(t *testing.T) {
toSchema: `id int primary key, i1 int default null, unique key i1_uidx(i1)`,
removedUniqueKeyNames: ``,
},
{
name: "removed foreign key",
fromSchema: "id int primary key, i int, constraint some_fk_1 foreign key (i) references parent (id) on delete cascade",
toSchema: "id int primary key, i int",
removedForeignKeyNames: "some_fk_1",
onlyIfFKOnlineDDLPossible: true,
},

{
name: "renamed foreign key",
fromSchema: "id int primary key, i int, constraint f1 foreign key (i) references parent (id) on delete cascade",
toSchema: "id int primary key, i int, constraint f2 foreign key (i) references parent (id) on delete cascade",
onlyIfFKOnlineDDLPossible: true,
},
{
name: "remove column without default",
fromSchema: `id int primary key, i1 int not null`,
Expand Down Expand Up @@ -344,24 +379,30 @@ func testRevertible(t *testing.T) {
dropTableStatement = `
DROP TABLE onlineddl_test
`
tableName = "onlineddl_test"
ddlStrategy = "online --declarative --allow-zero-in-date"
tableName = "onlineddl_test"
ddlStrategy = "online --declarative --allow-zero-in-date --unsafe-allow-foreign-keys"
createParentTable = "create table parent (id int primary key)"
)

onlineddl.VtgateExecQuery(t, &vtParams, createParentTable, "")

removeBackticks := func(s string) string {
return strings.Replace(s, "`", "", -1)
}

for _, testcase := range testCases {
t.Run(testcase.name, func(t *testing.T) {
if testcase.onlyIfFKOnlineDDLPossible && !fkOnlineDDLPossible {
t.Skipf("skipped because backing database does not support 'rename_table_preserve_foreign_key'")
return
}

t.Run("ensure table dropped", func(t *testing.T) {
// A preparation step, to clean up anything from the previous test case
uuid := testOnlineDDLStatement(t, dropTableStatement, ddlStrategy, "vtgate", tableName, "")
onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete)
checkTable(t, tableName, false)
})

t.Run("create from-table", func(t *testing.T) {
// A preparation step, to re-create the base table
fromStatement := fmt.Sprintf(createTableWrapper, testcase.fromSchema)
Expand All @@ -382,17 +423,56 @@ func testRevertible(t *testing.T) {
rs := onlineddl.ReadMigrations(t, &vtParams, uuid)
require.NotNil(t, rs)
for _, row := range rs.Named().Rows {
removedForeignKeyNames := row.AsString("removed_foreign_key_names", "")
removedUniqueKeyNames := row.AsString("removed_unique_key_names", "")
droppedNoDefaultColumnNames := row.AsString("dropped_no_default_column_names", "")
expandedColumnNames := row.AsString("expanded_column_names", "")

assert.Equal(t, testcase.removedForeignKeyNames, removeBackticks(removedForeignKeyNames))
assert.Equal(t, testcase.removedUniqueKeyNames, removeBackticks(removedUniqueKeyNames))
assert.Equal(t, testcase.droppedNoDefaultColumnNames, removeBackticks(droppedNoDefaultColumnNames))
assert.Equal(t, testcase.expandedColumnNames, removeBackticks(expandedColumnNames))
}
})
})
}

t.Run("drop fk child table", func(t *testing.T) {
t.Run("ensure table dropped", func(t *testing.T) {
// A preparation step, to clean up anything from the previous test case
uuid := testOnlineDDLStatement(t, dropTableStatement, ddlStrategy, "vtgate", tableName, "")
onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete)
checkTable(t, tableName, false)
})
t.Run("create child table", func(t *testing.T) {
fromStatement := fmt.Sprintf(createTableWrapper, "id int primary key, i int, constraint some_fk_2 foreign key (i) references parent (id) on delete cascade")
uuid := testOnlineDDLStatement(t, fromStatement, ddlStrategy, "vtgate", tableName, "")
onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete)
checkTable(t, tableName, true)
})
var uuid string
t.Run("drop", func(t *testing.T) {
uuid = testOnlineDDLStatement(t, dropTableStatement, ddlStrategy, "vtgate", tableName, "")
onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete)
checkTable(t, tableName, false)
})
t.Run("check migration", func(t *testing.T) {
// All right, the actual test
rs := onlineddl.ReadMigrations(t, &vtParams, uuid)
require.NotNil(t, rs)
for _, row := range rs.Named().Rows {
removedForeignKeyNames := row.AsString("removed_foreign_key_names", "")
removedUniqueKeyNames := row.AsString("removed_unique_key_names", "")
droppedNoDefaultColumnNames := row.AsString("dropped_no_default_column_names", "")
expandedColumnNames := row.AsString("expanded_column_names", "")

assert.Equal(t, "some_fk_2", removeBackticks(removedForeignKeyNames))
assert.Equal(t, "", removeBackticks(removedUniqueKeyNames))
assert.Equal(t, "", removeBackticks(droppedNoDefaultColumnNames))
assert.Equal(t, "", removeBackticks(expandedColumnNames))
}
})
})
}

func testRevert(t *testing.T) {
Expand Down Expand Up @@ -964,6 +1044,8 @@ func testRevert(t *testing.T) {
assert.Empty(t, specialPlan)
assert.NotEmpty(t, artifacts)
}
removedForeignKeyNames := row.AsString("removed_foreign_key_names", "")
assert.Empty(t, removedForeignKeyNames)
})
t.Run("INSTANT DDL: fail revert", func(t *testing.T) {
uuid := testRevertMigration(t, uuids[len(uuids)-1], ddlStrategy)
Expand Down
Loading

0 comments on commit 3aead9e

Please sign in to comment.