Online DDL plan via Send; "singleton" migrations on tablets by shlomi-noach · Pull Request #7785 · vitessio/vitess

shlomi-noach · 2021-04-05T08:30:25Z

Description

Two in one!

This PR introduces (WIP) "singleton migrations": a setting where you can't submit a new migration if any of the relevant shards still has any other pending migration (queued, ready, running`).
The use case resurfaced the question of the use of topo in Online DDL. And I'm using the particular use case to begin the refactor away from topo.

Elaborating on the refactor, first:

Online DDL is still the only mechanism that utilizes topo as a mean to communicate from VTGate or vtctl to vttablets. There's advantages to going through topo (decoupling, persisted "request" entry, auto-retries), but there's also disadvantages: too decoupled, non-idiomatic path, added constraint on vtctld availability.

With the recent introduction of migration queries, #7663 , #7656 , I see a nice path out of topo and into more fine-grained control over migrations.

The changes offered here provide 1-version backwards compatibility, and for now require opting-in.

How will the new online DDL plan/engine flow work?

For some DDL queries (where the user intentionally opted-in), we create a Send engine primitive, just as we would for direct DDLs. We decorate the DDL query with online DDL hints. For example, assuming our ddl_strategy is online, the query:

alter table corder engine innodb

is decorated as follows:

alter /*vt+ uuid="38ea1d8d_95da_11eb_b1fa_f875a4d24e90" context="vtctl:38be36cc-95da-11eb-b1fa-f875a4d24e90" strategy="online" options="" */ table corder engine innodb

and sent via Send. The tablet planner (TODO) will intercept the query and realize there's online DDL hints, and that's how it will know that the query should run via onlineddl.Executor rather than a direct commit on the backend database.

About singleton migrations

To be implemented; the refactor came first. The user will use -singleton strategy flag, e.g.:

set @@ddl_strategy='online -singleton';
alter table ... ;

or

$ vtctlclient ApplySchema -ddl_strategy="gh-ost -singleton" -sql "alter table ..." my_keyspace

The query will return with UUID if no other pending migration exists on relevant shards, and will atomically fail if there's at least one shard with a pending migration.

This is not implemented yet; I will implement this using a timeout-based mechanism, to avoid overly complex two-phase-commit logic. There may be some constraints on the user (e.g. if one migration is -singleton then all others must also be -singleton or else behavior is undefined).

Notes to the reviewer

There's many files affected here, but most are just affected by the initial refactor (e.g. change of function signature). The main changes right now are:

go/vt/sqlparser: all CREATE, DROP, ALTER statements now support comment_opt.
- Technically I only needed that for CREATE TABLE, DROP TABLE, ALTER TABLE, but yacc doesn't like it.
- DDLStatement now has SetComments() function which allows the us to inject comments into a statement.
go/vt/schema:
- created DDLStrategySetting, which is a formal representation of the ddl_strategy value; some logic moved outside of online_ddl.go and into ddl_strategy.go. OnlineDDL instance now uses said DDLStrategySetting.
- NewOnlineDDLs function: refactor parser.go logic into a more formal breakdown of a ddl statement
- NewOnlineDDL function chooses whether to decorate the SQL query with /*vt+ ... */ hints
go/vt/vtgate:
- engine/online_ddl.go: OnlineDDL primitive now chooses whether to write OnlineDDL request to topo, or to submit directly to tablets via Send
- TODO: apply the same in revert_migration.go

Most other changes (e.g. schemamanager) are just by-product of the introduction of DDLStrategySetting, and I'm happy to say the refactor formalized and I think simplified a bit, the logic in those places.

Related Issue(s)

Tracking: Online DDL: tracking issue #6926

Checklist

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

Deployment Notes

Impacted Areas in Vitess

Components that this PR will affect:

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

…ne-ddl-send-plan-comments Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

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

- introducing DDLStrategySetting, formal wrapper for strategy+options - NewOnlineDDLs generates multiple OnlineDDL from a given statement, instead of parser generating multiple statements - OnlineDDL utilizes DDLStrategySetting; some logic moved out from OnlineDDL and into DDLStrategySetting Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

…mal; also uses NewOnlineDDLs to break down queries Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

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

…to clean up VT comments from query Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

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

… instead of writing them to topo. It utilizes new DDLStrategySetting, and uses samd TaretDestination as non-online DDL engine. Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

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

…SQL string, but validates that it parses as a DDLStatement Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

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

…eDDL now parses query and expects either DDLStatement or RevertMigration Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

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

…rt statement on IsSkipTopo() Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

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

…ntial quotes and spaces in text. OnlineDDLFromCommentedStatement() function reads a commented statement and reconstructs an OnlineDDL instance Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

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

systay

Looks good from what I can see. Mostly looked over parser, engine and both planners

systay · 2021-04-20T09:21:22Z

go/vt/schema/ddl_strategy.go

