schemadiff: improved diff ordering with various foreign key strategies

go/vt/schemadiff/schema.go

@@ -234,7 +234,7 @@ func (s *Schema) normalize(hints *DiffHints) error {
  // already handled; skip
  continue
  }
- // Not handled. Is this view dependent on already handled objects?
+ // Not handled. Does this table reference an already handled table?
  referencedTableNames := getForeignKeyParentTableNames(t.CreateTable)
  if len(referencedTableNames) > 0 {
  s.foreignKeyChildren = append(s.foreignKeyChildren, t)

go/vt/schemadiff/schema_diff.go

@@ -88,27 +88,27 @@ Modified to have an early break
 
 // permutateDiffs calls `callback` with each permutation of a. If the function returns `true`, that means
 // the callback has returned `true` for an early break, thus possibly not all permutations have been evaluated.
-func permutateDiffs(ctx context.Context, diffs []EntityDiff, callback func([]EntityDiff) (earlyBreak bool)) (earlyBreak bool, err error) {
+func permutateDiffs(ctx context.Context, diffs []EntityDiff, hints *DiffHints, callback func([]EntityDiff, *DiffHints) (earlyBreak bool)) (earlyBreak bool, err error) {
  if len(diffs) == 0 {
  return false, nil
  }
  // Sort by a heuristic (DROPs first, ALTERs next, CREATEs last). This ordering is then used first in the permutation
  // search and serves as seed for the rest of permutations.
 
- return permDiff(ctx, diffs, callback, 0)
+ return permDiff(ctx, diffs, hints, callback, 0)
 }
 
 // permDiff is a recursive function to permutate given `a` and call `callback` for each permutation.
 // If `callback` returns `true`, then so does this function, and this indicates a request for an early
 // break, in which case this function will not be called again.
-func permDiff(ctx context.Context, a []EntityDiff, callback func([]EntityDiff) (earlyBreak bool), i int) (earlyBreak bool, err error) {
+func permDiff(ctx context.Context, a []EntityDiff, hints *DiffHints, callback func([]EntityDiff, *DiffHints) (earlyBreak bool), i int) (earlyBreak bool, err error) {
  if err := ctx.Err(); err != nil {
  return true, err // early break
  }
  if i > len(a) {
- return callback(a), nil
+ return callback(a, hints), nil
  }
- if brk, err := permDiff(ctx, a, callback, i+1); brk {
+ if brk, err := permDiff(ctx, a, hints, callback, i+1); brk {
  return true, err
  }
  for j := i + 1; j < len(a); j++ {
@@ -150,7 +150,7 @@ func permDiff(ctx context.Context, a []EntityDiff, callback func([]EntityDiff) (
  }
  // End of optimization
  a[i], a[j] = a[j], a[i]
- if brk, err := permDiff(ctx, a, callback, i+1); brk {
+ if brk, err := permDiff(ctx, a, hints, callback, i+1); brk {
  return true, err
  }
  a[i], a[j] = a[j], a[i]
@@ -315,21 +315,65 @@ func (d *SchemaDiff) OrderedDiffs(ctx context.Context) ([]EntityDiff, error) {
 
  // We will now permutate the diffs in this equivalence class, and hopefully find
  // a valid permutation (one where if we apply the diffs in-order, the schema remains valid throughout the process)
- foundValidPathForClass, err := permutateDiffs(ctx, classDiffs, func(permutatedDiffs []EntityDiff) bool {
- permutationSchema := lastGoodSchema.copy()
- // We want to apply the changes one by one, and validate the schema after each change
- for i := range permutatedDiffs {
- // apply inline
- if err := permutationSchema.apply(permutatedDiffs[i:i+1], d.hints); err != nil {
- // permutation is invalid
- return false // continue searching
+ tryPermutateDiffs := func(hints *DiffHints) (bool, error) {
+ return permutateDiffs(ctx, classDiffs, hints, func(permutatedDiffs []EntityDiff, hints *DiffHints) bool {
+ permutationSchema := lastGoodSchema.copy()
+ // We want to apply the changes one by one, and validate the schema after each change
+ for i := range permutatedDiffs {
+ // apply inline
+ applyHints := hints
+ if hints.ForeignKeyCheckStrategy == ForeignKeyCheckStrategyCreateTableFirst {
+ // This is a strategy that handles foreign key loops in a heuristic way.
+ // It means: we allow for the very first change to be a CREATE TABLE, and ignore
+ // any dependencies that CREATE TABLE may have. But then, we require the rest of
+ // changes to have a strict order.
+ overrideHints := *hints
+ overrideHints.ForeignKeyCheckStrategy = ForeignKeyCheckStrategyStrict
+ if i == 0 {
+ if _, ok := permutatedDiffs[i].(*CreateTableEntityDiff); ok {
+ overrideHints.ForeignKeyCheckStrategy = ForeignKeyCheckStrategyIgnore
+ }
+ }
+ applyHints = &overrideHints
+ }
+ if err := permutationSchema.apply(permutatedDiffs[i:i+1], applyHints); err != nil {
+ // permutation is invalid
+ return false // continue searching
+ }
+ }
+
+ // Good news, we managed to apply all of the permutations!
+ orderedDiffs = append(orderedDiffs, permutatedDiffs...)
+ lastGoodSchema = permutationSchema
+ return true // early break! No need to keep searching
+ })
+ }
+ // We prefer stricter strategy, because that gives best chance of finding a valid path.
+ // So on best effort basis, we try to find a valid path starting with the strictest strategy, easing
+ // to more relaxed ones, but never below the preconfigured.
+ // For example, if the preconfigured strategy is "strict", we will try "strict" and then stop.
+ // If the preconfigured strategy is "create-table-first", we will try "strict", then "create-table-first", then stop.
+ tryPermutateDiffsAcrossStrategies := func() (found bool, err error) {
+ for _, fkStrategy := range []int{ForeignKeyCheckStrategyStrict, ForeignKeyCheckStrategyCreateTableFirst, ForeignKeyCheckStrategyIgnore} {
+ hints := *d.hints
+ hints.ForeignKeyCheckStrategy = fkStrategy
+ found, err = tryPermutateDiffs(&hints)
+ if fkStrategy == d.hints.ForeignKeyCheckStrategy {
+ // end of the line.
+ return found, err
+ }
+ if err != nil {
+ // No luck with this strategy, let's try the next one.
+ continue
+ }
+ if found {
+ // Good news!
+ return true, nil
  }
  }
- // Good news, we managed to apply all of the permutations!
- orderedDiffs = append(orderedDiffs, permutatedDiffs...)
- lastGoodSchema = permutationSchema
- return true // early break! No need to keep searching
- })
+ return found, err
+ }
+ foundValidPathForClass, err := tryPermutateDiffsAcrossStrategies()
  if err != nil {
  return nil, err
  }

go/vt/schemadiff/schema_diff_test.go

@@ -195,7 +195,7 @@ func TestPermutations(t *testing.T) {
  allDiffs := schemaDiff.UnorderedDiffs()
  originalSingleString := toSingleString(allDiffs)
  numEquals := 0
- earlyBreak, err := permutateDiffs(ctx, allDiffs, func(pdiffs []EntityDiff) (earlyBreak bool) {
+ earlyBreak, err := permutateDiffs(ctx, allDiffs, hints, func(pdiffs []EntityDiff, hints *DiffHints) (earlyBreak bool) {
  defer func() { iteration++ }()
  // cover all permutations
  singleString := toSingleString(pdiffs)
@@ -218,7 +218,7 @@ func TestPermutations(t *testing.T) {
  allPerms := map[string]bool{}
  allDiffs := schemaDiff.UnorderedDiffs()
  originalSingleString := toSingleString(allDiffs)
- earlyBreak, err := permutateDiffs(ctx, allDiffs, func(pdiffs []EntityDiff) (earlyBreak bool) {
+ earlyBreak, err := permutateDiffs(ctx, allDiffs, hints, func(pdiffs []EntityDiff, hints *DiffHints) (earlyBreak bool) {
  // Single visit
  allPerms[toSingleString(pdiffs)] = true
  // First permutation should be the same as original
@@ -244,8 +244,9 @@ func TestPermutationsContext(t *testing.T) {
  ctx, cancel := context.WithCancel(context.Background())
  cancel()
 
+ hints := &DiffHints{RangeRotationStrategy: RangeRotationDistinctStatements}
  allDiffs := []EntityDiff{&DropViewEntityDiff{}}
- earlyBreak, err := permutateDiffs(ctx, allDiffs, func(pdiffs []EntityDiff) (earlyBreak bool) {
+ earlyBreak, err := permutateDiffs(ctx, allDiffs, hints, func(pdiffs []EntityDiff, hints *DiffHints) (earlyBreak bool) {
  return false
  })
  assert.True(t, earlyBreak) // proves that termination was due to context cancel
@@ -678,6 +679,142 @@ func TestSchemaDiff(t *testing.T) {
  instantCapability: InstantDDLCapabilityIrrelevant,
  fkStrategy: ForeignKeyCheckStrategyIgnore,
  },
+ {
+ name: "two table cycle with create, strict",
+ fromQueries: append(createQueries,
+ "create table t11 (id int primary key, i0 int);",
+ ),
+ toQueries: append(
+ createQueries,
+ "create table t12 (id int primary key, i int, constraint f1201 foreign key (i) references t11 (i) on delete set null);",
+ "create table t11 (id int primary key, i0 int, i int, key (i), constraint f1101 foreign key (i) references t12 (id) on delete restrict);",
+ ),
+ expectDiffs: 2,
+ expectDeps: 2,
+ sequential: true,
+ instantCapability: InstantDDLCapabilityIrrelevant,
+ fkStrategy: ForeignKeyCheckStrategyStrict,
+ expectOrderedError: "no valid applicable order for diffs",
+ },
+ {
+ name: "two table cycle with create, strict, changed lexicographic order",
+ fromQueries: append(createQueries,
+ "create table t12 (id int primary key, i0 int);",
+ ),
+ toQueries: append(
+ createQueries,
+ "create table t11 (id int primary key, i int, constraint f1201 foreign key (i) references t12 (i) on delete set null);",
+ "create table t12 (id int primary key, i0 int, i int, key (i), constraint f1101 foreign key (i) references t11 (id) on delete restrict);",
+ ),
+ expectDiffs: 2,
+ expectDeps: 2,
+ sequential: true,
+ instantCapability: InstantDDLCapabilityImpossible,
+ fkStrategy: ForeignKeyCheckStrategyStrict,
+ expectOrderedError: "no valid applicable order for diffs",
+ },
+ {
+ name: "two table cycle with create, ignore",
+ fromQueries: append(createQueries,
+ "create table t11 (id int primary key, i0 int);",
+ ),
+ toQueries: append(
+ createQueries,
+ "create table t12 (id int primary key, i int, constraint f1201 foreign key (i) references t11 (i) on delete set null);",
+ "create table t11 (id int primary key, i0 int, i int, key (i), constraint f1101 foreign key (i) references t12 (id) on delete restrict);",
+ ),
+ expectDiffs: 2,
+ expectDeps: 2,
+ entityOrder: []string{"t11", "t12"},
+ sequential: true,
+ instantCapability: InstantDDLCapabilityImpossible,
+ fkStrategy: ForeignKeyCheckStrategyIgnore,
+ },
+ {
+ name: "two table cycle with create, ignore, changed lexicographic order",
+ fromQueries: append(createQueries,
+ "create table t12 (id int primary key, i0 int);",
+ ),
+ toQueries: append(
+ createQueries,
+ "create table t11 (id int primary key, i int, constraint f1201 foreign key (i) references t12 (i) on delete set null);",
+ "create table t12 (id int primary key, i0 int, i int, key (i), constraint f1101 foreign key (i) references t11 (id) on delete restrict);",
+ ),
+ expectDiffs: 2,
+ expectDeps: 2,
+ entityOrder: []string{"t12", "t11"},
+ sequential: true,
+ instantCapability: InstantDDLCapabilityImpossible,
+ fkStrategy: ForeignKeyCheckStrategyIgnore,
+ },
+ {
+ name: "two table cycle with create, existing column, ignore",
+ fromQueries: append(createQueries,
+ "create table t12 (id int primary key, i int, key (i));",
+ ),
+ toQueries: append(
+ createQueries,
+ "create table t11 (id int primary key, i int, constraint f1201 foreign key (i) references t12 (i) on delete set null);",
+ "create table t12 (id int primary key, i int, key (i), constraint f1101 foreign key (i) references t11 (id) on delete restrict);",
+ ),
+ expectDiffs: 2,
+ expectDeps: 2,
+ entityOrder: []string{"t11", "t12"},
+ sequential: true,
+ instantCapability: InstantDDLCapabilityImpossible,
+ fkStrategy: ForeignKeyCheckStrategyIgnore,
+ },
+ {
+ name: "two table cycle with create, existing column, changed lexicographic order, ignore",
+ fromQueries: append(createQueries,
+ "create table t11 (id int primary key, i int, key (i));",
+ ),
+ toQueries: append(
+ createQueries,
+ "create table t12 (id int primary key, i int, constraint f1201 foreign key (i) references t11 (i) on delete set null);",
+ "create table t11 (id int primary key, i int, key (i), constraint f1101 foreign key (i) references t12 (id) on delete restrict);",
+ ),
+ expectDiffs: 2,
+ expectDeps: 2,
+ entityOrder: []string{"t12", "t11"},
+ sequential: true,
+ instantCapability: InstantDDLCapabilityImpossible,
+ fkStrategy: ForeignKeyCheckStrategyIgnore,
+ },
+ {
+ name: "two table cycle with create, create table first",
+ fromQueries: append(createQueries,
+ "create table t12 (id int primary key, i int, key (i));",
+ ),
+ toQueries: append(
+ createQueries,
+ "create table t11 (id int primary key, i int, constraint f1201 foreign key (i) references t12 (i) on delete set null);",
+ "create table t12 (id int primary key, i int, key (i), constraint f1101 foreign key (i) references t11 (id) on delete restrict);",
+ ),
+ expectDiffs: 2,
+ expectDeps: 2,
+ entityOrder: []string{"t11", "t12"},
+ sequential: true,
+ instantCapability: InstantDDLCapabilityImpossible,
+ fkStrategy: ForeignKeyCheckStrategyCreateTableFirst,
+ },
+ {
+ name: "two table cycle with create, changed lexicographic order, create table first",
+ fromQueries: append(createQueries,
+ "create table t11 (id int primary key, i int, key (i));",
+ ),
+ toQueries: append(
+ createQueries,
+ "create table t12 (id int primary key, i int, constraint f1201 foreign key (i) references t11 (i) on delete set null);",
+ "create table t11 (id int primary key, i int, key (i), constraint f1101 foreign key (i) references t12 (id) on delete restrict);",
+ ),
+ expectDiffs: 2,
+ expectDeps: 2,
+ entityOrder: []string{"t12", "t11"},
+ sequential: true,
+ instantCapability: InstantDDLCapabilityImpossible,
+ fkStrategy: ForeignKeyCheckStrategyCreateTableFirst,
+ },
  {
  name: "add FK",
  toQueries: []string{
@@ -980,7 +1117,41 @@ func TestSchemaDiff(t *testing.T) {
  instantCapability: InstantDDLCapabilityImpossible,
  },
  {
- name: "add and drop FK, add and drop respective tables",
+ name: "add and drop FK, add and drop column, impossible order even with create table first strategy",
+ fromQueries: []string{
+ "create table t1 (id int primary key, p int, key p_idx (p));",
+ "create table t2 (id int primary key, p int, key p_idx (p), foreign key (p) references t1 (p) on delete no action);",
+ },
+ toQueries: []string{
+ "create table t1 (id int primary key, q int, key q_idx (q));",
+ "create table t2 (id int primary key, q int, key q_idx (q), foreign key (q) references t1 (q) on delete no action);",
+ },
+ expectDiffs: 2,
+ expectDeps: 1,
+ sequential: true,
+ conflictingDiffs: 2,
+ instantCapability: InstantDDLCapabilityImpossible,
+ fkStrategy: ForeignKeyCheckStrategyCreateTableFirst,
+ },
+ {
+ name: "add and drop FK, add and drop column, impossible order even with ignore strategy",
+ fromQueries: []string{
+ "create table t1 (id int primary key, p int, key p_idx (p));",
+ "create table t2 (id int primary key, p int, key p_idx (p), foreign key (p) references t1 (p) on delete no action);",
+ },
+ toQueries: []string{
+ "create table t1 (id int primary key, q int, key q_idx (q));",
+ "create table t2 (id int primary key, q int, key q_idx (q), foreign key (q) references t1 (q) on delete no action);",
+ },
+ expectDiffs: 2,
+ expectDeps: 1,
+ sequential: true,
+ conflictingDiffs: 2,
+ instantCapability: InstantDDLCapabilityImpossible,
+ fkStrategy: ForeignKeyCheckStrategyIgnore,
+ },
+ {
+ name: "add and drop FK, add and drop respective tables, fk strict",
  fromQueries: []string{
  "create table t1 (id int primary key, p int, key p_idx (p));",
  "create table t2 (id int primary key, p int, key p_idx (p), foreign key (p) references t1 (p) on delete no action);",
@@ -994,6 +1165,41 @@ func TestSchemaDiff(t *testing.T) {
  sequential: true,
  entityOrder: []string{"t3", "t2", "t1"},
  instantCapability: InstantDDLCapabilityImpossible,
+ fkStrategy: ForeignKeyCheckStrategyStrict,
+ },
+ {
+ name: "add and drop FK, add and drop respective tables, fk create table first",
+ fromQueries: []string{
+ "create table t1 (id int primary key, p int, key p_idx (p));",
+ "create table t2 (id int primary key, p int, key p_idx (p), foreign key (p) references t1 (p) on delete no action);",
+ },
+ toQueries: []string{
+ "create table t2 (id int primary key, p int, key p_idx (p), foreign key (p) references t3 (p) on delete no action);",
+ "create table t3 (id int primary key, p int, key p_idx (p));",
+ },
+ expectDiffs: 3,
+ expectDeps: 2, // [alter t2]/[drop t1], [alter t2]/[create t3]
+ sequential: true,
+ entityOrder: []string{"t3", "t2", "t1"},
+ instantCapability: InstantDDLCapabilityImpossible,
+ fkStrategy: ForeignKeyCheckStrategyCreateTableFirst,
+ },
+ {
+ name: "add and drop FK, add and drop respective tables, fk ignore",
+ fromQueries: []string{
+ "create table t1 (id int primary key, p int, key p_idx (p));",
+ "create table t2 (id int primary key, p int, key p_idx (p), foreign key (p) references t1 (p) on delete no action);",
+ },
+ toQueries: []string{
+ "create table t2 (id int primary key, p int, key p_idx (p), foreign key (p) references t3 (p) on delete no action);",
+ "create table t3 (id int primary key, p int, key p_idx (p));",
+ },
+ expectDiffs: 3,
+ expectDeps: 2, // [alter t2]/[drop t1], [alter t2]/[create t3]
+ sequential: true,
+ entityOrder: []string{"t3", "t2", "t1"},
+ instantCapability: InstantDDLCapabilityImpossible,
+ fkStrategy: ForeignKeyCheckStrategyIgnore,
  },
  {
  name: "two identical foreign keys in table, drop one",
@@ -1111,7 +1317,7 @@ func TestSchemaDiff(t *testing.T) {
  require.NoError(t, err)
  }
  instantCapability := schemaDiff.InstantDDLCapability()
- assert.Equal(t, tc.instantCapability, instantCapability)
+ assert.Equal(t, tc.instantCapability, instantCapability, "for instant capability")
  })
 
  }