Online DDL: revert considerations for migrations with foreign key constraints (#14368)

Signed-off-by: Shlomi Noach <[email protected]>

Author: shlomi-noach

go/test/endtoend/onlineddl/revert/onlineddl_revert_test.go

@@ -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) {
@@ -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",
@@ -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`,
@@ -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)
@@ -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) {
@@ -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)