ddl: support alter index for multi-schema change (#36205)

ref #14766
pingcap · Jul 15, 2022 · 405f7a0 · 405f7a0
1 parent f47978c
commit 405f7a0
Show file tree

Hide file tree

Showing 5 changed files with 162 additions and 8 deletions.
diff --git a/ddl/db_cache_test.go b/ddl/db_cache_test.go
@@ -273,6 +273,7 @@ func TestIssue34069(t *testing.T) {
 	store, clean := testkit.CreateMockStore(t)
 	defer clean()
 	sem.Enable()
+	defer sem.Disable()
 
 	tk := testkit.NewTestKit(t, store)
 	tk.Session().Auth(&auth.UserIdentity{Username: "root", Hostname: "%"}, nil, nil)

diff --git a/ddl/ddl_api.go b/ddl/ddl_api.go
@@ -6623,7 +6623,7 @@ func (d *ddl) AlterIndexVisibility(ctx sessionctx.Context, ident ast.Ident, inde
 		invisible = true
 	}
 
-	skip, err := validateAlterIndexVisibility(indexName, invisible, tb.Meta())
+	skip, err := validateAlterIndexVisibility(ctx, indexName, invisible, tb.Meta())
 	if err != nil {
 		return errors.Trace(err)
 	}

diff --git a/ddl/index.go b/ddl/index.go
@@ -338,11 +338,16 @@ func onRenameIndex(d *ddlCtx, t *meta.Meta, job *model.Job) (ver int64, _ error)
 	return ver, nil
 }
 
-func validateAlterIndexVisibility(indexName model.CIStr, invisible bool, tbl *model.TableInfo) (bool, error) {
-	if idx := tbl.FindIndexByName(indexName.L); idx == nil {
+func validateAlterIndexVisibility(ctx sessionctx.Context, indexName model.CIStr, invisible bool, tbl *model.TableInfo) (bool, error) {
+	var idx *model.IndexInfo
+	if idx = tbl.FindIndexByName(indexName.L); idx == nil || idx.State != model.StatePublic {
 		return false, errors.Trace(infoschema.ErrKeyNotExists.GenWithStackByArgs(indexName.O, tbl.Name))
-	} else if idx.Invisible == invisible {
-		return true, nil
+	}
+	if ctx == nil || ctx.GetSessionVars() == nil || ctx.GetSessionVars().StmtCtx.MultiSchemaInfo == nil {
+		// Early return.
+		if idx.Invisible == invisible {
+			return true, nil
+		}
 	}
 	return false, nil
 }
@@ -352,8 +357,13 @@ func onAlterIndexVisibility(d *ddlCtx, t *meta.Meta, job *model.Job) (ver int64,
 	if err != nil || tblInfo == nil {
 		return ver, errors.Trace(err)
 	}
-	idx := tblInfo.FindIndexByName(from.L)
-	idx.Invisible = invisible
+
+	if job.MultiSchemaInfo != nil && job.MultiSchemaInfo.Revertible {
+		job.MarkNonRevertible()
+		return updateVersionAndTableInfo(d, t, job, tblInfo, false)
+	}
+
+	setIndexVisibility(tblInfo, from, invisible)
 	if ver, err = updateVersionAndTableInfoWithCheck(d, t, job, tblInfo, true); err != nil {
 		job.State = model.JobStateCancelled
 		return ver, errors.Trace(err)
@@ -362,6 +372,14 @@ func onAlterIndexVisibility(d *ddlCtx, t *meta.Meta, job *model.Job) (ver int64,
 	return ver, nil
 }
 
+func setIndexVisibility(tblInfo *model.TableInfo, name model.CIStr, invisible bool) {
+	for _, idx := range tblInfo.Indices {
+		if idx.Name.L == name.L || (isTempIdxInfo(idx, tblInfo) && getChangingIndexOriginName(idx) == name.O) {
+			idx.Invisible = invisible
+		}
+	}
+}
+
 func getNullColInfos(tblInfo *model.TableInfo, indexInfo *model.IndexInfo) ([]*model.ColumnInfo, error) {
 	nullCols := make([]*model.ColumnInfo, 0, len(indexInfo.Columns))
 	for _, colName := range indexInfo.Columns {
@@ -915,12 +933,13 @@ func checkAlterIndexVisibility(t *meta.Meta, job *model.Job) (*model.TableInfo,
 		return nil, indexName, invisible, errors.Trace(err)
 	}
 
-	skip, err := validateAlterIndexVisibility(indexName, invisible, tblInfo)
+	skip, err := validateAlterIndexVisibility(nil, indexName, invisible, tblInfo)
 	if err != nil {
 		job.State = model.JobStateCancelled
 		return nil, indexName, invisible, errors.Trace(err)
 	}
 	if skip {
+		job.State = model.JobStateDone
 		return nil, indexName, invisible, nil
 	}
 	return tblInfo, indexName, invisible, nil

diff --git a/ddl/multi_schema_change.go b/ddl/multi_schema_change.go
@@ -247,6 +247,9 @@ func fillMultiSchemaInfo(info *model.MultiSchemaInfo, job *model.Job) (err error
 	case model.ActionSetDefaultValue:
 		col := job.Args[0].(*table.Column)
 		info.ModifyColumns = append(info.ModifyColumns, col.Name)
+	case model.ActionAlterIndexVisibility:
+		idxName := job.Args[0].(model.CIStr)
+		info.AlterIndexes = append(info.AlterIndexes, idxName)
 	default:
 		return dbterror.ErrRunMultiSchemaChanges
 	}

diff --git a/ddl/multi_schema_change_test.go b/ddl/multi_schema_change_test.go
@@ -859,6 +859,10 @@ func TestMultiSchemaChangeModifyColumns(t *testing.T) {
 	tk.MustExec("create table t (a BIGINT NULL DEFAULT '-283977870758975838', b double);")
 	tk.MustExec("insert into t values (-283977870758975838, 0);")
 	tk.MustGetErrCode("alter table t change column a c tinyint null default '111' after b, modify column b time null default '13:51:02' FIRST;", errno.ErrDataOutOfRange)
+	rows := tk.MustQuery("admin show ddl jobs 1").Rows()
+	require.Equal(t, rows[0][11], "rollback done")
+	require.Equal(t, rows[1][11], "rollback done")
+	require.Equal(t, rows[2][11], "cancelled")
 
 	tk.MustExec("drop table if exists t;")
 	tk.MustExec("create table t(a int, b int);")
@@ -891,6 +895,107 @@ func TestMultiSchemaChangeModifyColumnsCancelled(t *testing.T) {
 		Check(testkit.Rows("int"))
 }
 
+func TestMultiSchemaChangeAlterIndex(t *testing.T) {
+	store, dom, clean := testkit.CreateMockStoreAndDomain(t)
+	defer clean()
+	tk := testkit.NewTestKit(t, store)
+	tk.MustExec("use test;")
+
+	// unsupported ddl operations
+	{
+		// Test alter the same index
+		tk.MustExec("drop table if exists t;")
+		tk.MustExec("create table t (a int, b int, index idx(a, b));")
+		tk.MustGetErrCode("alter table t alter index idx visible, alter index idx invisible;", errno.ErrUnsupportedDDLOperation)
+
+		// Test drop and alter the same index
+		tk.MustExec("drop table if exists t;")
+		tk.MustExec("create table t (a int, b int, index idx(a, b));")
+		tk.MustGetErrCode("alter table t drop index idx, alter index idx visible;", errno.ErrUnsupportedDDLOperation)
+
+		// Test add and alter the same index
+		tk.MustExec("drop table if exists t;")
+		tk.MustExec("create table t (a int, b int);")
+		tk.MustGetErrCode("alter table t add index idx(a, b), alter index idx invisible", errno.ErrKeyDoesNotExist)
+	}
+
+	tk.MustExec("drop table t;")
+	tk.MustExec("create table t (a int, b int, index i1(a, b), index i2(b));")
+	tk.MustExec("insert into t values (1, 2);")
+	tk.MustExec("alter table t modify column a tinyint, alter index i2 invisible, alter index i1 invisible;")
+	tk.MustGetErrCode("select * from t use index (i1);", errno.ErrKeyDoesNotExist)
+	tk.MustGetErrCode("select * from t use index (i2);", errno.ErrKeyDoesNotExist)
+	tk.MustQuery("select * from t;").Check(testkit.Rows("1 2"))
+	tk.MustExec("admin check table t;")
+
+	tk.MustExec("drop table t;")
+	tk.MustExec("create table t (a int, b int, index i1(a, b), index i2(b));")
+	tk.MustExec("insert into t values (1, 2);")
+	originHook := dom.DDL().GetHook()
+	var checked bool
+	dom.DDL().SetHook(&ddl.TestDDLCallback{Do: dom,
+		OnJobUpdatedExported: func(job *model.Job) {
+			assert.NotNil(t, job.MultiSchemaInfo)
+			// "modify column a tinyint" in write-reorg.
+			if job.MultiSchemaInfo.SubJobs[1].SchemaState == model.StateWriteReorganization {
+				checked = true
+				rs, err := tk.Exec("select * from t use index(i1);")
+				assert.NoError(t, err)
+				assert.NoError(t, rs.Close())
+			}
+		}})
+	tk.MustExec("alter table t alter index i1 invisible, modify column a tinyint, alter index i2 invisible;")
+	dom.DDL().SetHook(originHook)
+	require.True(t, checked)
+	tk.MustGetErrCode("select * from t use index (i1);", errno.ErrKeyDoesNotExist)
+	tk.MustGetErrCode("select * from t use index (i2);", errno.ErrKeyDoesNotExist)
+	tk.MustQuery("select * from t;").Check(testkit.Rows("1 2"))
+	tk.MustExec("admin check table t;")
+}
+
+func TestMultiSchemaChangeMix(t *testing.T) {
+	store, clean := testkit.CreateMockStore(t)
+	defer clean()
+	tk := testkit.NewTestKit(t, store)
+	tk.MustExec("use test;")
+
+	tk.MustExec("create table t (a int, b int, c int, index i1(c), index i2(c));")
+	tk.MustExec("insert into t values (1, 2, 3);")
+	tk.MustExec("alter table t add column d int default 4, add index i3(c), " +
+		"drop column a, drop column if exists z, add column if not exists e int default 5, " +
+		"drop index i2, add column f int default 6, drop column b, drop index i1, add column if not exists c int;")
+	tk.MustQuery("select * from t;").Check(testkit.Rows("3 4 5 6"))
+	tk.MustGetErrCode("select * from t use index (i1);", errno.ErrKeyDoesNotExist)
+	tk.MustGetErrCode("select * from t use index (i2);", errno.ErrKeyDoesNotExist)
+	tk.MustQuery("select * from t use index (i3);").Check(testkit.Rows("3 4 5 6"))
+}
+
+func TestMultiSchemaChangeMixCancelled(t *testing.T) {
+	store, dom, clean := testkit.CreateMockStoreAndDomain(t)
+	defer clean()
+	tk := testkit.NewTestKit(t, store)
+	tk.MustExec("use test;")
+
+	tk.MustExec("create table t (a int, b int, c int, index i1(c), index i2(c));")
+	tk.MustExec("insert into t values (1, 2, 3);")
+	origin := dom.DDL().GetHook()
+	cancelHook := newCancelJobHook(store, dom, func(job *model.Job) bool {
+		return job.MultiSchemaInfo != nil &&
+			len(job.MultiSchemaInfo.SubJobs) > 8 &&
+			job.MultiSchemaInfo.SubJobs[8].SchemaState == model.StateWriteReorganization
+	})
+	dom.DDL().SetHook(cancelHook)
+	tk.MustGetErrCode("alter table t add column d int default 4, add index i3(c), "+
+		"drop column a, drop column if exists z, add column if not exists e int default 5, "+
+		"drop index i2, add column f int default 6, drop column b, drop index i1, add column if not exists g int;",
+		errno.ErrCancelledDDLJob)
+	dom.DDL().SetHook(origin)
+	cancelHook.MustCancelDone(t)
+	tk.MustQuery("select * from t;").Check(testkit.Rows("1 2 3"))
+	tk.MustQuery("select * from t use index(i1, i2);").Check(testkit.Rows("1 2 3"))
+	tk.MustExec("admin check table t;")
+}
+
 func TestMultiSchemaChangeAdminShowDDLJobs(t *testing.T) {
 	store, dom, clean := testkit.CreateMockStoreAndDomain(t)
 	defer clean()
@@ -926,6 +1031,18 @@ func TestMultiSchemaChangeAdminShowDDLJobs(t *testing.T) {
 	dom.DDL().SetHook(originHook)
 }
 
+func TestMultiSchemaChangeAlterIndexVisibility(t *testing.T) {
+	store, clean := testkit.CreateMockStore(t)
+	defer clean()
+	tk := testkit.NewTestKit(t, store)
+	tk.MustExec("use test;")
+	tk.MustExec("create table t (a int, b int, index idx(b));")
+	tk.MustExec("alter table t add index idx2(a), alter index idx visible;")
+	tk.MustQuery("select * from t use index (idx, idx2);").Check(testkit.Rows( /* no rows */ ))
+	tk.MustGetErrCode("alter table t drop column b, alter index idx invisible;", errno.ErrKeyDoesNotExist)
+	tk.MustQuery("select a, b from t;").Check(testkit.Rows( /* no rows */ ))
+}
+
 func TestMultiSchemaChangeWithExpressionIndex(t *testing.T) {
 	store, dom, clean := testkit.CreateMockStoreAndDomain(t)
 	defer clean()
@@ -969,6 +1086,20 @@ func TestMultiSchemaChangeWithExpressionIndex(t *testing.T) {
 	tk.MustQuery("select * from t use index(idx1, idx2);").Check(testkit.Rows("1 2 10", "2 1 10"))
 }
 
+func TestMultiSchemaChangeNoSubJobs(t *testing.T) {
+	store, clean := testkit.CreateMockStore(t)
+	defer clean()
+	tk := testkit.NewTestKit(t, store)
+	tk.MustExec("use test;")
+
+	tk.MustExec("create table t (a int, b int);")
+	tk.MustExec("alter table t add column if not exists a int, add column if not exists b int;")
+	tk.MustQuery("show warnings;").Check(testkit.Rows(
+		"Note 1060 Duplicate column name 'a'", "Note 1060 Duplicate column name 'b'"))
+	rs := tk.MustQuery("admin show ddl jobs 1;").Rows()
+	require.Equal(t, "create table", rs[0][3])
+}
+
 type cancelOnceHook struct {
 	store     kv.Storage
 	triggered bool