From 5a531f3ff34be88762d16bb59ed750fa4a71d9cf Mon Sep 17 00:00:00 2001 From: PlanetScale Actions Bot <60239337+planetscale-actions-bot@users.noreply.github.com> Date: Mon, 11 Mar 2024 23:18:19 -0700 Subject: [PATCH] [latest-18.0](#4653): CherryPick(#15432): DDL strategy flag `--unsafe-allow-foreign-keys` implies setting `FOREIGN_KEY_CHECKS=0` (#4654) * backport of 4653 * resolved conflict Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --------- Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Co-authored-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- .../scheduler/onlineddl_scheduler_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 +-- 4 files changed, 48 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 c249cf535a3..07f91e968d4 100644 --- a/go/test/endtoend/onlineddl/scheduler/onlineddl_scheduler_test.go +++ b/go/test/endtoend/onlineddl/scheduler/onlineddl_scheduler_test.go @@ -2213,6 +2213,7 @@ func testForeignKeys(t *testing.T) { sql string allowForeignKeys bool expectHint string + expectCountUUIDs int onlyIfFKOnlineDDLPossible bool } var testCases = []testCase{ @@ -2285,6 +2286,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 @@ -2327,6 +2338,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'") @@ -2363,7 +2377,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) @@ -2383,7 +2400,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{} @@ -2393,7 +2410,7 @@ func testForeignKeys(t *testing.T) { continue } statement := fmt.Sprintf("DROP TABLE IF EXISTS %s", artifact) - _, err := clusterInstance.VtctlclientProcess.ApplySchemaWithOutput(keyspaceName, statement, cluster.VtctlClientParams{DDLStrategy: "direct"}) + _, err := clusterInstance.VtctlclientProcess.ApplySchemaWithOutput(keyspaceName, statement, cluster.VtctlClientParams{DDLStrategy: "direct --unsafe-allow-foreign-keys"}) if err == nil { droppedTables[artifact] = true } diff --git a/go/vt/schemamanager/tablet_executor.go b/go/vt/schemamanager/tablet_executor.go index a0a901abc35..02907dfc9e4 100644 --- a/go/vt/schemamanager/tablet_executor.go +++ b/go/vt/schemamanager/tablet_executor.go @@ -488,10 +488,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 6db32918bf9..d9bf14babb4 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 } @@ -1472,7 +1487,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 } @@ -2991,7 +3006,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 6471a672282..19734a97a3e 100644 --- a/go/vt/vttablet/onlineddl/executor_test.go +++ b/go/vt/vttablet/onlineddl/executor_test.go @@ -167,7 +167,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 @@ -283,7 +283,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) altersStrings := []string{} for _, alter := range alters {