Online DDL: evaluate REVERTibility of a migration #9232

shlomi-noach · 2021-11-15T14:05:23Z

Description

In this WIP PR we evaluate whether a migration is REVERTible. A migration may, for example, possibly be irrevertible if:

It drops a unique constraint (what happens if a user INSERTs a new row on the new table, which would create a conflict in the old schema?)
Id expands a data type (e.g. INT to BIGINT; what happens if a user INSERTs a value greater than INT on the new table? This value cannot be propagated back to the old table)
dropping a NOT NULL column with no DEFAULT value, then INSERTing any row on the new table. How can we populate the dropped column on the old table?
other?

We cannot say for certain that a migration is not revertible. It depends on how the user uses the new table in the interim.
However, assuming we cover all the cases, we can predict that a migration is in fact revertible. If we see nothing to block revertibility, then we have high confidence that we will make it.

As a side note, most cases illustrated above are IMO unlikely. It's unlikely that immediately after cut-over the user will suddenly populate a BIGINT value: their app needs to first realize it can populate such values. But of course we can't say for sure. We cannot predict an app's behavior in the generic case.

Related Issue(s)

Followup to #8495
Tracking: #6926

Checklist

Should this PR be backported?
Tests were added or are not required
Documentation was added or is not required

Deployment Notes

…s ewhen computing shared unique key or columns-covered unique key Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

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

shlomi-noach · 2021-11-16T06:06:33Z

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

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

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

…ropped non-generated columns. Added unit test Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

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

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

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

shlomi-noach · 2021-11-21T14:21:37Z

