From 3c5501e7361985520283a31bfa7bd4ce5a6d9d6f Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Mon, 28 Aug 2023 13:12:32 +0300 Subject: [PATCH 01/27] normalizedSchema: slight refactoring and adding unit tests Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/mysqlctl/schema.go | 28 ++++++++++++------- go/vt/mysqlctl/schema_test.go | 51 +++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 9 deletions(-) diff --git a/go/vt/mysqlctl/schema.go b/go/vt/mysqlctl/schema.go index 397668145ef..bb4589df546 100644 --- a/go/vt/mysqlctl/schema.go +++ b/go/vt/mysqlctl/schema.go @@ -249,6 +249,24 @@ func (mysqld *Mysqld) collectSchema(ctx context.Context, dbName, tableName, tabl return fields, columns, schema, nil } +// normalizedStatement returns a table schema with database names replaced, and auto_increment annotations removed. +func normalizedStatement(ctx context.Context, statementQuery, dbName, tableType string) string { + backtickDBName := sqlescape.EscapeID(dbName) + + // Normalize & remove auto_increment because it changes on every insert + // FIXME(alainjobart) find a way to share this with + // vt/tabletserver/table_info.go:162 + norm := statementQuery + norm = autoIncr.ReplaceAllLiteralString(norm, "") + if tableType == tmutils.TableView { + // Views will have the dbname in there, replace it + // with {{.DatabaseName}} + norm = strings.Replace(norm, backtickDBName, "{{.DatabaseName}}", -1) + } + + return norm +} + // normalizedSchema returns a table schema with database names replaced, and auto_increment annotations removed. func (mysqld *Mysqld) normalizedSchema(ctx context.Context, dbName, tableName, tableType string) (string, error) { backtickDBName := sqlescape.EscapeID(dbName) @@ -263,15 +281,7 @@ func (mysqld *Mysqld) normalizedSchema(ctx context.Context, dbName, tableName, t // Normalize & remove auto_increment because it changes on every insert // FIXME(alainjobart) find a way to share this with // vt/tabletserver/table_info.go:162 - norm := qr.Rows[0][1].ToString() - norm = autoIncr.ReplaceAllLiteralString(norm, "") - if tableType == tmutils.TableView { - // Views will have the dbname in there, replace it - // with {{.DatabaseName}} - norm = strings.Replace(norm, backtickDBName, "{{.DatabaseName}}", -1) - } - - return norm, nil + return normalizedStatement(ctx, qr.Rows[0][1].ToString(), dbName, tableType), nil } // ResolveTables returns a list of actual tables+views matching a list diff --git a/go/vt/mysqlctl/schema_test.go b/go/vt/mysqlctl/schema_test.go index fb64f8ca8ee..cf104fec3a5 100644 --- a/go/vt/mysqlctl/schema_test.go +++ b/go/vt/mysqlctl/schema_test.go @@ -1,13 +1,16 @@ package mysqlctl import ( + "context" "fmt" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "vitess.io/vitess/go/mysql/fakesqldb" "vitess.io/vitess/go/sqltypes" + "vitess.io/vitess/go/vt/mysqlctl/tmutils" querypb "vitess.io/vitess/go/vt/proto/query" ) @@ -103,3 +106,51 @@ func TestColumnList(t *testing.T) { require.Equal(t, `[name:"col1" type:VARCHAR]`, fmt.Sprintf("%+v", fields)) } + +func TestNormalizedStatement(t *testing.T) { + tcases := []struct { + statement string + db string + typ string + expect string + }{ + { + statement: "create table mydb.t (id int primary key)", + db: "mydb", + typ: tmutils.TableBaseTable, + expect: "create table mydb.t (id int primary key)", + }, + { + statement: "create table `mydb`.t (id int primary key)", + db: "mydb", + typ: tmutils.TableBaseTable, + expect: "create table `mydb`.t (id int primary key)", + }, + { + statement: "create view `mydb`.v as select * from t", + db: "mydb", + typ: tmutils.TableView, + expect: "create view {{.DatabaseName}}.v as select * from t", + }, + { + statement: "create view `mydb`.v as select * from `mydb`.`t`", + db: "mydb", + typ: tmutils.TableView, + expect: "create view {{.DatabaseName}}.v as select * from {{.DatabaseName}}.`t`", + }, + // { + // statement: "create view `mydb`.v as select * from `mydb`.`mydb`", + // db: "mydb", + // typ: tmutils.TableView, + // expect: "create view {{.DatabaseName}}.v as select * from {{.DatabaseName}}.`mydb`", + // }, + } + ctx := context.Background() + for _, tcase := range tcases { + testName := tcase.statement + t.Run(testName, func(t *testing.T) { + result := normalizedStatement(ctx, tcase.statement, tcase.db, tcase.typ) + assert.Equal(t, tcase.expect, result) + }) + } +} From 0acb7d99bf269f36b91c4c7f5353dd7f3b42ab3c Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Mon, 28 Aug 2023 13:25:23 +0300 Subject: [PATCH 02/27] introduce a test case that breaks Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/mysqlctl/schema_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/go/vt/mysqlctl/schema_test.go b/go/vt/mysqlctl/schema_test.go index cf104fec3a5..01d933c39c7 100644 --- a/go/vt/mysqlctl/schema_test.go +++ b/go/vt/mysqlctl/schema_test.go @@ -138,12 +138,12 @@ func TestNormalizedStatement(t *testing.T) { typ: tmutils.TableView, expect: "create view {{.DatabaseName}}.v as select * from {{.DatabaseName}}.`t`", }, - // { - // statement: "create view `mydb`.v as select * from `mydb`.`mydb`", - // db: "mydb", - // typ: tmutils.TableView, - // expect: "create view {{.DatabaseName}}.v as select * from {{.DatabaseName}}.`mydb`", - // }, + { + statement: "create view `mydb`.v as select * from `mydb`.`mydb`", + db: "mydb", + typ: tmutils.TableView, + expect: "create view {{.DatabaseName}}.v as select * from {{.DatabaseName}}.`mydb`", + }, } ctx := context.Background() for _, tcase := range tcases { From 2e93538c0259fbaf0c5fff105c76aaa4aa41b94b Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Mon, 28 Aug 2023 14:18:17 +0300 Subject: [PATCH 03/27] use 'sqlparser.ReplaceTableQualifiers()' to inject placeholder, then 'UnqualifyDatabaseNamePlaceholder()'. A bit of all-round refactoring. Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/mysqlctl/schema.go | 24 ++++++++++++++---------- go/vt/mysqlctl/schema_test.go | 13 ++++++++++--- go/vt/mysqlctl/tmutils/schema.go | 15 ++++++++++++--- 3 files changed, 36 insertions(+), 16 deletions(-) diff --git a/go/vt/mysqlctl/schema.go b/go/vt/mysqlctl/schema.go index bb4589df546..abb7a4e4f99 100644 --- a/go/vt/mysqlctl/schema.go +++ b/go/vt/mysqlctl/schema.go @@ -34,6 +34,7 @@ import ( querypb "vitess.io/vitess/go/vt/proto/query" tabletmanagerdatapb "vitess.io/vitess/go/vt/proto/tabletmanagerdata" "vitess.io/vitess/go/vt/proto/vtrpc" + "vitess.io/vitess/go/vt/sqlparser" "vitess.io/vitess/go/vt/vterrors" ) @@ -99,7 +100,7 @@ func (mysqld *Mysqld) GetSchema(ctx context.Context, dbName string, request *tab if len(qr.Rows) == 0 { return nil, fmt.Errorf("empty create database statement for %v", dbName) } - sd.DatabaseSchema = strings.Replace(qr.Rows[0][1].ToString(), backtickDBName, "{{.DatabaseName}}", 1) + sd.DatabaseSchema = strings.Replace(qr.Rows[0][1].ToString(), backtickDBName, tmutils.DatabaseNamePlaceholder, 1) tds, err := mysqld.collectBasicTableData(ctx, dbName, request.Tables, request.ExcludeTables, request.IncludeViews) if err != nil { @@ -250,21 +251,24 @@ func (mysqld *Mysqld) collectSchema(ctx context.Context, dbName, tableName, tabl } // normalizedStatement returns a table schema with database names replaced, and auto_increment annotations removed. -func normalizedStatement(ctx context.Context, statementQuery, dbName, tableType string) string { - backtickDBName := sqlescape.EscapeID(dbName) - +func normalizedStatement(ctx context.Context, statementQuery, dbName, tableType string) (string, error) { // Normalize & remove auto_increment because it changes on every insert // FIXME(alainjobart) find a way to share this with // vt/tabletserver/table_info.go:162 norm := statementQuery norm = autoIncr.ReplaceAllLiteralString(norm, "") if tableType == tmutils.TableView { - // Views will have the dbname in there, replace it - // with {{.DatabaseName}} - norm = strings.Replace(norm, backtickDBName, "{{.DatabaseName}}", -1) + replaced, err := sqlparser.ReplaceTableQualifiers(norm, dbName, tmutils.DatabaseNamePlaceholder) + if err != nil { + // parsing unsuccessful + return norm, err + } + // Parsing successful + replaced = tmutils.UnqualifyDatabaseNamePlaceholder(replaced) + return replaced, nil } - return norm + return norm, nil } // normalizedSchema returns a table schema with database names replaced, and auto_increment annotations removed. @@ -281,7 +285,7 @@ func (mysqld *Mysqld) normalizedSchema(ctx context.Context, dbName, tableName, t // Normalize & remove auto_increment because it changes on every insert // FIXME(alainjobart) find a way to share this with // vt/tabletserver/table_info.go:162 - return normalizedStatement(ctx, qr.Rows[0][1].ToString(), dbName, tableType), nil + return normalizedStatement(ctx, qr.Rows[0][1].ToString(), dbName, tableType) } // ResolveTables returns a list of actual tables+views matching a list @@ -449,7 +453,7 @@ func (mysqld *Mysqld) PreflightSchemaChange(ctx context.Context, dbName string, if td.Type == tmutils.TableView { // Views will have {{.DatabaseName}} in there, replace // it with _vt_preflight - s := strings.Replace(td.Schema, "{{.DatabaseName}}", "`_vt_preflight`", -1) + s := strings.Replace(td.Schema, tmutils.DatabaseNamePlaceholder, "`_vt_preflight`", -1) initialCopySQL += s + ";\n" } } diff --git a/go/vt/mysqlctl/schema_test.go b/go/vt/mysqlctl/schema_test.go index 01d933c39c7..e7b77c7752d 100644 --- a/go/vt/mysqlctl/schema_test.go +++ b/go/vt/mysqlctl/schema_test.go @@ -136,20 +136,27 @@ func TestNormalizedStatement(t *testing.T) { statement: "create view `mydb`.v as select * from `mydb`.`t`", db: "mydb", typ: tmutils.TableView, - expect: "create view {{.DatabaseName}}.v as select * from {{.DatabaseName}}.`t`", + expect: "create view {{.DatabaseName}}.v as select * from {{.DatabaseName}}.t", + }, + { + statement: "create view `mydb`.v as select * from `mydb`.mydb", + db: "mydb", + typ: tmutils.TableView, + expect: "create view {{.DatabaseName}}.v as select * from {{.DatabaseName}}.mydb", }, { statement: "create view `mydb`.v as select * from `mydb`.`mydb`", db: "mydb", typ: tmutils.TableView, - expect: "create view {{.DatabaseName}}.v as select * from {{.DatabaseName}}.`mydb`", + expect: "create view {{.DatabaseName}}.v as select * from {{.DatabaseName}}.mydb", }, } ctx := context.Background() for _, tcase := range tcases { testName := tcase.statement t.Run(testName, func(t *testing.T) { - result := normalizedStatement(ctx, tcase.statement, tcase.db, tcase.typ) + result, err := normalizedStatement(ctx, tcase.statement, tcase.db, tcase.typ) + assert.NoError(t, err) assert.Equal(t, tcase.expect, result) }) } diff --git a/go/vt/mysqlctl/tmutils/schema.go b/go/vt/mysqlctl/tmutils/schema.go index 694b02abf5e..a75b1712842 100644 --- a/go/vt/mysqlctl/tmutils/schema.go +++ b/go/vt/mysqlctl/tmutils/schema.go @@ -38,8 +38,17 @@ const ( TableBaseTable = "BASE TABLE" // TableView indicates the table type is a view. TableView = "VIEW" + + DatabaseNamePlaceholder = "{{.DatabaseName}}" ) +func UnqualifyDatabaseNamePlaceholder(s string) string { + return strings.Replace(s, sqlescape.EscapeID(DatabaseNamePlaceholder), DatabaseNamePlaceholder, -1) +} +func QualifyDatabaseNamePlaceholder(s string) string { + return strings.Replace(s, DatabaseNamePlaceholder, sqlescape.EscapeID(DatabaseNamePlaceholder), -1) +} + // TableDefinitionGetColumn returns the index of a column inside a // TableDefinition. func TableDefinitionGetColumn(td *tabletmanagerdatapb.TableDefinition, name string) (index int, ok bool) { @@ -212,8 +221,8 @@ func SchemaDefinitionToSQLStrings(sd *tabletmanagerdatapb.SchemaDefinition) []st // Backtick database name since keyspace names appear in the routing rules, and they might need to be escaped. // We unescape() them first in case we have an explicitly escaped string was specified. - createDatabaseSQL := strings.Replace(sd.DatabaseSchema, "`{{.DatabaseName}}`", "{{.DatabaseName}}", -1) - createDatabaseSQL = strings.Replace(createDatabaseSQL, "{{.DatabaseName}}", sqlescape.EscapeID("{{.DatabaseName}}"), -1) + createDatabaseSQL := UnqualifyDatabaseNamePlaceholder(sd.DatabaseSchema) + createDatabaseSQL = QualifyDatabaseNamePlaceholder(createDatabaseSQL) sqlStrings = append(sqlStrings, createDatabaseSQL) for _, td := range sd.TableDefinitions { @@ -226,7 +235,7 @@ func SchemaDefinitionToSQLStrings(sd *tabletmanagerdatapb.SchemaDefinition) []st lines := strings.Split(td.Schema, "\n") for i, line := range lines { if strings.HasPrefix(line, "CREATE TABLE `") { - lines[i] = strings.Replace(line, "CREATE TABLE `", "CREATE TABLE `{{.DatabaseName}}`.`", 1) + lines[i] = strings.Replace(line, "CREATE TABLE `", fmt.Sprintf("CREATE TABLE `%s`.`", DatabaseNamePlaceholder), 1) } } sqlStrings = append(sqlStrings, strings.Join(lines, "\n")) From f13fc87f179b220bd370e5fc53cab109e9b5aeed Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Mon, 28 Aug 2023 14:47:28 +0300 Subject: [PATCH 04/27] allow changing the empty qualifier Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/sqlparser/utils.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/go/vt/sqlparser/utils.go b/go/vt/sqlparser/utils.go index 0f3c66f2ea3..2ae6d9f8a3d 100644 --- a/go/vt/sqlparser/utils.go +++ b/go/vt/sqlparser/utils.go @@ -135,15 +135,13 @@ func ReplaceTableQualifiers(query, olddb, newdb string) (string, error) { upd := Rewrite(in, func(cursor *Cursor) bool { switch node := cursor.Node().(type) { case TableName: - if !node.Qualifier.IsEmpty() && - node.Qualifier.String() == oldQualifier.String() { + if node.Qualifier.String() == oldQualifier.String() { node.Qualifier = newQualifier cursor.Replace(node) modified = true } case *ShowBasic: // for things like 'show tables from _vt' - if !node.DbName.IsEmpty() && - node.DbName.String() == oldQualifier.String() { + if node.DbName.String() == oldQualifier.String() { node.DbName = newQualifier cursor.Replace(node) modified = true From 4e2f9a00e4b307593441e44c19f5398c38b92d27 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Mon, 28 Aug 2023 14:47:55 +0300 Subject: [PATCH 05/27] more test cases Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/sqlparser/utils_test.go | 124 ++++++++++++++++++++++------------ 1 file changed, 82 insertions(+), 42 deletions(-) diff --git a/go/vt/sqlparser/utils_test.go b/go/vt/sqlparser/utils_test.go index 63c9b10ba43..91cfbb40ad8 100644 --- a/go/vt/sqlparser/utils_test.go +++ b/go/vt/sqlparser/utils_test.go @@ -187,85 +187,125 @@ func TestReplaceTableQualifiers(t *testing.T) { tests := []struct { name string in string + origdb string newdb string out string wantErr bool }{ { name: "invalid select", + origdb: origDB, in: "select frog bar person", out: "", wantErr: true, }, { - name: "simple select", - in: "select * from _vt.foo", - out: "select * from foo", + name: "simple select", + origdb: origDB, + in: "select * from _vt.foo", + out: "select * from foo", }, { - name: "simple select with new db", - in: "select * from _vt.foo", - newdb: "_vt_test", - out: "select * from _vt_test.foo", + name: "simple select with new db", + in: "select * from _vt.foo", + origdb: origDB, + newdb: "_vt_test", + out: "select * from _vt_test.foo", }, { - name: "simple select with new db same", - in: "select * from _vt.foo where id=1", // should be unchanged - newdb: "_vt", - out: "select * from _vt.foo where id=1", + name: "simple select with new db same", + in: "select * from _vt.foo where id=1", // should be unchanged + origdb: origDB, + newdb: "_vt", + out: "select * from _vt.foo where id=1", }, { - name: "simple select with new db needing escaping", - in: "select * from _vt.foo", - newdb: "1_vt-test", - out: "select * from `1_vt-test`.foo", + name: "simple select with new db needing escaping", + in: "select * from _vt.foo", + origdb: origDB, + newdb: "1_vt-test", + out: "select * from `1_vt-test`.foo", }, { - name: "complex select", - in: "select table_name, lastpk from _vt.copy_state where vrepl_id = 1 and id in (select max(id) from _vt.copy_state where vrepl_id = 1 group by vrepl_id, table_name)", - out: "select table_name, lastpk from copy_state where vrepl_id = 1 and id in (select max(id) from copy_state where vrepl_id = 1 group by vrepl_id, table_name)", + name: "complex select", + origdb: origDB, + in: "select table_name, lastpk from _vt.copy_state where vrepl_id = 1 and id in (select max(id) from _vt.copy_state where vrepl_id = 1 group by vrepl_id, table_name)", + out: "select table_name, lastpk from copy_state where vrepl_id = 1 and id in (select max(id) from copy_state where vrepl_id = 1 group by vrepl_id, table_name)", }, { - name: "complex mixed exists select", - in: "select workflow_name, db_name from _vt.vreplication where id = 1 and exists (select v1 from mydb.foo where fname = 'matt') and not exists (select v2 from _vt.newsidecartable where _vt.newsidecartable.id = _vt.vreplication.workflow_name)", - newdb: "_vt_import", - out: "select workflow_name, db_name from _vt_import.vreplication where id = 1 and exists (select v1 from mydb.foo where fname = 'matt') and not exists (select v2 from _vt_import.newsidecartable where _vt_import.newsidecartable.id = _vt_import.vreplication.workflow_name)", + name: "complex mixed exists select", + in: "select workflow_name, db_name from _vt.vreplication where id = 1 and exists (select v1 from mydb.foo where fname = 'matt') and not exists (select v2 from _vt.newsidecartable where _vt.newsidecartable.id = _vt.vreplication.workflow_name)", + origdb: origDB, + newdb: "_vt_import", + out: "select workflow_name, db_name from _vt_import.vreplication where id = 1 and exists (select v1 from mydb.foo where fname = 'matt') and not exists (select v2 from _vt_import.newsidecartable where _vt_import.newsidecartable.id = _vt_import.vreplication.workflow_name)", }, { - name: "derived table select", - in: "select myder.id from (select max(id) as id from _vt.copy_state where vrepl_id = 1 group by vrepl_id, table_name) as myder where id = 1", - newdb: "__vt-metadata", - out: "select myder.id from (select max(id) as id from `__vt-metadata`.copy_state where vrepl_id = 1 group by vrepl_id, table_name) as myder where id = 1", + name: "derived table select", + in: "select myder.id from (select max(id) as id from _vt.copy_state where vrepl_id = 1 group by vrepl_id, table_name) as myder where id = 1", + origdb: origDB, + newdb: "__vt-metadata", + out: "select myder.id from (select max(id) as id from `__vt-metadata`.copy_state where vrepl_id = 1 group by vrepl_id, table_name) as myder where id = 1", }, { - name: "complex select", - in: "select t1.col1, t2.col2 from _vt.t1 as t1 join _vt.t2 as t2 on t1.id = t2.id", - out: "select t1.col1, t2.col2 from t1 as t1 join t2 as t2 on t1.id = t2.id", + name: "complex select", + origdb: origDB, + in: "select t1.col1, t2.col2 from _vt.t1 as t1 join _vt.t2 as t2 on t1.id = t2.id", + out: "select t1.col1, t2.col2 from t1 as t1 join t2 as t2 on t1.id = t2.id", }, { - name: "simple insert", - in: "insert into _vt.foo(id) values (1)", - out: "insert into foo(id) values (1)", + name: "simple insert", + origdb: origDB, + in: "insert into _vt.foo(id) values (1)", + out: "insert into foo(id) values (1)", }, { - name: "simple update", - in: "update _vt.foo set id = 1", - out: "update foo set id = 1", + name: "simple update", + origdb: origDB, + in: "update _vt.foo set id = 1", + out: "update foo set id = 1", }, { - name: "simple delete", - in: "delete from _vt.foo where id = 1", - out: "delete from foo where id = 1", + name: "simple delete", + origdb: origDB, + in: "delete from _vt.foo where id = 1", + out: "delete from foo where id = 1", }, { - name: "simple set", - in: "set names 'binary'", - out: "set names 'binary'", + name: "simple set", + origdb: origDB, + in: "set names 'binary'", + out: "set names 'binary'", + }, + { + name: "simple create table", + origdb: origDB, + in: "CREATE TABLE t (id int primary key)", + out: "CREATE TABLE t (id int primary key)", + }, + { + name: "qualified create table", + origdb: origDB, + in: "CREATE TABLE `t` (id int primary key)", + out: "CREATE TABLE `t` (id int primary key)", + }, + { + name: "qualified create table, _vt", + origdb: origDB, + newdb: "mydb", + in: "create table `_vt`.`t` (id int primary key)", + out: "create table mydb.t (\n\tid int primary key\n)", + }, + { + name: "empty qualifier create table", + origdb: "", + newdb: "mydb", + in: "CREATE TABLE `t` (id int primary key)", + out: "create table mydb.t (\n\tid int primary key\n)", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := ReplaceTableQualifiers(tt.in, origDB, tt.newdb) + got, err := ReplaceTableQualifiers(tt.in, tt.origdb, tt.newdb) if tt.wantErr { require.Error(t, err) } else { From 76e5cf3b694fdf8eb4b98ba117616fea8df5a33a Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Mon, 28 Aug 2023 15:03:54 +0300 Subject: [PATCH 06/27] fix unit tests (typo in table3 creation), adapt to allowing empty schema to translate into qualified schema Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/mysqlctl/tmutils/schema.go | 17 +++++++------ go/vt/mysqlctl/tmutils/schema_test.go | 36 ++++++++++++++++----------- 2 files changed, 31 insertions(+), 22 deletions(-) diff --git a/go/vt/mysqlctl/tmutils/schema.go b/go/vt/mysqlctl/tmutils/schema.go index a75b1712842..18041d3544e 100644 --- a/go/vt/mysqlctl/tmutils/schema.go +++ b/go/vt/mysqlctl/tmutils/schema.go @@ -27,6 +27,8 @@ import ( "vitess.io/vitess/go/vt/concurrency" "vitess.io/vitess/go/vt/schema" + "vitess.io/vitess/go/vt/sqlparser" + "vitess.io/vitess/go/vt/vterrors" tabletmanagerdatapb "vitess.io/vitess/go/vt/proto/tabletmanagerdata" ) @@ -215,7 +217,7 @@ func SchemaDefinitionGetTable(sd *tabletmanagerdatapb.SchemaDefinition, table st // SchemaDefinitionToSQLStrings converts a SchemaDefinition to an array of SQL strings. The array contains all // the SQL statements needed for creating the database, tables, and views - in that order. // All SQL statements will have {{.DatabaseName}} in place of the actual db name. -func SchemaDefinitionToSQLStrings(sd *tabletmanagerdatapb.SchemaDefinition) []string { +func SchemaDefinitionToSQLStrings(sd *tabletmanagerdatapb.SchemaDefinition) ([]string, error) { sqlStrings := make([]string, 0, len(sd.TableDefinitions)+1) createViewSQL := make([]string, 0, len(sd.TableDefinitions)) @@ -232,17 +234,16 @@ func SchemaDefinitionToSQLStrings(sd *tabletmanagerdatapb.SchemaDefinition) []st if td.Type == TableView { createViewSQL = append(createViewSQL, td.Schema) } else { - lines := strings.Split(td.Schema, "\n") - for i, line := range lines { - if strings.HasPrefix(line, "CREATE TABLE `") { - lines[i] = strings.Replace(line, "CREATE TABLE `", fmt.Sprintf("CREATE TABLE `%s`.`", DatabaseNamePlaceholder), 1) - } + replaced, err := sqlparser.ReplaceTableQualifiers(td.Schema, "", DatabaseNamePlaceholder) + if err != nil { + // parsing unsuccessful + return nil, vterrors.Wrapf(err, "parsing schema: %v", td.Schema) } - sqlStrings = append(sqlStrings, strings.Join(lines, "\n")) + sqlStrings = append(sqlStrings, replaced) } } - return append(sqlStrings, createViewSQL...) + return append(sqlStrings, createViewSQL...), nil } // DiffSchema generates a report on what's different between two SchemaDefinitions diff --git a/go/vt/mysqlctl/tmutils/schema_test.go b/go/vt/mysqlctl/tmutils/schema_test.go index 2b9ff3472b2..6e5cb1cc32c 100644 --- a/go/vt/mysqlctl/tmutils/schema_test.go +++ b/go/vt/mysqlctl/tmutils/schema_test.go @@ -30,19 +30,19 @@ import ( var basicTable1 = &tabletmanagerdatapb.TableDefinition{ Name: "table1", - Schema: "table schema 1", + Schema: "create table table1 (id int primary key)", Type: TableBaseTable, } var basicTable2 = &tabletmanagerdatapb.TableDefinition{ Name: "table2", - Schema: "table schema 2", + Schema: "create table table2 (id int primary key)", Type: TableBaseTable, } var table3 = &tabletmanagerdatapb.TableDefinition{ - Name: "table2", + Name: "table3", Schema: "CREATE TABLE `table3` (\n" + - "id bigint not null,\n" + + "id bigint not null\n" + ") Engine=InnoDB", Type: TableBaseTable, } @@ -73,7 +73,11 @@ func TestToSQLStrings(t *testing.T) { view1, }, }, - want: []string{"CREATE DATABASE `{{.DatabaseName}}`", basicTable1.Schema, view1.Schema}, + want: []string{ + "CREATE DATABASE `{{.DatabaseName}}`", + "create table `{{.DatabaseName}}`.table1 (\n\tid int primary key\n)", + "view schema 1", + }, }, { // SchemaDefinition doesn't need any tables or views @@ -96,7 +100,11 @@ func TestToSQLStrings(t *testing.T) { basicTable2, }, }, - want: []string{"CREATE DATABASE `{{.DatabaseName}}`", basicTable1.Schema, basicTable2.Schema}, + want: []string{ + "CREATE DATABASE `{{.DatabaseName}}`", + "create table `{{.DatabaseName}}`.table1 (\n\tid int primary key\n)", + "create table `{{.DatabaseName}}`.table2 (\n\tid int primary key\n)", + }, }, { // multiple tables and views should be ordered with all tables before views @@ -110,9 +118,10 @@ func TestToSQLStrings(t *testing.T) { }, }, want: []string{ - "CREATE DATABASE `{{.DatabaseName}}`", - basicTable1.Schema, basicTable2.Schema, - view1.Schema, view2.Schema, + "CREATE DATABASE `{{.DatabaseName}}`", "create table `{{.DatabaseName}}`.table1 (\n\tid int primary key\n)", + "create table `{{.DatabaseName}}`.table2 (\n\tid int primary key\n)", + "view schema 1", + "view schema 2", }, }, { @@ -126,16 +135,15 @@ func TestToSQLStrings(t *testing.T) { }, want: []string{ "CREATE DATABASE `{{.DatabaseName}}`", - basicTable1.Schema, - "CREATE TABLE `{{.DatabaseName}}`.`table3` (\n" + - "id bigint not null,\n" + - ") Engine=InnoDB", + "create table `{{.DatabaseName}}`.table1 (\n\tid int primary key\n)", + "create table `{{.DatabaseName}}`.table3 (\n\tid bigint not null\n) Engine InnoDB", }, }, } for _, tc := range testcases { - got := SchemaDefinitionToSQLStrings(tc.input) + got, err := SchemaDefinitionToSQLStrings(tc.input) + assert.NoError(t, err) assert.Equal(t, tc.want, got) } } From 62921bd8859be5331ab5160dff806a602aa95c1e Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Mon, 28 Aug 2023 15:07:12 +0300 Subject: [PATCH 07/27] PreflightSchemaChange is deprecated Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/mysqlctl/schema.go | 71 ++-------------------------------------- 1 file changed, 2 insertions(+), 69 deletions(-) diff --git a/go/vt/mysqlctl/schema.go b/go/vt/mysqlctl/schema.go index abb7a4e4f99..d60507b3a23 100644 --- a/go/vt/mysqlctl/schema.go +++ b/go/vt/mysqlctl/schema.go @@ -424,76 +424,9 @@ func (mysqld *Mysqld) getPrimaryKeyColumns(ctx context.Context, dbName string, t return colMap, err } -// PreflightSchemaChange checks the schema changes in "changes" by applying them -// to an intermediate database that has the same schema as the target database. +// PreflightSchemaChange is deprecated func (mysqld *Mysqld) PreflightSchemaChange(ctx context.Context, dbName string, changes []string) ([]*tabletmanagerdatapb.SchemaChangeResult, error) { - results := make([]*tabletmanagerdatapb.SchemaChangeResult, len(changes)) - - // Get current schema from the real database. - req := &tabletmanagerdatapb.GetSchemaRequest{IncludeViews: true, TableSchemaOnly: true} - originalSchema, err := mysqld.GetSchema(ctx, dbName, req) - if err != nil { - return nil, err - } - - // Populate temporary database with it. - initialCopySQL := "SET sql_log_bin = 0;\n" - initialCopySQL += "DROP DATABASE IF EXISTS _vt_preflight;\n" - initialCopySQL += "CREATE DATABASE _vt_preflight;\n" - initialCopySQL += "USE _vt_preflight;\n" - // We're not smart enough to create the tables in a foreign-key-compatible way, - // so we temporarily disable foreign key checks while adding the existing tables. - initialCopySQL += "SET foreign_key_checks = 0;\n" - for _, td := range originalSchema.TableDefinitions { - if td.Type == tmutils.TableBaseTable { - initialCopySQL += td.Schema + ";\n" - } - } - for _, td := range originalSchema.TableDefinitions { - if td.Type == tmutils.TableView { - // Views will have {{.DatabaseName}} in there, replace - // it with _vt_preflight - s := strings.Replace(td.Schema, tmutils.DatabaseNamePlaceholder, "`_vt_preflight`", -1) - initialCopySQL += s + ";\n" - } - } - if err = mysqld.executeSchemaCommands(ctx, initialCopySQL); err != nil { - return nil, err - } - - // For each change, record the schema before and after. - for i, change := range changes { - req := &tabletmanagerdatapb.GetSchemaRequest{IncludeViews: true} - beforeSchema, err := mysqld.GetSchema(ctx, "_vt_preflight", req) - if err != nil { - return nil, err - } - - // apply schema change to the temporary database - sql := "SET sql_log_bin = 0;\n" - sql += "USE _vt_preflight;\n" - sql += change - if err = mysqld.executeSchemaCommands(ctx, sql); err != nil { - return nil, err - } - - // get the result - afterSchema, err := mysqld.GetSchema(ctx, "_vt_preflight", req) - if err != nil { - return nil, err - } - - results[i] = &tabletmanagerdatapb.SchemaChangeResult{BeforeSchema: beforeSchema, AfterSchema: afterSchema} - } - - // and clean up the extra database - dropSQL := "SET sql_log_bin = 0;\n" - dropSQL += "DROP DATABASE _vt_preflight;\n" - if err = mysqld.executeSchemaCommands(ctx, dropSQL); err != nil { - return nil, err - } - - return results, nil + return nil, vterrors.Errorf(vtrpc.Code_UNIMPLEMENTED, "PreflightSchemaChange is deprecated") } // ApplySchemaChange will apply the schema change to the given database. From 239070219d8f41947862b8e03acc742e71577be3 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Mon, 28 Aug 2023 15:09:53 +0300 Subject: [PATCH 08/27] check error value Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/wrangler/schema.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/go/vt/wrangler/schema.go b/go/vt/wrangler/schema.go index 84bc078f240..79104329e9d 100644 --- a/go/vt/wrangler/schema.go +++ b/go/vt/wrangler/schema.go @@ -225,7 +225,10 @@ func (wr *Wrangler) CopySchemaShard(ctx context.Context, sourceTabletAlias *topo return fmt.Errorf("GetSchema(%v, %v, %v, %v) failed: %v", sourceTabletAlias, tables, excludeTables, includeViews, err) } - createSQLstmts := tmutils.SchemaDefinitionToSQLStrings(sourceSd) + createSQLstmts, err := tmutils.SchemaDefinitionToSQLStrings(sourceSd) + if err != nil { + return err + } destTabletInfo, err := wr.ts.GetTablet(ctx, destShardInfo.PrimaryAlias) if err != nil { From 0ac3f4985f5df26f079a48e3a3a0eea6808859b4 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Tue, 29 Aug 2023 09:24:21 +0300 Subject: [PATCH 09/27] Use CanonicalString Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/mysqlctl/schema_test.go | 8 ++++---- go/vt/mysqlctl/tmutils/schema_test.go | 24 ++++++++++++------------ go/vt/sqlparser/utils.go | 3 ++- go/vt/sqlparser/utils_test.go | 24 ++++++++++++------------ 4 files changed, 30 insertions(+), 29 deletions(-) diff --git a/go/vt/mysqlctl/schema_test.go b/go/vt/mysqlctl/schema_test.go index e7b77c7752d..6df13ce00fd 100644 --- a/go/vt/mysqlctl/schema_test.go +++ b/go/vt/mysqlctl/schema_test.go @@ -130,25 +130,25 @@ func TestNormalizedStatement(t *testing.T) { statement: "create view `mydb`.v as select * from t", db: "mydb", typ: tmutils.TableView, - expect: "create view {{.DatabaseName}}.v as select * from t", + expect: "CREATE VIEW {{.DatabaseName}}.`v` AS SELECT * FROM `t`", }, { statement: "create view `mydb`.v as select * from `mydb`.`t`", db: "mydb", typ: tmutils.TableView, - expect: "create view {{.DatabaseName}}.v as select * from {{.DatabaseName}}.t", + expect: "CREATE VIEW {{.DatabaseName}}.`v` AS SELECT * FROM {{.DatabaseName}}.`t`", }, { statement: "create view `mydb`.v as select * from `mydb`.mydb", db: "mydb", typ: tmutils.TableView, - expect: "create view {{.DatabaseName}}.v as select * from {{.DatabaseName}}.mydb", + expect: "CREATE VIEW {{.DatabaseName}}.`v` AS SELECT * FROM {{.DatabaseName}}.`mydb`", }, { statement: "create view `mydb`.v as select * from `mydb`.`mydb`", db: "mydb", typ: tmutils.TableView, - expect: "create view {{.DatabaseName}}.v as select * from {{.DatabaseName}}.mydb", + expect: "CREATE VIEW {{.DatabaseName}}.`v` AS SELECT * FROM {{.DatabaseName}}.`mydb`", }, } ctx := context.Background() diff --git a/go/vt/mysqlctl/tmutils/schema_test.go b/go/vt/mysqlctl/tmutils/schema_test.go index 6e5cb1cc32c..bf0b02ae62f 100644 --- a/go/vt/mysqlctl/tmutils/schema_test.go +++ b/go/vt/mysqlctl/tmutils/schema_test.go @@ -49,13 +49,13 @@ var table3 = &tabletmanagerdatapb.TableDefinition{ var view1 = &tabletmanagerdatapb.TableDefinition{ Name: "view1", - Schema: "view schema 1", + Schema: "create view view1 as select id from t1", Type: TableView, } var view2 = &tabletmanagerdatapb.TableDefinition{ Name: "view2", - Schema: "view schema 2", + Schema: "create view view2 as select id from t2", Type: TableView, } @@ -75,8 +75,8 @@ func TestToSQLStrings(t *testing.T) { }, want: []string{ "CREATE DATABASE `{{.DatabaseName}}`", - "create table `{{.DatabaseName}}`.table1 (\n\tid int primary key\n)", - "view schema 1", + "CREATE TABLE `{{.DatabaseName}}`.`table1` (\n\t`id` int PRIMARY KEY\n)", + "create view view1 as select id from t1", }, }, { @@ -102,8 +102,8 @@ func TestToSQLStrings(t *testing.T) { }, want: []string{ "CREATE DATABASE `{{.DatabaseName}}`", - "create table `{{.DatabaseName}}`.table1 (\n\tid int primary key\n)", - "create table `{{.DatabaseName}}`.table2 (\n\tid int primary key\n)", + "CREATE TABLE `{{.DatabaseName}}`.`table1` (\n\t`id` int PRIMARY KEY\n)", + "CREATE TABLE `{{.DatabaseName}}`.`table2` (\n\t`id` int PRIMARY KEY\n)", }, }, { @@ -118,10 +118,10 @@ func TestToSQLStrings(t *testing.T) { }, }, want: []string{ - "CREATE DATABASE `{{.DatabaseName}}`", "create table `{{.DatabaseName}}`.table1 (\n\tid int primary key\n)", - "create table `{{.DatabaseName}}`.table2 (\n\tid int primary key\n)", - "view schema 1", - "view schema 2", + "CREATE DATABASE `{{.DatabaseName}}`", "CREATE TABLE `{{.DatabaseName}}`.`table1` (\n\t`id` int PRIMARY KEY\n)", + "CREATE TABLE `{{.DatabaseName}}`.`table2` (\n\t`id` int PRIMARY KEY\n)", + "create view view1 as select id from t1", + "create view view2 as select id from t2", }, }, { @@ -135,8 +135,8 @@ func TestToSQLStrings(t *testing.T) { }, want: []string{ "CREATE DATABASE `{{.DatabaseName}}`", - "create table `{{.DatabaseName}}`.table1 (\n\tid int primary key\n)", - "create table `{{.DatabaseName}}`.table3 (\n\tid bigint not null\n) Engine InnoDB", + "CREATE TABLE `{{.DatabaseName}}`.`table1` (\n\t`id` int PRIMARY KEY\n)", + "CREATE TABLE `{{.DatabaseName}}`.`table3` (\n\t`id` bigint NOT NULL\n) ENGINE InnoDB", }, }, } diff --git a/go/vt/sqlparser/utils.go b/go/vt/sqlparser/utils.go index 2ae6d9f8a3d..31bf4ca8e33 100644 --- a/go/vt/sqlparser/utils.go +++ b/go/vt/sqlparser/utils.go @@ -118,6 +118,7 @@ func NormalizeAlphabetically(query string) (normalized string, err error) { // replaces any cases of the provided database name with the // specified replacement name. // Note: both database names provided should be unescaped strings. +// The returned query is formatted canonically (will look different than original query) func ReplaceTableQualifiers(query, olddb, newdb string) (string, error) { if newdb == olddb { // Nothing to do here. @@ -154,7 +155,7 @@ func ReplaceTableQualifiers(query, olddb, newdb string) (string, error) { // execute a query which slightly differs from the parsed // version: e.g. 'where id=1' becomes 'where id = 1'. if modified { - return String(upd), nil + return CanonicalString(upd), nil } return query, nil } diff --git a/go/vt/sqlparser/utils_test.go b/go/vt/sqlparser/utils_test.go index 91cfbb40ad8..602baa5230c 100644 --- a/go/vt/sqlparser/utils_test.go +++ b/go/vt/sqlparser/utils_test.go @@ -203,14 +203,14 @@ func TestReplaceTableQualifiers(t *testing.T) { name: "simple select", origdb: origDB, in: "select * from _vt.foo", - out: "select * from foo", + out: "SELECT * FROM `foo`", }, { name: "simple select with new db", in: "select * from _vt.foo", origdb: origDB, newdb: "_vt_test", - out: "select * from _vt_test.foo", + out: "SELECT * FROM `_vt_test`.`foo`", }, { name: "simple select with new db same", @@ -224,51 +224,51 @@ func TestReplaceTableQualifiers(t *testing.T) { in: "select * from _vt.foo", origdb: origDB, newdb: "1_vt-test", - out: "select * from `1_vt-test`.foo", + out: "SELECT * FROM `1_vt-test`.`foo`", }, { name: "complex select", origdb: origDB, in: "select table_name, lastpk from _vt.copy_state where vrepl_id = 1 and id in (select max(id) from _vt.copy_state where vrepl_id = 1 group by vrepl_id, table_name)", - out: "select table_name, lastpk from copy_state where vrepl_id = 1 and id in (select max(id) from copy_state where vrepl_id = 1 group by vrepl_id, table_name)", + out: "SELECT `table_name`, `lastpk` FROM `copy_state` WHERE `vrepl_id` = 1 AND `id` IN (SELECT max(`id`) FROM `copy_state` WHERE `vrepl_id` = 1 GROUP BY `vrepl_id`, `table_name`)", }, { name: "complex mixed exists select", in: "select workflow_name, db_name from _vt.vreplication where id = 1 and exists (select v1 from mydb.foo where fname = 'matt') and not exists (select v2 from _vt.newsidecartable where _vt.newsidecartable.id = _vt.vreplication.workflow_name)", origdb: origDB, newdb: "_vt_import", - out: "select workflow_name, db_name from _vt_import.vreplication where id = 1 and exists (select v1 from mydb.foo where fname = 'matt') and not exists (select v2 from _vt_import.newsidecartable where _vt_import.newsidecartable.id = _vt_import.vreplication.workflow_name)", + out: "SELECT `workflow_name`, `db_name` FROM `_vt_import`.`vreplication` WHERE `id` = 1 AND EXISTS (SELECT `v1` FROM `mydb`.`foo` WHERE `fname` = 'matt') AND NOT EXISTS (SELECT `v2` FROM `_vt_import`.`newsidecartable` WHERE `_vt_import`.`newsidecartable`.`id` = `_vt_import`.`vreplication`.`workflow_name`)", }, { name: "derived table select", in: "select myder.id from (select max(id) as id from _vt.copy_state where vrepl_id = 1 group by vrepl_id, table_name) as myder where id = 1", origdb: origDB, newdb: "__vt-metadata", - out: "select myder.id from (select max(id) as id from `__vt-metadata`.copy_state where vrepl_id = 1 group by vrepl_id, table_name) as myder where id = 1", + out: "SELECT `myder`.`id` FROM (SELECT max(`id`) AS `id` FROM `__vt-metadata`.`copy_state` WHERE `vrepl_id` = 1 GROUP BY `vrepl_id`, `table_name`) AS `myder` WHERE `id` = 1", }, { name: "complex select", origdb: origDB, in: "select t1.col1, t2.col2 from _vt.t1 as t1 join _vt.t2 as t2 on t1.id = t2.id", - out: "select t1.col1, t2.col2 from t1 as t1 join t2 as t2 on t1.id = t2.id", + out: "SELECT `t1`.`col1`, `t2`.`col2` FROM `t1` AS `t1` JOIN `t2` AS `t2` ON `t1`.`id` = `t2`.`id`", }, { name: "simple insert", origdb: origDB, in: "insert into _vt.foo(id) values (1)", - out: "insert into foo(id) values (1)", + out: "INSERT INTO `foo`(`id`) VALUES (1)", }, { name: "simple update", origdb: origDB, in: "update _vt.foo set id = 1", - out: "update foo set id = 1", + out: "UPDATE `foo` SET `id` = 1", }, { name: "simple delete", origdb: origDB, in: "delete from _vt.foo where id = 1", - out: "delete from foo where id = 1", + out: "DELETE FROM `foo` WHERE `id` = 1", }, { name: "simple set", @@ -293,14 +293,14 @@ func TestReplaceTableQualifiers(t *testing.T) { origdb: origDB, newdb: "mydb", in: "create table `_vt`.`t` (id int primary key)", - out: "create table mydb.t (\n\tid int primary key\n)", + out: "CREATE TABLE `mydb`.`t` (\n\t`id` int PRIMARY KEY\n)", }, { name: "empty qualifier create table", origdb: "", newdb: "mydb", in: "CREATE TABLE `t` (id int primary key)", - out: "create table mydb.t (\n\tid int primary key\n)", + out: "CREATE TABLE `mydb`.`t` (\n\t`id` int PRIMARY KEY\n)", }, } for _, tt := range tests { From 7d30621c1515171a3c789ed7dbca95402b33bdb3 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Tue, 29 Aug 2023 10:38:03 +0300 Subject: [PATCH 10/27] normalize expected queries Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/wrangler/testlib/copy_schema_shard_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/go/vt/wrangler/testlib/copy_schema_shard_test.go b/go/vt/wrangler/testlib/copy_schema_shard_test.go index af80203ea9e..59d19a4021c 100644 --- a/go/vt/wrangler/testlib/copy_schema_shard_test.go +++ b/go/vt/wrangler/testlib/copy_schema_shard_test.go @@ -94,12 +94,12 @@ func copySchema(t *testing.T, useShardAsSource bool) { TableDefinitions: []*tabletmanagerdatapb.TableDefinition{ { Name: "table1", - Schema: "CREATE TABLE `table1` (\n `id` bigint(20) NOT NULL AUTO_INCREMENT,\n `msg` varchar(64) DEFAULT NULL,\n `keyspace_id` bigint(20) unsigned NOT NULL,\n PRIMARY KEY (`id`),\n KEY `by_msg` (`msg`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8", + Schema: "CREATE TABLE `vt_ks`.`table1` (\n `id` bigint(20) NOT NULL AUTO_INCREMENT,\n `msg` varchar(64) DEFAULT NULL,\n `keyspace_id` bigint(20) unsigned NOT NULL,\n PRIMARY KEY (`id`),\n KEY `by_msg` (`msg`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8", Type: tmutils.TableBaseTable, }, { Name: "view1", - Schema: "CREATE TABLE `view1` (\n `id` bigint(20) NOT NULL AUTO_INCREMENT,\n `msg` varchar(64) DEFAULT NULL,\n `keyspace_id` bigint(20) unsigned NOT NULL,\n PRIMARY KEY (`id`),\n KEY `by_msg` (`msg`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8", + Schema: "CREATE TABLE `vt_ks`.`view1` (\n `id` bigint(20) NOT NULL AUTO_INCREMENT,\n `msg` varchar(64) DEFAULT NULL,\n `keyspace_id` bigint(20) unsigned NOT NULL,\n PRIMARY KEY (`id`),\n KEY `by_msg` (`msg`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8", Type: tmutils.TableView, }, }, @@ -121,7 +121,7 @@ func copySchema(t *testing.T, useShardAsSource bool) { " PRIMARY KEY (`id`),\n" + " KEY `by_msg` (`msg`)\n" + ") ENGINE=InnoDB DEFAULT CHARSET=utf8" - createTableView := "CREATE TABLE `view1` (\n" + + createTableView := "CREATE TABLE `vt_ks`.`view1` (\n" + " `id` bigint(20) NOT NULL AUTO_INCREMENT,\n" + " `msg` varchar(64) DEFAULT NULL,\n" + " `keyspace_id` bigint(20) unsigned NOT NULL,\n" + From d170ce9af01fe107adeef499c891480660d4fd94 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Tue, 29 Aug 2023 11:16:40 +0300 Subject: [PATCH 11/27] normalize definition Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/test/endtoend/tabletmanager/commands_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/test/endtoend/tabletmanager/commands_test.go b/go/test/endtoend/tabletmanager/commands_test.go index 1a2d2424cb4..7a6236b6a76 100644 --- a/go/test/endtoend/tabletmanager/commands_test.go +++ b/go/test/endtoend/tabletmanager/commands_test.go @@ -39,7 +39,7 @@ var ( getSchemaT1Results8030 = "CREATE TABLE `t1` (\n `id` bigint NOT NULL,\n `value` varchar(16) DEFAULT NULL,\n PRIMARY KEY (`id`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8mb3" getSchemaT1Results80 = "CREATE TABLE `t1` (\n `id` bigint NOT NULL,\n `value` varchar(16) DEFAULT NULL,\n PRIMARY KEY (`id`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8" getSchemaT1Results57 = "CREATE TABLE `t1` (\n `id` bigint(20) NOT NULL,\n `value` varchar(16) DEFAULT NULL,\n PRIMARY KEY (`id`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8" - getSchemaV1Results = fmt.Sprintf("CREATE ALGORITHM=UNDEFINED DEFINER=`%s`@`%s` SQL SECURITY DEFINER VIEW {{.DatabaseName}}.`v1` AS select {{.DatabaseName}}.`t1`.`id` AS `id`,{{.DatabaseName}}.`t1`.`value` AS `value` from {{.DatabaseName}}.`t1`", username, hostname) + getSchemaV1Results = fmt.Sprintf("CREATE ALGORITHM = UNDEFINED DEFINER = `%s`@`%s` SQL SECURITY DEFINER VIEW {{.DatabaseName}}.`v1` AS SELECT {{.DatabaseName}}.`t1`.`id` AS `id`,{{.DatabaseName}}.`t1`.`value` AS `value` FROM {{.DatabaseName}}.`t1`", username, hostname) ) // TabletCommands tests the basic tablet commands From bad39ffe39c0723285669b7eb51e7c27850d4ebb Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Tue, 29 Aug 2023 11:37:39 +0300 Subject: [PATCH 12/27] normalize definition Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/test/endtoend/tabletmanager/commands_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/test/endtoend/tabletmanager/commands_test.go b/go/test/endtoend/tabletmanager/commands_test.go index 7a6236b6a76..bc6fba6cf52 100644 --- a/go/test/endtoend/tabletmanager/commands_test.go +++ b/go/test/endtoend/tabletmanager/commands_test.go @@ -39,7 +39,7 @@ var ( getSchemaT1Results8030 = "CREATE TABLE `t1` (\n `id` bigint NOT NULL,\n `value` varchar(16) DEFAULT NULL,\n PRIMARY KEY (`id`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8mb3" getSchemaT1Results80 = "CREATE TABLE `t1` (\n `id` bigint NOT NULL,\n `value` varchar(16) DEFAULT NULL,\n PRIMARY KEY (`id`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8" getSchemaT1Results57 = "CREATE TABLE `t1` (\n `id` bigint(20) NOT NULL,\n `value` varchar(16) DEFAULT NULL,\n PRIMARY KEY (`id`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8" - getSchemaV1Results = fmt.Sprintf("CREATE ALGORITHM = UNDEFINED DEFINER = `%s`@`%s` SQL SECURITY DEFINER VIEW {{.DatabaseName}}.`v1` AS SELECT {{.DatabaseName}}.`t1`.`id` AS `id`,{{.DatabaseName}}.`t1`.`value` AS `value` FROM {{.DatabaseName}}.`t1`", username, hostname) + getSchemaV1Results = fmt.Sprintf("CREATE ALGORITHM = UNDEFINED DEFINER = %s@%s SQL SECURITY DEFINER VIEW {{.DatabaseName}}.`v1` AS SELECT {{.DatabaseName}}.`t1`.`id` AS `id`, {{.DatabaseName}}.`t1`.`value` AS `value` FROM {{.DatabaseName}}.`t1`", username, hostname) ) // TabletCommands tests the basic tablet commands From cb8100a2c70801fed5cb6befed08be7e8e7c8cc6 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Tue, 29 Aug 2023 15:53:44 +0300 Subject: [PATCH 13/27] handle CreateDatabase, AlterDatabase, DropDatabase statements Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/sqlparser/utils.go | 18 ++++++++++++++++++ go/vt/sqlparser/utils_test.go | 28 ++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/go/vt/sqlparser/utils.go b/go/vt/sqlparser/utils.go index 31bf4ca8e33..88ba74c11be 100644 --- a/go/vt/sqlparser/utils.go +++ b/go/vt/sqlparser/utils.go @@ -141,6 +141,24 @@ func ReplaceTableQualifiers(query, olddb, newdb string) (string, error) { cursor.Replace(node) modified = true } + case *CreateDatabase: + if node.DBName.String() == oldQualifier.String() { + node.DBName = newQualifier + cursor.Replace(node) + modified = true + } + case *AlterDatabase: + if node.DBName.String() == oldQualifier.String() { + node.DBName = newQualifier + cursor.Replace(node) + modified = true + } + case *DropDatabase: + if node.DBName.String() == oldQualifier.String() { + node.DBName = newQualifier + cursor.Replace(node) + modified = true + } case *ShowBasic: // for things like 'show tables from _vt' if node.DbName.String() == oldQualifier.String() { node.DbName = newQualifier diff --git a/go/vt/sqlparser/utils_test.go b/go/vt/sqlparser/utils_test.go index 602baa5230c..a47e739d25d 100644 --- a/go/vt/sqlparser/utils_test.go +++ b/go/vt/sqlparser/utils_test.go @@ -302,6 +302,34 @@ func TestReplaceTableQualifiers(t *testing.T) { in: "CREATE TABLE `t` (id int primary key)", out: "CREATE TABLE `mydb`.`t` (\n\t`id` int PRIMARY KEY\n)", }, + { + name: "create database", + origdb: "{{.DatabaseName}}", + newdb: "mydb", + in: "CREATE DATABASE `{{.DatabaseName}}`", + out: "CREATE DATABASE `mydb`", + }, + { + name: "create database, comments", + origdb: "{{.DatabaseName}}", + newdb: "mydb", + in: "CREATE DATABASE /*!32312 IF NOT EXISTS*/ `{{.DatabaseName}}` /*!40100 DEFAULT CHARACTER SET utf8mb4 COLLATE utf8mb4_0900_ai_ci */ /*!80016 DEFAULT ENCRYPTION='N' */", + out: "CREATE DATABASE IF NOT EXISTS `mydb` DEFAULT CHARACTER SET UTF8MB4 COLLATE UTF8MB4_0900_AI_CI DEFAULT ENCRYPTION 'N'", + }, + { + name: "create database, unqualified", + origdb: origDB, + newdb: "mydb", + in: "CREATE DATABASE _vt", + out: "CREATE DATABASE `mydb`", + }, + { + name: "drop database, unqualified", + origdb: origDB, + newdb: "mydb", + in: "drop database _vt", + out: "DROP DATABASE `mydb`", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From 6db7847f51331c417c06fd88735a2836f0b43438 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Tue, 29 Aug 2023 15:54:16 +0300 Subject: [PATCH 14/27] slight refactor Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/mysqlctl/tmutils/schema.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/go/vt/mysqlctl/tmutils/schema.go b/go/vt/mysqlctl/tmutils/schema.go index 18041d3544e..15a1d070bb2 100644 --- a/go/vt/mysqlctl/tmutils/schema.go +++ b/go/vt/mysqlctl/tmutils/schema.go @@ -223,7 +223,8 @@ func SchemaDefinitionToSQLStrings(sd *tabletmanagerdatapb.SchemaDefinition) ([]s // Backtick database name since keyspace names appear in the routing rules, and they might need to be escaped. // We unescape() them first in case we have an explicitly escaped string was specified. - createDatabaseSQL := UnqualifyDatabaseNamePlaceholder(sd.DatabaseSchema) + createDatabaseSQL := sd.DatabaseSchema + createDatabaseSQL = UnqualifyDatabaseNamePlaceholder(createDatabaseSQL) createDatabaseSQL = QualifyDatabaseNamePlaceholder(createDatabaseSQL) sqlStrings = append(sqlStrings, createDatabaseSQL) From 01c83f3a070638687e45ed297f47055a27bf1bd3 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Tue, 29 Aug 2023 15:54:42 +0300 Subject: [PATCH 15/27] get rid of text/template, use 'ReplaceTableQualifiers()' Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/wrangler/schema.go | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/go/vt/wrangler/schema.go b/go/vt/wrangler/schema.go index 79104329e9d..adc45a73a8e 100644 --- a/go/vt/wrangler/schema.go +++ b/go/vt/wrangler/schema.go @@ -17,11 +17,9 @@ limitations under the License. package wrangler import ( - "bytes" "context" "fmt" "sync" - "text/template" "time" "vitess.io/vitess/go/vt/concurrency" @@ -29,9 +27,11 @@ import ( "vitess.io/vitess/go/vt/logutil" "vitess.io/vitess/go/vt/mysqlctl/tmutils" "vitess.io/vitess/go/vt/schema" + "vitess.io/vitess/go/vt/sqlparser" "vitess.io/vitess/go/vt/topo" "vitess.io/vitess/go/vt/topo/topoproto" "vitess.io/vitess/go/vt/vtctl/schematools" + "vitess.io/vitess/go/vt/vterrors" "vitess.io/vitess/go/vt/vttablet/tabletmanager/vreplication" tabletmanagerdatapb "vitess.io/vitess/go/vt/proto/tabletmanagerdata" @@ -237,10 +237,10 @@ func (wr *Wrangler) CopySchemaShard(ctx context.Context, sourceTabletAlias *topo for _, createSQL := range createSQLstmts { err = wr.applySQLShard(ctx, destTabletInfo, createSQL) if err != nil { - return fmt.Errorf("creating a table failed."+ + return vterrors.Wrapf(err, "creating a table failed."+ " Most likely some tables already exist on the destination and differ from the source."+ " Please remove all to be copied tables from the destination manually and run this command again."+ - " Full error: %v", err) + " CREATE statement: %v", createSQL) } } @@ -291,7 +291,7 @@ func (wr *Wrangler) CopySchemaShard(ctx context.Context, sourceTabletAlias *topo // it shouldn't be used for anything that will require a pivot. // The SQL statement string is expected to have {{.DatabaseName}} in place of the actual db name. func (wr *Wrangler) applySQLShard(ctx context.Context, tabletInfo *topo.TabletInfo, change string) error { - filledChange, err := fillStringTemplate(change, map[string]string{"DatabaseName": tabletInfo.DbName()}) + filledChange, err := sqlparser.ReplaceTableQualifiers(change, tmutils.DatabaseNamePlaceholder, tabletInfo.DbName()) if err != nil { return fmt.Errorf("fillStringTemplate failed: %v", err) } @@ -306,13 +306,3 @@ func (wr *Wrangler) applySQLShard(ctx context.Context, tabletInfo *topo.TabletIn }) return err } - -// fillStringTemplate returns the string template filled -func fillStringTemplate(tmpl string, vars any) (string, error) { - myTemplate := template.Must(template.New("").Parse(tmpl)) - data := new(bytes.Buffer) - if err := myTemplate.Execute(data, vars); err != nil { - return "", err - } - return data.String(), nil -} From 514e70a10c25ca8912c1ab37c9fca7aa4bdcbd34 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Tue, 29 Aug 2023 17:56:46 +0300 Subject: [PATCH 16/27] normalize CREATE DATABASE query Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/wrangler/testlib/copy_schema_shard_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/vt/wrangler/testlib/copy_schema_shard_test.go b/go/vt/wrangler/testlib/copy_schema_shard_test.go index 59d19a4021c..a0e99966a91 100644 --- a/go/vt/wrangler/testlib/copy_schema_shard_test.go +++ b/go/vt/wrangler/testlib/copy_schema_shard_test.go @@ -113,7 +113,7 @@ func copySchema(t *testing.T, useShardAsSource bool) { setSQLMode := fmt.Sprintf("SET @@session.sql_mode='%v'", vreplication.SQLMode) changeToDb := "USE `vt_ks`" - createDb := "CREATE DATABASE `vt_ks` /*!40100 DEFAULT CHARACTER SET utf8 */" + createDb := "CREATE DATABASE `vt_ks` DEFAULT CHARACTER SET UTF8" createTable := "CREATE TABLE `vt_ks`.`table1` (\n" + " `id` bigint(20) NOT NULL AUTO_INCREMENT,\n" + " `msg` varchar(64) DEFAULT NULL,\n" + From efebccb29c86869ac71aaf8097d58b02b5b613f1 Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Tue, 29 Aug 2023 14:08:44 -0400 Subject: [PATCH 17/27] Add tablespace name test case to TestCanonicalOutput unit test Signed-off-by: Matt Lord --- go/vt/sqlparser/tracked_buffer_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/go/vt/sqlparser/tracked_buffer_test.go b/go/vt/sqlparser/tracked_buffer_test.go index af1de7c843e..28fefcbc240 100644 --- a/go/vt/sqlparser/tracked_buffer_test.go +++ b/go/vt/sqlparser/tracked_buffer_test.go @@ -104,6 +104,10 @@ func TestCanonicalOutput(t *testing.T) { "create table a (v varchar(32)) engine=InnoDB", "CREATE TABLE `a` (\n\t`v` varchar(32)\n) ENGINE InnoDB", }, + { // tablespace names are case-sensitive: https://dev.mysql.com/doc/refman/en/general-tablespaces.html + "create table a (v varchar(32)) engine=InnoDB tablespace innodb_system", + "CREATE TABLE `a` (\n\t`v` varchar(32)\n) ENGINE InnoDB TABLESPACE innodb_system", + }, { "create table a (id int not null primary key) engine InnoDB, charset utf8mb4, collate utf8mb4_0900_ai_ci partition by range (`id`) (partition `p10` values less than(10) engine InnoDB tablespace foo)", "CREATE TABLE `a` (\n\t`id` int NOT NULL PRIMARY KEY\n) ENGINE InnoDB,\n CHARSET utf8mb4,\n COLLATE utf8mb4_0900_ai_ci\nPARTITION BY RANGE (`id`)\n(PARTITION `p10` VALUES LESS THAN (10) ENGINE InnoDB TABLESPACE foo)", From 9ff992f70157cfff645a1d7d64f9e0f26dad590b Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Tue, 29 Aug 2023 14:16:36 -0400 Subject: [PATCH 18/27] Correct test case expectation Signed-off-by: Matt Lord --- go/vt/sqlparser/tracked_buffer_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/vt/sqlparser/tracked_buffer_test.go b/go/vt/sqlparser/tracked_buffer_test.go index 28fefcbc240..6924bf11911 100644 --- a/go/vt/sqlparser/tracked_buffer_test.go +++ b/go/vt/sqlparser/tracked_buffer_test.go @@ -106,7 +106,7 @@ func TestCanonicalOutput(t *testing.T) { }, { // tablespace names are case-sensitive: https://dev.mysql.com/doc/refman/en/general-tablespaces.html "create table a (v varchar(32)) engine=InnoDB tablespace innodb_system", - "CREATE TABLE `a` (\n\t`v` varchar(32)\n) ENGINE InnoDB TABLESPACE innodb_system", + "CREATE TABLE `a` (\n\t`v` varchar(32)\n) ENGINE InnoDB,\n TABLESPACE innodb_system", }, { "create table a (id int not null primary key) engine InnoDB, charset utf8mb4, collate utf8mb4_0900_ai_ci partition by range (`id`) (partition `p10` values less than(10) engine InnoDB tablespace foo)", From ad6c6c7fe4f09a405362ee2ad1a20a347b261a0e Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Tue, 29 Aug 2023 16:19:55 -0400 Subject: [PATCH 19/27] Properly treat TABLESPACE clause values as case sensitive https://dev.mysql.com/doc/refman/en/general-tablespaces.html Signed-off-by: Matt Lord --- go/vt/sqlparser/ast_funcs.go | 3 ++- go/vt/sqlparser/sql.go | 2 +- go/vt/sqlparser/sql.y | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/go/vt/sqlparser/ast_funcs.go b/go/vt/sqlparser/ast_funcs.go index 7ca1b7e92e3..100b7e5609a 100644 --- a/go/vt/sqlparser/ast_funcs.go +++ b/go/vt/sqlparser/ast_funcs.go @@ -86,7 +86,8 @@ type IndexOption struct { String string } -// TableOption is used for create table options like AUTO_INCREMENT, INSERT_METHOD, etc +// TableOption is used for create table options like AUTO_INCREMENT, INSERT_METHOD, +// TABLESPACE, etc type TableOption struct { Name string Value *Literal diff --git a/go/vt/sqlparser/sql.go b/go/vt/sqlparser/sql.go index fefff2361e7..3e801c79658 100644 --- a/go/vt/sqlparser/sql.go +++ b/go/vt/sqlparser/sql.go @@ -13470,7 +13470,7 @@ yydefault: var yyLOCAL *TableOption //line sql.y:2829 { - yyLOCAL = &TableOption{Name: string(yyDollar[1].str), String: (yyDollar[3].identifierCI.String() + yyDollar[4].str)} + yyLOCAL = &TableOption{Name: string(yyDollar[1].str), String: (yyDollar[3].identifierCI.String() + yyDollar[4].str), CaseSensitive: true} } yyVAL.union = yyLOCAL case 495: diff --git a/go/vt/sqlparser/sql.y b/go/vt/sqlparser/sql.y index a8160c3d463..7c7c2b1a85b 100644 --- a/go/vt/sqlparser/sql.y +++ b/go/vt/sqlparser/sql.y @@ -2827,7 +2827,7 @@ table_option: } | TABLESPACE equal_opt sql_id storage_opt { - $$ = &TableOption{Name:string($1), String: ($3.String() + $4)} + $$ = &TableOption{Name:string($1), String: ($3.String() + $4), CaseSensitive: true} } | UNION equal_opt '(' table_name_list ')' { From ab8e8f29c7e88305ccb9f57a3168232e50047c6c Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Wed, 30 Aug 2023 17:55:46 +0300 Subject: [PATCH 20/27] Revert CanonicalString() to String(). Adapt some of the unit tests Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/mysqlctl/schema_test.go | 8 ++++---- go/vt/mysqlctl/tmutils/schema_test.go | 14 +++++++------- go/vt/sqlparser/utils.go | 2 +- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/go/vt/mysqlctl/schema_test.go b/go/vt/mysqlctl/schema_test.go index 6df13ce00fd..e7b77c7752d 100644 --- a/go/vt/mysqlctl/schema_test.go +++ b/go/vt/mysqlctl/schema_test.go @@ -130,25 +130,25 @@ func TestNormalizedStatement(t *testing.T) { statement: "create view `mydb`.v as select * from t", db: "mydb", typ: tmutils.TableView, - expect: "CREATE VIEW {{.DatabaseName}}.`v` AS SELECT * FROM `t`", + expect: "create view {{.DatabaseName}}.v as select * from t", }, { statement: "create view `mydb`.v as select * from `mydb`.`t`", db: "mydb", typ: tmutils.TableView, - expect: "CREATE VIEW {{.DatabaseName}}.`v` AS SELECT * FROM {{.DatabaseName}}.`t`", + expect: "create view {{.DatabaseName}}.v as select * from {{.DatabaseName}}.t", }, { statement: "create view `mydb`.v as select * from `mydb`.mydb", db: "mydb", typ: tmutils.TableView, - expect: "CREATE VIEW {{.DatabaseName}}.`v` AS SELECT * FROM {{.DatabaseName}}.`mydb`", + expect: "create view {{.DatabaseName}}.v as select * from {{.DatabaseName}}.mydb", }, { statement: "create view `mydb`.v as select * from `mydb`.`mydb`", db: "mydb", typ: tmutils.TableView, - expect: "CREATE VIEW {{.DatabaseName}}.`v` AS SELECT * FROM {{.DatabaseName}}.`mydb`", + expect: "create view {{.DatabaseName}}.v as select * from {{.DatabaseName}}.mydb", }, } ctx := context.Background() diff --git a/go/vt/mysqlctl/tmutils/schema_test.go b/go/vt/mysqlctl/tmutils/schema_test.go index bf0b02ae62f..ebaf4dc871d 100644 --- a/go/vt/mysqlctl/tmutils/schema_test.go +++ b/go/vt/mysqlctl/tmutils/schema_test.go @@ -75,7 +75,7 @@ func TestToSQLStrings(t *testing.T) { }, want: []string{ "CREATE DATABASE `{{.DatabaseName}}`", - "CREATE TABLE `{{.DatabaseName}}`.`table1` (\n\t`id` int PRIMARY KEY\n)", + "create table `{{.DatabaseName}}`.table1 (\n\tid int primary key\n)", "create view view1 as select id from t1", }, }, @@ -102,8 +102,8 @@ func TestToSQLStrings(t *testing.T) { }, want: []string{ "CREATE DATABASE `{{.DatabaseName}}`", - "CREATE TABLE `{{.DatabaseName}}`.`table1` (\n\t`id` int PRIMARY KEY\n)", - "CREATE TABLE `{{.DatabaseName}}`.`table2` (\n\t`id` int PRIMARY KEY\n)", + "create table `{{.DatabaseName}}`.table1 (\n\tid int primary key\n)", + "create table `{{.DatabaseName}}`.table2 (\n\tid int primary key\n)", }, }, { @@ -118,8 +118,8 @@ func TestToSQLStrings(t *testing.T) { }, }, want: []string{ - "CREATE DATABASE `{{.DatabaseName}}`", "CREATE TABLE `{{.DatabaseName}}`.`table1` (\n\t`id` int PRIMARY KEY\n)", - "CREATE TABLE `{{.DatabaseName}}`.`table2` (\n\t`id` int PRIMARY KEY\n)", + "CREATE DATABASE `{{.DatabaseName}}`", "create table `{{.DatabaseName}}`.table1 (\n\tid int primary key\n)", + "create table `{{.DatabaseName}}`.table2 (\n\tid int primary key\n)", "create view view1 as select id from t1", "create view view2 as select id from t2", }, @@ -135,8 +135,8 @@ func TestToSQLStrings(t *testing.T) { }, want: []string{ "CREATE DATABASE `{{.DatabaseName}}`", - "CREATE TABLE `{{.DatabaseName}}`.`table1` (\n\t`id` int PRIMARY KEY\n)", - "CREATE TABLE `{{.DatabaseName}}`.`table3` (\n\t`id` bigint NOT NULL\n) ENGINE InnoDB", + "create table `{{.DatabaseName}}`.table1 (\n\tid int primary key\n)", + "create table `{{.DatabaseName}}`.table3 (\n\tid bigint not null\n) Engine InnoDB", }, }, } diff --git a/go/vt/sqlparser/utils.go b/go/vt/sqlparser/utils.go index 88ba74c11be..6dd6f0d059f 100644 --- a/go/vt/sqlparser/utils.go +++ b/go/vt/sqlparser/utils.go @@ -173,7 +173,7 @@ func ReplaceTableQualifiers(query, olddb, newdb string) (string, error) { // execute a query which slightly differs from the parsed // version: e.g. 'where id=1' becomes 'where id = 1'. if modified { - return CanonicalString(upd), nil + return String(upd), nil } return query, nil } From e784181eecfc39b96d4bc101b5bec60be90b52f9 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Wed, 30 Aug 2023 18:04:12 +0300 Subject: [PATCH 21/27] adapt tests Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/sqlparser/utils_test.go | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/go/vt/sqlparser/utils_test.go b/go/vt/sqlparser/utils_test.go index a47e739d25d..b4f41241e94 100644 --- a/go/vt/sqlparser/utils_test.go +++ b/go/vt/sqlparser/utils_test.go @@ -203,14 +203,14 @@ func TestReplaceTableQualifiers(t *testing.T) { name: "simple select", origdb: origDB, in: "select * from _vt.foo", - out: "SELECT * FROM `foo`", + out: "select * from foo", }, { name: "simple select with new db", in: "select * from _vt.foo", origdb: origDB, newdb: "_vt_test", - out: "SELECT * FROM `_vt_test`.`foo`", + out: "select * from _vt_test.foo", }, { name: "simple select with new db same", @@ -224,51 +224,51 @@ func TestReplaceTableQualifiers(t *testing.T) { in: "select * from _vt.foo", origdb: origDB, newdb: "1_vt-test", - out: "SELECT * FROM `1_vt-test`.`foo`", + out: "select * from `1_vt-test`.foo", }, { name: "complex select", origdb: origDB, in: "select table_name, lastpk from _vt.copy_state where vrepl_id = 1 and id in (select max(id) from _vt.copy_state where vrepl_id = 1 group by vrepl_id, table_name)", - out: "SELECT `table_name`, `lastpk` FROM `copy_state` WHERE `vrepl_id` = 1 AND `id` IN (SELECT max(`id`) FROM `copy_state` WHERE `vrepl_id` = 1 GROUP BY `vrepl_id`, `table_name`)", + out: "select table_name, lastpk from copy_state where vrepl_id = 1 and id in (select max(id) from copy_state where vrepl_id = 1 group by vrepl_id, table_name)", }, { name: "complex mixed exists select", in: "select workflow_name, db_name from _vt.vreplication where id = 1 and exists (select v1 from mydb.foo where fname = 'matt') and not exists (select v2 from _vt.newsidecartable where _vt.newsidecartable.id = _vt.vreplication.workflow_name)", origdb: origDB, newdb: "_vt_import", - out: "SELECT `workflow_name`, `db_name` FROM `_vt_import`.`vreplication` WHERE `id` = 1 AND EXISTS (SELECT `v1` FROM `mydb`.`foo` WHERE `fname` = 'matt') AND NOT EXISTS (SELECT `v2` FROM `_vt_import`.`newsidecartable` WHERE `_vt_import`.`newsidecartable`.`id` = `_vt_import`.`vreplication`.`workflow_name`)", + out: "select workflow_name, db_name from _vt_import.vreplication where id = 1 and exists (select v1 from mydb.foo where fname = 'matt') and not exists (select v2 from _vt_import.newsidecartable where _vt_import.newsidecartable.id = _vt_import.vreplication.workflow_name)", }, { name: "derived table select", in: "select myder.id from (select max(id) as id from _vt.copy_state where vrepl_id = 1 group by vrepl_id, table_name) as myder where id = 1", origdb: origDB, newdb: "__vt-metadata", - out: "SELECT `myder`.`id` FROM (SELECT max(`id`) AS `id` FROM `__vt-metadata`.`copy_state` WHERE `vrepl_id` = 1 GROUP BY `vrepl_id`, `table_name`) AS `myder` WHERE `id` = 1", + out: "select myder.id from (select max(id) as id from `__vt-metadata`.copy_state where vrepl_id = 1 group by vrepl_id, table_name) as myder where id = 1", }, { name: "complex select", origdb: origDB, in: "select t1.col1, t2.col2 from _vt.t1 as t1 join _vt.t2 as t2 on t1.id = t2.id", - out: "SELECT `t1`.`col1`, `t2`.`col2` FROM `t1` AS `t1` JOIN `t2` AS `t2` ON `t1`.`id` = `t2`.`id`", + out: "select t1.col1, t2.col2 from t1 as t1 join t2 as t2 on t1.id = t2.id", }, { name: "simple insert", origdb: origDB, in: "insert into _vt.foo(id) values (1)", - out: "INSERT INTO `foo`(`id`) VALUES (1)", + out: "insert into foo(id) values (1)", }, { name: "simple update", origdb: origDB, in: "update _vt.foo set id = 1", - out: "UPDATE `foo` SET `id` = 1", + out: "update foo set id = 1", }, { name: "simple delete", origdb: origDB, in: "delete from _vt.foo where id = 1", - out: "DELETE FROM `foo` WHERE `id` = 1", + out: "delete from foo where id = 1", }, { name: "simple set", @@ -293,42 +293,42 @@ func TestReplaceTableQualifiers(t *testing.T) { origdb: origDB, newdb: "mydb", in: "create table `_vt`.`t` (id int primary key)", - out: "CREATE TABLE `mydb`.`t` (\n\t`id` int PRIMARY KEY\n)", + out: "create table mydb.t (\n\tid int primary key\n)", }, { name: "empty qualifier create table", origdb: "", newdb: "mydb", in: "CREATE TABLE `t` (id int primary key)", - out: "CREATE TABLE `mydb`.`t` (\n\t`id` int PRIMARY KEY\n)", + out: "create table mydb.t (\n\tid int primary key\n)", }, { name: "create database", origdb: "{{.DatabaseName}}", newdb: "mydb", in: "CREATE DATABASE `{{.DatabaseName}}`", - out: "CREATE DATABASE `mydb`", + out: "create database mydb", }, { name: "create database, comments", origdb: "{{.DatabaseName}}", newdb: "mydb", in: "CREATE DATABASE /*!32312 IF NOT EXISTS*/ `{{.DatabaseName}}` /*!40100 DEFAULT CHARACTER SET utf8mb4 COLLATE utf8mb4_0900_ai_ci */ /*!80016 DEFAULT ENCRYPTION='N' */", - out: "CREATE DATABASE IF NOT EXISTS `mydb` DEFAULT CHARACTER SET UTF8MB4 COLLATE UTF8MB4_0900_AI_CI DEFAULT ENCRYPTION 'N'", + out: "create database if not exists mydb default character set utf8mb4 collate utf8mb4_0900_ai_ci default encryption 'N'", }, { name: "create database, unqualified", origdb: origDB, newdb: "mydb", in: "CREATE DATABASE _vt", - out: "CREATE DATABASE `mydb`", + out: "create database mydb", }, { name: "drop database, unqualified", origdb: origDB, newdb: "mydb", in: "drop database _vt", - out: "DROP DATABASE `mydb`", + out: "drop database mydb", }, } for _, tt := range tests { From 6e0ae754eb7ca242e3debafd7a0542a72d205287 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Thu, 31 Aug 2023 08:11:11 +0300 Subject: [PATCH 22/27] adapting test results Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/test/endtoend/tabletmanager/commands_test.go | 2 +- go/vt/wrangler/testlib/copy_schema_shard_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/go/test/endtoend/tabletmanager/commands_test.go b/go/test/endtoend/tabletmanager/commands_test.go index bc6fba6cf52..eb6856dbf88 100644 --- a/go/test/endtoend/tabletmanager/commands_test.go +++ b/go/test/endtoend/tabletmanager/commands_test.go @@ -39,7 +39,7 @@ var ( getSchemaT1Results8030 = "CREATE TABLE `t1` (\n `id` bigint NOT NULL,\n `value` varchar(16) DEFAULT NULL,\n PRIMARY KEY (`id`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8mb3" getSchemaT1Results80 = "CREATE TABLE `t1` (\n `id` bigint NOT NULL,\n `value` varchar(16) DEFAULT NULL,\n PRIMARY KEY (`id`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8" getSchemaT1Results57 = "CREATE TABLE `t1` (\n `id` bigint(20) NOT NULL,\n `value` varchar(16) DEFAULT NULL,\n PRIMARY KEY (`id`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8" - getSchemaV1Results = fmt.Sprintf("CREATE ALGORITHM = UNDEFINED DEFINER = %s@%s SQL SECURITY DEFINER VIEW {{.DatabaseName}}.`v1` AS SELECT {{.DatabaseName}}.`t1`.`id` AS `id`, {{.DatabaseName}}.`t1`.`value` AS `value` FROM {{.DatabaseName}}.`t1`", username, hostname) + getSchemaV1Results = fmt.Sprintf("create algorithm = UNDEFINED definer = %s@%s sql security DEFINER view {{.DatabaseName}}.`v1` as select {{.DatabaseName}}.t1.id as id, {{.DatabaseName}}.t1.value as value from {{.DatabaseName}}.t1", username, hostname) ) // TabletCommands tests the basic tablet commands diff --git a/go/vt/wrangler/testlib/copy_schema_shard_test.go b/go/vt/wrangler/testlib/copy_schema_shard_test.go index 1919355824f..dfb1745d8a6 100644 --- a/go/vt/wrangler/testlib/copy_schema_shard_test.go +++ b/go/vt/wrangler/testlib/copy_schema_shard_test.go @@ -115,7 +115,7 @@ func copySchema(t *testing.T, useShardAsSource bool) { setSQLMode := fmt.Sprintf("SET @@session.sql_mode='%v'", vreplication.SQLMode) changeToDb := "USE `vt_ks`" - createDb := "CREATE DATABASE `vt_ks` DEFAULT CHARACTER SET UTF8" + createDb := "create database vt_ks default character set utf8" createTable := "CREATE TABLE `vt_ks`.`table1` (\n" + " `id` bigint(20) NOT NULL AUTO_INCREMENT,\n" + " `msg` varchar(64) DEFAULT NULL,\n" + From 48dafe83d5856f363dab635e49d53730dce69f86 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Thu, 31 Aug 2023 08:40:41 +0300 Subject: [PATCH 23/27] adapting test results Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/test/endtoend/tabletmanager/commands_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/go/test/endtoend/tabletmanager/commands_test.go b/go/test/endtoend/tabletmanager/commands_test.go index eb6856dbf88..64598e4a891 100644 --- a/go/test/endtoend/tabletmanager/commands_test.go +++ b/go/test/endtoend/tabletmanager/commands_test.go @@ -39,7 +39,7 @@ var ( getSchemaT1Results8030 = "CREATE TABLE `t1` (\n `id` bigint NOT NULL,\n `value` varchar(16) DEFAULT NULL,\n PRIMARY KEY (`id`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8mb3" getSchemaT1Results80 = "CREATE TABLE `t1` (\n `id` bigint NOT NULL,\n `value` varchar(16) DEFAULT NULL,\n PRIMARY KEY (`id`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8" getSchemaT1Results57 = "CREATE TABLE `t1` (\n `id` bigint(20) NOT NULL,\n `value` varchar(16) DEFAULT NULL,\n PRIMARY KEY (`id`)\n) ENGINE=InnoDB DEFAULT CHARSET=utf8" - getSchemaV1Results = fmt.Sprintf("create algorithm = UNDEFINED definer = %s@%s sql security DEFINER view {{.DatabaseName}}.`v1` as select {{.DatabaseName}}.t1.id as id, {{.DatabaseName}}.t1.value as value from {{.DatabaseName}}.t1", username, hostname) + getSchemaV1Results = fmt.Sprintf("create algorithm = UNDEFINED definer = %s@%s sql security DEFINER view {{.DatabaseName}}.v1 as select {{.DatabaseName}}.t1.id as id, {{.DatabaseName}}.t1.value as value from {{.DatabaseName}}.t1", username, hostname) ) // TabletCommands tests the basic tablet commands From 0be16bf668ba5b5d1054ee185051f4c678bd4657 Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Tue, 19 Sep 2023 18:04:31 -0400 Subject: [PATCH 24/27] Test auto_increment clause removal as well Signed-off-by: Matt Lord --- go/vt/mysqlctl/schema_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/go/vt/mysqlctl/schema_test.go b/go/vt/mysqlctl/schema_test.go index e7b77c7752d..cf4e42376a6 100644 --- a/go/vt/mysqlctl/schema_test.go +++ b/go/vt/mysqlctl/schema_test.go @@ -115,10 +115,10 @@ func TestNormalizedStatement(t *testing.T) { expect string }{ { - statement: "create table mydb.t (id int primary key)", + statement: "create table mydb.t (id int auto_increment primary key) AUTO_INCREMENT=4", db: "mydb", typ: tmutils.TableBaseTable, - expect: "create table mydb.t (id int primary key)", + expect: "create table mydb.t (id int auto_increment primary key)", }, { statement: "create table `mydb`.t (id int primary key)", From 8fbeb88736c88b7fff35a0bd3aac6daa159ca927 Mon Sep 17 00:00:00 2001 From: Matt Lord Date: Tue, 19 Sep 2023 18:25:07 -0400 Subject: [PATCH 25/27] Adjust usage of function added after PR branch Signed-off-by: Matt Lord --- go/vt/vtctl/workflow/server.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/go/vt/vtctl/workflow/server.go b/go/vt/vtctl/workflow/server.go index f42a2dda59c..793e278291b 100644 --- a/go/vt/vtctl/workflow/server.go +++ b/go/vt/vtctl/workflow/server.go @@ -2788,7 +2788,10 @@ func (s *Server) CopySchemaShard(ctx context.Context, sourceTabletAlias *topodat return fmt.Errorf("GetSchema(%v, %v, %v, %v) failed: %v", sourceTabletAlias, tables, excludeTables, includeViews, err) } - createSQLstmts := tmutils.SchemaDefinitionToSQLStrings(sourceSd) + createSQLstmts, err := tmutils.SchemaDefinitionToSQLStrings(sourceSd) + if err != nil { + return fmt.Errorf("SchemaDefinitionToSQLStrings(%v) failed: %v", sourceSd, err) + } destTabletInfo, err := s.ts.GetTablet(ctx, destShardInfo.PrimaryAlias) if err != nil { From b3b38822637ec7788a3dd68035557d8a29802609 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Thu, 21 Sep 2023 08:11:47 +0300 Subject: [PATCH 26/27] updated comment Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/mysqlctl/schema.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/go/vt/mysqlctl/schema.go b/go/vt/mysqlctl/schema.go index d60507b3a23..c2ecb5a50dc 100644 --- a/go/vt/mysqlctl/schema.go +++ b/go/vt/mysqlctl/schema.go @@ -250,7 +250,9 @@ func (mysqld *Mysqld) collectSchema(ctx context.Context, dbName, tableName, tabl return fields, columns, schema, nil } -// normalizedStatement returns a table schema with database names replaced, and auto_increment annotations removed. +// normalizedStatement normalizes a CREATE TABLE or CREATE VIEW statement as follows: +// - For CREATE TABLE, it stripts away any AUTO_INCREMENT=... clause. +// - For CREATE VIEW, it replaces the schema name with given `dbName` func normalizedStatement(ctx context.Context, statementQuery, dbName, tableType string) (string, error) { // Normalize & remove auto_increment because it changes on every insert // FIXME(alainjobart) find a way to share this with From 4b65366bb8f63b18c92c66365fbeaeff1443dd56 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Thu, 21 Sep 2023 08:12:51 +0300 Subject: [PATCH 27/27] updated comment Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/sqlparser/utils.go | 1 - 1 file changed, 1 deletion(-) diff --git a/go/vt/sqlparser/utils.go b/go/vt/sqlparser/utils.go index 6dd6f0d059f..b237831c8a4 100644 --- a/go/vt/sqlparser/utils.go +++ b/go/vt/sqlparser/utils.go @@ -118,7 +118,6 @@ func NormalizeAlphabetically(query string) (normalized string, err error) { // replaces any cases of the provided database name with the // specified replacement name. // Note: both database names provided should be unescaped strings. -// The returned query is formatted canonically (will look different than original query) func ReplaceTableQualifiers(query, olddb, newdb string) (string, error) { if newdb == olddb { // Nothing to do here.