Skip to content

Commit

Permalink
A CREATE TABLE ... FOREIGN KEY introduces a foreign key just as ALTER…
Browse files Browse the repository at this point in the history 
… 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>
  • Loading branch information
@shlomi-noach
shlomi-noach committed Oct 31, 2023
1 parent f0f786e commit df90035
Show file tree
Hide file tree
Showing 2 changed files with 94 additions and 61 deletions.
137 changes: 77 additions & 60 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,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
Expand Down
18 changes: 17 additions & 1 deletion 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 Down Expand Up @@ -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 {
Expand Down

0 comments on commit df90035

Please sign in to comment.