From 6caaa92f709b4885b3d82e0f83144dde00d6aba6 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Tue, 31 Oct 2023 07:59:21 +0200 Subject: [PATCH 1/4] schemadiff: identify a FK sequential execution scenario where adding an index over foreign key parent referenced columns Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schemadiff/schema.go | 17 ++++++++++++++++- go/vt/schemadiff/schema_diff_test.go | 28 ++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/go/vt/schemadiff/schema.go b/go/vt/schemadiff/schema.go index a9ef60fbb27..145ddd0c46f 100644 --- a/go/vt/schemadiff/schema.go +++ b/go/vt/schemadiff/schema.go @@ -832,7 +832,9 @@ func (s *Schema) SchemaDiff(other *Schema, hints *DiffHints) (*SchemaDiff, error // The current diff is ALTER TABLE ... ADD FOREIGN KEY // and the parent table also has an ALTER TABLE. // so if the parent's ALTER in any way modifies the referenced FK columns, that's - // a sequential execution dependency + // a sequential execution dependency. + // Also, if there is no index on the parent's referenced columns, and a migration adds an index + // on those columns, that's a sequential execution dependency. referencedColumnNames := map[string]bool{} for _, referencedColumn := range fk.ReferenceDefinition.ReferencedColumns { referencedColumnNames[referencedColumn.Lowered()] = true @@ -854,6 +856,19 @@ func (s *Schema) SchemaDiff(other *Schema, hints *DiffHints) (*SchemaDiff, error if referencedColumnNames[node.Name.Name.Lowered()] { schemaDiff.addDep(diff, parentDiff, DiffDependencySequentialExecution) } + case *sqlparser.AddIndexDefinition: + referencedTableEntity, _ := parentDiff.Entities() + // We _know_ the type is *CreateTableEntity + referencedTable, _ := referencedTableEntity.(*CreateTableEntity) + if indexCoversColumnsInOrder(node.IndexDefinition, fk.ReferenceDefinition.ReferencedColumns) { + // This diff adds an index covering referenced columns + if !referencedTable.columnsCoveredByInOrderIndex(fk.ReferenceDefinition.ReferencedColumns) { + // And there was no earlier index on referenced columns. So this is a new index. + // In MySQL, you can't add a foreign key constraint on a child, before the parent + // has an index of referenced columns. This is a sequential dependency. + schemaDiff.addDep(diff, parentDiff, DiffDependencySequentialExecution) + } + } } return true, nil }, parentDiff.Statement()) diff --git a/go/vt/schemadiff/schema_diff_test.go b/go/vt/schemadiff/schema_diff_test.go index d835177c795..ee5a5663a76 100644 --- a/go/vt/schemadiff/schema_diff_test.go +++ b/go/vt/schemadiff/schema_diff_test.go @@ -672,6 +672,34 @@ func TestSchemaDiff(t *testing.T) { expectDeps: 1, sequential: true, entityOrder: []string{"t2", "t1"}, + }, { + name: "add index on parent. add FK to index column", + toQueries: []string{ + "create table t1 (id int primary key, info int not null, key info_idx(info));", + "create table t2 (id int primary key, ts timestamp, t1_info int not null, constraint parent_info_fk foreign key (t1_info) references t1 (info));", + "create view v1 as select id from t1", + }, + expectDiffs: 2, + expectDeps: 1, + sequential: true, + entityOrder: []string{"t1", "t2"}, + }, + { + name: "add index on parent with existing index. add FK to index column", + fromQueries: []string{ + "create table t1 (id int primary key, info int not null, key existing_info_idx(info));", + "create table t2 (id int primary key, ts timestamp);", + "create view v1 as select id from t1", + }, + toQueries: []string{ + "create table t1 (id int primary key, info int not null, key existing_info_idx(info), key info_idx(info));", + "create table t2 (id int primary key, ts timestamp, t1_info int not null, constraint parent_info_fk foreign key (t1_info) references t1 (info));", + "create view v1 as select id from t1", + }, + expectDiffs: 2, + expectDeps: 1, + sequential: false, + entityOrder: []string{"t1", "t2"}, }, { name: "drop fk", From f0db592dd9b892b0957ad3ea558bc26b555d95d4 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Tue, 31 Oct 2023 10:30:47 +0200 Subject: [PATCH 2/4] while we're here, added a test for an impossible order FK scenario Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schemadiff/schema_diff_test.go | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/go/vt/schemadiff/schema_diff_test.go b/go/vt/schemadiff/schema_diff_test.go index ee5a5663a76..58b4c43afee 100644 --- a/go/vt/schemadiff/schema_diff_test.go +++ b/go/vt/schemadiff/schema_diff_test.go @@ -701,6 +701,21 @@ func TestSchemaDiff(t *testing.T) { sequential: false, entityOrder: []string{"t1", "t2"}, }, + { + name: "modify fk column types, fail", + fromQueries: []string{ + "create table t1 (id int primary key);", + "create table t2 (id int primary key, ts timestamp, t1_id int, foreign key (t1_id) references t1 (id) on delete no action);", + }, + toQueries: []string{ + "create table t1 (id bigint primary key);", + "create table t2 (id int primary key, ts timestamp, t1_id bigint, foreign key (t1_id) references t1 (id) on delete no action);", + }, + expectDiffs: 2, + expectDeps: 0, + sequential: false, + conflictingDiffs: 1, + }, { name: "drop fk", fromQueries: []string{ @@ -818,11 +833,14 @@ func TestSchemaDiff(t *testing.T) { orderedDiffs, err := schemaDiff.OrderedDiffs(ctx) if tc.conflictingDiffs > 0 { - require.Greater(t, tc.conflictingDiffs, 1) // self integrity. If there's a conflict, then obviously there's at least two conflicting diffs (a single diff has nothing to conflict with) assert.Error(t, err) impossibleOrderErr, ok := err.(*ImpossibleApplyDiffOrderError) assert.True(t, ok) - assert.Equal(t, tc.conflictingDiffs, len(impossibleOrderErr.ConflictingDiffs)) + conflictingDiffsStatements := []string{} + for _, diff := range impossibleOrderErr.ConflictingDiffs { + conflictingDiffsStatements = append(conflictingDiffsStatements, diff.CanonicalStatementString()) + } + assert.Equalf(t, tc.conflictingDiffs, len(impossibleOrderErr.ConflictingDiffs), "found conflicting diffs: %+v\n diff statements=%+v", conflictingDiffsStatements, allDiffsStatements) } else { require.NoErrorf(t, err, "Unordered diffs: %v", allDiffsStatements) } From f0f786ef34a0e9df93316d133579a0ec7e6b82c3 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Tue, 31 Oct 2023 10:45:11 +0200 Subject: [PATCH 3/4] yet another interesting FK unit test Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schemadiff/schema_diff_test.go | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/go/vt/schemadiff/schema_diff_test.go b/go/vt/schemadiff/schema_diff_test.go index 58b4c43afee..0a0eb94712c 100644 --- a/go/vt/schemadiff/schema_diff_test.go +++ b/go/vt/schemadiff/schema_diff_test.go @@ -716,6 +716,27 @@ func TestSchemaDiff(t *testing.T) { sequential: false, conflictingDiffs: 1, }, + { + name: "add hierarchical constraints", + fromQueries: []string{ + "create table t1 (id int primary key, ref int, key ref_idx (ref));", + "create table t2 (id int primary key, ref int, key ref_idx (ref));", + "create table t3 (id int primary key, ref int, key ref_idx (ref));", + "create table t4 (id int primary key, ref int, key ref_idx (ref));", + "create table t5 (id int primary key, ref int, key ref_idx (ref));", + }, + toQueries: []string{ + "create table t1 (id int primary key, ref int, key ref_idx (ref));", + "create table t2 (id int primary key, ref int, key ref_idx (ref), foreign key (ref) references t1 (id) on delete no action);", + "create table t3 (id int primary key, ref int, key ref_idx (ref), foreign key (ref) references t2 (id) on delete no action);", + "create table t4 (id int primary key, ref int, key ref_idx (ref), foreign key (ref) references t3 (id) on delete no action);", + "create table t5 (id int primary key, ref int, key ref_idx (ref), foreign key (ref) references t1 (id) on delete no action);", + }, + expectDiffs: 4, + expectDeps: 2, // t2<->t3, t3<->t4 + sequential: false, + entityOrder: []string{"t2", "t3", "t4", "t5"}, + }, { name: "drop fk", fromQueries: []string{ From df9003584a638b626fc1facc2c1f31318a4cd717 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Tue, 31 Oct 2023 11:37:19 +0200 Subject: [PATCH 4/4] A CREATE TABLE ... FOREIGN KEY introduces a foreign key just as ALTER TABLE ... ADD FOREIGN KEY does, and the dependency resolving logic applies to both in the same way Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schemadiff/schema.go | 137 +++++++++++++++------------ go/vt/schemadiff/schema_diff_test.go | 18 +++- 2 files changed, 94 insertions(+), 61 deletions(-) diff --git a/go/vt/schemadiff/schema.go b/go/vt/schemadiff/schema.go index 145ddd0c46f..9180012676f 100644 --- a/go/vt/schemadiff/schema.go +++ b/go/vt/schemadiff/schema.go @@ -795,6 +795,69 @@ func (s *Schema) SchemaDiff(other *Schema, hints *DiffHints) (*SchemaDiff, error return dependentDiffs, relationsMade } + checkChildForeignKeyDefinition := func(fk *sqlparser.ForeignKeyDefinition, diff EntityDiff) (bool, error) { + // We add a foreign key. Normally that's fine, expect for a couple specific scenarios + parentTableName := fk.ReferenceDefinition.ReferencedTable.Name.String() + dependentDiffs, ok := checkDependencies(diff, []string{parentTableName}) + if !ok { + // No dependency. Not interesting + return true, nil + } + for _, parentDiff := range dependentDiffs { + switch parentDiff := parentDiff.(type) { + case *CreateTableEntityDiff: + // We add a foreign key constraint onto a new table... That table must therefore be first created, + // and only then can we proceed to add the FK + schemaDiff.addDep(diff, parentDiff, DiffDependencySequentialExecution) + case *AlterTableEntityDiff: + // The current diff is ALTER TABLE ... ADD FOREIGN KEY, or it is a CREATE TABLE with a FOREIGN KEY + // and the parent table also has an ALTER TABLE. + // so if the parent's ALTER in any way modifies the referenced FK columns, that's + // a sequential execution dependency. + // Also, if there is no index on the parent's referenced columns, and a migration adds an index + // on those columns, that's a sequential execution dependency. + referencedColumnNames := map[string]bool{} + for _, referencedColumn := range fk.ReferenceDefinition.ReferencedColumns { + referencedColumnNames[referencedColumn.Lowered()] = true + } + // Walk parentDiff.Statement() + _ = sqlparser.Walk(func(node sqlparser.SQLNode) (kontinue bool, err error) { + switch node := node.(type) { + case *sqlparser.ModifyColumn: + if referencedColumnNames[node.NewColDefinition.Name.Lowered()] { + schemaDiff.addDep(diff, parentDiff, DiffDependencySequentialExecution) + } + case *sqlparser.AddColumns: + for _, col := range node.Columns { + if referencedColumnNames[col.Name.Lowered()] { + schemaDiff.addDep(diff, parentDiff, DiffDependencySequentialExecution) + } + } + case *sqlparser.DropColumn: + if referencedColumnNames[node.Name.Name.Lowered()] { + schemaDiff.addDep(diff, parentDiff, DiffDependencySequentialExecution) + } + case *sqlparser.AddIndexDefinition: + referencedTableEntity, _ := parentDiff.Entities() + // We _know_ the type is *CreateTableEntity + referencedTable, _ := referencedTableEntity.(*CreateTableEntity) + if indexCoversColumnsInOrder(node.IndexDefinition, fk.ReferenceDefinition.ReferencedColumns) { + // This diff adds an index covering referenced columns + if !referencedTable.columnsCoveredByInOrderIndex(fk.ReferenceDefinition.ReferencedColumns) { + // And there was no earlier index on referenced columns. So this is a new index. + // In MySQL, you can't add a foreign key constraint on a child, before the parent + // has an index of referenced columns. This is a sequential dependency. + schemaDiff.addDep(diff, parentDiff, DiffDependencySequentialExecution) + } + } + } + return true, nil + }, parentDiff.Statement()) + } + } + return true, nil + } + for _, diff := range schemaDiff.UnorderedDiffs() { switch diff := diff.(type) { case *CreateViewEntityDiff: @@ -806,6 +869,19 @@ func (s *Schema) SchemaDiff(other *Schema, hints *DiffHints) (*SchemaDiff, error checkDependencies(diff, getViewDependentTableNames(diff.from.CreateView)) case *CreateTableEntityDiff: checkDependencies(diff, getForeignKeyParentTableNames(diff.CreateTable())) + _ = sqlparser.Walk(func(node sqlparser.SQLNode) (kontinue bool, err error) { + switch node := node.(type) { + case *sqlparser.ConstraintDefinition: + // Only interested in a foreign key + fk, ok := node.Details.(*sqlparser.ForeignKeyDefinition) + if !ok { + return true, nil + } + return checkChildForeignKeyDefinition(fk, diff) + } + return true, nil + }, diff.Statement()) + case *AlterTableEntityDiff: _ = sqlparser.Walk(func(node sqlparser.SQLNode) (kontinue bool, err error) { switch node := node.(type) { @@ -815,66 +891,7 @@ func (s *Schema) SchemaDiff(other *Schema, hints *DiffHints) (*SchemaDiff, error if !ok { return true, nil } - // We add a foreign key. Normally that's fine, expect for a couple specific scenarios - parentTableName := fk.ReferenceDefinition.ReferencedTable.Name.String() - dependentDiffs, ok := checkDependencies(diff, []string{parentTableName}) - if !ok { - // No dependency. Not interesting - return true, nil - } - for _, parentDiff := range dependentDiffs { - switch parentDiff := parentDiff.(type) { - case *CreateTableEntityDiff: - // We add a foreign key constraint onto a new table... That table must therefore be first created, - // and only then can we proceed to add the FK - schemaDiff.addDep(diff, parentDiff, DiffDependencySequentialExecution) - case *AlterTableEntityDiff: - // The current diff is ALTER TABLE ... ADD FOREIGN KEY - // and the parent table also has an ALTER TABLE. - // so if the parent's ALTER in any way modifies the referenced FK columns, that's - // a sequential execution dependency. - // Also, if there is no index on the parent's referenced columns, and a migration adds an index - // on those columns, that's a sequential execution dependency. - referencedColumnNames := map[string]bool{} - for _, referencedColumn := range fk.ReferenceDefinition.ReferencedColumns { - referencedColumnNames[referencedColumn.Lowered()] = true - } - // Walk parentDiff.Statement() - _ = sqlparser.Walk(func(node sqlparser.SQLNode) (kontinue bool, err error) { - switch node := node.(type) { - case *sqlparser.ModifyColumn: - if referencedColumnNames[node.NewColDefinition.Name.Lowered()] { - schemaDiff.addDep(diff, parentDiff, DiffDependencySequentialExecution) - } - case *sqlparser.AddColumns: - for _, col := range node.Columns { - if referencedColumnNames[col.Name.Lowered()] { - schemaDiff.addDep(diff, parentDiff, DiffDependencySequentialExecution) - } - } - case *sqlparser.DropColumn: - if referencedColumnNames[node.Name.Name.Lowered()] { - schemaDiff.addDep(diff, parentDiff, DiffDependencySequentialExecution) - } - case *sqlparser.AddIndexDefinition: - referencedTableEntity, _ := parentDiff.Entities() - // We _know_ the type is *CreateTableEntity - referencedTable, _ := referencedTableEntity.(*CreateTableEntity) - if indexCoversColumnsInOrder(node.IndexDefinition, fk.ReferenceDefinition.ReferencedColumns) { - // This diff adds an index covering referenced columns - if !referencedTable.columnsCoveredByInOrderIndex(fk.ReferenceDefinition.ReferencedColumns) { - // And there was no earlier index on referenced columns. So this is a new index. - // In MySQL, you can't add a foreign key constraint on a child, before the parent - // has an index of referenced columns. This is a sequential dependency. - schemaDiff.addDep(diff, parentDiff, DiffDependencySequentialExecution) - } - } - } - return true, nil - }, parentDiff.Statement()) - } - } - + return checkChildForeignKeyDefinition(fk, diff) case *sqlparser.DropKey: if node.Type != sqlparser.ForeignKeyType { // Not interesting diff --git a/go/vt/schemadiff/schema_diff_test.go b/go/vt/schemadiff/schema_diff_test.go index 0a0eb94712c..df7d893356f 100644 --- a/go/vt/schemadiff/schema_diff_test.go +++ b/go/vt/schemadiff/schema_diff_test.go @@ -558,7 +558,7 @@ func TestSchemaDiff(t *testing.T) { entityOrder: []string{"t3"}, }, { - name: "create two table with fk", + name: "create two tables with fk", toQueries: append( createQueries, "create table tp (id int primary key, info int not null);", @@ -567,6 +567,7 @@ func TestSchemaDiff(t *testing.T) { expectDiffs: 2, expectDeps: 1, entityOrder: []string{"tp", "t3"}, + sequential: true, }, { name: "add FK", @@ -648,6 +649,7 @@ func TestSchemaDiff(t *testing.T) { expectDiffs: 2, expectDeps: 1, entityOrder: []string{"t1", "t3"}, + sequential: true, }, { name: "add column. add FK referencing new column", @@ -819,6 +821,20 @@ func TestSchemaDiff(t *testing.T) { expectDeps: 0, entityOrder: []string{"t1"}, }, + { + name: "test", + fromQueries: []string{ + "CREATE TABLE t1 (id bigint NOT NULL, name varchar(255), PRIMARY KEY (id))", + }, + toQueries: []string{ + "CREATE TABLE t1 (id bigint NOT NULL, name varchar(255), PRIMARY KEY (id), KEY idx_name (name))", + "CREATE TABLE t3 (id bigint NOT NULL, name varchar(255), t1_id bigint, PRIMARY KEY (id), KEY t1_id (t1_id), KEY nameidx (name), CONSTRAINT t3_ibfk_1 FOREIGN KEY (t1_id) REFERENCES t1 (id) ON DELETE CASCADE ON UPDATE CASCADE, CONSTRAINT t3_ibfk_2 FOREIGN KEY (name) REFERENCES t1 (name) ON DELETE CASCADE ON UPDATE CASCADE)", + }, + expectDiffs: 2, + expectDeps: 1, + sequential: true, + entityOrder: []string{"t1", "t3"}, + }, } hints := &DiffHints{RangeRotationStrategy: RangeRotationDistinctStatements} for _, tc := range tt {