[latest-18.0](vitessio#4653): CherryPick(vitessio#15432): DDL strateg…

…y flag `--unsafe-allow-foreign-keys` implies setting `FOREIGN_KEY_CHECKS=0` (vitessio#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>

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 ... <list of all tables>`
  // 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
  }

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 {

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 <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

@@ -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 {