DDL strategy flag --unsafe-allow-foreign-keys implies setting FOREIGN_KEY_CHECKS=0 (#15432)

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Author: shlomi-noach

go/test/endtoend/onlineddl/scheduler/onlineddl_scheduler_test.go

@@ -2214,6 +2214,7 @@ func testForeignKeys(t *testing.T) {
 		sql                       string
 		allowForeignKeys          bool
 		expectHint                string
+		expectCountUUIDs          int
 		onlyIfFKOnlineDDLPossible bool
 	}
 	var testCases = []testCase{
@@ -2286,6 +2287,16 @@ func testForeignKeys(t *testing.T) {
 			expectHint:                "child_hint",
 			onlyIfFKOnlineDDLPossible: true,
 		},
+		{
+			name: "add two tables with cyclic fk relationship",
+			sql: `
+				create table t11 (id int primary key, i int, constraint f11 foreign key (i) references t12 (id));
+				create table t12 (id int primary key, i int, constraint f12 foreign key (i) references t11 (id));
+				`,
+			allowForeignKeys: true,
+			expectCountUUIDs: 2,
+			expectHint:       "t11",
+		},
 	}
 
 	fkOnlineDDLPossible := false
@@ -2328,6 +2339,9 @@ func testForeignKeys(t *testing.T) {
 		return testOnlineDDLStatement(t, createParams(sql, ddlStrategy, "vtctl", expectHint, errorHint, false))
 	}
 	for _, testcase := range testCases {
+		if testcase.expectCountUUIDs == 0 {
+			testcase.expectCountUUIDs = 1
+		}
 		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'")
@@ -2364,7 +2378,10 @@ func testForeignKeys(t *testing.T) {
 			var uuid string
 			t.Run("run migration", func(t *testing.T) {
 				if testcase.allowForeignKeys {
-					uuid = testStatement(t, testcase.sql, ddlStrategyAllowFK, testcase.expectHint, false)
+					output := testStatement(t, testcase.sql, ddlStrategyAllowFK, testcase.expectHint, false)
+					uuids := strings.Split(output, "\n")
+					assert.Equal(t, testcase.expectCountUUIDs, len(uuids))
+					uuid = uuids[0] // in case of multiple statements, we only check the first
 					onlineddl.CheckMigrationStatus(t, &vtParams, shards, uuid, schema.OnlineDDLStatusComplete)
 				} else {
 					uuid = testStatement(t, testcase.sql, ddlStrategy, "", true)
@@ -2384,7 +2401,7 @@ func testForeignKeys(t *testing.T) {
 					artifacts = textutil.SplitDelimitedList(row.AsString("artifacts", ""))
 				}
 
-				artifacts = append(artifacts, "child_table", "child_nofk_table", "parent_table")
+				artifacts = append(artifacts, "child_table", "child_nofk_table", "parent_table", "t11", "t12")
 				// brute force drop all tables. In MySQL 8.0 you can do a single `DROP TABLE ... <list of all tables>`
 				// which auto-resovled order. But in 5.7 you can't.
 				droppedTables := map[string]bool{}
@@ -2394,7 +2411,7 @@ func testForeignKeys(t *testing.T) {
 							continue
 						}
 						statement := fmt.Sprintf("DROP TABLE IF EXISTS %s", artifact)
-						_, err := clusterInstance.VtctldClientProcess.ApplySchemaWithOutput(keyspaceName, statement, cluster.ApplySchemaParams{DDLStrategy: "direct"})
+						_, err := clusterInstance.VtctldClientProcess.ApplySchemaWithOutput(keyspaceName, statement, cluster.ApplySchemaParams{DDLStrategy: "direct --unsafe-allow-foreign-keys"})
 						if err == nil {
 							droppedTables[artifact] = true
 						}

go/test/endtoend/vtgate/schema/schema_test.go

@@ -108,6 +108,7 @@ func TestSchemaChange(t *testing.T) {
 	testWithDropCreateSchema(t)
 	testDropNonExistentTables(t)
 	testApplySchemaBatch(t)
+	testUnsafeAllowForeignKeys(t)
 	testCreateInvalidView(t)
 	testCopySchemaShards(t, clusterInstance.Keyspaces[0].Shards[0].Vttablets[0].VttabletProcess.TabletPath, 2)
 	testCopySchemaShards(t, fmt.Sprintf("%s/0", keyspaceName), 3)
@@ -252,6 +253,28 @@ func testApplySchemaBatch(t *testing.T) {
 	}
 }
 
+func testUnsafeAllowForeignKeys(t *testing.T) {
+	sqls := `
+		create table t11 (id int primary key, i int, constraint f1101 foreign key (i) references t12 (id) on delete restrict);
+		create table t12 (id int primary key, i int, constraint f1201 foreign key (i) references t11 (id) on delete set null);
+	`
+	{
+		_, err := clusterInstance.VtctldClientProcess.ExecuteCommandWithOutput("ApplySchema", "--ddl-strategy", "direct --allow-zero-in-date", "--sql", sqls, keyspaceName)
+		assert.Error(t, err)
+		checkTables(t, totalTableCount)
+	}
+	{
+		_, err := clusterInstance.VtctldClientProcess.ExecuteCommandWithOutput("ApplySchema", "--ddl-strategy", "direct --unsafe-allow-foreign-keys --allow-zero-in-date", "--sql", sqls, keyspaceName)
+		require.NoError(t, err)
+		checkTables(t, totalTableCount+2)
+	}
+	{
+		_, err := clusterInstance.VtctldClientProcess.ExecuteCommandWithOutput("ApplySchema", "--sql", "drop table t11, t12", keyspaceName)
+		require.NoError(t, err)
+		checkTables(t, totalTableCount)
+	}
+}
+
 // checkTables checks the number of tables in the first two shards.
 func checkTables(t *testing.T, count int) {
 	checkTablesCount(t, clusterInstance.Keyspaces[0].Shards[0].Vttablets[0], count)

go/vt/schemamanager/tablet_executor.go

@@ -490,10 +490,14 @@ func (exec *TabletExecutor) executeOneTablet(
 				return
 			}
 		}
-		result, err = exec.tmc.ExecuteFetchAsDba(ctx, tablet, false, &tabletmanagerdatapb.ExecuteFetchAsDbaRequest{
+		request := &tabletmanagerdatapb.ExecuteFetchAsDbaRequest{
 			Query:   []byte(sql),
 			MaxRows: 10,
-		})
+		}
+		if exec.ddlStrategySetting != nil && exec.ddlStrategySetting.IsAllowForeignKeysFlag() {
+			request.DisableForeignKeyChecks = true
+		}
+		result, err = exec.tmc.ExecuteFetchAsDba(ctx, tablet, false, request)
 
 	}
 	if err != nil {

go/vt/vttablet/onlineddl/executor.go

@@ -646,6 +646,21 @@ func (e *Executor) executeDirectly(ctx context.Context, onlineDDL *schema.Online
 	}
 
 	_ = e.onSchemaMigrationStatus(ctx, onlineDDL.UUID, schema.OnlineDDLStatusRunning, false, progressPctStarted, etaSecondsUnknown, rowsCopiedUnknown, emptyHint)
+	if onlineDDL.StrategySetting().IsAllowForeignKeysFlag() {
+		// Foreign key support is curently "unsafe". We further put the burden on the user
+		// by disabling foreign key checks. With this, the user is able to create cyclic
+		// foreign key references (e.g. t1<->t2) without going through the trouble of
+		// CREATE TABLE t1->CREATE TABLE t2->ALTER TABLE t1 ADD FOREIGN KEY ... REFERENCES ts
+		// Grab current sql_mode value
+		if _, err := conn.ExecuteFetch(`set @vt_onlineddl_foreign_key_checks=@@foreign_key_checks`, 0, false); err != nil {
+			return false, vterrors.Errorf(vtrpcpb.Code_UNKNOWN, "could not read foreign_key_checks: %v", err)
+		}
+		_, err = conn.ExecuteFetch("SET foreign_key_checks=0", 0, false)
+		if err != nil {
+			return false, err
+		}
+		defer conn.ExecuteFetch("SET foreign_key_checks=@vt_onlineddl_foreign_key_checks", 0, false)
+	}
 	_, err = conn.ExecuteFetch(onlineDDL.SQL, 0, false)
 
 	if err != nil {
@@ -1288,7 +1303,7 @@ func (e *Executor) newConstraintName(onlineDDL *schema.OnlineDDL, constraintType
 // validateAndEditCreateTableStatement inspects the CreateTable AST and does the following:
 // - extra validation (no FKs for now...)
 // - generate new and unique names for all constraints (CHECK and FK; yes, why not handle FK names; even as we don't support FKs today, we may in the future)
-func (e *Executor) validateAndEditCreateTableStatement(ctx context.Context, onlineDDL *schema.OnlineDDL, createTable *sqlparser.CreateTable) (constraintMap map[string]string, err error) {
+func (e *Executor) validateAndEditCreateTableStatement(onlineDDL *schema.OnlineDDL, createTable *sqlparser.CreateTable) (constraintMap map[string]string, err error) {
 	constraintMap = map[string]string{}
 	hashExists := map[string]bool{}
 
@@ -1315,7 +1330,7 @@ func (e *Executor) validateAndEditCreateTableStatement(ctx context.Context, onli
 // validateAndEditAlterTableStatement inspects the AlterTable statement and:
 // - modifies any CONSTRAINT name according to given name mapping
 // - explode ADD FULLTEXT KEY into multiple statements
-func (e *Executor) validateAndEditAlterTableStatement(ctx context.Context, capableOf capabilities.CapableOf, onlineDDL *schema.OnlineDDL, alterTable *sqlparser.AlterTable, constraintMap map[string]string) (alters []*sqlparser.AlterTable, err error) {
+func (e *Executor) validateAndEditAlterTableStatement(capableOf capabilities.CapableOf, onlineDDL *schema.OnlineDDL, alterTable *sqlparser.AlterTable, constraintMap map[string]string) (alters []*sqlparser.AlterTable, err error) {
 	capableOfInstantDDLXtrabackup, err := capableOf(capabilities.InstantDDLXtrabackupCapability)
 	if err != nil {
 		return nil, err
@@ -1405,7 +1420,7 @@ func (e *Executor) duplicateCreateTable(ctx context.Context, onlineDDL *schema.O
 	newCreateTable.SetTable(newCreateTable.GetTable().Qualifier.CompliantName(), newTableName)
 	// manipulate CreateTable statement: take care of constraints names which have to be
 	// unique across the schema
-	constraintMap, err = e.validateAndEditCreateTableStatement(ctx, onlineDDL, newCreateTable)
+	constraintMap, err = e.validateAndEditCreateTableStatement(onlineDDL, newCreateTable)
 	if err != nil {
 		return nil, nil, nil, err
 	}
@@ -1475,7 +1490,7 @@ func (e *Executor) initVreplicationOriginalMigration(ctx context.Context, online
 	// Also, change any constraint names:
 
 	capableOf := mysql.ServerVersionCapableOf(conn.ServerVersion)
-	alters, err := e.validateAndEditAlterTableStatement(ctx, capableOf, onlineDDL, alterTable, constraintMap)
+	alters, err := e.validateAndEditAlterTableStatement(capableOf, onlineDDL, alterTable, constraintMap)
 	if err != nil {
 		return v, err
 	}
@@ -2995,7 +3010,7 @@ func (e *Executor) executeCreateDDLActionMigration(ctx context.Context, onlineDD
 		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 {
+		if _, err := e.validateAndEditCreateTableStatement(onlineDDL, newCreateTable); err != nil {
 			return failMigration(err)
 		}
 		ddlStmt = newCreateTable

go/vt/vttablet/onlineddl/executor_test.go

@@ -172,7 +172,7 @@ func TestValidateAndEditCreateTableStatement(t *testing.T) {
 			require.True(t, ok)
 
 			onlineDDL := &schema.OnlineDDL{UUID: "a5a563da_dc1a_11ec_a416_0a43f95f28a3", Table: "onlineddl_test", Options: tc.strategyOptions}
-			constraintMap, err := e.validateAndEditCreateTableStatement(context.Background(), onlineDDL, createTable)
+			constraintMap, err := e.validateAndEditCreateTableStatement(onlineDDL, createTable)
 			if tc.expectError != "" {
 				assert.ErrorContains(t, err, tc.expectError)
 				return
@@ -290,7 +290,7 @@ func TestValidateAndEditAlterTableStatement(t *testing.T) {
 			}
 			capableOf := mysql.ServerVersionCapableOf(tc.mySQLVersion)
 			onlineDDL := &schema.OnlineDDL{UUID: "a5a563da_dc1a_11ec_a416_0a43f95f28a3", Table: "t", Options: "--unsafe-allow-foreign-keys"}
-			alters, err := e.validateAndEditAlterTableStatement(context.Background(), capableOf, onlineDDL, alterTable, m)
+			alters, err := e.validateAndEditAlterTableStatement(capableOf, onlineDDL, alterTable, m)
 			assert.NoError(t, err)
 			var altersStrings []string
 			for _, alter := range alters {