schemadiff: diffing two schemas generates a rich SchemaDiff object #12551

shlomi-noach · 2023-03-06T07:49:58Z

Description

This is a schemadiff enhancement. Previously, when diffing two schemas (via Schema.Diff(other)) we'd return a list of Diff objects, each of which would represent a single change, .e.g an ALTER TABLE, DROP VIEW, etc. The diffs order was partly heuristic (DROP VIEW -> DROP TABLE -> ALTER TABLE -> CREATE TABLE -> ALTER VIEW -> CREATE VIEW) and partly evaluated (order views by dependencies). However, the order was not necessarily applicable.

This PR introduces a new SchemaDiff function which returns a new SchemaDiff object:

func (s *Schema) SchemaDiff(other *Schema, hints *DiffHints) (*SchemaDiff, error)

SchemaDiff is a rich object which includes all diffs between the two schemas, but also:

Diffs are guaranteed to be sorted in lienarized, applicable order, if possible: you'd be able to apply the diffs one by one, and keep the schema consistent at all times while doing so, resolving any dependencies between the diffs. This includes VIEW and FOREIGN KEY dependencies.
There are scenarios where the declarative diffs between two schema have no single linearized sequence off diffs. This happens because we only attempt a single ALTER operation per table/view. In the future, we may allow a breakdown into granular diffs.
The diffs are arranged into equivalence classes, where diffs in the same equivalence class are said to depend on each other.
"depend on each other" has a formal definition of increasing severity or strictness. Two diffs may not have any dependency, or may have an unknown dependency (the current algorithm does not lead to any such result), an in-order completion dependency, or a strictly sequential dependency. This information is made available by SchemaDiff. This lets the user/automation that gets SchemaDiff to know whether some migrations may run --in-order, or if one migration must strictly wait for another to complete.
The algorithm follows MySQL's capabilities; it attempts the least strict dependency type between any two diffs, where possible. An in-order dependency type is known to be applicable in Online DDL's vitess strategy.

The implementation relies heavily on the various Validate() functionalities. The order of diffs begins with a heuristic order, which is likely to be the final order in most cases; but it may permute that order in between dependent diffs, if the heuristic order is not linearly valid.

Note: VIEW column dependencies support is provided in unmerged #12147 ; #12147 works, but we're looking to refactor it and reuse an internal implementation (UPDATE: reimplemented in #12565). The tests are prepared and commented out.

Related Issue(s)

Online DDL: --in-order-completion ddl strategy and logic #12113
Supersedes *WIP* schemadiff: Diff() of schemas ordered by valid dependencies #12028
WIP: schemadiff: validate views' referenced columns #12147
tracking: schemadiff: tracking issue #10203
Adds necessary support for foreign keys Experimental foreign keys tracking issue #11975

Checklist

"Backport to:" labels have been added if this change should be back-ported
Tests were added or are not required
Did the new or modified tests pass consistently locally and on the CI
Documentation was added or is not required

Deployment Notes

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

shlomi-noach · 2023-03-22T13:38:48Z

Good merger with #12565, and the tests we were waiting on exposed a bug, now fixed.

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

shlomi-noach · 2023-03-28T05:54:19Z

Ping for review 🙏 @dbussink in particular

shlomi-noach · 2023-03-30T09:17:46Z

go/mysql/collations/mysqldata.go

The diff here is due to gofmt -s -w .... the file is not formatted correctly in main.

mattlord

LGTM! I only had minor nits/comments/questions.

Thank you for all of the unit tests! ❤️

mattlord · 2023-04-03T17:41:29Z

go/mathutil/equivalence_relation.go

+	r.classElementsMap[r.classCounter] = []string{element}
+	r.classCounter++


So I take it this is used to enforce order?

Right. As the equivalence relation goes, it's only important that any new element gets its own distinct class-number. So r.classCounter++ ensures the next element is added onto its own distinct class.

The users of the equivalence relation, though, are interested in the order, just so that we have kind of a stable sorting mechanism.

go/mathutil/equivalence_relation.go

go/vt/schemadiff/schema.go

go/vt/schemadiff/schema_diff.go

go/vt/schemadiff/schema_diff_test.go

