schemadiff: improved diff ordering with various foreign key strateg…

…ies (#16081)

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

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")
 		})
 
 	}