Skip to content

Commit

Permalink
ddl: improve compatibility for ALTER TABLE algorithms (#19270)
Browse files Browse the repository at this point in the history 
* ddl: always try a better algorithm

* ddl: adapt the test

* ddl: fix TestAlterAlgorithm

* ddl: better test fix

* ddl: typo

* ddl: update comment

Co-authored-by: bb7133 <[email protected]>

* executor: typo

Co-authored-by: djshow832 <[email protected]>

Co-authored-by: bb7133 <[email protected]>
Co-authored-by: djshow832 <[email protected]>
Co-authored-by: ti-srebot <[email protected]>
  • Loading branch information
@xhebox @bb7133
@djshow832 @ti-srebot
4 people authored Aug 21, 2020
1 parent 69d4517 commit f851898
Show file tree
Hide file tree
Showing 5 changed files with 73 additions and 55 deletions.
27 changes: 14 additions & 13 deletions ddl/db_integration_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1350,14 +1350,14 @@ func (s *testIntegrationSuite3) TestChangeColumnPosition(c *C) {
tk.MustExec("alter table position4 add index t(b)")
tk.MustExec("alter table position4 change column b c int first")
createSQL := tk.MustQuery("show create table position4").Rows()[0][1]
exceptedSQL := []string{
expectedSQL := []string{
"CREATE TABLE `position4` (",
" `c` int(11) DEFAULT NULL,",
" `a` int(11) DEFAULT NULL,",
" KEY `t` (`c`)",
") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin",
}
c.Assert(createSQL, Equals, strings.Join(exceptedSQL, "\n"))
c.Assert(createSQL, Equals, strings.Join(expectedSQL, "\n"))
}

func (s *testIntegrationSuite2) TestAddIndexAfterAddColumn(c *C) {
Expand Down Expand Up @@ -1575,51 +1575,52 @@ func (s *testIntegrationSuite3) TestAlterAlgorithm(c *C) {
PARTITION p2 VALUES LESS THAN (16),
PARTITION p3 VALUES LESS THAN (21)
)`)
s.assertAlterErrorExec(tk, c, "alter table t modify column a bigint, ALGORITHM=INPLACE;")
s.assertAlterWarnExec(tk, c, "alter table t modify column a bigint, ALGORITHM=INPLACE;")
tk.MustExec("alter table t modify column a bigint, ALGORITHM=INPLACE, ALGORITHM=INSTANT;")
tk.MustExec("alter table t modify column a bigint, ALGORITHM=DEFAULT;")

// Test add/drop index
s.assertAlterErrorExec(tk, c, "alter table t add index idx_b(b), ALGORITHM=INSTANT")
s.assertAlterWarnExec(tk, c, "alter table t add index idx_b1(b), ALGORITHM=COPY")
tk.MustExec("alter table t add index idx_b2(b), ALGORITHM=INPLACE")
s.assertAlterErrorExec(tk, c, "alter table t drop index idx_b, ALGORITHM=INPLACE")
tk.MustExec("alter table t add index idx_b3(b), ALGORITHM=DEFAULT")
s.assertAlterWarnExec(tk, c, "alter table t drop index idx_b3, ALGORITHM=INPLACE")
s.assertAlterWarnExec(tk, c, "alter table t drop index idx_b1, ALGORITHM=COPY")
tk.MustExec("alter table t drop index idx_b2, ALGORITHM=INSTANT")

// Test rename
s.assertAlterWarnExec(tk, c, "alter table t rename to t1, ALGORITHM=COPY")
s.assertAlterErrorExec(tk, c, "alter table t1 rename to t, ALGORITHM=INPLACE")
tk.MustExec("alter table t1 rename to t, ALGORITHM=INSTANT")
s.assertAlterWarnExec(tk, c, "alter table t1 rename to t2, ALGORITHM=INPLACE")
tk.MustExec("alter table t2 rename to t, ALGORITHM=INSTANT")
tk.MustExec("alter table t rename to t1, ALGORITHM=DEFAULT")
tk.MustExec("alter table t1 rename to t")

// Test rename index
s.assertAlterWarnExec(tk, c, "alter table t rename index idx_c to idx_c1, ALGORITHM=COPY")
s.assertAlterErrorExec(tk, c, "alter table t rename index idx_c1 to idx_c, ALGORITHM=INPLACE")
tk.MustExec("alter table t rename index idx_c1 to idx_c, ALGORITHM=INSTANT")
s.assertAlterWarnExec(tk, c, "alter table t rename index idx_c1 to idx_c2, ALGORITHM=INPLACE")
tk.MustExec("alter table t rename index idx_c2 to idx_c, ALGORITHM=INSTANT")
tk.MustExec("alter table t rename index idx_c to idx_c1, ALGORITHM=DEFAULT")

// partition.
s.assertAlterWarnExec(tk, c, "alter table t ALGORITHM=COPY, truncate partition p1")
s.assertAlterErrorExec(tk, c, "alter table t ALGORITHM=INPLACE, truncate partition p2")
s.assertAlterWarnExec(tk, c, "alter table t ALGORITHM=INPLACE, truncate partition p2")
tk.MustExec("alter table t ALGORITHM=INSTANT, truncate partition p3")

s.assertAlterWarnExec(tk, c, "alter table t add partition (partition p4 values less than (2002)), ALGORITHM=COPY")
s.assertAlterErrorExec(tk, c, "alter table t add partition (partition p5 values less than (3002)), ALGORITHM=INPLACE")
s.assertAlterWarnExec(tk, c, "alter table t add partition (partition p5 values less than (3002)), ALGORITHM=INPLACE")
tk.MustExec("alter table t add partition (partition p6 values less than (4002)), ALGORITHM=INSTANT")

s.assertAlterWarnExec(tk, c, "alter table t ALGORITHM=COPY, drop partition p4")
s.assertAlterErrorExec(tk, c, "alter table t ALGORITHM=INPLACE, drop partition p5")
s.assertAlterWarnExec(tk, c, "alter table t ALGORITHM=INPLACE, drop partition p5")
tk.MustExec("alter table t ALGORITHM=INSTANT, drop partition p6")

// Table options
s.assertAlterWarnExec(tk, c, "alter table t comment = 'test', ALGORITHM=COPY")
s.assertAlterErrorExec(tk, c, "alter table t comment = 'test', ALGORITHM=INPLACE")
s.assertAlterWarnExec(tk, c, "alter table t comment = 'test', ALGORITHM=INPLACE")
tk.MustExec("alter table t comment = 'test', ALGORITHM=INSTANT")

s.assertAlterWarnExec(tk, c, "alter table t default charset = utf8mb4, ALGORITHM=COPY")
s.assertAlterErrorExec(tk, c, "alter table t default charset = utf8mb4, ALGORITHM=INPLACE")
s.assertAlterWarnExec(tk, c, "alter table t default charset = utf8mb4, ALGORITHM=INPLACE")
tk.MustExec("alter table t default charset = utf8mb4, ALGORITHM=INSTANT")
}

Expand Down
18 changes: 14 additions & 4 deletions ddl/ddl_algorithm.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
// because we never block the DML but costs some time to backfill the index data)
// See https://dev.mysql.com/doc/refman/8.0/en/alter-table.html#alter-table-performance.
type AlterAlgorithm struct {
// supported MUST store algorithms in the order 'INSTANT, INPLACE, COPY'
supported []ast.AlgorithmType
// If the alter algorithm is not given, the defAlgorithm will be used.
defAlgorithm ast.AlgorithmType
Expand All @@ -47,18 +48,27 @@ func getProperAlgorithm(specify ast.AlgorithmType, algorithm *AlterAlgorithm) (a
return algorithm.defAlgorithm, nil
}

r := ast.AlgorithmTypeDefault

for _, a := range algorithm.supported {
if specify == a {
return specify, nil
if specify <= a {
r = a
break
}
}

return algorithm.defAlgorithm, ErrAlterOperationNotSupported.GenWithStackByArgs(fmt.Sprintf("ALGORITHM=%s", specify), fmt.Sprintf("Cannot alter table by %s", specify), fmt.Sprintf("ALGORITHM=%s", algorithm.defAlgorithm))
var err error
if specify != r {
err = ErrAlterOperationNotSupported.GenWithStackByArgs(fmt.Sprintf("ALGORITHM=%s", specify), fmt.Sprintf("Cannot alter table by %s", specify), fmt.Sprintf("ALGORITHM=%s", algorithm.defAlgorithm))
}
return r, err
}

// ResolveAlterAlgorithm resolves the algorithm of the alterSpec.
// If specify algorithm is not supported by the alter action, errAlterOperationNotSupported will be returned.
// If specify is the ast.AlterAlgorithmDefault, then the default algorithm of the alter action will be returned.
// If specify algorithm is not supported by the alter action, it will try to find a better algorithm in the order `INSTANT > INPLACE > COPY`, errAlterOperationNotSupported will be returned.
// E.g. INSTANT may be returned if specify=INPLACE
// If failed to choose any valid algorithm, AlgorithmTypeDefault and errAlterOperationNotSupported will be returned
func ResolveAlterAlgorithm(alterSpec *ast.AlterTableSpec, specify ast.AlgorithmType) (ast.AlgorithmType, error) {
switch alterSpec.Tp {
// For now, TiDB only support inplace algorithm and instant algorithm.
Expand Down
73 changes: 40 additions & 33 deletions ddl/ddl_algorithm_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,43 +31,48 @@ type testDDLAlgorithmSuite struct{}
type testCase struct {
alterSpec ast.AlterTableSpec
supportedAlgorithm []ast.AlgorithmType
defAlgorithm ast.AlgorithmType
expectedAlgorithm []ast.AlgorithmType
}

func (s *testDDLAlgorithmSuite) TestFindAlterAlgorithm(c *C) {
instantAlgorithm := []ast.AlgorithmType{ast.AlgorithmTypeInstant}
inplaceAlgorithm := []ast.AlgorithmType{ast.AlgorithmTypeInplace}
supportedInstantAlgorithms := []ast.AlgorithmType{ast.AlgorithmTypeDefault, ast.AlgorithmTypeCopy, ast.AlgorithmTypeInplace, ast.AlgorithmTypeInstant}
expectedInstantAlgorithms := []ast.AlgorithmType{ast.AlgorithmTypeInstant, ast.AlgorithmTypeInstant, ast.AlgorithmTypeInstant, ast.AlgorithmTypeInstant}

testCases := []testCase{
{ast.AlterTableSpec{Tp: ast.AlterTableAddConstraint}, inplaceAlgorithm, ast.AlgorithmTypeInplace},
{ast.AlterTableSpec{Tp: ast.AlterTableAddColumns}, instantAlgorithm, ast.AlgorithmTypeInstant},
{ast.AlterTableSpec{Tp: ast.AlterTableDropColumn}, instantAlgorithm, ast.AlgorithmTypeInstant},
{ast.AlterTableSpec{Tp: ast.AlterTableDropPrimaryKey}, instantAlgorithm, ast.AlgorithmTypeInstant},
{ast.AlterTableSpec{Tp: ast.AlterTableDropIndex}, instantAlgorithm, ast.AlgorithmTypeInstant},
{ast.AlterTableSpec{Tp: ast.AlterTableDropForeignKey}, instantAlgorithm, ast.AlgorithmTypeInstant},
{ast.AlterTableSpec{Tp: ast.AlterTableRenameTable}, instantAlgorithm, ast.AlgorithmTypeInstant},
{ast.AlterTableSpec{Tp: ast.AlterTableRenameIndex}, instantAlgorithm, ast.AlgorithmTypeInstant},
{
ast.AlterTableSpec{Tp: ast.AlterTableAddConstraint},
[]ast.AlgorithmType{ast.AlgorithmTypeDefault, ast.AlgorithmTypeCopy, ast.AlgorithmTypeInplace},
[]ast.AlgorithmType{ast.AlgorithmTypeInplace, ast.AlgorithmTypeInplace, ast.AlgorithmTypeInplace},
},

{ast.AlterTableSpec{Tp: ast.AlterTableAddColumns}, supportedInstantAlgorithms, expectedInstantAlgorithms},
{ast.AlterTableSpec{Tp: ast.AlterTableDropColumn}, supportedInstantAlgorithms, expectedInstantAlgorithms},
{ast.AlterTableSpec{Tp: ast.AlterTableDropPrimaryKey}, supportedInstantAlgorithms, expectedInstantAlgorithms},
{ast.AlterTableSpec{Tp: ast.AlterTableDropIndex}, supportedInstantAlgorithms, expectedInstantAlgorithms},
{ast.AlterTableSpec{Tp: ast.AlterTableDropForeignKey}, supportedInstantAlgorithms, expectedInstantAlgorithms},
{ast.AlterTableSpec{Tp: ast.AlterTableRenameTable}, supportedInstantAlgorithms, expectedInstantAlgorithms},
{ast.AlterTableSpec{Tp: ast.AlterTableRenameIndex}, supportedInstantAlgorithms, expectedInstantAlgorithms},

// Alter table options.
{ast.AlterTableSpec{Tp: ast.AlterTableOption, Options: []*ast.TableOption{{Tp: ast.TableOptionShardRowID}}}, instantAlgorithm, ast.AlgorithmTypeInstant},
{ast.AlterTableSpec{Tp: ast.AlterTableOption, Options: []*ast.TableOption{{Tp: ast.TableOptionAutoIncrement}}}, instantAlgorithm, ast.AlgorithmTypeInstant},
{ast.AlterTableSpec{Tp: ast.AlterTableOption, Options: []*ast.TableOption{{Tp: ast.TableOptionComment}}}, instantAlgorithm, ast.AlgorithmTypeInstant},
{ast.AlterTableSpec{Tp: ast.AlterTableOption, Options: []*ast.TableOption{{Tp: ast.TableOptionCharset}}}, instantAlgorithm, ast.AlgorithmTypeInstant},
{ast.AlterTableSpec{Tp: ast.AlterTableOption, Options: []*ast.TableOption{{Tp: ast.TableOptionCollate}}}, instantAlgorithm, ast.AlgorithmTypeInstant},
{ast.AlterTableSpec{Tp: ast.AlterTableOption, Options: []*ast.TableOption{{Tp: ast.TableOptionShardRowID}}}, supportedInstantAlgorithms, expectedInstantAlgorithms},
{ast.AlterTableSpec{Tp: ast.AlterTableOption, Options: []*ast.TableOption{{Tp: ast.TableOptionAutoIncrement}}}, supportedInstantAlgorithms, expectedInstantAlgorithms},
{ast.AlterTableSpec{Tp: ast.AlterTableOption, Options: []*ast.TableOption{{Tp: ast.TableOptionComment}}}, supportedInstantAlgorithms, expectedInstantAlgorithms},
{ast.AlterTableSpec{Tp: ast.AlterTableOption, Options: []*ast.TableOption{{Tp: ast.TableOptionCharset}}}, supportedInstantAlgorithms, expectedInstantAlgorithms},
{ast.AlterTableSpec{Tp: ast.AlterTableOption, Options: []*ast.TableOption{{Tp: ast.TableOptionCollate}}}, supportedInstantAlgorithms, expectedInstantAlgorithms},

// TODO: after we support migrate the data of partitions, change below cases.
{ast.AlterTableSpec{Tp: ast.AlterTableCoalescePartitions}, instantAlgorithm, ast.AlgorithmTypeInstant},
{ast.AlterTableSpec{Tp: ast.AlterTableAddPartitions}, instantAlgorithm, ast.AlgorithmTypeInstant},
{ast.AlterTableSpec{Tp: ast.AlterTableDropPartition}, instantAlgorithm, ast.AlgorithmTypeInstant},
{ast.AlterTableSpec{Tp: ast.AlterTableTruncatePartition}, instantAlgorithm, ast.AlgorithmTypeInstant},
{ast.AlterTableSpec{Tp: ast.AlterTableExchangePartition}, instantAlgorithm, ast.AlgorithmTypeInstant},
{ast.AlterTableSpec{Tp: ast.AlterTableCoalescePartitions}, supportedInstantAlgorithms, expectedInstantAlgorithms},
{ast.AlterTableSpec{Tp: ast.AlterTableAddPartitions}, supportedInstantAlgorithms, expectedInstantAlgorithms},
{ast.AlterTableSpec{Tp: ast.AlterTableDropPartition}, supportedInstantAlgorithms, expectedInstantAlgorithms},
{ast.AlterTableSpec{Tp: ast.AlterTableTruncatePartition}, supportedInstantAlgorithms, expectedInstantAlgorithms},
{ast.AlterTableSpec{Tp: ast.AlterTableExchangePartition}, supportedInstantAlgorithms, expectedInstantAlgorithms},

// TODO: after we support lock a table, change the below case.
{ast.AlterTableSpec{Tp: ast.AlterTableLock}, instantAlgorithm, ast.AlgorithmTypeInstant},
{ast.AlterTableSpec{Tp: ast.AlterTableLock}, supportedInstantAlgorithms, expectedInstantAlgorithms},
// TODO: after we support changing the column type, below cases need to change.
{ast.AlterTableSpec{Tp: ast.AlterTableModifyColumn}, instantAlgorithm, ast.AlgorithmTypeInstant},
{ast.AlterTableSpec{Tp: ast.AlterTableChangeColumn}, instantAlgorithm, ast.AlgorithmTypeInstant},
{ast.AlterTableSpec{Tp: ast.AlterTableAlterColumn}, instantAlgorithm, ast.AlgorithmTypeInstant},
{ast.AlterTableSpec{Tp: ast.AlterTableModifyColumn}, supportedInstantAlgorithms, expectedInstantAlgorithms},
{ast.AlterTableSpec{Tp: ast.AlterTableChangeColumn}, supportedInstantAlgorithms, expectedInstantAlgorithms},
{ast.AlterTableSpec{Tp: ast.AlterTableAlterColumn}, supportedInstantAlgorithms, expectedInstantAlgorithms},
}

for _, tc := range testCases {
Expand All @@ -76,10 +81,6 @@ func (s *testDDLAlgorithmSuite) TestFindAlterAlgorithm(c *C) {
}

func runAlterAlgorithmTestCases(c *C, tc *testCase) {
algorithm, err := ddl.ResolveAlterAlgorithm(&tc.alterSpec, ast.AlgorithmTypeDefault)
c.Assert(err, IsNil)
c.Assert(algorithm, Equals, tc.defAlgorithm)

unsupported := make([]ast.AlgorithmType, 0, len(allAlgorithm))
Loop:
for _, alm := range allAlgorithm {
Expand All @@ -92,16 +93,22 @@ Loop:
unsupported = append(unsupported, alm)
}

var algorithm ast.AlgorithmType
var err error

// Test supported.
for _, alm := range tc.supportedAlgorithm {
for i, alm := range tc.supportedAlgorithm {
algorithm, err = ddl.ResolveAlterAlgorithm(&tc.alterSpec, alm)
c.Assert(err, IsNil)
c.Assert(algorithm, Equals, alm)
if err != nil {
c.Assert(ddl.ErrAlterOperationNotSupported.Equal(err), IsTrue)
}
c.Assert(algorithm, Equals, tc.expectedAlgorithm[i])
}

// Test unsupported.
for _, alm := range unsupported {
_, err = ddl.ResolveAlterAlgorithm(&tc.alterSpec, alm)
algorithm, err = ddl.ResolveAlterAlgorithm(&tc.alterSpec, alm)
c.Assert(algorithm, Equals, ast.AlgorithmTypeDefault)
c.Assert(err, NotNil, Commentf("Tp:%v, alm:%s", tc.alterSpec.Tp, alm))
c.Assert(ddl.ErrAlterOperationNotSupported.Equal(err), IsTrue)
}
Expand Down
6 changes: 3 additions & 3 deletions ddl/ddl_api.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2131,11 +2131,11 @@ func resolveAlterTableSpec(ctx sessionctx.Context, specs []*ast.AlterTableSpec)
for _, spec := range validSpecs {
resolvedAlgorithm, err := ResolveAlterAlgorithm(spec, algorithm)
if err != nil {
if algorithm != ast.AlgorithmTypeCopy {
// If TiDB failed to choose a better algorithm, report the error
if resolvedAlgorithm == ast.AlgorithmTypeDefault {
return nil, errors.Trace(err)
}
// For the compatibility, we return warning instead of error when the algorithm is COPY,
// because the COPY ALGORITHM is not supported in TiDB.
// For the compatibility, we return warning instead of error when a better algorithm is chosed by TiDB
ctx.GetSessionVars().StmtCtx.AppendError(err)
}

Expand Down
4 changes: 2 additions & 2 deletions executor/seqtest/seq_executor_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ func (s *seqTestSuite) TestShow(c *C) {
d int UNIQUE KEY,
index invisible_idx_b (b) invisible,
index (d) invisible)`)
excepted :=
expected :=
"t CREATE TABLE `t` (\n" +
" `a` int(11) DEFAULT NULL,\n" +
" `b` int(11) DEFAULT NULL,\n" +
Expand All @@ -296,7 +296,7 @@ func (s *seqTestSuite) TestShow(c *C) {
" UNIQUE KEY `c` (`c`),\n" +
" UNIQUE KEY `d_2` (`d`)\n" +
") ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin"
tk.MustQuery("show create table t").Check(testkit.Rows(excepted))
tk.MustQuery("show create table t").Check(testkit.Rows(expected))
tk.MustExec("drop table t")

testSQL = "SHOW VARIABLES LIKE 'character_set_results';"
Expand Down

0 comments on commit f851898

Please sign in to comment.