Remove ALGORITHM=COPY from Online DDL in MySQL >= 8.0.32

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/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 {