It occurs to me that a user may change a column's type from anything to anything, really. e.g. INT to VARCHAR, CHAR to DATETIME etc. We can't (won't?) review all possible transformations, just the most sensible ones (between numerics, between texts, etc.)

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

…changes, that may affect revertibility Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach · 2021-11-22T07:18:08Z

go/vt/vttablet/onlineddl/vrepl/unique_key.go

@@ -0,0 +1,161 @@
+/*


This was refactored out of vrepl.go without code changes

shlomi-noach · 2021-11-22T07:18:39Z

go/vt/vttablet/onlineddl/vrepl/columns.go

@@ -0,0 +1,192 @@
+/*


This file was refactored out of vrepl.go with changes as noted below.

shlomi-noach · 2021-11-22T07:19:01Z

go/vt/vttablet/onlineddl/vrepl/columns.go

+}
+
+// GetSharedColumns returns the intersection of two lists of columns in same order as the first list
+func GetSharedColumns(


This function is unchanged, refactored out of vrepl.go

shlomi-noach · 2021-11-22T07:19:21Z

go/vt/vttablet/onlineddl/vrepl/columns.go

+}
+
+// isExpandedColumn sees if target column has any value set/range that is impossible in source column. See GetExpandedColumns comment for examples
+func isExpandedColumn(sourceColumn *Column, targetColumn *Column) (bool, string) {


New functionality, and at the heart of this PR

shlomi-noach · 2021-11-22T07:19:38Z

go/vt/vttablet/onlineddl/vrepl/columns.go

+// - BIGINT UNSIGNED -> INT SIGNED (negative values)
+// - TIMESTAMP -> TIMESTAMP(3)
+// etc.
+func GetExpandedColumnNames(


new functionality

shlomi-noach · 2021-11-22T07:19:55Z

go/vt/vttablet/onlineddl/vrepl/columns.go

+}
+
+// GetNoDefaultColumnNames returns names of columns which have no default value, out of given list of columns
+func GetNoDefaultColumnNames(columns *ColumnList) (names []string) {


new functionality. Though small, it's at the heart of this PR

shlomi-noach · 2021-11-22T07:21:13Z

go/vt/vttablet/onlineddl/schema.go

+	alterSchemaMigrationsTableRemovedUniqueNames,
+	alterSchemaMigrationsTableRemovedNoDefaultColNames,
+	alterSchemaMigrationsTableExpandedColNames,
+	alterSchemaMigrationsTableRevertibleNotes,


These new columns in schema_migrations table help us to understand better the nature of the schema change, and in particular inform us what may be blockers/issues to reverting the migration.

shlomi-noach · 2021-11-22T07:21:31Z

go/vt/vttablet/onlineddl/schema.go

+	alterSchemaMigrationsTableRemovedUniqueNames       = "ALTER TABLE _vt.schema_migrations add column removed_unique_key_names text NOT NULL"
+	alterSchemaMigrationsTableRemovedNoDefaultColNames = "ALTER TABLE _vt.schema_migrations add column dropped_no_default_column_names text NOT NULL"
+	alterSchemaMigrationsTableExpandedColNames         = "ALTER TABLE _vt.schema_migrations add column expanded_column_names text NOT NULL"
+	alterSchemaMigrationsTableRevertibleNotes          = "ALTER TABLE _vt.schema_migrations add column revertible_notes text NOT NULL"


These new columns in schema_migrations table help us to understand better the nature of the schema change, and in particular inform us what may be blockers/issues to reverting the migration.

shlomi-noach · 2021-11-22T07:22:08Z

Ready for review!

Notes notes for the reviewers:

Added endtoend tests in go/test/endtoend/onlineddl/revertible/onlineddl_revertible_test.go.
This is a suite of tests. Each test has two table definitions: "from" and "to", and runs an actual migrations between the two table. We then validate the analysis of the change: which unique constraints were dropped, which columns dropped which had no default value, which column changed type into a borader scope.
go/vt/vttablet/onlineddl/vrepl/columns.go refactored out side of vrepl.go, and then also added the functions isExpandedColumn, GetExpandedColumnNames
go/vt/vttablet/onlineddl/vrepl/unique_key.go refactored out side of vrepl.go without code changes
likewise, tests were refactored outside vrepl_test.go.

deepthi

Very nicely written PR (as usual). The online DDL stuff blows my mind.
A few questions/clarification requests inline.

deepthi · 2021-12-03T01:09:39Z

go/vt/schema/parser.go

@@ -54,7 +54,7 @@ var (
 	createTableRegexp     = regexp.MustCompile(`(?s)(?i)(CREATE\s+TABLE\s+)` + "`" + `([^` + "`" + `]+)` + "`" + `(\s*[(].*$)`)
 	revertStatementRegexp = regexp.MustCompile(`(?i)^revert\s+([\S]*)$`)

-	enumValuesRegexp = regexp.MustCompile("(?i)^enum[(](.*)[)]$")
+	enumValuesRegexp = regexp.MustCompile("(?i)^(enum|set)[(](.*)[)]$")


Does this change affect anything besides online DDL?

Double checking.

go/vt/schema/parser.go

deepthi · 2021-12-03T01:17:07Z

go/vt/vttablet/onlineddl/executor.go

+	if err := e.updateSchemaAnalysis(ctx, onlineDDL.UUID,
+		len(v.addedUniqueKeys),
+		len(v.removedUniqueKeys),
+		strings.Join(removedUniqueKeyNames, ","),


Is it safe to concatenate like this? MySQL lets you use commas in column names.

You are right -- but this is going to be informational only. That is, we're not going to then re-parse this. This is about us being able to tell the user "there is a chance we can't revert this migration due to these columns: a,b,c". So in a way, I consider this to be free text.
However, you're right and why no make the extra effort of qualifying the names. I'll proceed to do that.

deepthi · 2021-12-03T01:20:15Z

go/vt/vttablet/onlineddl/vrepl/columns.go

+   Copyright 2016 GitHub Inc.
+	 See https://github.com/github/gh-ost/blob/master/LICENSE
+*/
+


Has this been copied from gh-ost with no changes?

Right! It was copied from gh-ost and there have been changes. I'll append the Vitess copyright.

deepthi · 2021-12-03T01:21:47Z

go/vt/vttablet/onlineddl/vrepl/unique_key.go

+	return true // good to go!
+}
+
+// GetSharedUniqueKeys returns the unique keys shared between the two source&target tables


nit: it seems redundant to say "two source&target". Just "source and target"?

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

shlomi-noach · 2021-12-05T08:48:19Z

Addressed all review comments:

Copyright notices updated
Separate parsing for enum and for set data types
Qualifying column names in schema analysis columns
Typos & grammar

deepthi

👍

read all unique keys, even if NULLable or have FLOAT; then ignore tho…

3af31d7

…s ewhen computing shared unique key or columns-covered unique key Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach added Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: Query Serving release notes labels Nov 15, 2021

shlomi-noach self-assigned this Nov 15, 2021

unit test case

5c39d24

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

shlomi-noach added 14 commits November 16, 2021 09:23

refactor: move logic to analysis.go

