Fixing Mysqld.normalizedSchema() by formally parsing the query

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

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 {
@@ -249,6 +250,29 @@ func (mysqld *Mysqld) collectSchema(ctx context.Context, dbName, tableName, tabl
 	return fields, columns, schema, nil
 }
 
+// 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
+	// vt/tabletserver/table_info.go:162
+	norm := statementQuery
+	norm = autoIncr.ReplaceAllLiteralString(norm, "")
+	if tableType == tmutils.TableView {
+		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, nil
+}
+
 // 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 +287,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)
 }
 
 // ResolveTables returns a list of actual tables+views matching a list
@@ -410,76 +426,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, "{{.DatabaseName}}", "`_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.

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,58 @@ 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 auto_increment primary key) AUTO_INCREMENT=4",
+			db:        "mydb",
+			typ:       tmutils.TableBaseTable,
+			expect:    "create table mydb.t (id int auto_increment 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",
+		},
+		{
+			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, err := normalizedStatement(ctx, tcase.statement, tcase.db, tcase.typ)
+			assert.NoError(t, err)
+			assert.Equal(t, tcase.expect, result)
+		})
+	}
+}

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"
 )
@@ -38,8 +40,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) {
@@ -206,14 +217,15 @@ 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))
 
 	// 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 := sd.DatabaseSchema
+	createDatabaseSQL = UnqualifyDatabaseNamePlaceholder(createDatabaseSQL)
+	createDatabaseSQL = QualifyDatabaseNamePlaceholder(createDatabaseSQL)
 	sqlStrings = append(sqlStrings, createDatabaseSQL)
 
 	for _, td := range sd.TableDefinitions {
@@ -223,17 +235,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 `", "CREATE TABLE `{{.DatabaseName}}`.`", 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

go/vt/mysqlctl/tmutils/schema_test.go

@@ -30,32 +30,32 @@ 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,
 }
 
 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,
 }
 
@@ -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)",
+				"create view view1 as select id from t1",
+			},
 		},
 		{
 			// 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)",
+				"create view view1 as select id from t1",
+				"create view view2 as select id from t2",
 			},
 		},
 		{
@@ -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)
 	}
 }