Online DDL: ddl_strategy session variable and vtctl command line argument #7045

shlomi-noach · 2020-11-16T14:08:26Z

Context: #6782 (comment)
Additional contexts: #6926, #6689
Extends #7042

In this PR we utilize the new ddl_strategy session variable. Consider the following:

mysql> show create table t \G

Create Table: CREATE TABLE `t` (
  `id` smallint(6) NOT NULL,
  `i` int(11) DEFAULT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8

mysql> select @@ddl_strategy;
+----------------+
| @@ddl_strategy |
+----------------+
|                |
+----------------+

mysql> alter table t modify id bigint;
Query OK, 0 rows affected (0.03 sec)

mysql> show create table t \G

Create Table: CREATE TABLE `t` (
  `id` bigint(20) NOT NULL,
  `i` int(11) DEFAULT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8

mysql> set @@ddl_strategy='gh-ost';
Query OK, 0 rows affected (0.00 sec)

mysql> alter table t modify id mediumint;
+--------------------------------------+
| uuid                                 |
+--------------------------------------+
| b87f9824_2814_11eb_bd3d_f875a4d24e90 |
+--------------------------------------+

mysql> show create table t \G

Create Table: CREATE TABLE `t` (
  `id` bigint(20) NOT NULL,
  `i` int(11) DEFAULT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8

mysql> -- time passes

mysql> show create table t \G

Create Table: CREATE TABLE `t` (
  `id` mediumint(9) NOT NULL,
  `i` int(11) DEFAULT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8

Thus, the choice whether to user "normal" DDL or Online DDL, can depend on the valud of @@ddl_strategy session variable, which is either:

empty (normal DD)
gh-ost
pt-osc

I'm not sure yet how to represent migration options. Would that be set @@ddl_strategy='gh-ost; max-load=Threads_running=100'? Seems quite the syntax.

- no pre-generates primitives for normal DDL and Online DDL, wrapped in a new DDL primitive - DDL.Execute() chooses which of the underlying primitives to invoke Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

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

…ee if it's online or not Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

harshit-gangal · 2020-11-17T07:53:15Z

you can take the input as map in a separate session variable like ddl_strategy_settings = 'a=1,b=2'

shlomi-noach · 2020-11-17T08:07:45Z

you can take the input as map in a separate session variable like ddl_strategy_settings = 'a=1,b=2'

Thank you. I'm thinnking two variables are difficult to maintain. The user will set one, and forget that the other one is already set from a previous cycle.

I'm thinking to simplify as set @@ddl_strategy='gh-ost --max-load=Threads_running=100' which actually reads like a normal command line execution.

shlomi-noach · 2020-11-17T08:08:54Z

I am still unsure whether this approach of using session variables is any better for our users. Users want to be able to run exact same command on MySQL and on Vitess, such that the Vitess execution will run an online DDL.

So, the ALTER TABLE is fine. But how will they be able to run set @@ddl_strategy='gh-ost'; on their MySQL server? I feel like we;re back to square one.

harshit-gangal · 2020-11-17T08:00:44Z

go/vt/vtgate/engine/ddl.go

+
+//StreamExecute implements the Primitive interface
+func (v *DDL) StreamExecute(vcursor VCursor, bindVars map[string]*query.BindVariable, wantields bool, callback func(*sqltypes.Result) error) error {
+	return vterrors.Errorf(vtrpcpb.Code_INTERNAL, "not reachable") // TODO: shlomi - have no idea if this should work


you can call Execute and pass the result in callback function. The plan is to merge Execute and StreamExecute with streaming only so we need to support then.

Implemented

harshit-gangal · 2020-11-17T08:02:04Z

go/vt/vtgate/engine/ddl.go

+//GetFields implements the Primitive interface
+func (v *DDL) GetFields(vcursor VCursor, bindVars map[string]*query.BindVariable) (*sqltypes.Result, error) {
+	return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "not reachable") // TODO: shlomi - have no idea if this should work
+}


this will not be called. todo can be removed.

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

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

harshit-gangal · 2020-11-17T08:22:37Z

I am still unsure whether this approach of using session variables is any better for our users. Users want to be able to run exact same command on MySQL and on Vitess, such that the Vitess execution will run an online DDL.

So, the ALTER TABLE is fine. But how will they be able to run set @@ddl_strategy='gh-ost'; on their MySQL server? I feel like we;re back to square one.

Currently there are 2 ways to execute DDL in Vitess.

ApplySchema command in vtctl: we can add online-ddl option there. That can internally handle how to call things
Through VTGate

via some MySQL clients (cli or UI) - we can drive the experience there to execute the set statements first to enable online_ddl and then execute the ddl query.
via programatically - this we can give flag on vtgate that can be set at startup time so that all ddl are treated with that strategy and user will have option to change it with set statement.

harshit-gangal · 2020-11-17T08:26:09Z

There are few places that would need additional test

engine/ddl
executor
end to end test if that are not present.

shlomi-noach · 2020-11-17T09:13:38Z

ApplySchema command in vtctl: we can add online-ddl option there. That can internally handle how to call things

How would it look like in ApplySchema in your opinion?

we can drive the experience there

How do you mean?

we can give flag on vtgate

Yes, I was also thinking about that. One problem I have now is that, say we have a command line flag that sets to gh-ost. Now the user overrides with set session ddl_strategy=''. How do we know that the user set the strategy to (therefore, we should run a normal alter table) as opposed to not setting the session variable (thus, we should run gh-ost)?

set @@ddl_strategy='gh-ost --max-load=Threads_running=100'; Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

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

…ts the default value for @@ddl_strategy 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 @@ddl_strategy session variable. Added unit and endtoend tests to support the new flag 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 · 2020-11-22T10:19:25Z

OK, this PR now supports:

ddl_strategy session variable. Use as follows:

set session ddl_strategy='gh-ost';
set @@ddl_strategy='gh-ost';
set @@ddl_strategy='gh-ost --max-load=Threads_running=100';
set @@ddl_strategy='';
select @@ddl_strategy;

vtgate command line argument --default-ddl-strategy
the session variable @@ddl_strategy defaults to that value
vtctl ApplySchema now supports --ddl_strategy command line argument
in exact same format as @@ddl_strategy
onlineDDL endtoend tests cover all variations (vtgate, vtctl, with/out session variables, with/out params)

A Followup PR will remove the WITH 'gh-ost' syntax from parser, ast etc. It's too much for this single PR.

Documentation to follow.

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

shlomi-noach · 2020-11-22T15:39:22Z

@harshit-gangal I've made changes since your last review; see #7045 (comment)

harshit-gangal · 2020-11-23T10:24:00Z

go/vt/schema/online_ddl_test.go

+func TestParseDDLStrategy(t *testing.T) {
+	tt := []struct {
+		strategyVariable string
+		strategy         sqlparser.DDLStrategy
+		options          string
+		err              error
+	}{


add an invalid parsing test.

There is one, at the end of the function

harshit-gangal · 2020-11-23T10:27:18Z

go/vt/vtgate/engine/ddl.go

+
+var _ Primitive = (*DDL)(nil)
+
+//DDL represents the a DDL statement, either normal or online DDL


nit: convention in the code base is to have space between // and comment.

harshit-gangal · 2020-11-23T10:32:03Z

go/vt/vtgate/vtgate.go

@@ -60,6 +61,7 @@ var (
 	_                  = flag.Bool("disable_local_gateway", false, "deprecated: if specified, this process will not route any queries to local tablets in the local cell")
 	maxMemoryRows      = flag.Int("max_memory_rows", 300000, "Maximum number of rows that will be held in memory for intermediate results as well as the final result.")
 	warnMemoryRows     = flag.Int("warn_memory_rows", 30000, "Warning threshold for in-memory results. A row count higher than this amount will cause the VtGateWarnings.ResultsExceeded counter to be incremented.")
+	defaultDDLStrategy = flag.String("default-ddl-strategy", "", "Set default strategy for DDL statements. Override with @@ddl_strategy session variable")


I think we can drop the word default from here. This is how some of the other startup parameters behave.

harshit-gangal

Few nitpick comments. Everything else looks good to me.

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

shlomi-noach · 2020-11-24T13:11:37Z

Documentation: vitessio/website#598

shlomi-noach added 2 commits November 16, 2020 13:51

refactored DDL builder:

ec573d3

- no pre-generates primitives for normal DDL and Online DDL, wrapped in a new DDL primitive - DDL.Execute() chooses which of the underlying primitives to invoke Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

a bit more refactoring, generalizing DDL, SQL and Keyspace

f8db287

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

shlomi-noach requested review from systay and harshit-gangal November 16, 2020 14:08

Adding GetDDLStrategy(); using it when analyzing a DDL statement to s…

7a46387

…ee if it's online or not Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

harshit-gangal approved these changes Nov 17, 2020

View reviewed changes

shlomi-noach added 2 commits November 17, 2020 10:18

implementing StreamExecute() functionality

5168c2f

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

Merge remote-tracking branch 'upstream/master' into online-ddl-sessio…

c2ce6fc

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

shlomi-noach added 7 commits November 17, 2020 13:25

ddl_strategy variable may contain migration options, e.g.:

9d911a0

set @@ddl_strategy='gh-ost --max-load=Threads_running=100'; Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

minor refactor

3fa7c5d

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

introducing --default-ddl-strategy vtgate command line flag, which se…

78cc6ea

…ts the default value for @@ddl_strategy Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

testing online DDL via vtgate and @@dl_strategy

3114de7

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

vtctl ApplySchema now supports -ddl_strategy flag, which is identical…

3656e97

… to @@ddl_strategy session variable. Added unit and endtoend tests to support the new flag Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

merge master

7f39222

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

adapt to recent primitive changes

dc71492

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

shlomi-noach added 3 commits November 22, 2020 13:04

adapt to recent ALTER INDEX query

fc08dda

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

refactor, simplify sql generation

f82453f

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

reverted commit

7ef15b6

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

shlomi-noach changed the title ~~Online ddl session variable use~~ Online DDL: ddl_strategy session variable and vtctl command line argument Nov 22, 2020

shlomi-noach marked this pull request as ready for review November 22, 2020 15:38

shlomi-noach requested review from deepthi and sougou as code owners November 22, 2020 15:38

harshit-gangal reviewed Nov 23, 2020

View reviewed changes

harshit-gangal approved these changes Nov 23, 2020

View reviewed changes

shlomi-noach added 2 commits November 23, 2020 13:32

minor spacing changes

e9d500b

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

variable name changed from --default-ddl-strategy to --ddl-strategy

0e30200

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

harshit-gangal approved these changes Nov 23, 2020

View reviewed changes

harshit-gangal merged commit 19b7b5e into vitessio:master Nov 23, 2020

harshit-gangal deleted the online-ddl-session-variable-use branch November 23, 2020 12:14

This was referenced Nov 23, 2020

Removing online ddl query hint from ALTER TABLE #7069

Merged

vtgate -ddl-strategy flag renamed to -ddl_strategy #7074

Merged

Online DDL: removal of 'ALTER WITH...' syntax, introducing 'ddl_strategy' vitessio/website#598

Merged

askdba added this to the v9.0 milestone Nov 27, 2020

rafael mentioned this pull request Mar 17, 2021

Slack vitess 9 2021.03.16r0 tinyspeck/vitess#200

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Online DDL: ddl_strategy session variable and vtctl command line argument #7045

Online DDL: ddl_strategy session variable and vtctl command line argument #7045

shlomi-noach commented Nov 16, 2020

harshit-gangal commented Nov 17, 2020

shlomi-noach commented Nov 17, 2020

shlomi-noach commented Nov 17, 2020

harshit-gangal Nov 17, 2020

shlomi-noach Nov 17, 2020

harshit-gangal Nov 17, 2020

harshit-gangal commented Nov 17, 2020

harshit-gangal commented Nov 17, 2020

shlomi-noach commented Nov 17, 2020

shlomi-noach commented Nov 22, 2020 •

edited

Loading

shlomi-noach commented Nov 22, 2020

harshit-gangal Nov 23, 2020

shlomi-noach Nov 23, 2020

harshit-gangal Nov 23, 2020

shlomi-noach Nov 23, 2020

harshit-gangal Nov 23, 2020

shlomi-noach Nov 23, 2020

harshit-gangal left a comment

shlomi-noach commented Nov 24, 2020


		var _ Primitive = (*DDL)(nil)

		//DDL represents the a DDL statement, either normal or online DDL

Online DDL: ddl_strategy session variable and vtctl command line argument #7045

Online DDL: ddl_strategy session variable and vtctl command line argument #7045

Conversation

shlomi-noach commented Nov 16, 2020

harshit-gangal commented Nov 17, 2020

shlomi-noach commented Nov 17, 2020

shlomi-noach commented Nov 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harshit-gangal commented Nov 17, 2020

harshit-gangal commented Nov 17, 2020

shlomi-noach commented Nov 17, 2020

shlomi-noach commented Nov 22, 2020 • edited Loading

shlomi-noach commented Nov 22, 2020

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

harshit-gangal left a comment

Choose a reason for hiding this comment

shlomi-noach commented Nov 24, 2020

shlomi-noach commented Nov 22, 2020 •

edited

Loading