@@ -0,0 +1,156 @@
+/*
+Copyright 2019 The Vitess Authors.


systay · 2021-04-20T09:22:38Z

go/vt/schema/ddl_strategy_test.go

@@ -0,0 +1,109 @@
+/*
+Copyright 2019 The Vitess Authors.


systay · 2021-04-20T09:25:53Z

go/vt/schema/online_ddl.go

+			}
+		}
+		return onlineDDLs, nil
+	case *sqlparser.CreateTable:


instead of adding a comment that this is done later, why not just do it in here straight away? just to keep the logic easier to follow

I see what you mean, but there is code that is shard to all DropTable, CreateTable, AlterTable that is performed after the switch statement, when DropTable has, on top, special handling. I've clarified that in code comments.

Refactored. It does look better now

systay · 2021-04-20T09:26:11Z

go/vt/schema/online_ddl.go

+	case *sqlparser.AlterTable:
+		// handled later on
+	default:
+		return nil, fmt.Errorf("Unsupported statement for Online DDL: %v", sqlparser.String(ddlStmt))


nit:vterrors is your friend. this applies to quite a few errors in this file.

also, I believe errors should not start with a capitalised letter

Replaced all fmt.Errorf() in this file with vterrors, lower case as needed.

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

shlomi-noach added 30 commits April 1, 2021 14:43

support vitess Comments in CREATE/ALTER/DROP TABLE statements

0a5c2b1

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

improved upon comment_opt in CREATE, ALTER and DROP statements

a5b39a2

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

normalized comments for DROP statements

1234cab

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

minor comment spacing change

be290ef

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

Merge remote-tracking branch 'origin/online-ddl-idempotent' into onli…

db392d7

…ne-ddl-send-plan-comments Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

DDL statement: SetComments

ef5172f

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

TabletExecutor utilizes DDLStrategySetting, now more DRY and more for…

552b50a

…mal; also uses NewOnlineDDLs to break down queries Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

vtctl utilizes new DDLStrategySetting

6df2060

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

onlineddl.Executor utilizes new DDLStrategySetting. Also, it ensures …

8fd6222

…to clean up VT comments from query Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

endtoend tests adapt to new DDLStrategySetting

6d4db17

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

go.sum

852e9a7

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

utilizing DDLStrategySetting to validate @@ddl_strategy

63fac67

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

adapt to DDLStrategySetting

7124709

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

adapt to DDLStrategySetting

1e4ace2

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

merge master, resolve conflicts

580393b

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

REVERT supports comment_opt

9c60403

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

NewOnlineDDL now expects DDLStatement. NewOnlineDDLBySQL supports an …

b637c0b

…SQL string, but validates that it parses as a DDLStatement Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

RevertMigration prints comments in Format, supports SetComments()

4271063

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

removed 'NewOnlineDDLBySQL()' introduced in previous commit. NewOnlin…

55e3c31

…eDDL now parses query and expects either DDLStatement or RevertMigration Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

revert schema.NewOnlineDDLBySQL

617656a

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

formalizing 'revert <uuid>' into 'revert vitess_migration '<uuid>''

417c1c4

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

support legacy revert statement parser; only generate new formal reve…

cc2aa00

…rt statement on IsSkipTopo() Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

vtgate/plan: revert uses Send when IsSkipTopo()

e3d29b6

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

initial implementation for RevertMigration plan in tabletserver

beaff22

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

merge master, resolve conflicts

3c00298

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

sizegen

3736bf2

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

DDLStatement supports GetComments()

45508e0

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

shlomi-noach added 2 commits April 20, 2021 11:33

extend CI port range: apply

b5127c5

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

Merge branch 'master' into online-ddl-send-plan-comments

eb1a784

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

systay approved these changes Apr 20, 2021

View reviewed changes

shlomi-noach added 4 commits April 20, 2021 12:45

copyright year

3737c8f

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

clarifies case statement

ed94bb9

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

vterrors

2f81d42

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

patch OS fo rupgrade test

942ad9b

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

shlomi-noach mentioned this pull request Apr 20, 2021

fixing flaky upgrade test #7901

Merged

8 tasks

shlomi-noach added 17 commits April 20, 2021 20:48

more verbose execution of vtctlclient

f027d46

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

Merge branch 'master' into online-ddl-send-plan-comments

ff49712

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

-skip_preflight must be v9.0.0 compatible

051bb2a

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

Merge branch 'master' into online-ddl-send-plan-comments

e06d1b8

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

Merge branch 'master' into online-ddl-send-plan-comments

5cd4a09

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

empty commit to kick CI

86f4b27

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

empty commit to kick CI

10d95ad

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

more verbose logging on 'xtrabackup failed with error'

09e3e90

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

experiment: sleep 20sec

86fa94b

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

lower sleep

cab62e2

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

10 secs

54a4721

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

20 secs

036cb43

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

30 secs

4512c31

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

sleep on terminatedRestore

71d576f

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

refactor switch statement per review

7397b8b

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

fixed race condition

34e0a4c

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

reduce sleep time

5bb8ffd

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

shlomi-noach merged commit 4bb1ee7 into vitessio:master Apr 22, 2021

shlomi-noach deleted the online-ddl-send-plan-comments branch April 22, 2021 11:58

This was referenced Apr 22, 2021

fix backup_xtrabackup flaky test #7928

Closed

ddl_strategy: -singleton scope is now migration context #7945

Closed

Online DDL: introducing ddl_strategy -singleton-context flag #7946

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Online DDL plan via Send; "singleton" migrations on tablets#7785

Online DDL plan via Send; "singleton" migrations on tablets#7785
shlomi-noach merged 77 commits intovitessio:masterfrom
planetscale:online-ddl-send-plan-comments

shlomi-noach commented Apr 5, 2021 •

edited

Loading

Uh oh!

systay left a comment

Uh oh!

systay Apr 20, 2021

Uh oh!

shlomi-noach Apr 20, 2021

Uh oh!

systay Apr 20, 2021

Uh oh!

shlomi-noach Apr 20, 2021

Uh oh!

systay Apr 20, 2021

Uh oh!

shlomi-noach Apr 20, 2021

Uh oh!

shlomi-noach Apr 22, 2021

Uh oh!

systay Apr 20, 2021

Uh oh!

shlomi-noach Apr 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

shlomi-noach commented Apr 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Elaborating on the refactor, first:

How will the new online DDL plan/engine flow work?

About singleton migrations

Notes to the reviewer

Related Issue(s)

Checklist

Deployment Notes

Impacted Areas in Vitess

Uh oh!

systay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

shlomi-noach commented Apr 5, 2021 •

edited

Loading