From 92051921f492f4221819b9568257347aa48936c9 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Thu, 12 Sep 2024 14:54:25 +0300 Subject: [PATCH 1/4] schemadiff/OnlineDDL refactor functionality into schemadiff Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schemadiff/names.go | 47 ++++ go/vt/schemadiff/onlineddl.go | 149 +++++++++++ go/vt/schemadiff/onlineddl_test.go | 307 +++++++++++++++++++++ go/vt/vttablet/onlineddl/executor.go | 190 +------------ go/vt/vttablet/onlineddl/executor_test.go | 311 ---------------------- 5 files changed, 508 insertions(+), 496 deletions(-) diff --git a/go/vt/schemadiff/names.go b/go/vt/schemadiff/names.go index c0878d22eeb..e1d12cc80d4 100644 --- a/go/vt/schemadiff/names.go +++ b/go/vt/schemadiff/names.go @@ -20,6 +20,9 @@ import ( "fmt" "regexp" "strings" + + "vitess.io/vitess/go/textutil" + "vitess.io/vitess/go/vt/sqlparser" ) // constraint name examples: @@ -48,3 +51,47 @@ func ExtractConstraintOriginalName(tableName string, constraintName string) stri return constraintName } + +// newConstraintName generates a new, unique name for a constraint. Our problem is that a MySQL +// constraint's name is unique in the schema (!). And so as we duplicate the original table, we must +// create completely new names for all constraints. +// Moreover, we really want this name to be consistent across all shards. We therefore use a deterministic +// UUIDv5 (SHA) function over the migration UUID, table name, and constraint's _contents_. +// We _also_ include the original constraint name as prefix, as room allows +// for example, if the original constraint name is "check_1", +// we might generate "check_1_cps1okb4uafunfqusi2lp22u3". +// If we then again migrate a table whose constraint name is "check_1_cps1okb4uafunfqusi2lp22u3 " we +// get for example "check_1_19l09s37kbhj4axnzmi10e18k" (hash changes, and we still try to preserve original name) +// +// Furthermore, per bug report https://bugs.mysql.com/bug.php?id=107772, if the user doesn't provide a name for +// their CHECK constraint, then MySQL picks a name in this format _chk_. +// Example: sometable_chk_1 +// Next, when MySQL is asked to RENAME TABLE and sees a constraint with this format, it attempts to rename +// the constraint with the new table's name. This is problematic for Vitess, because we often rename tables to +// very long names, such as _vt_HOLD_394f9e6dfc3d11eca0390a43f95f28a3_20220706091048. +// As we rename the constraint to e.g. `sometable_chk_1_cps1okb4uafunfqusi2lp22u3`, this makes MySQL want to +// call the new constraint something like _vt_HOLD_394f9e6dfc3d11eca0390a43f95f28a3_20220706091048_chk_1_cps1okb4uafunfqusi2lp22u3, +// which exceeds the 64 character limit for table names. Long story short, we also trim down if the constraint seems +// to be auto-generated. +func newConstraintName(tableName string, baseUUID string, constraintDefinition *sqlparser.ConstraintDefinition, hashExists map[string]bool, seed string, oldName string) string { + constraintType := GetConstraintType(constraintDefinition.Details) + + constraintIndicator := constraintIndicatorMap[int(constraintType)] + oldName = ExtractConstraintOriginalName(tableName, oldName) + hash := textutil.UUIDv5Base36(baseUUID, tableName, seed) + for i := 1; hashExists[hash]; i++ { + hash = textutil.UUIDv5Base36(baseUUID, tableName, seed, fmt.Sprintf("%d", i)) + } + hashExists[hash] = true + suffix := "_" + hash + maxAllowedNameLength := maxConstraintNameLength - len(suffix) + newName := oldName + if newName == "" { + newName = constraintIndicator // start with something that looks consistent with MySQL's naming + } + if len(newName) > maxAllowedNameLength { + newName = newName[0:maxAllowedNameLength] + } + newName = newName + suffix + return newName +} diff --git a/go/vt/schemadiff/onlineddl.go b/go/vt/schemadiff/onlineddl.go index 66908e502f5..54bb9810e20 100644 --- a/go/vt/schemadiff/onlineddl.go +++ b/go/vt/schemadiff/onlineddl.go @@ -17,14 +17,51 @@ limitations under the License. package schemadiff import ( + "errors" "fmt" "math" "sort" "strings" + "vitess.io/vitess/go/mysql/capabilities" "vitess.io/vitess/go/vt/sqlparser" ) +var ( + ErrForeignKeyFound = errors.New("Foreign key found") + + copyAlgorithm = sqlparser.AlgorithmValue(sqlparser.CopyStr) +) + +const ( + maxConstraintNameLength = 64 +) + +type ConstraintType int + +const ( + UnknownConstraintType ConstraintType = iota + CheckConstraintType + ForeignKeyConstraintType +) + +var ( + constraintIndicatorMap = map[int]string{ + int(CheckConstraintType): "chk", + int(ForeignKeyConstraintType): "fk", + } +) + +func GetConstraintType(constraintInfo sqlparser.ConstraintInfo) ConstraintType { + if _, ok := constraintInfo.(*sqlparser.CheckConstraintDefinition); ok { + return CheckConstraintType + } + if _, ok := constraintInfo.(*sqlparser.ForeignKeyDefinition); ok { + return ForeignKeyConstraintType + } + return UnknownConstraintType +} + // ColumnChangeExpandsDataRange sees if target column has any value set/range that is impossible in source column. func ColumnChangeExpandsDataRange(source *ColumnDefinitionEntity, target *ColumnDefinitionEntity) (bool, string) { if target.IsNullable() && !source.IsNullable() { @@ -588,3 +625,115 @@ func OnlineDDLMigrationTablesAnalysis( return analysis, nil } + +// ValidateAndEditCreateTableStatement inspects the CreateTable AST and does the following: +// - extra validation (no FKs for now...) +// - generate new and unique names for all constraints (CHECK and FK; yes, why not handle FK names; even as we don't support FKs today, we may in the future) +func ValidateAndEditCreateTableStatement(originalTableName string, baseUUID string, createTable *sqlparser.CreateTable, allowForeignKeys bool) (constraintMap map[string]string, err error) { + constraintMap = map[string]string{} + hashExists := map[string]bool{} + + validateWalk := func(node sqlparser.SQLNode) (kontinue bool, err error) { + switch node := node.(type) { + case *sqlparser.ForeignKeyDefinition: + if !allowForeignKeys { + return false, ErrForeignKeyFound + } + case *sqlparser.ConstraintDefinition: + oldName := node.Name.String() + newName := newConstraintName(originalTableName, baseUUID, node, hashExists, sqlparser.CanonicalString(node.Details), oldName) + node.Name = sqlparser.NewIdentifierCI(newName) + constraintMap[oldName] = newName + } + return true, nil + } + if err := sqlparser.Walk(validateWalk, createTable); err != nil { + return constraintMap, err + } + return constraintMap, nil +} + +// ValidateAndEditAlterTableStatement inspects the AlterTable statement and: +// - modifies any CONSTRAINT name according to given name mapping +// - explode ADD FULLTEXT KEY into multiple statements +func ValidateAndEditAlterTableStatement(originalTableName string, baseUUID string, capableOf capabilities.CapableOf, alterTable *sqlparser.AlterTable, constraintMap map[string]string) (alters []*sqlparser.AlterTable, err error) { + capableOfInstantDDLXtrabackup, err := capableOf(capabilities.InstantDDLXtrabackupCapability) + if err != nil { + return nil, err + } + + hashExists := map[string]bool{} + validateWalk := func(node sqlparser.SQLNode) (kontinue bool, err error) { + switch node := node.(type) { + case *sqlparser.DropKey: + if node.Type == sqlparser.CheckKeyType || node.Type == sqlparser.ForeignKeyType { + // drop a check or a foreign key constraint + mappedName, ok := constraintMap[node.Name.String()] + if !ok { + return false, fmt.Errorf("Found DROP CONSTRAINT: %v, but could not find constraint name in map", sqlparser.CanonicalString(node)) + } + node.Name = sqlparser.NewIdentifierCI(mappedName) + } + case *sqlparser.AddConstraintDefinition: + oldName := node.ConstraintDefinition.Name.String() + newName := newConstraintName(originalTableName, baseUUID, node.ConstraintDefinition, hashExists, sqlparser.CanonicalString(node.ConstraintDefinition.Details), oldName) + node.ConstraintDefinition.Name = sqlparser.NewIdentifierCI(newName) + constraintMap[oldName] = newName + } + return true, nil + } + if err := sqlparser.Walk(validateWalk, alterTable); err != nil { + return alters, err + } + alters = append(alters, alterTable) + // Handle ADD FULLTEXT KEY statements + countAddFullTextStatements := 0 + redactedOptions := make([]sqlparser.AlterOption, 0, len(alterTable.AlterOptions)) + for i := range alterTable.AlterOptions { + opt := alterTable.AlterOptions[i] + switch opt := opt.(type) { + case sqlparser.AlgorithmValue: + if !capableOfInstantDDLXtrabackup { + // we do not pass ALGORITHM. We choose our own ALGORITHM. + continue + } + case *sqlparser.AddIndexDefinition: + if opt.IndexDefinition.Info.Type == sqlparser.IndexTypeFullText { + countAddFullTextStatements++ + if countAddFullTextStatements > 1 { + // We've already got one ADD FULLTEXT KEY. We can't have another + // in the same statement + extraAlterTable := &sqlparser.AlterTable{ + Table: alterTable.Table, + AlterOptions: []sqlparser.AlterOption{opt}, + } + if !capableOfInstantDDLXtrabackup { + extraAlterTable.AlterOptions = append(extraAlterTable.AlterOptions, copyAlgorithm) + } + alters = append(alters, extraAlterTable) + continue + } + } + } + redactedOptions = append(redactedOptions, opt) + } + alterTable.AlterOptions = redactedOptions + if !capableOfInstantDDLXtrabackup { + alterTable.AlterOptions = append(alterTable.AlterOptions, copyAlgorithm) + } + return alters, nil +} + +// AddInstantAlgorithm adds or modifies the AlterTable's ALGORITHM to INSTANT +func AddInstantAlgorithm(alterTable *sqlparser.AlterTable) { + instantOpt := sqlparser.AlgorithmValue("INSTANT") + for i, opt := range alterTable.AlterOptions { + if _, ok := opt.(sqlparser.AlgorithmValue); ok { + // replace an existing algorithm + alterTable.AlterOptions[i] = instantOpt + return + } + } + // append an algorithm + alterTable.AlterOptions = append(alterTable.AlterOptions, instantOpt) +} diff --git a/go/vt/schemadiff/onlineddl_test.go b/go/vt/schemadiff/onlineddl_test.go index bd08bedfe8a..ce93a86f4f7 100644 --- a/go/vt/schemadiff/onlineddl_test.go +++ b/go/vt/schemadiff/onlineddl_test.go @@ -24,9 +24,26 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "vitess.io/vitess/go/mysql" + "vitess.io/vitess/go/vt/schema" "vitess.io/vitess/go/vt/sqlparser" ) +var ( + testMySQLVersion = "8.0.34" +) + +func TestGetConstraintType(t *testing.T) { + { + typ := GetConstraintType(&sqlparser.CheckConstraintDefinition{}) + assert.Equal(t, CheckConstraintType, typ) + } + { + typ := GetConstraintType(&sqlparser.ForeignKeyDefinition{}) + assert.Equal(t, ForeignKeyConstraintType, typ) + } +} + func TestPrioritizedUniqueKeys(t *testing.T) { table := ` create table t ( @@ -958,3 +975,293 @@ func TestRevertible(t *testing.T) { }) } } + +func TestValidateAndEditCreateTableStatement(t *testing.T) { + tt := []struct { + name string + query string + allowForeignKeys bool + expectError string + countConstraints int + expectConstraintMap map[string]string + }{ + { + name: "table with FK, not allowed", + query: ` + create table onlineddl_test ( + id int auto_increment, + i int not null, + parent_id int not null, + primary key(id), + constraint test_ibfk foreign key (parent_id) references onlineddl_test_parent (id) on delete no action + ) + `, + expectError: schema.ErrForeignKeyFound.Error(), + }, + { + name: "table with FK, allowed", + query: ` + create table onlineddl_test ( + id int auto_increment, + i int not null, + parent_id int not null, + primary key(id), + constraint test_ibfk foreign key (parent_id) references onlineddl_test_parent (id) on delete no action + ) + `, + allowForeignKeys: true, + countConstraints: 1, + expectConstraintMap: map[string]string{"test_ibfk": "test_ibfk_2wtivm6zk4lthpz14g9uoyaqk"}, + }, + { + name: "table with default FK name, strip table name", + query: ` + create table onlineddl_test ( + id int auto_increment, + i int not null, + parent_id int not null, + primary key(id), + constraint onlineddl_test_ibfk_1 foreign key (parent_id) references onlineddl_test_parent (id) on delete no action + ) + `, + allowForeignKeys: true, + countConstraints: 1, + // we want 'onlineddl_test_' to be stripped out: + expectConstraintMap: map[string]string{"onlineddl_test_ibfk_1": "ibfk_1_2wtivm6zk4lthpz14g9uoyaqk"}, + }, + { + name: "table with anonymous FK, allowed", + query: ` + create table onlineddl_test ( + id int auto_increment, + i int not null, + parent_id int not null, + primary key(id), + foreign key (parent_id) references onlineddl_test_parent (id) on delete no action + ) + `, + allowForeignKeys: true, + countConstraints: 1, + expectConstraintMap: map[string]string{"": "fk_2wtivm6zk4lthpz14g9uoyaqk"}, + }, + { + name: "table with CHECK constraints", + query: ` + create table onlineddl_test ( + id int auto_increment, + i int not null, + primary key(id), + constraint check_1 CHECK ((i >= 0)), + constraint check_2 CHECK ((i <> 5)), + constraint check_3 CHECK ((i >= 0)), + constraint chk_1111033c1d2d5908bf1f956ba900b192_check_4 CHECK ((i >= 0)) + ) + `, + countConstraints: 4, + expectConstraintMap: map[string]string{ + "check_1": "check_1_7dbssrkwdaxhdunwi5zj53q83", + "check_2": "check_2_ehg3rtk6ejvbxpucimeess30o", + "check_3": "check_3_0se0t8x98mf8v7lqmj2la8j9u", + "chk_1111033c1d2d5908bf1f956ba900b192_check_4": "chk_1111033c1d2d5908bf1f956ba900b192_c_0c2c3bxi9jp4evqrct44wg3xh", + }, + }, + { + name: "table with both FOREIGN and CHECK constraints", + query: ` + create table onlineddl_test ( + id int auto_increment, + i int not null, + primary key(id), + constraint check_1 CHECK ((i >= 0)), + constraint test_ibfk foreign key (parent_id) references onlineddl_test_parent (id) on delete no action, + constraint chk_1111033c1d2d5908bf1f956ba900b192_check_4 CHECK ((i >= 0)) + ) + `, + allowForeignKeys: true, + countConstraints: 3, + expectConstraintMap: map[string]string{ + "check_1": "check_1_7dbssrkwdaxhdunwi5zj53q83", + "chk_1111033c1d2d5908bf1f956ba900b192_check_4": "chk_1111033c1d2d5908bf1f956ba900b192_c_0se0t8x98mf8v7lqmj2la8j9u", + "test_ibfk": "test_ibfk_2wtivm6zk4lthpz14g9uoyaqk", + }, + }, + } + env := NewTestEnv() + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + stmt, err := env.Parser().ParseStrictDDL(tc.query) + require.NoError(t, err) + createTable, ok := stmt.(*sqlparser.CreateTable) + require.True(t, ok) + + table := "onlineddl_test" + baseUUID := "a5a563da_dc1a_11ec_a416_0a43f95f28a3" + constraintMap, err := ValidateAndEditCreateTableStatement(table, baseUUID, createTable, tc.allowForeignKeys) + if tc.expectError != "" { + assert.ErrorContains(t, err, tc.expectError) + return + } + assert.NoError(t, err) + assert.Equal(t, tc.expectConstraintMap, constraintMap) + + uniqueConstraintNames := map[string]bool{} + err = sqlparser.Walk(func(node sqlparser.SQLNode) (kontinue bool, err error) { + switch node := node.(type) { + case *sqlparser.ConstraintDefinition: + uniqueConstraintNames[node.Name.String()] = true + } + return true, nil + }, createTable) + assert.NoError(t, err) + assert.Equal(t, tc.countConstraints, len(uniqueConstraintNames)) + assert.Equalf(t, tc.countConstraints, len(constraintMap), "got contraints: %v", constraintMap) + }) + } +} + +func TestValidateAndEditAlterTableStatement(t *testing.T) { + tt := []struct { + alter string + mySQLVersion string + m map[string]string + expect []string + }{ + { + alter: "alter table t add column i int", + mySQLVersion: "8.0.29", + expect: []string{"alter table t add column i int, algorithm = copy"}, + }, + { + alter: "alter table t add column i int", + mySQLVersion: "8.0.32", + expect: []string{"alter table t add column i int"}, + }, + { + alter: "alter table t add column i int, add fulltext key name1_ft (name1)", + expect: []string{"alter table t add column i int, add fulltext key name1_ft (name1)"}, + }, + { + alter: "alter table t add column i int, add fulltext key name1_ft (name1), add fulltext key name2_ft (name2)", + expect: []string{"alter table t add column i int, add fulltext key name1_ft (name1)", "alter table t add fulltext key name2_ft (name2)"}, + }, + { + alter: "alter table t add fulltext key name0_ft (name0), add column i int, add fulltext key name1_ft (name1), add fulltext key name2_ft (name2)", + expect: []string{"alter table t add fulltext key name0_ft (name0), add column i int", "alter table t add fulltext key name1_ft (name1)", "alter table t add fulltext key name2_ft (name2)"}, + }, + { + alter: "alter table t add constraint check (id != 1)", + expect: []string{"alter table t add constraint chk_aulpn7bjeortljhguy86phdn9 check (id != 1)"}, + }, + { + alter: "alter table t add constraint t_chk_1 check (id != 1)", + expect: []string{"alter table t add constraint chk_1_aulpn7bjeortljhguy86phdn9 check (id != 1)"}, + }, + { + alter: "alter table t add constraint some_check check (id != 1)", + expect: []string{"alter table t add constraint some_check_aulpn7bjeortljhguy86phdn9 check (id != 1)"}, + }, + { + alter: "alter table t add constraint some_check check (id != 1), add constraint another_check check (id != 2)", + expect: []string{"alter table t add constraint some_check_aulpn7bjeortljhguy86phdn9 check (id != 1), add constraint another_check_4fa197273p3w96267pzm3gfi3 check (id != 2)"}, + }, + { + alter: "alter table t add foreign key (parent_id) references onlineddl_test_parent (id) on delete no action", + expect: []string{"alter table t add constraint fk_6fmhzdlya89128u5j3xapq34i foreign key (parent_id) references onlineddl_test_parent (id) on delete no action"}, + }, + { + alter: "alter table t add constraint myfk foreign key (parent_id) references onlineddl_test_parent (id) on delete no action", + expect: []string{"alter table t add constraint myfk_6fmhzdlya89128u5j3xapq34i foreign key (parent_id) references onlineddl_test_parent (id) on delete no action"}, + }, + { + // strip out table name + alter: "alter table t add constraint t_ibfk_1 foreign key (parent_id) references onlineddl_test_parent (id) on delete no action", + expect: []string{"alter table t add constraint ibfk_1_6fmhzdlya89128u5j3xapq34i foreign key (parent_id) references onlineddl_test_parent (id) on delete no action"}, + }, + { + // stript out table name + alter: "alter table t add constraint t_ibfk_1 foreign key (parent_id) references onlineddl_test_parent (id) on delete no action", + expect: []string{"alter table t add constraint ibfk_1_6fmhzdlya89128u5j3xapq34i foreign key (parent_id) references onlineddl_test_parent (id) on delete no action"}, + }, + { + alter: "alter table t add constraint t_ibfk_1 foreign key (parent_id) references onlineddl_test_parent (id) on delete no action, add constraint some_check check (id != 1)", + expect: []string{"alter table t add constraint ibfk_1_6fmhzdlya89128u5j3xapq34i foreign key (parent_id) references onlineddl_test_parent (id) on delete no action, add constraint some_check_aulpn7bjeortljhguy86phdn9 check (id != 1)"}, + }, + { + alter: "alter table t drop foreign key t_ibfk_1", + m: map[string]string{ + "t_ibfk_1": "ibfk_1_aaaaaaaaaaaaaa", + }, + expect: []string{"alter table t drop foreign key ibfk_1_aaaaaaaaaaaaaa"}, + }, + } + + env := NewTestEnv() + for _, tc := range tt { + t.Run(tc.alter, func(t *testing.T) { + stmt, err := env.Parser().ParseStrictDDL(tc.alter) + require.NoError(t, err) + alterTable, ok := stmt.(*sqlparser.AlterTable) + require.True(t, ok) + + m := map[string]string{} + for k, v := range tc.m { + m[k] = v + } + if tc.mySQLVersion == "" { + tc.mySQLVersion = testMySQLVersion + } + capableOf := mysql.ServerVersionCapableOf(tc.mySQLVersion) + onlineDDL := &schema.OnlineDDL{UUID: "a5a563da_dc1a_11ec_a416_0a43f95f28a3", Table: "t"} + alters, err := ValidateAndEditAlterTableStatement(onlineDDL.Table, onlineDDL.UUID, capableOf, alterTable, m) + assert.NoError(t, err) + var altersStrings []string + for _, alter := range alters { + altersStrings = append(altersStrings, sqlparser.String(alter)) + } + assert.Equal(t, tc.expect, altersStrings) + }) + } +} + +func TestAddInstantAlgorithm(t *testing.T) { + tt := []struct { + alter string + expect string + }{ + { + alter: "alter table t add column i2 int not null", + expect: "ALTER TABLE `t` ADD COLUMN `i2` int NOT NULL, ALGORITHM = INSTANT", + }, + { + alter: "alter table t add column i2 int not null, lock=none", + expect: "ALTER TABLE `t` ADD COLUMN `i2` int NOT NULL, LOCK NONE, ALGORITHM = INSTANT", + }, + { + alter: "alter table t add column i2 int not null, algorithm=inplace", + expect: "ALTER TABLE `t` ADD COLUMN `i2` int NOT NULL, ALGORITHM = INSTANT", + }, + { + alter: "alter table t add column i2 int not null, algorithm=inplace, lock=none", + expect: "ALTER TABLE `t` ADD COLUMN `i2` int NOT NULL, ALGORITHM = INSTANT, LOCK NONE", + }, + } + env := NewTestEnv() + for _, tc := range tt { + t.Run(tc.alter, func(t *testing.T) { + stmt, err := env.Parser().ParseStrictDDL(tc.alter) + require.NoError(t, err) + alterTable, ok := stmt.(*sqlparser.AlterTable) + require.True(t, ok) + + AddInstantAlgorithm(alterTable) + alterInstant := sqlparser.CanonicalString(alterTable) + + assert.Equal(t, tc.expect, alterInstant) + + stmt, err = env.Parser().ParseStrictDDL(alterInstant) + require.NoError(t, err) + _, ok = stmt.(*sqlparser.AlterTable) + require.True(t, ok) + }) + } +} diff --git a/go/vt/vttablet/onlineddl/executor.go b/go/vt/vttablet/onlineddl/executor.go index f693dfe135c..416feabada8 100644 --- a/go/vt/vttablet/onlineddl/executor.go +++ b/go/vt/vttablet/onlineddl/executor.go @@ -97,7 +97,6 @@ var ( maxConcurrentOnlineDDLs = 256 migrationNextCheckIntervals = []time.Duration{1 * time.Second, 5 * time.Second, 10 * time.Second, 20 * time.Second} - maxConstraintNameLength = 64 cutoverIntervals = []time.Duration{0, 1 * time.Minute, 5 * time.Minute, 10 * time.Minute, 30 * time.Minute} ) @@ -137,31 +136,6 @@ var ( onlineDDLGrant = fmt.Sprintf("'%s'@'%s'", onlineDDLUser, "%") ) -type ConstraintType int - -const ( - UnknownConstraintType ConstraintType = iota - CheckConstraintType - ForeignKeyConstraintType -) - -var ( - constraintIndicatorMap = map[int]string{ - int(CheckConstraintType): "chk", - int(ForeignKeyConstraintType): "fk", - } -) - -func GetConstraintType(constraintInfo sqlparser.ConstraintInfo) ConstraintType { - if _, ok := constraintInfo.(*sqlparser.CheckConstraintDefinition); ok { - return CheckConstraintType - } - if _, ok := constraintInfo.(*sqlparser.ForeignKeyDefinition); ok { - return ForeignKeyConstraintType - } - return UnknownConstraintType -} - type mysqlVariables struct { host string port int @@ -1304,146 +1278,6 @@ func (e *Executor) initConnectionLockWaitTimeout(ctx context.Context, conn *conn return deferFunc, nil } -// newConstraintName generates a new, unique name for a constraint. Our problem is that a MySQL -// constraint's name is unique in the schema (!). And so as we duplicate the original table, we must -// create completely new names for all constraints. -// Moreover, we really want this name to be consistent across all shards. We therefore use a deterministic -// UUIDv5 (SHA) function over the migration UUID, table name, and constraint's _contents_. -// We _also_ include the original constraint name as prefix, as room allows -// for example, if the original constraint name is "check_1", -// we might generate "check_1_cps1okb4uafunfqusi2lp22u3". -// If we then again migrate a table whose constraint name is "check_1_cps1okb4uafunfqusi2lp22u3 " we -// get for example "check_1_19l09s37kbhj4axnzmi10e18k" (hash changes, and we still try to preserve original name) -// -// Furthermore, per bug report https://bugs.mysql.com/bug.php?id=107772, if the user doesn't provide a name for -// their CHECK constraint, then MySQL picks a name in this format _chk_. -// Example: sometable_chk_1 -// Next, when MySQL is asked to RENAME TABLE and sees a constraint with this format, it attempts to rename -// the constraint with the new table's name. This is problematic for Vitess, because we often rename tables to -// very long names, such as _vt_HOLD_394f9e6dfc3d11eca0390a43f95f28a3_20220706091048. -// As we rename the constraint to e.g. `sometable_chk_1_cps1okb4uafunfqusi2lp22u3`, this makes MySQL want to -// call the new constraint something like _vt_HOLD_394f9e6dfc3d11eca0390a43f95f28a3_20220706091048_chk_1_cps1okb4uafunfqusi2lp22u3, -// which exceeds the 64 character limit for table names. Long story short, we also trim down if the constraint seems -// to be auto-generated. -func (e *Executor) newConstraintName(onlineDDL *schema.OnlineDDL, constraintType ConstraintType, hashExists map[string]bool, seed string, oldName string) string { - constraintIndicator := constraintIndicatorMap[int(constraintType)] - oldName = schemadiff.ExtractConstraintOriginalName(onlineDDL.Table, oldName) - hash := textutil.UUIDv5Base36(onlineDDL.UUID, onlineDDL.Table, seed) - for i := 1; hashExists[hash]; i++ { - hash = textutil.UUIDv5Base36(onlineDDL.UUID, onlineDDL.Table, seed, fmt.Sprintf("%d", i)) - } - hashExists[hash] = true - suffix := "_" + hash - maxAllowedNameLength := maxConstraintNameLength - len(suffix) - newName := oldName - if newName == "" { - newName = constraintIndicator // start with something that looks consistent with MySQL's naming - } - if len(newName) > maxAllowedNameLength { - newName = newName[0:maxAllowedNameLength] - } - newName = newName + suffix - return newName -} - -// validateAndEditCreateTableStatement inspects the CreateTable AST and does the following: -// - extra validation (no FKs for now...) -// - generate new and unique names for all constraints (CHECK and FK; yes, why not handle FK names; even as we don't support FKs today, we may in the future) -func (e *Executor) validateAndEditCreateTableStatement(onlineDDL *schema.OnlineDDL, createTable *sqlparser.CreateTable) (constraintMap map[string]string, err error) { - constraintMap = map[string]string{} - hashExists := map[string]bool{} - - validateWalk := func(node sqlparser.SQLNode) (kontinue bool, err error) { - switch node := node.(type) { - case *sqlparser.ForeignKeyDefinition: - if !onlineDDL.StrategySetting().IsAllowForeignKeysFlag() { - return false, schema.ErrForeignKeyFound - } - case *sqlparser.ConstraintDefinition: - oldName := node.Name.String() - newName := e.newConstraintName(onlineDDL, GetConstraintType(node.Details), hashExists, sqlparser.CanonicalString(node.Details), oldName) - node.Name = sqlparser.NewIdentifierCI(newName) - constraintMap[oldName] = newName - } - return true, nil - } - if err := sqlparser.Walk(validateWalk, createTable); err != nil { - return constraintMap, err - } - return constraintMap, nil -} - -// validateAndEditAlterTableStatement inspects the AlterTable statement and: -// - modifies any CONSTRAINT name according to given name mapping -// - explode ADD FULLTEXT KEY into multiple statements -func (e *Executor) validateAndEditAlterTableStatement(capableOf capabilities.CapableOf, onlineDDL *schema.OnlineDDL, alterTable *sqlparser.AlterTable, constraintMap map[string]string) (alters []*sqlparser.AlterTable, err error) { - capableOfInstantDDLXtrabackup, err := capableOf(capabilities.InstantDDLXtrabackupCapability) - if err != nil { - return nil, err - } - - hashExists := map[string]bool{} - validateWalk := func(node sqlparser.SQLNode) (kontinue bool, err error) { - switch node := node.(type) { - case *sqlparser.DropKey: - if node.Type == sqlparser.CheckKeyType || node.Type == sqlparser.ForeignKeyType { - // drop a check or a foreign key constraint - mappedName, ok := constraintMap[node.Name.String()] - if !ok { - return false, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "Found DROP CONSTRAINT: %v, but could not find constraint name in map", sqlparser.CanonicalString(node)) - } - node.Name = sqlparser.NewIdentifierCI(mappedName) - } - case *sqlparser.AddConstraintDefinition: - oldName := node.ConstraintDefinition.Name.String() - newName := e.newConstraintName(onlineDDL, GetConstraintType(node.ConstraintDefinition.Details), hashExists, sqlparser.CanonicalString(node.ConstraintDefinition.Details), oldName) - node.ConstraintDefinition.Name = sqlparser.NewIdentifierCI(newName) - constraintMap[oldName] = newName - } - return true, nil - } - if err := sqlparser.Walk(validateWalk, alterTable); err != nil { - return alters, err - } - alters = append(alters, alterTable) - // Handle ADD FULLTEXT KEY statements - countAddFullTextStatements := 0 - redactedOptions := make([]sqlparser.AlterOption, 0, len(alterTable.AlterOptions)) - for i := range alterTable.AlterOptions { - opt := alterTable.AlterOptions[i] - switch opt := opt.(type) { - case sqlparser.AlgorithmValue: - if !capableOfInstantDDLXtrabackup { - // we do not pass ALGORITHM. We choose our own ALGORITHM. - continue - } - case *sqlparser.AddIndexDefinition: - if opt.IndexDefinition.Info.Type == sqlparser.IndexTypeFullText { - countAddFullTextStatements++ - if countAddFullTextStatements > 1 { - // We've already got one ADD FULLTEXT KEY. We can't have another - // in the same statement - extraAlterTable := &sqlparser.AlterTable{ - Table: alterTable.Table, - AlterOptions: []sqlparser.AlterOption{opt}, - } - if !capableOfInstantDDLXtrabackup { - extraAlterTable.AlterOptions = append(extraAlterTable.AlterOptions, copyAlgorithm) - } - alters = append(alters, extraAlterTable) - continue - } - } - } - redactedOptions = append(redactedOptions, opt) - } - alterTable.AlterOptions = redactedOptions - if !capableOfInstantDDLXtrabackup { - alterTable.AlterOptions = append(alterTable.AlterOptions, copyAlgorithm) - } - return alters, nil -} - // duplicateCreateTable parses the given `CREATE TABLE` statement, and returns: // - The format CreateTable AST // - A new CreateTable AST, with the table renamed as `newTableName`, and with constraints renamed deterministically @@ -1476,7 +1310,7 @@ func (e *Executor) duplicateCreateTable(ctx context.Context, onlineDDL *schema.O // manipulate CreateTable statement: take care of constraints names which have to be // unique across the schema - constraintMap, err = e.validateAndEditCreateTableStatement(onlineDDL, newCreateTable) + constraintMap, err = schemadiff.ValidateAndEditCreateTableStatement(onlineDDL.Table, onlineDDL.UUID, newCreateTable, onlineDDL.StrategySetting().IsAllowForeignKeysFlag()) if err != nil { return nil, nil, err } @@ -1546,7 +1380,7 @@ func (e *Executor) initVreplicationOriginalMigration(ctx context.Context, online // Also, change any constraint names: capableOf := mysql.ServerVersionCapableOf(conn.ServerVersion) - alters, err := e.validateAndEditAlterTableStatement(capableOf, onlineDDL, alterTable, constraintMap) + alters, err := schemadiff.ValidateAndEditAlterTableStatement(onlineDDL.Table, onlineDDL.UUID, capableOf, alterTable, constraintMap) if err != nil { return v, err } @@ -3014,7 +2848,7 @@ func (e *Executor) analyzeDropDDLActionMigration(ctx context.Context, onlineDDL // Analyze foreign keys: for _, constraint := range createTable.TableSpec.Constraints { - if GetConstraintType(constraint.Details) == ForeignKeyConstraintType { + if schemadiff.GetConstraintType(constraint.Details) == schemadiff.ForeignKeyConstraintType { removedForeignKeyNames = append(removedForeignKeyNames, constraint.Name.String()) } } @@ -3116,7 +2950,7 @@ func (e *Executor) executeCreateDDLActionMigration(ctx context.Context, onlineDD newCreateTable := sqlparser.Clone(originalCreateTable) // Rewrite this CREATE TABLE statement such that CONSTRAINT names are edited, // specifically removing any prefix. - if _, err := e.validateAndEditCreateTableStatement(onlineDDL, newCreateTable); err != nil { + if _, err := schemadiff.ValidateAndEditCreateTableStatement(onlineDDL.Table, onlineDDL.UUID, newCreateTable, onlineDDL.StrategySetting().IsAllowForeignKeysFlag()); err != nil { return failMigration(err) } ddlStmt = newCreateTable @@ -3243,20 +3077,6 @@ func (e *Executor) executeAlterViewOnline(ctx context.Context, onlineDDL *schema return nil } -// addInstantAlgorithm adds or modifies the AlterTable's ALGORITHM to INSTANT -func (e *Executor) addInstantAlgorithm(alterTable *sqlparser.AlterTable) { - instantOpt := sqlparser.AlgorithmValue("INSTANT") - for i, opt := range alterTable.AlterOptions { - if _, ok := opt.(sqlparser.AlgorithmValue); ok { - // replace an existing algorithm - alterTable.AlterOptions[i] = instantOpt - return - } - } - // append an algorithm - alterTable.AlterOptions = append(alterTable.AlterOptions, instantOpt) -} - // executeSpecialAlterDDLActionMigrationIfApplicable sees if the given migration can be executed via special execution path, that isn't a full blown online schema change process. func (e *Executor) executeSpecialAlterDDLActionMigrationIfApplicable(ctx context.Context, onlineDDL *schema.OnlineDDL) (specialMigrationExecuted bool, err error) { // Before we jump on to strategies... Some ALTERs can be optimized without having to run through @@ -3278,7 +3098,7 @@ func (e *Executor) executeSpecialAlterDDLActionMigrationIfApplicable(ctx context switch specialPlan.operation { case instantDDLSpecialOperation: - e.addInstantAlgorithm(specialPlan.alterTable) + schemadiff.AddInstantAlgorithm(specialPlan.alterTable) onlineDDL.SQL = sqlparser.CanonicalString(specialPlan.alterTable) if _, err := e.executeDirectly(ctx, onlineDDL); err != nil { return false, err diff --git a/go/vt/vttablet/onlineddl/executor_test.go b/go/vt/vttablet/onlineddl/executor_test.go index d5e7f635a19..a789ff63cd3 100644 --- a/go/vt/vttablet/onlineddl/executor_test.go +++ b/go/vt/vttablet/onlineddl/executor_test.go @@ -28,7 +28,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "vitess.io/vitess/go/mysql" "vitess.io/vitess/go/vt/vtenv" "vitess.io/vitess/go/vt/vttablet/tabletserver/tabletenv" @@ -36,316 +35,6 @@ import ( "vitess.io/vitess/go/vt/sqlparser" ) -var ( - testMySQLVersion = "8.0.34" -) - -func TestGetConstraintType(t *testing.T) { - { - typ := GetConstraintType(&sqlparser.CheckConstraintDefinition{}) - assert.Equal(t, CheckConstraintType, typ) - } - { - typ := GetConstraintType(&sqlparser.ForeignKeyDefinition{}) - assert.Equal(t, ForeignKeyConstraintType, typ) - } -} - -func TestValidateAndEditCreateTableStatement(t *testing.T) { - e := Executor{ - env: tabletenv.NewEnv(vtenv.NewTestEnv(), nil, "ValidateAndEditCreateTableStatementTest"), - } - tt := []struct { - name string - query string - strategyOptions string - expectError string - countConstraints int - expectConstraintMap map[string]string - }{ - { - name: "table with FK, not allowed", - query: ` - create table onlineddl_test ( - id int auto_increment, - i int not null, - parent_id int not null, - primary key(id), - constraint test_ibfk foreign key (parent_id) references onlineddl_test_parent (id) on delete no action - ) - `, - expectError: schema.ErrForeignKeyFound.Error(), - }, - { - name: "table with FK, allowed", - query: ` - create table onlineddl_test ( - id int auto_increment, - i int not null, - parent_id int not null, - primary key(id), - constraint test_ibfk foreign key (parent_id) references onlineddl_test_parent (id) on delete no action - ) - `, - strategyOptions: "--unsafe-allow-foreign-keys", - countConstraints: 1, - expectConstraintMap: map[string]string{"test_ibfk": "test_ibfk_2wtivm6zk4lthpz14g9uoyaqk"}, - }, - { - name: "table with default FK name, strip table name", - query: ` - create table onlineddl_test ( - id int auto_increment, - i int not null, - parent_id int not null, - primary key(id), - constraint onlineddl_test_ibfk_1 foreign key (parent_id) references onlineddl_test_parent (id) on delete no action - ) - `, - strategyOptions: "--unsafe-allow-foreign-keys", - countConstraints: 1, - // we want 'onlineddl_test_' to be stripped out: - expectConstraintMap: map[string]string{"onlineddl_test_ibfk_1": "ibfk_1_2wtivm6zk4lthpz14g9uoyaqk"}, - }, - { - name: "table with anonymous FK, allowed", - query: ` - create table onlineddl_test ( - id int auto_increment, - i int not null, - parent_id int not null, - primary key(id), - foreign key (parent_id) references onlineddl_test_parent (id) on delete no action - ) - `, - strategyOptions: "--unsafe-allow-foreign-keys", - countConstraints: 1, - expectConstraintMap: map[string]string{"": "fk_2wtivm6zk4lthpz14g9uoyaqk"}, - }, - { - name: "table with CHECK constraints", - query: ` - create table onlineddl_test ( - id int auto_increment, - i int not null, - primary key(id), - constraint check_1 CHECK ((i >= 0)), - constraint check_2 CHECK ((i <> 5)), - constraint check_3 CHECK ((i >= 0)), - constraint chk_1111033c1d2d5908bf1f956ba900b192_check_4 CHECK ((i >= 0)) - ) - `, - countConstraints: 4, - expectConstraintMap: map[string]string{ - "check_1": "check_1_7dbssrkwdaxhdunwi5zj53q83", - "check_2": "check_2_ehg3rtk6ejvbxpucimeess30o", - "check_3": "check_3_0se0t8x98mf8v7lqmj2la8j9u", - "chk_1111033c1d2d5908bf1f956ba900b192_check_4": "chk_1111033c1d2d5908bf1f956ba900b192_c_0c2c3bxi9jp4evqrct44wg3xh", - }, - }, - { - name: "table with both FOREIGN and CHECK constraints", - query: ` - create table onlineddl_test ( - id int auto_increment, - i int not null, - primary key(id), - constraint check_1 CHECK ((i >= 0)), - constraint test_ibfk foreign key (parent_id) references onlineddl_test_parent (id) on delete no action, - constraint chk_1111033c1d2d5908bf1f956ba900b192_check_4 CHECK ((i >= 0)) - ) - `, - strategyOptions: "--unsafe-allow-foreign-keys", - countConstraints: 3, - expectConstraintMap: map[string]string{ - "check_1": "check_1_7dbssrkwdaxhdunwi5zj53q83", - "chk_1111033c1d2d5908bf1f956ba900b192_check_4": "chk_1111033c1d2d5908bf1f956ba900b192_c_0se0t8x98mf8v7lqmj2la8j9u", - "test_ibfk": "test_ibfk_2wtivm6zk4lthpz14g9uoyaqk", - }, - }, - } - for _, tc := range tt { - t.Run(tc.name, func(t *testing.T) { - stmt, err := e.env.Environment().Parser().ParseStrictDDL(tc.query) - require.NoError(t, err) - createTable, ok := stmt.(*sqlparser.CreateTable) - require.True(t, ok) - - onlineDDL := &schema.OnlineDDL{UUID: "a5a563da_dc1a_11ec_a416_0a43f95f28a3", Table: "onlineddl_test", Options: tc.strategyOptions} - constraintMap, err := e.validateAndEditCreateTableStatement(onlineDDL, createTable) - if tc.expectError != "" { - assert.ErrorContains(t, err, tc.expectError) - return - } - assert.NoError(t, err) - assert.Equal(t, tc.expectConstraintMap, constraintMap) - - uniqueConstraintNames := map[string]bool{} - err = sqlparser.Walk(func(node sqlparser.SQLNode) (kontinue bool, err error) { - switch node := node.(type) { - case *sqlparser.ConstraintDefinition: - uniqueConstraintNames[node.Name.String()] = true - } - return true, nil - }, createTable) - assert.NoError(t, err) - assert.Equal(t, tc.countConstraints, len(uniqueConstraintNames)) - assert.Equalf(t, tc.countConstraints, len(constraintMap), "got contraints: %v", constraintMap) - }) - } -} - -func TestValidateAndEditAlterTableStatement(t *testing.T) { - e := Executor{ - env: tabletenv.NewEnv(vtenv.NewTestEnv(), nil, "TestValidateAndEditAlterTableStatementTest"), - } - tt := []struct { - alter string - mySQLVersion string - m map[string]string - expect []string - }{ - { - alter: "alter table t add column i int", - mySQLVersion: "8.0.29", - expect: []string{"alter table t add column i int, algorithm = copy"}, - }, - { - alter: "alter table t add column i int", - mySQLVersion: "8.0.32", - expect: []string{"alter table t add column i int"}, - }, - { - alter: "alter table t add column i int, add fulltext key name1_ft (name1)", - expect: []string{"alter table t add column i int, add fulltext key name1_ft (name1)"}, - }, - { - alter: "alter table t add column i int, add fulltext key name1_ft (name1), add fulltext key name2_ft (name2)", - expect: []string{"alter table t add column i int, add fulltext key name1_ft (name1)", "alter table t add fulltext key name2_ft (name2)"}, - }, - { - alter: "alter table t add fulltext key name0_ft (name0), add column i int, add fulltext key name1_ft (name1), add fulltext key name2_ft (name2)", - expect: []string{"alter table t add fulltext key name0_ft (name0), add column i int", "alter table t add fulltext key name1_ft (name1)", "alter table t add fulltext key name2_ft (name2)"}, - }, - { - alter: "alter table t add constraint check (id != 1)", - expect: []string{"alter table t add constraint chk_aulpn7bjeortljhguy86phdn9 check (id != 1)"}, - }, - { - alter: "alter table t add constraint t_chk_1 check (id != 1)", - expect: []string{"alter table t add constraint chk_1_aulpn7bjeortljhguy86phdn9 check (id != 1)"}, - }, - { - alter: "alter table t add constraint some_check check (id != 1)", - expect: []string{"alter table t add constraint some_check_aulpn7bjeortljhguy86phdn9 check (id != 1)"}, - }, - { - alter: "alter table t add constraint some_check check (id != 1), add constraint another_check check (id != 2)", - expect: []string{"alter table t add constraint some_check_aulpn7bjeortljhguy86phdn9 check (id != 1), add constraint another_check_4fa197273p3w96267pzm3gfi3 check (id != 2)"}, - }, - { - alter: "alter table t add foreign key (parent_id) references onlineddl_test_parent (id) on delete no action", - expect: []string{"alter table t add constraint fk_6fmhzdlya89128u5j3xapq34i foreign key (parent_id) references onlineddl_test_parent (id) on delete no action"}, - }, - { - alter: "alter table t add constraint myfk foreign key (parent_id) references onlineddl_test_parent (id) on delete no action", - expect: []string{"alter table t add constraint myfk_6fmhzdlya89128u5j3xapq34i foreign key (parent_id) references onlineddl_test_parent (id) on delete no action"}, - }, - { - // strip out table name - alter: "alter table t add constraint t_ibfk_1 foreign key (parent_id) references onlineddl_test_parent (id) on delete no action", - expect: []string{"alter table t add constraint ibfk_1_6fmhzdlya89128u5j3xapq34i foreign key (parent_id) references onlineddl_test_parent (id) on delete no action"}, - }, - { - // stript out table name - alter: "alter table t add constraint t_ibfk_1 foreign key (parent_id) references onlineddl_test_parent (id) on delete no action", - expect: []string{"alter table t add constraint ibfk_1_6fmhzdlya89128u5j3xapq34i foreign key (parent_id) references onlineddl_test_parent (id) on delete no action"}, - }, - { - alter: "alter table t add constraint t_ibfk_1 foreign key (parent_id) references onlineddl_test_parent (id) on delete no action, add constraint some_check check (id != 1)", - expect: []string{"alter table t add constraint ibfk_1_6fmhzdlya89128u5j3xapq34i foreign key (parent_id) references onlineddl_test_parent (id) on delete no action, add constraint some_check_aulpn7bjeortljhguy86phdn9 check (id != 1)"}, - }, - { - alter: "alter table t drop foreign key t_ibfk_1", - m: map[string]string{ - "t_ibfk_1": "ibfk_1_aaaaaaaaaaaaaa", - }, - expect: []string{"alter table t drop foreign key ibfk_1_aaaaaaaaaaaaaa"}, - }, - } - - for _, tc := range tt { - t.Run(tc.alter, func(t *testing.T) { - stmt, err := e.env.Environment().Parser().ParseStrictDDL(tc.alter) - require.NoError(t, err) - alterTable, ok := stmt.(*sqlparser.AlterTable) - require.True(t, ok) - - m := map[string]string{} - for k, v := range tc.m { - m[k] = v - } - if tc.mySQLVersion == "" { - tc.mySQLVersion = testMySQLVersion - } - capableOf := mysql.ServerVersionCapableOf(tc.mySQLVersion) - onlineDDL := &schema.OnlineDDL{UUID: "a5a563da_dc1a_11ec_a416_0a43f95f28a3", Table: "t", Options: "--unsafe-allow-foreign-keys"} - alters, err := e.validateAndEditAlterTableStatement(capableOf, onlineDDL, alterTable, m) - assert.NoError(t, err) - var altersStrings []string - for _, alter := range alters { - altersStrings = append(altersStrings, sqlparser.String(alter)) - } - assert.Equal(t, tc.expect, altersStrings) - }) - } -} - -func TestAddInstantAlgorithm(t *testing.T) { - e := Executor{ - env: tabletenv.NewEnv(vtenv.NewTestEnv(), nil, "AddInstantAlgorithmTest"), - } - tt := []struct { - alter string - expect string - }{ - { - alter: "alter table t add column i2 int not null", - expect: "ALTER TABLE `t` ADD COLUMN `i2` int NOT NULL, ALGORITHM = INSTANT", - }, - { - alter: "alter table t add column i2 int not null, lock=none", - expect: "ALTER TABLE `t` ADD COLUMN `i2` int NOT NULL, LOCK NONE, ALGORITHM = INSTANT", - }, - { - alter: "alter table t add column i2 int not null, algorithm=inplace", - expect: "ALTER TABLE `t` ADD COLUMN `i2` int NOT NULL, ALGORITHM = INSTANT", - }, - { - alter: "alter table t add column i2 int not null, algorithm=inplace, lock=none", - expect: "ALTER TABLE `t` ADD COLUMN `i2` int NOT NULL, ALGORITHM = INSTANT, LOCK NONE", - }, - } - for _, tc := range tt { - t.Run(tc.alter, func(t *testing.T) { - stmt, err := e.env.Environment().Parser().ParseStrictDDL(tc.alter) - require.NoError(t, err) - alterTable, ok := stmt.(*sqlparser.AlterTable) - require.True(t, ok) - - e.addInstantAlgorithm(alterTable) - alterInstant := sqlparser.CanonicalString(alterTable) - - assert.Equal(t, tc.expect, alterInstant) - - stmt, err = e.env.Environment().Parser().ParseStrictDDL(alterInstant) - require.NoError(t, err) - _, ok = stmt.(*sqlparser.AlterTable) - require.True(t, ok) - }) - } -} - func TestDuplicateCreateTable(t *testing.T) { e := Executor{ env: tabletenv.NewEnv(vtenv.NewTestEnv(), nil, "DuplicateCreateTableTest"), From 35361348137a9a80b116b89797e2eb0691ef376a Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Thu, 12 Sep 2024 15:04:00 +0300 Subject: [PATCH 2/4] refactor DuplicateCreateTable into schemadiff Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schemadiff/onlineddl.go | 39 +++++++++++++ go/vt/schemadiff/onlineddl_test.go | 60 +++++++++++++++++++ go/vt/vttablet/onlineddl/executor.go | 41 +------------ go/vt/vttablet/onlineddl/executor_test.go | 70 ----------------------- 4 files changed, 100 insertions(+), 110 deletions(-) diff --git a/go/vt/schemadiff/onlineddl.go b/go/vt/schemadiff/onlineddl.go index 54bb9810e20..f02ccb1224d 100644 --- a/go/vt/schemadiff/onlineddl.go +++ b/go/vt/schemadiff/onlineddl.go @@ -737,3 +737,42 @@ func AddInstantAlgorithm(alterTable *sqlparser.AlterTable) { // append an algorithm alterTable.AlterOptions = append(alterTable.AlterOptions, instantOpt) } + +// DuplicateCreateTable parses the given `CREATE TABLE` statement, and returns: +// - The format CreateTable AST +// - A new CreateTable AST, with the table renamed as `newTableName`, and with constraints renamed deterministically +// - Map of renamed constraints +func DuplicateCreateTable(originalCreateTable *sqlparser.CreateTable, baseUUID string, newTableName string, allowForeignKeys bool) ( + newCreateTable *sqlparser.CreateTable, + constraintMap map[string]string, + err error, +) { + newCreateTable = sqlparser.Clone(originalCreateTable) + newCreateTable.SetTable(newCreateTable.GetTable().Qualifier.CompliantName(), newTableName) + + // If this table has a self-referencing foreign key constraint, ensure the referenced table gets renamed: + renameSelfFK := func(node sqlparser.SQLNode) (kontinue bool, err error) { + switch node := node.(type) { + case *sqlparser.ConstraintDefinition: + fk, ok := node.Details.(*sqlparser.ForeignKeyDefinition) + if !ok { + return true, nil + } + if referencedTableName := fk.ReferenceDefinition.ReferencedTable.Name.String(); referencedTableName == originalCreateTable.Table.Name.String() { + // This is a self-referencing foreign key + // We need to rename the referenced table as well + fk.ReferenceDefinition.ReferencedTable.Name = sqlparser.NewIdentifierCS(newTableName) + } + } + return true, nil + } + _ = sqlparser.Walk(renameSelfFK, newCreateTable) + + // manipulate CreateTable statement: take care of constraints names which have to be + // unique across the schema + constraintMap, err = ValidateAndEditCreateTableStatement(originalCreateTable.Table.Name.String(), baseUUID, newCreateTable, allowForeignKeys) + if err != nil { + return nil, nil, err + } + return newCreateTable, constraintMap, nil +} diff --git a/go/vt/schemadiff/onlineddl_test.go b/go/vt/schemadiff/onlineddl_test.go index ce93a86f4f7..7cbcfc4aef4 100644 --- a/go/vt/schemadiff/onlineddl_test.go +++ b/go/vt/schemadiff/onlineddl_test.go @@ -1265,3 +1265,63 @@ func TestAddInstantAlgorithm(t *testing.T) { }) } } + +func TestDuplicateCreateTable(t *testing.T) { + baseUUID := "a5a563da_dc1a_11ec_a416_0a43f95f28a3" + allowForeignKeys := true + + tcases := []struct { + sql string + newName string + expectSQL string + expectMapSize int + }{ + { + sql: "create table t (id int primary key)", + newName: "mytable", + expectSQL: "create table mytable (\n\tid int primary key\n)", + }, + { + sql: "create table t (id int primary key, i int, constraint f foreign key (i) references parent (id) on delete cascade)", + newName: "mytable", + expectSQL: "create table mytable (\n\tid int primary key,\n\ti int,\n\tconstraint f_ewl7lthyax2xxocpib3hyyvxx foreign key (i) references parent (id) on delete cascade\n)", + expectMapSize: 1, + }, + { + sql: "create table self (id int primary key, i int, constraint f foreign key (i) references self (id))", + newName: "mytable", + expectSQL: "create table mytable (\n\tid int primary key,\n\ti int,\n\tconstraint f_6tlv90d9gcf4h6roalfnkdhar foreign key (i) references mytable (id)\n)", + expectMapSize: 1, + }, + { + sql: "create table self (id int primary key, i1 int, i2 int, constraint f1 foreign key (i1) references self (id), constraint f1 foreign key (i2) references parent (id))", + newName: "mytable", + expectSQL: `create table mytable ( + id int primary key, + i1 int, + i2 int, + constraint f1_95jpox7sx4w0cv7dpspzmjbxu foreign key (i1) references mytable (id), + constraint f1_1fg002b1cuqoavgti5zp8pu91 foreign key (i2) references parent (id) +)`, + expectMapSize: 1, + }, + } + env := NewTestEnv() + for _, tcase := range tcases { + t.Run(tcase.sql, func(t *testing.T) { + stmt, err := env.Parser().ParseStrictDDL(tcase.sql) + require.NoError(t, err) + originalCreateTable, ok := stmt.(*sqlparser.CreateTable) + require.True(t, ok) + require.NotNil(t, originalCreateTable) + newCreateTable, constraintMap, err := DuplicateCreateTable(originalCreateTable, baseUUID, tcase.newName, allowForeignKeys) + assert.NoError(t, err) + assert.NotNil(t, newCreateTable) + assert.NotNil(t, constraintMap) + + newSQL := sqlparser.String(newCreateTable) + assert.Equal(t, tcase.expectSQL, newSQL) + assert.Equal(t, tcase.expectMapSize, len(constraintMap)) + }) + } +} diff --git a/go/vt/vttablet/onlineddl/executor.go b/go/vt/vttablet/onlineddl/executor.go index 416feabada8..aad8417237e 100644 --- a/go/vt/vttablet/onlineddl/executor.go +++ b/go/vt/vttablet/onlineddl/executor.go @@ -1278,45 +1278,6 @@ func (e *Executor) initConnectionLockWaitTimeout(ctx context.Context, conn *conn return deferFunc, nil } -// duplicateCreateTable parses the given `CREATE TABLE` statement, and returns: -// - The format CreateTable AST -// - A new CreateTable AST, with the table renamed as `newTableName`, and with constraints renamed deterministically -// - Map of renamed constraints -func (e *Executor) duplicateCreateTable(ctx context.Context, onlineDDL *schema.OnlineDDL, originalCreateTable *sqlparser.CreateTable, newTableName string) ( - newCreateTable *sqlparser.CreateTable, - constraintMap map[string]string, - err error, -) { - newCreateTable = sqlparser.Clone(originalCreateTable) - newCreateTable.SetTable(newCreateTable.GetTable().Qualifier.CompliantName(), newTableName) - - // If this table has a self-referencing foreign key constraint, ensure the referenced table gets renamed: - renameSelfFK := func(node sqlparser.SQLNode) (kontinue bool, err error) { - switch node := node.(type) { - case *sqlparser.ConstraintDefinition: - fk, ok := node.Details.(*sqlparser.ForeignKeyDefinition) - if !ok { - return true, nil - } - if referencedTableName := fk.ReferenceDefinition.ReferencedTable.Name.String(); referencedTableName == originalCreateTable.Table.Name.String() { - // This is a self-referencing foreign key - // We need to rename the referenced table as well - fk.ReferenceDefinition.ReferencedTable.Name = sqlparser.NewIdentifierCS(newTableName) - } - } - return true, nil - } - _ = sqlparser.Walk(renameSelfFK, newCreateTable) - - // manipulate CreateTable statement: take care of constraints names which have to be - // unique across the schema - constraintMap, err = schemadiff.ValidateAndEditCreateTableStatement(onlineDDL.Table, onlineDDL.UUID, newCreateTable, onlineDDL.StrategySetting().IsAllowForeignKeysFlag()) - if err != nil { - return nil, nil, err - } - return newCreateTable, constraintMap, nil -} - // createDuplicateTableLike creates the table named by `newTableName` in the likeness of onlineDDL.Table // This function emulates MySQL's `CREATE TABLE LIKE ...` statement. The difference is that this function takes control over the generated CONSTRAINT names, // if any, such that they are deterministic across shards, as well as preserve original names where possible. @@ -1329,7 +1290,7 @@ func (e *Executor) createDuplicateTableLike(ctx context.Context, newTableName st if err != nil { return nil, nil, err } - vreplCreateTable, constraintMap, err := e.duplicateCreateTable(ctx, onlineDDL, originalCreateTable, newTableName) + vreplCreateTable, constraintMap, err := schemadiff.DuplicateCreateTable(originalCreateTable, onlineDDL.UUID, newTableName, onlineDDL.StrategySetting().IsAllowForeignKeysFlag()) if err != nil { return nil, nil, err } diff --git a/go/vt/vttablet/onlineddl/executor_test.go b/go/vt/vttablet/onlineddl/executor_test.go index a789ff63cd3..2533f3a4b48 100644 --- a/go/vt/vttablet/onlineddl/executor_test.go +++ b/go/vt/vttablet/onlineddl/executor_test.go @@ -21,82 +21,12 @@ Functionality of this Executor is tested in go/test/endtoend/onlineddl/... package onlineddl import ( - "context" "testing" "time" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - - "vitess.io/vitess/go/vt/vtenv" - "vitess.io/vitess/go/vt/vttablet/tabletserver/tabletenv" - - "vitess.io/vitess/go/vt/schema" - "vitess.io/vitess/go/vt/sqlparser" ) -func TestDuplicateCreateTable(t *testing.T) { - e := Executor{ - env: tabletenv.NewEnv(vtenv.NewTestEnv(), nil, "DuplicateCreateTableTest"), - } - ctx := context.Background() - onlineDDL := &schema.OnlineDDL{UUID: "a5a563da_dc1a_11ec_a416_0a43f95f28a3", Table: "something", Strategy: "vitess", Options: "--unsafe-allow-foreign-keys"} - - tcases := []struct { - sql string - newName string - expectSQL string - expectMapSize int - }{ - { - sql: "create table t (id int primary key)", - newName: "mytable", - expectSQL: "create table mytable (\n\tid int primary key\n)", - }, - { - sql: "create table t (id int primary key, i int, constraint f foreign key (i) references parent (id) on delete cascade)", - newName: "mytable", - expectSQL: "create table mytable (\n\tid int primary key,\n\ti int,\n\tconstraint f_bjj16562shq086ozik3zf6kjg foreign key (i) references parent (id) on delete cascade\n)", - expectMapSize: 1, - }, - { - sql: "create table self (id int primary key, i int, constraint f foreign key (i) references self (id))", - newName: "mytable", - expectSQL: "create table mytable (\n\tid int primary key,\n\ti int,\n\tconstraint f_8aymb58nzb78l5jhq600veg6y foreign key (i) references mytable (id)\n)", - expectMapSize: 1, - }, - { - sql: "create table self (id int primary key, i1 int, i2 int, constraint f1 foreign key (i1) references self (id), constraint f1 foreign key (i2) references parent (id))", - newName: "mytable", - expectSQL: `create table mytable ( - id int primary key, - i1 int, - i2 int, - constraint f1_1rlsg9yls1t91i35zq5gyeoq7 foreign key (i1) references mytable (id), - constraint f1_59t4lvb1ncti6fxy27drad4jp foreign key (i2) references parent (id) -)`, - expectMapSize: 1, - }, - } - for _, tcase := range tcases { - t.Run(tcase.sql, func(t *testing.T) { - stmt, err := e.env.Environment().Parser().ParseStrictDDL(tcase.sql) - require.NoError(t, err) - originalCreateTable, ok := stmt.(*sqlparser.CreateTable) - require.True(t, ok) - require.NotNil(t, originalCreateTable) - newCreateTable, constraintMap, err := e.duplicateCreateTable(ctx, onlineDDL, originalCreateTable, tcase.newName) - assert.NoError(t, err) - assert.NotNil(t, newCreateTable) - assert.NotNil(t, constraintMap) - - newSQL := sqlparser.String(newCreateTable) - assert.Equal(t, tcase.expectSQL, newSQL) - assert.Equal(t, tcase.expectMapSize, len(constraintMap)) - }) - } -} - func TestShouldCutOverAccordingToBackoff(t *testing.T) { tcases := []struct { name string From 23f2d36bc6f432f58b6bfbbfd11e20ade8813df1 Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Thu, 12 Sep 2024 15:09:59 +0300 Subject: [PATCH 3/4] remove dependency Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schemadiff/onlineddl_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/go/vt/schemadiff/onlineddl_test.go b/go/vt/schemadiff/onlineddl_test.go index 7cbcfc4aef4..de1aff93692 100644 --- a/go/vt/schemadiff/onlineddl_test.go +++ b/go/vt/schemadiff/onlineddl_test.go @@ -25,7 +25,6 @@ import ( "github.com/stretchr/testify/require" "vitess.io/vitess/go/mysql" - "vitess.io/vitess/go/vt/schema" "vitess.io/vitess/go/vt/sqlparser" ) @@ -996,7 +995,7 @@ func TestValidateAndEditCreateTableStatement(t *testing.T) { constraint test_ibfk foreign key (parent_id) references onlineddl_test_parent (id) on delete no action ) `, - expectError: schema.ErrForeignKeyFound.Error(), + expectError: ErrForeignKeyFound.Error(), }, { name: "table with FK, allowed", @@ -1211,8 +1210,9 @@ func TestValidateAndEditAlterTableStatement(t *testing.T) { tc.mySQLVersion = testMySQLVersion } capableOf := mysql.ServerVersionCapableOf(tc.mySQLVersion) - onlineDDL := &schema.OnlineDDL{UUID: "a5a563da_dc1a_11ec_a416_0a43f95f28a3", Table: "t"} - alters, err := ValidateAndEditAlterTableStatement(onlineDDL.Table, onlineDDL.UUID, capableOf, alterTable, m) + baseUUID := "a5a563da_dc1a_11ec_a416_0a43f95f28a3" + tableName := "t" + alters, err := ValidateAndEditAlterTableStatement(tableName, baseUUID, capableOf, alterTable, m) assert.NoError(t, err) var altersStrings []string for _, alter := range alters { From 05d751c9d3e4b628173fe04482a228da6c863cdd Mon Sep 17 00:00:00 2001 From: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Date: Thu, 12 Sep 2024 15:16:39 +0300 Subject: [PATCH 4/4] use capableOf Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --- go/vt/schemadiff/onlineddl_test.go | 54 +++++++++++++++++++----------- 1 file changed, 34 insertions(+), 20 deletions(-) diff --git a/go/vt/schemadiff/onlineddl_test.go b/go/vt/schemadiff/onlineddl_test.go index de1aff93692..834490dca1b 100644 --- a/go/vt/schemadiff/onlineddl_test.go +++ b/go/vt/schemadiff/onlineddl_test.go @@ -24,14 +24,10 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "vitess.io/vitess/go/mysql" + "vitess.io/vitess/go/mysql/capabilities" "vitess.io/vitess/go/vt/sqlparser" ) -var ( - testMySQLVersion = "8.0.34" -) - func TestGetConstraintType(t *testing.T) { { typ := GetConstraintType(&sqlparser.CheckConstraintDefinition{}) @@ -1119,21 +1115,40 @@ func TestValidateAndEditCreateTableStatement(t *testing.T) { } func TestValidateAndEditAlterTableStatement(t *testing.T) { + capableOf := func(capability capabilities.FlavorCapability) (bool, error) { + switch capability { + case + capabilities.InstantDDLXtrabackupCapability, + capabilities.InstantDDLFlavorCapability, + capabilities.InstantAddLastColumnFlavorCapability, + capabilities.InstantAddDropVirtualColumnFlavorCapability, + capabilities.InstantAddDropColumnFlavorCapability, + capabilities.InstantChangeColumnDefaultFlavorCapability, + capabilities.InstantChangeColumnVisibilityCapability, + capabilities.InstantExpandEnumCapability: + return true, nil + } + return false, nil + } + incapableOf := func(capability capabilities.FlavorCapability) (bool, error) { + return false, nil + } + tt := []struct { - alter string - mySQLVersion string - m map[string]string - expect []string + alter string + capableOf capabilities.CapableOf + m map[string]string + expect []string }{ { - alter: "alter table t add column i int", - mySQLVersion: "8.0.29", - expect: []string{"alter table t add column i int, algorithm = copy"}, + alter: "alter table t add column i int", + capableOf: incapableOf, + expect: []string{"alter table t add column i int, algorithm = copy"}, }, { - alter: "alter table t add column i int", - mySQLVersion: "8.0.32", - expect: []string{"alter table t add column i int"}, + alter: "alter table t add column i int", + capableOf: capableOf, + expect: []string{"alter table t add column i int"}, }, { alter: "alter table t add column i int, add fulltext key name1_ft (name1)", @@ -1197,6 +1212,9 @@ func TestValidateAndEditAlterTableStatement(t *testing.T) { env := NewTestEnv() for _, tc := range tt { t.Run(tc.alter, func(t *testing.T) { + if tc.capableOf == nil { + tc.capableOf = capableOf + } stmt, err := env.Parser().ParseStrictDDL(tc.alter) require.NoError(t, err) alterTable, ok := stmt.(*sqlparser.AlterTable) @@ -1206,13 +1224,9 @@ func TestValidateAndEditAlterTableStatement(t *testing.T) { for k, v := range tc.m { m[k] = v } - if tc.mySQLVersion == "" { - tc.mySQLVersion = testMySQLVersion - } - capableOf := mysql.ServerVersionCapableOf(tc.mySQLVersion) baseUUID := "a5a563da_dc1a_11ec_a416_0a43f95f28a3" tableName := "t" - alters, err := ValidateAndEditAlterTableStatement(tableName, baseUUID, capableOf, alterTable, m) + alters, err := ValidateAndEditAlterTableStatement(tableName, baseUUID, tc.capableOf, alterTable, m) assert.NoError(t, err) var altersStrings []string for _, alter := range alters {