Remove ALGORITHM=COPY from Online DDL in MySQL >= 8.0.32 (#15376)

Signed-off-by: Shlomi Noach <[email protected]>

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
 	}

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:    "",

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)

go/test/endtoend/onlineddl/vrepl_suite/testdata/fk-child-modify-not-null/alter

@@ -0,0 +1 @@
+modify parent_id int not null

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
+);

go/test/endtoend/onlineddl/vrepl_suite/testdata/fk-child-modify-not-null/ddl_strategy

@@ -0,0 +1 @@
+--unsafe-allow-foreign-keys

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)",

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
 	}

go/vt/vttablet/onlineddl/executor_test.go

@@ -28,13 +28,18 @@ 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"
 
 	"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{})
@@ -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 {