VReplication: Properly Handle FK Constraints When Deferring Secondary…

… Keys (#14543) Signed-off-by: Matt Lord <[email protected]>
vitessio · Dec 4, 2023 · 69e65bd · 69e65bd
1 parent 645ae47
commit 69e65bd
Show file tree

Hide file tree

Showing 2 changed files with 77 additions and 3 deletions.
diff --git a/go/vt/vttablet/tabletmanager/vreplication/vreplicator.go b/go/vt/vttablet/tabletmanager/vreplication/vreplicator.go
@@ -739,8 +739,27 @@ func (vr *vreplicator) getTableSecondaryKeys(ctx context.Context, tableName stri
 		return nil, fmt.Errorf("could not determine CREATE TABLE statement from table schema %q", tableSchema)
 	}
 
-	for _, index := range createTable.GetTableSpec().Indexes {
+	tableSpec := createTable.GetTableSpec()
+	fkIndexCols := make(map[string]bool)
+	for _, constraint := range tableSpec.Constraints {
+		if fkDef, ok := constraint.Details.(*sqlparser.ForeignKeyDefinition); ok {
+			fkCols := make([]string, len(fkDef.Source))
+			for i, fkCol := range fkDef.Source {
+				fkCols[i] = fkCol.Lowered()
+			}
+			fkIndexCols[strings.Join(fkCols, ",")] = true
+		}
+	}
+	for _, index := range tableSpec.Indexes {
 		if index.Info.Type != sqlparser.IndexTypePrimary {
+			cols := make([]string, len(index.Columns))
+			for i, col := range index.Columns {
+				cols[i] = col.Column.Lowered()
+			}
+			if fkIndexCols[strings.Join(cols, ",")] {
+				// This index is needed for a FK constraint so we cannot drop it.
+				continue
+			}
 			secondaryKeys = append(secondaryKeys, index)
 		}
 	}

diff --git a/go/vt/vttablet/tabletmanager/vreplication/vreplicator_test.go b/go/vt/vttablet/tabletmanager/vreplication/vreplicator_test.go
@@ -34,9 +34,10 @@ import (
 	"vitess.io/vitess/go/vt/binlog/binlogplayer"
 	"vitess.io/vitess/go/vt/dbconfigs"
 	"vitess.io/vitess/go/vt/mysqlctl"
+	"vitess.io/vitess/go/vt/schemadiff"
+
 	binlogdatapb "vitess.io/vitess/go/vt/proto/binlogdata"
 	tabletmanagerdatapb "vitess.io/vitess/go/vt/proto/tabletmanagerdata"
-	"vitess.io/vitess/go/vt/schemadiff"
 )
 
 func TestRecalculatePKColsInfoByColumnNames(t *testing.T) {
@@ -256,6 +257,7 @@ func TestDeferSecondaryKeys(t *testing.T) {
 		wantStashErr          string
 		wantExecErr           string
 		expectFinalSchemaDiff bool
+		preStashHook          func() error
 		postStashHook         func() error
 	}{
 		{
@@ -297,6 +299,54 @@ func TestDeferSecondaryKeys(t *testing.T) {
 			actionDDL:    "alter table %s.t1 add key c1 (c1), add key c2 (c2)",
 			WorkflowType: int32(binlogdatapb.VReplicationWorkflowType_MoveTables),
 		},
+		{
+			name:       "2SK:1FK",
+			tableName:  "t1",
+			initialDDL: "create table t1 (id int not null, c1 int default null, c2 int default null, t2_id int not null, primary key (id), key c1 (c1), key c2 (c2), foreign key (t2_id) references t2 (id))",
+			// Secondary key t2_id is needed to enforce the FK constraint so we do not drop it.
+			strippedDDL:  "create table t1 (id int not null, c1 int default null, c2 int default null, t2_id int not null, primary key (id), key t2_id (t2_id), constraint t1_ibfk_1 foreign key (t2_id) references t2 (id))",
+			actionDDL:    "alter table %s.t1 add key c1 (c1), add key c2 (c2)",
+			WorkflowType: int32(binlogdatapb.VReplicationWorkflowType_MoveTables),
+			preStashHook: func() error {
+				if _, err := dbClient.ExecuteFetch("drop table if exists t2", 1); err != nil {
+					return err
+				}
+				_, err = dbClient.ExecuteFetch("create table t2 (id int not null, c1 int not null, primary key (id))", 1)
+				return err
+			},
+		},
+		{
+			name:       "3SK:2FK",
+			tableName:  "t1",
+			initialDDL: "create table t1 (id int not null, id2 int default null, c1 int default null, c2 int default null, c3 int default null, t2_id int not null, t2_id2 int not null, primary key (id), key c1 (c1), key c2 (c2), foreign key (t2_id) references t2 (id), key c3 (c3), foreign key (t2_id2) references t2 (id2))",
+			// Secondary keys t2_id and t2_id2 are needed to enforce the FK constraint so we do not drop them.
+			strippedDDL:  "create table t1 (id int not null, id2 int default null, c1 int default null, c2 int default null, c3 int default null, t2_id int not null, t2_id2 int not null, primary key (id), key t2_id (t2_id), key t2_id2 (t2_id2), constraint t1_ibfk_1 foreign key (t2_id) references t2 (id), constraint t1_ibfk_2 foreign key (t2_id2) references t2 (id2))",
+			actionDDL:    "alter table %s.t1 add key c1 (c1), add key c2 (c2), add key c3 (c3)",
+			WorkflowType: int32(binlogdatapb.VReplicationWorkflowType_MoveTables),
+			preStashHook: func() error {
+				if _, err := dbClient.ExecuteFetch("drop table if exists t2", 1); err != nil {
+					return err
+				}
+				_, err = dbClient.ExecuteFetch("create table t2 (id int not null, id2 int default null, c1 int not null, primary key (id), key (id2))", 1)
+				return err
+			},
+		},
+		{
+			name:       "5SK:2FK_multi-column",
+			tableName:  "t1",
+			initialDDL: "create table t1 (id int not null, id2 int default null, c1 int default null, c2 int default null, c3 int default null, t2_id int not null, t2_id2 int not null, primary key (id), key c1 (c1), key c2 (c2), key t2_cs (c1,c2), key t2_ids (t2_id,t2_id2), foreign key (t2_id,t2_id2) references t2 (id, id2), key c3 (c3), foreign key (c1, c2) references t2 (c1, c2))",
+			// Secondary keys t2_ids and t2_cs are needed to enforce the FK constraint so we do not drop them.
+			strippedDDL:  "create table t1 (id int not null, id2 int default null, c1 int default null, c2 int default null, c3 int default null, t2_id int not null, t2_id2 int not null, primary key (id), key t2_cs (c1,c2), key t2_ids (t2_id,t2_id2), constraint t1_ibfk_1 foreign key (t2_id, t2_id2) references t2 (id, id2), constraint t1_ibfk_2 foreign key (c1, c2) references t2 (c1, c2))",
+			actionDDL:    "alter table %s.t1 add key c1 (c1), add key c2 (c2), add key c3 (c3)",
+			WorkflowType: int32(binlogdatapb.VReplicationWorkflowType_MoveTables),
+			preStashHook: func() error {
+				if _, err := dbClient.ExecuteFetch("drop table if exists t2", 1); err != nil {
+					return err
+				}
+				_, err = dbClient.ExecuteFetch("create table t2 (id int not null, id2 int not null, c1 int not null, c2 int not null, primary key (id,id2), key (c1,c2))", 1)
+				return err
+			},
+		},
 		{
 			name:         "2tSK",
 			tableName:    "t1",
@@ -425,6 +475,11 @@ func TestDeferSecondaryKeys(t *testing.T) {
 			// MoveTables and Reshard workflows.
 			vr.WorkflowType = tcase.WorkflowType
 
+			if tcase.preStashHook != nil {
+				err = tcase.preStashHook()
+				require.NoError(t, err, "error executing pre stash hook: %v", err)
+			}
+
 			// Create the table.
 			_, err := dbClient.ExecuteFetch(tcase.initialDDL, 1)
 			require.NoError(t, err)
@@ -456,7 +511,7 @@ func TestDeferSecondaryKeys(t *testing.T) {
 
 			if tcase.postStashHook != nil {
 				err = tcase.postStashHook()
-				require.NoError(t, err)
+				require.NoError(t, err, "error executing post stash hook: %v", err)
 
 				// We should still NOT have any secondary keys because there's still
 				// a running controller/vreplicator in the copy phase.