From d5bd597736f05da41c011593a38f843bd7bf8472 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Mon, 11 Mar 2024 22:26:49 +0200 Subject: [PATCH] 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> --- .../scheduler/onlineddl_scheduler_test.go | 23 ++++++++++++++--- go/test/endtoend/vtgate/schema/schema_test.go | 23 +++++++++++++++++ go/vt/schemamanager/tablet_executor.go | 8 ++++-- go/vt/vttablet/onlineddl/executor.go | 25 +++++++++++++++---- go/vt/vttablet/onlineddl/executor_test.go | 4 +-- 5 files changed, 71 insertions(+), 12 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..e0f665ff278 100644 --- a/go/test/endtoend/onlineddl/scheduler/onlineddl_scheduler_test.go +++ b/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 ... ` // 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 } 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 19f5c82ca5a..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 { @@ -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 prefix. - if _, err := e.validateAndEditCreateTableStatement(ctx, onlineDDL, newCreateTable); err != nil { + if _, err := e.validateAndEditCreateTableStatement(onlineDDL, newCreateTable); err != nil { return failMigration(err) } ddlStmt = newCreateTable diff --git a/go/vt/vttablet/onlineddl/executor_test.go b/go/vt/vttablet/onlineddl/executor_test.go index 1279f18e45b..92740548250 100644 --- a/go/vt/vttablet/onlineddl/executor_test.go +++ b/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 {