diff --git a/go/mysql/capabilities/capability.go b/go/mysql/capabilities/capability.go index d4b66821096..6364f950022 100644 --- a/go/mysql/capabilities/capability.go +++ b/go/mysql/capabilities/capability.go @@ -45,7 +45,8 @@ const ( DynamicRedoLogCapacityFlavorCapability // supported in MySQL 8.0.30 and above: https://dev.mysql.com/doc/relnotes/mysql/8.0/en/news-8-0-30.html DisableRedoLogFlavorCapability // supported in MySQL 8.0.21 and above: https://dev.mysql.com/doc/relnotes/mysql/8.0/en/news-8-0-21.html CheckConstraintsCapability // supported in MySQL 8.0.16 and above: https://dev.mysql.com/doc/relnotes/mysql/8.0/en/news-8-0-16.html - PerformanceSchemaDataLocksTableCapability + PerformanceSchemaDataLocksTableCapability // supported in MySQL 8.0.1 and above: https://dev.mysql.com/doc/relnotes/mysql/8.0/en/news-8-0-1.html + InstantDDLXtrabackupCapability // Supported in 8.0.32 and above, solving a MySQL-vs-Xtrabackup bug starting 8.0.29 ) type CapableOf func(capability FlavorCapability) (bool, error) @@ -109,6 +110,8 @@ func MySQLVersionHasCapability(serverVersion string, capability FlavorCapability return atLeast(8, 0, 29) case DynamicRedoLogCapacityFlavorCapability: return atLeast(8, 0, 30) + case InstantDDLXtrabackupCapability: + return atLeast(8, 0, 32) default: return false, nil } diff --git a/go/mysql/capabilities/capability_test.go b/go/mysql/capabilities/capability_test.go index 339c05f1017..6e96c3487f5 100644 --- a/go/mysql/capabilities/capability_test.go +++ b/go/mysql/capabilities/capability_test.go @@ -229,6 +229,16 @@ func TestMySQLVersionCapableOf(t *testing.T) { capability: PerformanceSchemaDataLocksTableCapability, isCapable: true, }, + { + version: "8.0.29", + capability: InstantDDLXtrabackupCapability, + isCapable: false, + }, + { + version: "8.0.32", + capability: InstantDDLXtrabackupCapability, + isCapable: true, + }, { // What happens if server version is unspecified version: "", diff --git a/go/test/endtoend/onlineddl/vrepl_suite/onlineddl_vrepl_suite_test.go b/go/test/endtoend/onlineddl/vrepl_suite/onlineddl_vrepl_suite_test.go index 56818069e05..6122a71aa44 100644 --- a/go/test/endtoend/onlineddl/vrepl_suite/onlineddl_vrepl_suite_test.go +++ b/go/test/endtoend/onlineddl/vrepl_suite/onlineddl_vrepl_suite_test.go @@ -134,6 +134,27 @@ func TestSchemaChange(t *testing.T) { throttler.EnableLagThrottlerAndWaitForStatus(t, clusterInstance, time.Second) + fkOnlineDDLPossible := false + t.Run("check 'rename_table_preserve_foreign_key' variable", func(t *testing.T) { + // Online DDL is not possible on vanilla MySQL 8.0 for reasons described in https://vitess.io/blog/2021-06-15-online-ddl-why-no-fk/. + // However, Online DDL is made possible in via these changes: + // - https://github.com/planetscale/mysql-server/commit/bb777e3e86387571c044fb4a2beb4f8c60462ced + // - https://github.com/planetscale/mysql-server/commit/c2f1344a6863518d749f2eb01a4c74ca08a5b889 + // as part of https://github.com/planetscale/mysql-server/releases/tag/8.0.34-ps3. + // Said changes introduce a new global/session boolean variable named 'rename_table_preserve_foreign_key'. It defaults 'false'/0 for backwards compatibility. + // When enabled, a `RENAME TABLE` to a FK parent "pins" the children's foreign keys to the table name rather than the table pointer. Which means after the RENAME, + // the children will point to the newly instated table rather than the original, renamed table. + // (Note: this applies to a particular type of RENAME where we swap tables, see the above blog post). + // For FK children, the MySQL changes simply ignore any Vitess-internal table. + // + // In this stress test, we enable Online DDL if the variable 'rename_table_preserve_foreign_key' is present. The Online DDL mechanism will in turn + // query for this variable, and manipulate it, when starting the migration and when cutting over. + rs, err := shards[0].Vttablets[0].VttabletProcess.QueryTablet("show global variables like 'rename_table_preserve_foreign_key'", keyspaceName, false) + require.NoError(t, err) + fkOnlineDDLPossible = len(rs.Rows) > 0 + t.Logf("MySQL support for 'rename_table_preserve_foreign_key': %v", fkOnlineDDLPossible) + }) + files, err := os.ReadDir(testDataPath) require.NoError(t, err) for _, f := range files { @@ -142,7 +163,7 @@ func TestSchemaChange(t *testing.T) { } // this is a test! t.Run(f.Name(), func(t *testing.T) { - testSingle(t, f.Name()) + testSingle(t, f.Name(), fkOnlineDDLPossible) }) } } @@ -161,7 +182,14 @@ func readTestFile(t *testing.T, testName string, fileName string) (content strin // testSingle is the main testing function for a single test in the suite. // It prepares the grounds, creates the test data, runs a migration, expects results/error, cleans up. -func testSingle(t *testing.T, testName string) { +func testSingle(t *testing.T, testName string, fkOnlineDDLPossible bool) { + if _, exists := readTestFile(t, testName, "require_rename_table_preserve_foreign_key"); exists { + if !fkOnlineDDLPossible { + t.Skipf("Skipping test due to require_rename_table_preserve_foreign_key") + return + } + } + if ignoreVersions, exists := readTestFile(t, testName, "ignore_versions"); exists { // ignoreVersions is a regexp re, err := regexp.Compile(ignoreVersions) diff --git a/go/test/endtoend/onlineddl/vrepl_suite/testdata/fk-child-modify-not-null/alter b/go/test/endtoend/onlineddl/vrepl_suite/testdata/fk-child-modify-not-null/alter new file mode 100644 index 00000000000..0660453e839 --- /dev/null +++ b/go/test/endtoend/onlineddl/vrepl_suite/testdata/fk-child-modify-not-null/alter @@ -0,0 +1 @@ +modify parent_id int not null diff --git a/go/test/endtoend/onlineddl/vrepl_suite/testdata/fk-child-modify-not-null/create.sql b/go/test/endtoend/onlineddl/vrepl_suite/testdata/fk-child-modify-not-null/create.sql new file mode 100644 index 00000000000..22113932f4f --- /dev/null +++ b/go/test/endtoend/onlineddl/vrepl_suite/testdata/fk-child-modify-not-null/create.sql @@ -0,0 +1,15 @@ +set session foreign_key_checks=0; +drop table if exists onlineddl_test_child; +drop table if exists onlineddl_test; +drop table if exists onlineddl_test_parent; +set session foreign_key_checks=1; +create table onlineddl_test_parent ( + id int auto_increment, + primary key(id) +); +create table onlineddl_test ( + id int auto_increment, + parent_id int null, + primary key(id), + constraint test_fk foreign key (parent_id) references onlineddl_test_parent (id) on delete no action +); diff --git a/go/test/endtoend/onlineddl/vrepl_suite/testdata/fk-child-modify-not-null/ddl_strategy b/go/test/endtoend/onlineddl/vrepl_suite/testdata/fk-child-modify-not-null/ddl_strategy new file mode 100644 index 00000000000..f48a3989618 --- /dev/null +++ b/go/test/endtoend/onlineddl/vrepl_suite/testdata/fk-child-modify-not-null/ddl_strategy @@ -0,0 +1 @@ +--unsafe-allow-foreign-keys \ No newline at end of file diff --git a/go/test/endtoend/onlineddl/vrepl_suite/testdata/fk-child-modify-not-null/require_rename_table_preserve_foreign_key b/go/test/endtoend/onlineddl/vrepl_suite/testdata/fk-child-modify-not-null/require_rename_table_preserve_foreign_key new file mode 100644 index 00000000000..e69de29bb2d diff --git a/go/vt/schemadiff/schema_test.go b/go/vt/schemadiff/schema_test.go index 0913b9a1165..a979e521216 100644 --- a/go/vt/schemadiff/schema_test.go +++ b/go/vt/schemadiff/schema_test.go @@ -424,6 +424,18 @@ func TestInvalidSchema(t *testing.T) { schema: "create table t10(id bigint primary key); create table t11 (id int primary key, i varchar(100), key ix(i), constraint f10 foreign key (i) references t10(id) on delete restrict)", expectErr: &ForeignKeyColumnTypeMismatchError{Table: "t11", Constraint: "f10", Column: "i", ReferencedTable: "t10", ReferencedColumn: "id"}, }, + { + schema: "create table t10(id int primary key, pid int null, key (pid)); create table t11 (id int primary key, pid int unsigned, key ix(pid), constraint f10 foreign key (pid) references t10(pid))", + expectErr: &ForeignKeyColumnTypeMismatchError{Table: "t11", Constraint: "f10", Column: "pid", ReferencedTable: "t10", ReferencedColumn: "pid"}, + }, + { + // NULL vs NOT NULL should be fine + schema: "create table t10(id int primary key, pid int null, key (pid)); create table t11 (id int primary key, pid int not null, key ix(pid), constraint f10 foreign key (pid) references t10(pid))", + }, + { + // NOT NULL vs NULL should be fine + schema: "create table t10(id int primary key, pid int not null, key (pid)); create table t11 (id int primary key, pid int null, key ix(pid), constraint f10 foreign key (pid) references t10(pid))", + }, { // InnoDB allows different string length schema: "create table t10(id varchar(50) primary key); create table t11 (id int primary key, i varchar(100), key ix(i), constraint f10 foreign key (i) references t10(id) on delete restrict)", diff --git a/go/vt/vttablet/onlineddl/executor.go b/go/vt/vttablet/onlineddl/executor.go index 0fff5d920eb..19f5c82ca5a 100644 --- a/go/vt/vttablet/onlineddl/executor.go +++ b/go/vt/vttablet/onlineddl/executor.go @@ -1315,7 +1315,12 @@ func (e *Executor) validateAndEditCreateTableStatement(ctx context.Context, onli // 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(ctx context.Context, onlineDDL *schema.OnlineDDL, alterTable *sqlparser.AlterTable, constraintMap map[string]string) (alters []*sqlparser.AlterTable, err error) { +func (e *Executor) validateAndEditAlterTableStatement(ctx context.Context, 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) { @@ -1347,8 +1352,10 @@ func (e *Executor) validateAndEditAlterTableStatement(ctx context.Context, onlin opt := alterTable.AlterOptions[i] switch opt := opt.(type) { case sqlparser.AlgorithmValue: - // we do not pass ALGORITHM. We choose our own ALGORITHM. - continue + if !capableOfInstantDDLXtrabackup { + // we do not pass ALGORITHM. We choose our own ALGORITHM. + continue + } case *sqlparser.AddIndexDefinition: if opt.IndexDefinition.Info.Type == sqlparser.IndexTypeFullText { countAddFullTextStatements++ @@ -1357,7 +1364,10 @@ func (e *Executor) validateAndEditAlterTableStatement(ctx context.Context, onlin // in the same statement extraAlterTable := &sqlparser.AlterTable{ Table: alterTable.Table, - AlterOptions: []sqlparser.AlterOption{opt, copyAlgorithm}, + AlterOptions: []sqlparser.AlterOption{opt}, + } + if !capableOfInstantDDLXtrabackup { + extraAlterTable.AlterOptions = append(extraAlterTable.AlterOptions, copyAlgorithm) } alters = append(alters, extraAlterTable) continue @@ -1367,7 +1377,9 @@ func (e *Executor) validateAndEditAlterTableStatement(ctx context.Context, onlin redactedOptions = append(redactedOptions, opt) } alterTable.AlterOptions = redactedOptions - alterTable.AlterOptions = append(alterTable.AlterOptions, copyAlgorithm) + if !capableOfInstantDDLXtrabackup { + alterTable.AlterOptions = append(alterTable.AlterOptions, copyAlgorithm) + } return alters, nil } @@ -1461,7 +1473,9 @@ func (e *Executor) initVreplicationOriginalMigration(ctx context.Context, online // ALTER TABLE should apply to the vrepl table alterTable.SetTable(alterTable.GetTable().Qualifier.CompliantName(), vreplTableName) // Also, change any constraint names: - alters, err := e.validateAndEditAlterTableStatement(ctx, onlineDDL, alterTable, constraintMap) + + capableOf := mysql.ServerVersionCapableOf(conn.ServerVersion) + alters, err := e.validateAndEditAlterTableStatement(ctx, capableOf, onlineDDL, alterTable, constraintMap) if err != nil { return v, err } diff --git a/go/vt/vttablet/onlineddl/executor_test.go b/go/vt/vttablet/onlineddl/executor_test.go index c6fc0044c91..1279f18e45b 100644 --- a/go/vt/vttablet/onlineddl/executor_test.go +++ b/go/vt/vttablet/onlineddl/executor_test.go @@ -28,6 +28,7 @@ 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" @@ -35,6 +36,10 @@ import ( "vitess.io/vitess/go/vt/sqlparser" ) +var ( + testMySQLVersion = "8.0.34" +) + func TestGetConstraintType(t *testing.T) { { typ := GetConstraintType(&sqlparser.CheckConstraintDefinition{}) @@ -195,72 +200,80 @@ func TestValidateAndEditAlterTableStatement(t *testing.T) { env: tabletenv.NewEnv(vtenv.NewTestEnv(), nil, "TestValidateAndEditAlterTableStatementTest"), } tt := []struct { - alter string - m map[string]string - expect []string + alter string + mySQLVersion string + m map[string]string + expect []string }{ { - alter: "alter table t add column i int", - expect: []string{"alter table t add column i int, algorithm = copy"}, + 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), algorithm = copy"}, + 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), algorithm = copy", "alter table t add fulltext key name2_ft (name2), algorithm = copy"}, + 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, algorithm = copy", "alter table t add fulltext key name1_ft (name1), algorithm = copy", "alter table t add fulltext key name2_ft (name2), algorithm = copy"}, + 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), algorithm = copy"}, + 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), algorithm = copy"}, + 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), algorithm = copy"}, + 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), algorithm = copy"}, + 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, algorithm = copy"}, + 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, algorithm = copy"}, + 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, algorithm = copy"}, + 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, algorithm = copy"}, + 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), algorithm = copy"}, + 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, algorithm = copy"}, + 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) @@ -272,8 +285,12 @@ 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) onlineDDL := &schema.OnlineDDL{UUID: "a5a563da_dc1a_11ec_a416_0a43f95f28a3", Table: "t", Options: "--unsafe-allow-foreign-keys"} - alters, err := e.validateAndEditAlterTableStatement(context.Background(), onlineDDL, alterTable, m) + alters, err := e.validateAndEditAlterTableStatement(context.Background(), capableOf, onlineDDL, alterTable, m) assert.NoError(t, err) var altersStrings []string for _, alter := range alters {