Skip to content

Commit

Permalink
ddl: improve compatibility for ALTER TABLE algorithms (#19270) (#19364)
Browse files Browse the repository at this point in the history 
* cherry pick #19270 to release-4.0

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

* fix conflict

Co-authored-by: xhe <xw897002528@gmail.com>
Co-authored-by: bb7133 <bb7133@gmail.com>
  • Loading branch information
@ti-srebot @xhebox @bb7133
3 people authored Aug 21, 2020
1 parent fb95293 commit 829c8a7
Show file tree
Hide file tree
Showing 5 changed files with 74 additions and 54 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 @@ -1252,14 +1252,14 @@ func (s *testIntegrationSuite3) TestChangeColumnPosition(c *C) {
s.tk.MustExec("alter table position4 add index t(b)")
s.tk.MustExec("alter table position4 change column b c int first")
createSQL := s.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 @@ -1477,51 +1477,52 @@ func (s *testIntegrationSuite3) TestAlterAlgorithm(c *C) {
PARTITION p2 VALUES LESS THAN (16),
PARTITION p3 VALUES LESS THAN (21)
)`)
s.assertAlterErrorExec(c, "alter table t modify column a bigint, ALGORITHM=INPLACE;")
s.assertAlterWarnExec(c, "alter table t modify column a bigint, ALGORITHM=INPLACE;")
s.tk.MustExec("alter table t modify column a bigint, ALGORITHM=INPLACE, ALGORITHM=INSTANT;")
s.tk.MustExec("alter table t modify column a bigint, ALGORITHM=DEFAULT;")

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

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

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

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

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

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

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

s.assertAlterWarnExec(c, "alter table t default charset = utf8mb4, ALGORITHM=COPY")
s.assertAlterErrorExec(c, "alter table t default charset = utf8mb4, ALGORITHM=INPLACE")
s.assertAlterWarnExec(c, "alter table t default charset = utf8mb4, ALGORITHM=INPLACE")
s.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
72 changes: 40 additions & 32 deletions ddl/ddl_algorithm_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,42 +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.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 @@ -75,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 @@ -91,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 @@ -2053,11 +2053,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
5 changes: 3 additions & 2 deletions executor/seqtest/seq_executor_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ func (s *seqTestSuite) TestShow(c *C) {
d int UNIQUE KEY,
index (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 @@ -298,7 +298,8 @@ 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';"
result = tk.MustQuery(testSQL)
Expand Down

0 comments on commit 829c8a7

Please sign in to comment.