mattlord · 2023-04-03T21:25:28Z

go/vt/schemadiff/schema_diff_test.go

+	{
+		iteration := 0
+		allPerms := map[string]bool{}
+		allDiffs := schemaDiff.allDiffs()[:]


Curious why you use [:] here?

I'm curious too! Removed.

go/vt/schemadiff/schema_diff_test.go

mattlord · 2023-04-03T21:33:55Z

go/vt/schemadiff/schema_diff_test.go

+			}
+			for _, diff := range orderedDiffs {
+				s := diff.CanonicalStatementString()
+				// Internal sanity, while we're here: see that the equivalence relation has entries for all diffs.


FWIW, I think as part of the inclusive language effort we're supposed to avoid sanity-check related language.

Changed to integrity

go/vt/schemadiff/table_test.go

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

dbussink · 2023-04-13T07:51:19Z

go/mysql/collations/mysqldata.go

@@ -119,7 +119,7 @@ var tounicode_dec8_swedish_ci = [...]uint16{
 	0x00f8, 0x00f9, 0x00fa, 0x00fb, 0x00fc, 0x00ff, 0x0000, 0x0000,


@shlomi-noach Any reason why the collations data was updated in this PR?

@dbussink yes! Per #12551 (review), this is just a gofmt issue.

I think the only change is to fix the lint warnings, removing the redundant eightbit.UnicodeMapping{} type.

@dbussink yes! Per #12551 (review), this is just a gofmt issue.

dbussink · 2023-04-13T07:53:02Z

go/vt/schemadiff/schema.go

+// SchemaDiff calulates a rich diff between this schema and the given schema. It is stronger than Diff().
+// On top of returning the list of diffs that can take this schema into the given schema, this function also
+// evaluates the dependencies between those diffs, if any.
+func (s *Schema) SchemaDiff(other *Schema, hints *DiffHints) (*SchemaDiff, error) {


Do we want to deprecate / remove Diff? I guess it uses it internally, but should we then rename Diff to diff to not export it?

Yes, I intend to do that, but in a separate PR, to have a grace period of a couple weeks where we can use either.

dbussink · 2023-04-13T07:54:41Z

go/vt/schemadiff/table.go

@@ -231,6 +246,11 @@ func (d *RenameTableEntityDiff) IsEmpty() bool {
 	return d.Statement() == nil
 }

+// EntityName implements EntityDiff
+func (d *RenameTableEntityDiff) EntityName() string {
+	return d.from.Name()


Is this deliberately the from name? Or should it be the to name? Or does it not matter here?

While this isn't strictly in-use right now, it is deliberately from name. This is because the diff operates on the from table -- so that table is the one being affected by the operation. This is in line with the rest of diff types (with the exception of CREATE where the affected table is to).

rohit-nayak-ps

lgtm

shlomi-noach added 30 commits January 30, 2023 08:58

WIP: schemadiff, analyzing diff dependencies

b1c22fb

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

ongoing work; many tests added

64257eb

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

partitioning tests

3fd1a3e

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

function refactor

b388e64

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

merge main, resolve conflict

4162b34

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

minor reordering of variables

dfdfee5

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

copyright year

fc1a12d

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

stages to be commited

5eb1968

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

math: permutations

bc6170f

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

mathutil

5371c72

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

sort table alter options on diff

ba8b51b

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

alter reordering: first drop foreign key, then drop key

d706fb4

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

remove tags logic from EquivalenceRelation

f4daabe

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

removed go/mathutil/permutations.go

fda6baa

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

refactor/rename

b864b74

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

cleanup unused code

05d550d

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

OrderedDiffs

4712531

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

added an impossible-order test

d8d1202

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

clarify comment

d7f8b3a

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

AllSequentialExecutionDeps()

90f3b8f

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

Merge branch 'main' into schemadiff-diffs-dependency-evaluation

d96d131

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

Merge branch 'main' into schemadiff-diffs-dependency-evaluation

4ba8c0f

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

test: expected order of entities in ordered diffs

1f90011

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

Merge branch 'main' into schemadiff-diffs-dependency-evaluation

2457f07

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

Merge branch 'main' into schemadiff-diffs-dependency-evaluation

32b0b2e

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

Merge branch 'main' into schemadiff-diffs-dependency-evaluation

4611e11

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

function comments

cdb905f

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

remove unused code

4d42c90

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

code comments; privatize functions; remove unused code

01e2d1e

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

code comments. Privatize functoin

e634d29

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

shlomi-noach requested review from systay and mattlord as code owners March 22, 2023 13:37

shlomi-noach added 6 commits March 22, 2023 22:02

Merge branch 'main' into schemadiff-diffs-dependency-evaluation

8663190

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

unindent using early returns

5e5f20b

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

code comment

d931513

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

reposition some functions in file (no actualy changes)

83b827b

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

relocated the functions back because the diff became really confusing

4a9ea25

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

Merge branch 'main' into schemadiff-diffs-dependency-evaluation

fa2be58

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

shlomi-noach commented Mar 30, 2023

View reviewed changes

mattlord approved these changes Apr 3, 2023

View reviewed changes

shlomi-noach added 7 commits April 4, 2023 09:08

grammar

a02abb4

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

code comments, minor optimization

566ff46

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

typo

40a7578

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

Added tests, minor edits per review. changed language

65b0730

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

handling empty diff slice; improved comments

99072c7

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

Clarify test reasoning

ae1f225

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

grammar

75c366a

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

dbussink reviewed Apr 13, 2023

View reviewed changes

rohit-nayak-ps approved these changes Apr 13, 2023

View reviewed changes

shlomi-noach merged commit 46069ea into vitessio:main Apr 13, 2023

shlomi-noach deleted the schemadiff-diffs-dependency-evaluation branch April 13, 2023 10:31

This was referenced Apr 13, 2023

schemadiff: formalize SchemaDiff as the only schema diffing mechanism #12885

Merged

Proposed schemadiff blog post vitessio/website#1435

Merged

This was referenced Apr 18, 2023

schemadiff: rich ImpossibleApplyDiffOrderError #12918

Merged

schemadiff: better naming; formalizing SchemaDiff use. #12955

Merged

shlomi-noach mentioned this pull request Oct 31, 2023

schemadiff: identify a FK sequential execution scenario, and more #14397

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

schemadiff: diffing two schemas generates a rich SchemaDiff object #12551

schemadiff: diffing two schemas generates a rich SchemaDiff object #12551

shlomi-noach commented Mar 6, 2023 •

edited

Loading

shlomi-noach commented Mar 22, 2023

shlomi-noach commented Mar 28, 2023

shlomi-noach Mar 30, 2023

mattlord left a comment

mattlord Apr 3, 2023

shlomi-noach Apr 4, 2023

mattlord Apr 3, 2023

shlomi-noach Apr 4, 2023

mattlord Apr 3, 2023

shlomi-noach Apr 4, 2023

dbussink Apr 13, 2023

shlomi-noach Apr 13, 2023

rohit-nayak-ps Apr 13, 2023

shlomi-noach Apr 13, 2023 •

edited

Loading

dbussink Apr 13, 2023

shlomi-noach Apr 13, 2023

dbussink Apr 13, 2023

shlomi-noach Apr 13, 2023

rohit-nayak-ps left a comment

		r.classElementsMap[r.classCounter] = []string{element}
		r.classCounter++

		@@ -119,7 +119,7 @@ var tounicode_dec8_swedish_ci = [...]uint16{
		0x00f8, 0x00f9, 0x00fa, 0x00fb, 0x00fc, 0x00ff, 0x0000, 0x0000,

schemadiff: diffing two schemas generates a rich SchemaDiff object #12551

schemadiff: diffing two schemas generates a rich SchemaDiff object #12551

Conversation

shlomi-noach commented Mar 6, 2023 • edited Loading

Description

Related Issue(s)

Checklist

Deployment Notes

shlomi-noach commented Mar 22, 2023

shlomi-noach commented Mar 28, 2023

Choose a reason for hiding this comment

mattlord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlomi-noach Apr 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohit-nayak-ps left a comment

Choose a reason for hiding this comment

shlomi-noach commented Mar 6, 2023 •

edited

Loading

shlomi-noach Apr 13, 2023 •

edited

Loading