Skip to content

Commit

Permalink
schemadiff: identify a FK sequential execution scenario, and more (#1…
Browse files Browse the repository at this point in the history 
…4397)

Signed-off-by: Shlomi Noach <[email protected]>
  • Loading branch information
@shlomi-noach
shlomi-noach authored Oct 31, 2023
1 parent ae2c8ba commit 54eedf8
Show file tree
Hide file tree
Showing 2 changed files with 163 additions and 48 deletions.
122 changes: 77 additions & 45 deletions go/vt/schemadiff/schema.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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) {
Expand All @@ -815,51 +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
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)
}
}
return true, nil
}, parentDiff.Statement())
}
}

return checkChildForeignKeyDefinition(fk, diff)
case *sqlparser.DropKey:
if node.Type != sqlparser.ForeignKeyType {
// Not interesting
Expand Down
89 changes: 86 additions & 3 deletions go/vt/schemadiff/schema_diff_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);",
Expand All @@ -567,6 +567,7 @@ func TestSchemaDiff(t *testing.T) {
expectDiffs: 2,
expectDeps: 1,
entityOrder: []string{"tp", "t3"},
sequential: true,
},
{
name: "add FK",
Expand Down Expand Up @@ -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",
Expand All @@ -672,6 +674,70 @@ 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: "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: "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",
Expand Down Expand Up @@ -755,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 {
Expand Down Expand Up @@ -790,11 +870,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)
}
Expand Down

0 comments on commit 54eedf8

Please sign in to comment.