cbac830

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

refactor: rename files

b2086da

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

reading more information about a column: is it nullable? is the defau…

25c4278

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

whoops, YES, not NO

0d66e9d

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

GetSharedColumns refactores to columns.go, now also returns list of d…

344d8a2

…ropped non-generated columns. Added unit test Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

helper function

87668f6

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

populating new 'dropped_no_default_column_names' column in schema_mig…

5c8181d

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

added expanded_column_names column, not yet populated

19e3ae2

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

initial work on endtoend tests. Right now it's just a lot of copy+paste

11d2b62

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

more tests

0a69893

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

revertible endtoend test workflow

5086d62

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

ignore expectCount if negative

66e9cf7

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

new onlineddl_revertible CI workflow file

35c5d69

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

identifying expanded columns

1cb0abb

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

shlomi-noach added 8 commits November 21, 2021 16:33

compare unsigned range

df153ac

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

more test cases

061d421

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

merged main; resolved conflict

6ab4a76

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

manually analyzed expanded types

b42867d

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

identifying (utf8-related) expanded character set types

ee2f18c

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

check expanded enums

24e17e5

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

enum validation tested

fe51709

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

expanded set types

3a2aafb

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

add and populate 'revertible_notes' column, based on analyzed schema …

fd9d5d5

…changes, that may affect revertibility Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach commented Nov 22, 2021

View reviewed changes

shlomi-noach marked this pull request as ready for review November 22, 2021 07:22

shlomi-noach requested review from dbussink, deepthi and sougou November 22, 2021 07:22

deepthi reviewed Dec 3, 2021

View reviewed changes

shlomi-noach added 7 commits December 5, 2021 08:59

typos and grammar

9321a94

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

Merge branch 'main' into onlineddl-analyze-revert-ability

0505214

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

vitess copyright

fdc16ff

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

distinct parsing and handling of enum/set data type

15a5e34

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

EscapeIDs

a1637bf

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

EscapeIDs

9a19d53

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

adapt tests

3a33b37

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

shlomi-noach requested a review from rohit-nayak-ps as a code owner December 5, 2021 08:43

merged main; resolved conflict

002cd53

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

deepthi approved these changes Dec 9, 2021

View reviewed changes

deepthi merged commit 9cb60cd into vitessio:main Dec 9, 2021

deepthi deleted the onlineddl-analyze-revert-ability branch December 9, 2021 02:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Online DDL: evaluate REVERTibility of a migration #9232

Online DDL: evaluate REVERTibility of a migration #9232

shlomi-noach commented Nov 15, 2021 •

edited

Loading

shlomi-noach commented Nov 16, 2021 •

edited

Loading

shlomi-noach commented Nov 21, 2021

shlomi-noach Nov 22, 2021

shlomi-noach Nov 22, 2021

shlomi-noach Nov 22, 2021

shlomi-noach Nov 22, 2021

shlomi-noach Nov 22, 2021

shlomi-noach Nov 22, 2021

shlomi-noach Nov 22, 2021 •

edited

Loading

shlomi-noach Nov 22, 2021

shlomi-noach commented Nov 22, 2021

deepthi left a comment

deepthi Dec 3, 2021

shlomi-noach Dec 5, 2021

deepthi Dec 3, 2021

shlomi-noach Dec 5, 2021

deepthi Dec 3, 2021

shlomi-noach Dec 5, 2021

deepthi Dec 3, 2021

shlomi-noach commented Dec 5, 2021 •

edited

Loading

deepthi left a comment

Online DDL: evaluate REVERTibility of a migration #9232

Online DDL: evaluate REVERTibility of a migration #9232

Conversation

shlomi-noach commented Nov 15, 2021 • edited Loading

Description

Related Issue(s)

Checklist

Deployment Notes

shlomi-noach commented Nov 16, 2021 • edited Loading

shlomi-noach commented Nov 21, 2021

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 Nov 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlomi-noach commented Nov 22, 2021

deepthi 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

shlomi-noach commented Dec 5, 2021 • edited Loading

deepthi left a comment

Choose a reason for hiding this comment

shlomi-noach commented Nov 15, 2021 •

edited

Loading

shlomi-noach commented Nov 16, 2021 •

edited

Loading

shlomi-noach Nov 22, 2021 •

edited

Loading

shlomi-noach commented Dec 5, 2021 •

edited

Loading