From 4911e5b8f58e940f22745b8940ca96f9f6660928 Mon Sep 17 00:00:00 2001
From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Date: Sun, 10 Mar 2024 14:16:09 +0200
Subject: [PATCH] DDL strategy flag '--unsafe-allow-foreign-keys' implies
 setting 'FOREIGN_KEY_CHECKS=0' in both direct and Online DDL strategies

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
---
 .../scheduler/onlineddl_scheduler_test.go     |  9 ++++++++
 go/test/endtoend/vtgate/schema/schema_test.go | 23 +++++++++++++++++++
 go/vt/schemamanager/tablet_executor.go        |  8 +++++--
 go/vt/vttablet/onlineddl/executor.go          | 15 ++++++++++++
 4 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/go/test/endtoend/onlineddl/scheduler/onlineddl_scheduler_test.go b/go/test/endtoend/onlineddl/scheduler/onlineddl_scheduler_test.go
index 5a72cbfc839..396bb0fad77 100644
--- a/go/test/endtoend/onlineddl/scheduler/onlineddl_scheduler_test.go
+++ b/go/test/endtoend/onlineddl/scheduler/onlineddl_scheduler_test.go
@@ -2286,6 +2286,15 @@ 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,
+			expectHint:       "t11",
+		},
 	}
 
 	fkOnlineDDLPossible := false
diff --git a/go/test/endtoend/vtgate/schema/schema_test.go b/go/test/endtoend/vtgate/schema/schema_test.go
index 14b6c13034e..6b2e8ef7e61 100644
--- a/go/test/endtoend/vtgate/schema/schema_test.go
+++ b/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)
diff --git a/go/vt/schemamanager/tablet_executor.go b/go/vt/schemamanager/tablet_executor.go
index 4f0326f70b1..592c64e7073 100644
--- a/go/vt/schemamanager/tablet_executor.go
+++ b/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 {
diff --git a/go/vt/vttablet/onlineddl/executor.go b/go/vt/vttablet/onlineddl/executor.go
index e89af79fd1d..48d09db0f42 100644
--- a/go/vt/vttablet/onlineddl/executor.go
+++ b/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 {