From 6fb0f0e0f507e6e6e829783ae840fd4063eca9e2 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Mon, 10 Jun 2024 21:29:41 +0300 Subject: [PATCH] `schemadiff`: improved diff ordering with various foreign key strategies (#16081) Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schemadiff/schema.go | 2 +- go/vt/schemadiff/schema_diff.go | 82 +++++++--- go/vt/schemadiff/schema_diff_test.go | 216 ++++++++++++++++++++++++++- go/vt/schemadiff/types.go | 1 + 4 files changed, 276 insertions(+), 25 deletions(-) diff --git a/go/vt/schemadiff/schema.go b/go/vt/schemadiff/schema.go index f4cc0b67217..1738b9a4836 100644 --- a/go/vt/schemadiff/schema.go +++ b/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) diff --git a/go/vt/schemadiff/schema_diff.go b/go/vt/schemadiff/schema_diff.go index 3fbc1e6c9d3..60285265986 100644 --- a/go/vt/schemadiff/schema_diff.go +++ b/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 } diff --git a/go/vt/schemadiff/schema_diff_test.go b/go/vt/schemadiff/schema_diff_test.go index 4d6ca522d1e..8adc4fc8d68 100644 --- a/go/vt/schemadiff/schema_diff_test.go +++ b/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") }) } diff --git a/go/vt/schemadiff/types.go b/go/vt/schemadiff/types.go index 2049387140c..cb6bc09df85 100644 --- a/go/vt/schemadiff/types.go +++ b/go/vt/schemadiff/types.go @@ -132,6 +132,7 @@ const ( const ( ForeignKeyCheckStrategyStrict int = iota + ForeignKeyCheckStrategyCreateTableFirst ForeignKeyCheckStrategyIgnore )