From ea28019fb4d9a338a5eec938fb0db330f3a193be Mon Sep 17 00:00:00 2001 From: Xiaoyuan Jin Date: Fri, 17 Jan 2025 06:16:02 +0100 Subject: [PATCH 01/27] init, currently no tests --- pkg/ddl/executor.go | 42 ++++++++++++++++ pkg/ddl/job_worker.go | 2 + pkg/ddl/schematracker/checker.go | 4 ++ pkg/ddl/schematracker/dm_tracker.go | 5 ++ pkg/ddl/table_mode.go | 67 ++++++++++++++++++++++++++ pkg/executor/infoschema_reader.go | 53 ++++++++++---------- pkg/executor/infoschema_reader_test.go | 1 + pkg/infoschema/error.go | 7 +++ pkg/infoschema/tables.go | 1 + pkg/meta/model/job.go | 1 + pkg/meta/model/job_args.go | 19 ++++++++ pkg/meta/model/table.go | 24 +++++++++ pkg/planner/core/optimizer.go | 18 +++++++ pkg/planner/optimize.go | 4 ++ 14 files changed, 223 insertions(+), 25 deletions(-) create mode 100644 pkg/ddl/table_mode.go diff --git a/pkg/ddl/executor.go b/pkg/ddl/executor.go index 8b45f286483b0..770857b72b075 100644 --- a/pkg/ddl/executor.go +++ b/pkg/ddl/executor.go @@ -116,6 +116,7 @@ type Executor interface { RenameTable(ctx sessionctx.Context, stmt *ast.RenameTableStmt) error LockTables(ctx sessionctx.Context, stmt *ast.LockTablesStmt) error UnlockTables(ctx sessionctx.Context, lockedTables []model.TableLockTpInfo) error + AlterTableMode(ctx sessionctx.Context, args *model.AlterTableModeArgs) error CleanupTableLock(ctx sessionctx.Context, tables []*ast.TableName) error UpdateTableReplicaInfo(ctx sessionctx.Context, physicalID int64, available bool) error RepairTable(ctx sessionctx.Context, createStmt *ast.CreateTableStmt) error @@ -1059,6 +1060,15 @@ func (e *executor) createTableWithInfoJob( switch cfg.OnExist { case OnExistIgnore: ctx.GetSessionVars().StmtCtx.AppendNote(err) + // If table exists, changing from Normal/Import to Restore is not allowed. + if tbInfo.TableMode == model.TableModeRestore { + oldTableMode := oldTable.Meta().TableMode + if oldTableMode != model.TableModeRestore { + return nil, infoschema.ErrTableModeInvalidTransition.GenWithStackByArgs( + fmt.Sprintf("invalid transition from '%s' to '%s'", oldTableMode, tbInfo.TableMode), + ) + } + } return nil, nil case OnExistReplace: // only CREATE OR REPLACE VIEW is supported at the moment. @@ -1202,6 +1212,7 @@ func (e *executor) CreateTableWithInfo( if err != nil { // table exists, but if_not_exists flags is true, so we ignore this error. if c.OnExist == OnExistIgnore && infoschema.ErrTableExists.Equal(err) { + // TODO(xiaoyuan): I think this branch will never be reached, please see e.createTableWithInfoJob ctx.GetSessionVars().StmtCtx.AppendNote(err) err = nil } @@ -5646,6 +5657,37 @@ func (e *executor) UnlockTables(ctx sessionctx.Context, unlockTables []model.Tab return errors.Trace(err) } +func (e *executor) AlterTableMode(ctx sessionctx.Context, args *model.AlterTableModeArgs) error { + is := e.infoCache.GetLatest() + + t, ok := is.TableByID(e.ctx, args.TableID) + if !ok { + return infoschema.ErrTableNotExists.GenWithStackByArgs() + } + + schema, ok := is.SchemaByID(args.SchemaID) + if !ok { + return infoschema.ErrDatabaseNotExists.GenWithStackByArgs() + } + + job := &model.Job{ + Version: model.GetJobVerInUse(), + SchemaID: args.SchemaID, + TableID: args.TableID, + Type: model.ActionAlterTableMode, + BinlogInfo: &model.HistoryInfo{}, + CDCWriteSource: ctx.GetSessionVars().CDCWriteSource, + InvolvingSchemaInfo: []model.InvolvingSchemaInfo{ + { + Database: schema.Name.L, + Table: t.Meta().Name.L, + }, + }, + } + err := e.doDDLJob2(ctx, job, args) + return errors.Trace(err) +} + func throwErrIfInMemOrSysDB(ctx sessionctx.Context, dbLowerName string) error { if util.IsMemOrSysDB(dbLowerName) { if ctx.GetSessionVars().User != nil { diff --git a/pkg/ddl/job_worker.go b/pkg/ddl/job_worker.go index 4d30700cb3b37..8982be7024fc0 100644 --- a/pkg/ddl/job_worker.go +++ b/pkg/ddl/job_worker.go @@ -980,6 +980,8 @@ func (w *worker) runOneJobStep( ver, err = onLockTables(jobCtx, job) case model.ActionUnlockTable: ver, err = onUnlockTables(jobCtx, job) + case model.ActionAlterTableMode: + ver, err = onAlterTableMode(jobCtx, job) case model.ActionSetTiFlashReplica: ver, err = w.onSetTableFlashReplica(jobCtx, job) case model.ActionUpdateTiFlashReplicaStatus: diff --git a/pkg/ddl/schematracker/checker.go b/pkg/ddl/schematracker/checker.go index 936f874b4f831..2eba44f2fb293 100644 --- a/pkg/ddl/schematracker/checker.go +++ b/pkg/ddl/schematracker/checker.go @@ -398,6 +398,10 @@ func (d *Checker) UnlockTables(ctx sessionctx.Context, lockedTables []model.Tabl return d.realExecutor.UnlockTables(ctx, lockedTables) } +func (d *Checker) AlterTableMode(ctx sessionctx.Context, stmt *ast.AlterTableModeStmt) error { + return d.realExecutor.AlterTableMode(ctx, stmt) +} + // CleanupTableLock implements the DDL interface. func (d *Checker) CleanupTableLock(ctx sessionctx.Context, tables []*ast.TableName) error { return d.realExecutor.CleanupTableLock(ctx, tables) diff --git a/pkg/ddl/schematracker/dm_tracker.go b/pkg/ddl/schematracker/dm_tracker.go index e029e7495d2cf..52d4f84df90fc 100644 --- a/pkg/ddl/schematracker/dm_tracker.go +++ b/pkg/ddl/schematracker/dm_tracker.go @@ -1118,6 +1118,11 @@ func (*SchemaTracker) UnlockTables(_ sessionctx.Context, _ []model.TableLockTpIn return nil } +// AlterTableMode implements the DDL interface, it's no-op in DM's case. +func (*SchemaTracker) AlterTableMode(_ sessionctx.Context, _ *ast.AlterTableModeStmt) error { + return nil +} + // CleanupTableLock implements the DDL interface, it's no-op in DM's case. func (*SchemaTracker) CleanupTableLock(_ sessionctx.Context, _ []*ast.TableName) error { return nil diff --git a/pkg/ddl/table_mode.go b/pkg/ddl/table_mode.go new file mode 100644 index 0000000000000..1f9da21541bf9 --- /dev/null +++ b/pkg/ddl/table_mode.go @@ -0,0 +1,67 @@ +// Copyright 2019 PingCAP, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package ddl + +import ( + "fmt" + "github.com/pingcap/tidb/pkg/infoschema" + "github.com/pingcap/tidb/pkg/meta/model" +) + +// onAlterTableMode should only be called by alterTableMode, will call updateVersionAndTableInfo +func onAlterTableMode(jobCtx *jobContext, job *model.Job) (ver int64, err error) { + args, err := model.GetAlterTableModeArgs(job) + var tbInfo *model.TableInfo + metaMut := jobCtx.metaMut + tbInfo, err = GetTableInfoAndCancelFaultJob(metaMut, job, job.SchemaID) + if err != nil { + return ver, err + } + + switch tbInfo.TableMode { + case model.TableModeNormal, model.TableModeImport, model.TableModeRestore: + // directly change table mode to target mode + err = alterTableMode(tbInfo, args) + if err != nil { + job.State = model.JobStateCancelled + } else { + // update table info and schema version + ver, err = updateVersionAndTableInfo(jobCtx, job, tbInfo, true) + job.FinishTableJob(model.JobStateDone, model.StatePublic, ver, tbInfo) // TODO: change of schema state + } + default: + job.State = model.JobStateCancelled + err = infoschema.ErrTableModeInvalidTransition.GenWithStackByArgs( + fmt.Sprintf("invalid transition from '%s' to '%s'", tbInfo.TableMode, args.TableMode), + ) + } + + return ver, err +} + +// alterTableMode first checks if the change is valid and changes table mode to target mode +func alterTableMode(tbInfo *model.TableInfo, args *model.AlterTableModeArgs) error { + // currently we can assume args.TableMode will not be model.TableModeRestore + if args.TableMode == model.TableModeImport { + // only transition from ModeNormal to ModeImport is allowed + if tbInfo.TableMode != model.TableModeNormal { + return infoschema.ErrTableModeInvalidTransition.GenWithStackByArgs( + fmt.Sprintf("invalid transition from '%s' to '%s'", tbInfo.TableMode, args.TableMode), + ) + } + } + tbInfo.TableMode = args.TableMode + return nil +} diff --git a/pkg/executor/infoschema_reader.go b/pkg/executor/infoschema_reader.go index 30e47113a70ae..6f3c210d65fa7 100644 --- a/pkg/executor/infoschema_reader.go +++ b/pkg/executor/infoschema_reader.go @@ -674,31 +674,32 @@ func (e *memtableRetriever) setDataFromOneTable( } record := types.MakeDatums( - infoschema.CatalogVal, // TABLE_CATALOG - schema.O, // TABLE_SCHEMA - table.Name.O, // TABLE_NAME - tableType, // TABLE_TYPE - "InnoDB", // ENGINE - uint64(10), // VERSION - "Compact", // ROW_FORMAT - rowCount, // TABLE_ROWS - avgRowLength, // AVG_ROW_LENGTH - dataLength, // DATA_LENGTH - uint64(0), // MAX_DATA_LENGTH - indexLength, // INDEX_LENGTH - uint64(0), // DATA_FREE - autoIncID, // AUTO_INCREMENT - createTime, // CREATE_TIME - nil, // UPDATE_TIME - nil, // CHECK_TIME - collation, // TABLE_COLLATION - nil, // CHECKSUM - createOptions, // CREATE_OPTIONS - table.Comment, // TABLE_COMMENT - table.ID, // TIDB_TABLE_ID - shardingInfo, // TIDB_ROW_ID_SHARDING_INFO - pkType, // TIDB_PK_TYPE - policyName, // TIDB_PLACEMENT_POLICY_NAME + infoschema.CatalogVal, // TABLE_CATALOG + schema.O, // TABLE_SCHEMA + table.Name.O, // TABLE_NAME + tableType, // TABLE_TYPE + "InnoDB", // ENGINE + uint64(10), // VERSION + "Compact", // ROW_FORMAT + rowCount, // TABLE_ROWS + avgRowLength, // AVG_ROW_LENGTH + dataLength, // DATA_LENGTH + uint64(0), // MAX_DATA_LENGTH + indexLength, // INDEX_LENGTH + uint64(0), // DATA_FREE + autoIncID, // AUTO_INCREMENT + createTime, // CREATE_TIME + nil, // UPDATE_TIME + nil, // CHECK_TIME + collation, // TABLE_COLLATION + nil, // CHECKSUM + createOptions, // CREATE_OPTIONS + table.Comment, // TABLE_COMMENT + table.ID, // TIDB_TABLE_ID + shardingInfo, // TIDB_ROW_ID_SHARDING_INFO + pkType, // TIDB_PK_TYPE + policyName, // TIDB_PLACEMENT_POLICY_NAME + table.TableMode.String(), // TIDB_TABLE_MODE ) rows = append(rows, record) } else { @@ -728,6 +729,7 @@ func (e *memtableRetriever) setDataFromOneTable( nil, // TIDB_ROW_ID_SHARDING_INFO pkType, // TIDB_PK_TYPE nil, // TIDB_PLACEMENT_POLICY_NAME + nil, // TIDB_TABLE_MODE ) rows = append(rows, record) } @@ -831,6 +833,7 @@ func (e *memtableRetriever) setDataFromTables(ctx context.Context, sctx sessionc nil, // TIDB_ROW_ID_SHARDING_INFO nil, // TIDB_PK_TYPE nil, // TIDB_PLACEMENT_POLICY_NAME + nil, // TIDB_TABLE_MODE ) rows = append(rows, record) return true diff --git a/pkg/executor/infoschema_reader_test.go b/pkg/executor/infoschema_reader_test.go index f016696520c84..bf425423ad3c5 100644 --- a/pkg/executor/infoschema_reader_test.go +++ b/pkg/executor/infoschema_reader_test.go @@ -536,6 +536,7 @@ func TestTablesTable(t *testing.T) { // Predicates are extracted in CNF, so we separate the test cases by the number of disjunctions in the predicate. // predicate covers one disjunction + tk.MustExec(`select tidb_table_mode from information_schema.tables where table_schema = 'db1'`) tk.MustQuery(`select table_schema, table_name, tidb_table_id from information_schema.tables where table_schema = 'db1'`).Sort().Check(testkit.Rows(toString(tableMetas[0]), toString(tableMetas[1]))) tk.MustQuery(`select table_schema, table_name, tidb_table_id from information_schema.tables diff --git a/pkg/infoschema/error.go b/pkg/infoschema/error.go index 0dacfca35da00..0aa359ee0cdc1 100644 --- a/pkg/infoschema/error.go +++ b/pkg/infoschema/error.go @@ -108,4 +108,11 @@ var ( ErrResourceGroupSupportDisabled = dbterror.ClassSchema.NewStd(mysql.ErrResourceGroupSupportDisabled) // ErrCheckConstraintDupName returns for duplicate constraint names. ErrCheckConstraintDupName = dbterror.ClassSchema.NewStd(mysql.ErrCheckConstraintDupName) + // TODO(xiaoyuan): what is the proper mysql error I should use for TableMode? + // ErrTableModeImport returns for accessing table in import mode. + ErrTableModeImport = dbterror.ClassSchema.NewStd(mysql.ErrTableLocked) + // ErrTableModeRestore returns for accessing table in restore mode. + ErrTableModeRestore = dbterror.ClassSchema.NewStd(mysql.ErrTableLocked) + // ErrTableModeInvalidTransition returns for invalid TableMode transition. + ErrTableModeInvalidTransition = dbterror.ClassSchema.NewStd(mysql.ErrTableLocked) ) diff --git a/pkg/infoschema/tables.go b/pkg/infoschema/tables.go index 06d9c69bb41c8..810b4646be0bf 100644 --- a/pkg/infoschema/tables.go +++ b/pkg/infoschema/tables.go @@ -466,6 +466,7 @@ var tablesCols = []columnInfo{ {name: "TIDB_ROW_ID_SHARDING_INFO", tp: mysql.TypeVarchar, size: 255}, {name: "TIDB_PK_TYPE", tp: mysql.TypeVarchar, size: 64}, {name: "TIDB_PLACEMENT_POLICY_NAME", tp: mysql.TypeVarchar, size: 64}, + {name: "TIDB_TABLE_MODE", tp: mysql.TypeVarchar, size: 16}, } // See: http://dev.mysql.com/doc/refman/5.7/en/information-schema-columns-table.html diff --git a/pkg/meta/model/job.go b/pkg/meta/model/job.go index 1b0919fe2c01b..eefb01411518c 100644 --- a/pkg/meta/model/job.go +++ b/pkg/meta/model/job.go @@ -111,6 +111,7 @@ const ( ActionAlterTablePartitioning ActionType = 71 ActionRemovePartitioning ActionType = 72 ActionAddVectorIndex ActionType = 73 + ActionAlterTableMode ActionType = 74 ) // ActionMap is the map of DDL ActionType to string. diff --git a/pkg/meta/model/job_args.go b/pkg/meta/model/job_args.go index 1c62c726d9184..4676d1c651cb8 100644 --- a/pkg/meta/model/job_args.go +++ b/pkg/meta/model/job_args.go @@ -1083,6 +1083,25 @@ func GetLockTablesArgs(job *Job) (*LockTablesArgs, error) { return getOrDecodeArgs[*LockTablesArgs](&LockTablesArgs{}, job) } +type AlterTableModeArgs struct { + TableMode TableModeState + SchemaID int64 + TableID int64 +} + +func (a *AlterTableModeArgs) getArgsV1(*Job) []any { + return []any{a} +} + +func (a *AlterTableModeArgs) decodeV1(job *Job) error { + return errors.Trace(job.decodeArgs(a)) +} + +// GetAlterTableModeArgs get the AlterTableModeArgs argument. +func GetAlterTableModeArgs(job *Job) (*AlterTableModeArgs, error) { + return getOrDecodeArgs[*AlterTableModeArgs](&AlterTableModeArgs{}, job) +} + // RepairTableArgs is the argument for repair table type RepairTableArgs struct { TableInfo *TableInfo `json:"table_info"` diff --git a/pkg/meta/model/table.go b/pkg/meta/model/table.go index 750bb5f0fd0ea..cb965ea4179a2 100644 --- a/pkg/meta/model/table.go +++ b/pkg/meta/model/table.go @@ -195,6 +195,8 @@ type TableInfo struct { Revision uint64 `json:"revision"` DBID int64 `json:"-"` + + TableMode TableModeState `json:"table_mode"` } // Hash64 implement HashEquals interface. @@ -647,6 +649,28 @@ const ( TableLockStatePublic ) +type TableModeState byte + +const ( + TableModeNormal TableModeState = iota + TableModeImport + TableModeRestore +) + +// String implements fmt.Stringer interface. +func (t TableModeState) String() string { + switch t { + case TableModeNormal: + return "ModeNormal" + case TableModeImport: + return "ModeImport" + case TableModeRestore: + return "ModeRestore" + default: + return "" + } +} + // String implements fmt.Stringer interface. func (t TableLockState) String() string { switch t { diff --git a/pkg/planner/core/optimizer.go b/pkg/planner/core/optimizer.go index 5a82e05fdf9e5..51713cd392578 100644 --- a/pkg/planner/core/optimizer.go +++ b/pkg/planner/core/optimizer.go @@ -247,6 +247,24 @@ func CheckTableLock(ctx tablelock.TableLockReadContext, is infoschema.InfoSchema return nil } +func CheckTableMode(is infoschema.InfoSchema, vs []visitInfo) error { + for i := range vs { + tb, err := is.TableByName(context.Background(), ast.NewCIStr(vs[i].db), ast.NewCIStr(vs[i].table)) + if infoschema.ErrTableNotExists.Equal(err) { + return nil + } + if err != nil { + return err + } + if tb.Meta().TableMode == model.TableModeImport { + return infoschema.ErrTableModeImport.GenWithStackByArgs(tb.Meta().Name.O) + } else if tb.Meta().TableMode == model.TableModeRestore { + return infoschema.ErrTableModeRestore.GenWithStackByArgs(tb.Meta().Name.O) + } + } + return nil +} + func checkStableResultMode(sctx base.PlanContext) bool { s := sctx.GetSessionVars() st := s.StmtCtx diff --git a/pkg/planner/optimize.go b/pkg/planner/optimize.go index 526e23c14905e..7aa2aba8318c6 100644 --- a/pkg/planner/optimize.go +++ b/pkg/planner/optimize.go @@ -474,6 +474,10 @@ func optimize(ctx context.Context, sctx planctx.PlanContext, node *resolve.NodeW return nil, nil, 0, err } + if err := core.CheckTableMode(is, builder.GetVisitInfo()); err != nil { + return nil, nil, 0, err + } + names := p.OutputNames() // Handle the non-logical plan statement. From 01d2e1ffa81498aed48fd53f17b220441d92362a Mon Sep 17 00:00:00 2001 From: Xiaoyuan Jin Date: Fri, 17 Jan 2025 10:21:11 +0100 Subject: [PATCH 02/27] fix some small problems, add comments --- pkg/ddl/executor.go | 8 ++++---- pkg/ddl/schematracker/checker.go | 5 +++-- pkg/ddl/schematracker/dm_tracker.go | 2 +- pkg/executor/infoschema_reader_test.go | 5 ++++- pkg/planner/core/optimizer.go | 1 + 5 files changed, 13 insertions(+), 8 deletions(-) diff --git a/pkg/ddl/executor.go b/pkg/ddl/executor.go index 770857b72b075..2a1ad7e3e51ed 100644 --- a/pkg/ddl/executor.go +++ b/pkg/ddl/executor.go @@ -5660,14 +5660,14 @@ func (e *executor) UnlockTables(ctx sessionctx.Context, unlockTables []model.Tab func (e *executor) AlterTableMode(ctx sessionctx.Context, args *model.AlterTableModeArgs) error { is := e.infoCache.GetLatest() - t, ok := is.TableByID(e.ctx, args.TableID) + schema, ok := is.SchemaByID(args.SchemaID) if !ok { - return infoschema.ErrTableNotExists.GenWithStackByArgs() + return infoschema.ErrDatabaseNotExists.GenWithStackByArgs() } - schema, ok := is.SchemaByID(args.SchemaID) + t, ok := is.TableByID(e.ctx, args.TableID) if !ok { - return infoschema.ErrDatabaseNotExists.GenWithStackByArgs() + return infoschema.ErrTableNotExists.GenWithStackByArgs() } job := &model.Job{ diff --git a/pkg/ddl/schematracker/checker.go b/pkg/ddl/schematracker/checker.go index 2eba44f2fb293..f8f2a03b1c599 100644 --- a/pkg/ddl/schematracker/checker.go +++ b/pkg/ddl/schematracker/checker.go @@ -398,8 +398,9 @@ func (d *Checker) UnlockTables(ctx sessionctx.Context, lockedTables []model.Tabl return d.realExecutor.UnlockTables(ctx, lockedTables) } -func (d *Checker) AlterTableMode(ctx sessionctx.Context, stmt *ast.AlterTableModeStmt) error { - return d.realExecutor.AlterTableMode(ctx, stmt) +// AlterTableMode implements the DDL interface. +func (d *Checker) AlterTableMode(ctx sessionctx.Context, args *model.AlterTableModeArgs) error { + return d.realExecutor.AlterTableMode(ctx, args) } // CleanupTableLock implements the DDL interface. diff --git a/pkg/ddl/schematracker/dm_tracker.go b/pkg/ddl/schematracker/dm_tracker.go index 52d4f84df90fc..c36ea37cfc695 100644 --- a/pkg/ddl/schematracker/dm_tracker.go +++ b/pkg/ddl/schematracker/dm_tracker.go @@ -1119,7 +1119,7 @@ func (*SchemaTracker) UnlockTables(_ sessionctx.Context, _ []model.TableLockTpIn } // AlterTableMode implements the DDL interface, it's no-op in DM's case. -func (*SchemaTracker) AlterTableMode(_ sessionctx.Context, _ *ast.AlterTableModeStmt) error { +func (*SchemaTracker) AlterTableMode(_ sessionctx.Context, _ *model.AlterTableModeArgs) error { return nil } diff --git a/pkg/executor/infoschema_reader_test.go b/pkg/executor/infoschema_reader_test.go index bf425423ad3c5..15e229c489a2b 100644 --- a/pkg/executor/infoschema_reader_test.go +++ b/pkg/executor/infoschema_reader_test.go @@ -533,10 +533,13 @@ func TestTablesTable(t *testing.T) { } } + // test table mode + tk.MustQuery(`select tidb_table_mode from information_schema.tables where table_schema = 'db1' and + table_name = 't1'`).Check(testkit.Rows("ModeNormal")) + // Predicates are extracted in CNF, so we separate the test cases by the number of disjunctions in the predicate. // predicate covers one disjunction - tk.MustExec(`select tidb_table_mode from information_schema.tables where table_schema = 'db1'`) tk.MustQuery(`select table_schema, table_name, tidb_table_id from information_schema.tables where table_schema = 'db1'`).Sort().Check(testkit.Rows(toString(tableMetas[0]), toString(tableMetas[1]))) tk.MustQuery(`select table_schema, table_name, tidb_table_id from information_schema.tables diff --git a/pkg/planner/core/optimizer.go b/pkg/planner/core/optimizer.go index 51713cd392578..fc64149ae7504 100644 --- a/pkg/planner/core/optimizer.go +++ b/pkg/planner/core/optimizer.go @@ -247,6 +247,7 @@ func CheckTableLock(ctx tablelock.TableLockReadContext, is infoschema.InfoSchema return nil } +// CheckTableMode checks if the table is accessible by table mode. func CheckTableMode(is infoschema.InfoSchema, vs []visitInfo) error { for i := range vs { tb, err := is.TableByName(context.Background(), ast.NewCIStr(vs[i].db), ast.NewCIStr(vs[i].table)) From be92a6e271a79358ff03f17654c7c60058660e46 Mon Sep 17 00:00:00 2001 From: Xiaoyuan Jin Date: Fri, 24 Jan 2025 06:12:56 +0800 Subject: [PATCH 03/27] test alter table mode --- pkg/ddl/executor.go | 1 + pkg/ddl/table_test.go | 54 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+) diff --git a/pkg/ddl/executor.go b/pkg/ddl/executor.go index 2a1ad7e3e51ed..75e5f65956a38 100644 --- a/pkg/ddl/executor.go +++ b/pkg/ddl/executor.go @@ -5677,6 +5677,7 @@ func (e *executor) AlterTableMode(ctx sessionctx.Context, args *model.AlterTable Type: model.ActionAlterTableMode, BinlogInfo: &model.HistoryInfo{}, CDCWriteSource: ctx.GetSessionVars().CDCWriteSource, + // TODO(xiaoyuan): when is this information needed? InvolvingSchemaInfo: []model.InvolvingSchemaInfo{ { Database: schema.Name.L, diff --git a/pkg/ddl/table_test.go b/pkg/ddl/table_test.go index 369f0820ced53..3e1d85d17586c 100644 --- a/pkg/ddl/table_test.go +++ b/pkg/ddl/table_test.go @@ -163,6 +163,54 @@ func checkTableLockedTest(t *testing.T, store kv.Storage, dbInfo *model.DBInfo, require.NoError(t, err) } +func testAlterTableMode( + t *testing.T, + ctx sessionctx.Context, + d ddl.ExecutorForTest, + dbInfo *model.DBInfo, + tblInfo *model.TableInfo, + mode model.TableModeState, +) *model.Job { + args := &model.AlterTableModeArgs{ + TableMode: mode, + SchemaID: dbInfo.ID, + TableID: tblInfo.ID, + } + job := &model.Job{ + Version: model.GetJobVerInUse(), + SchemaID: dbInfo.ID, + TableID: tblInfo.ID, + Type: model.ActionAlterTableMode, + BinlogInfo: &model.HistoryInfo{}, + InvolvingSchemaInfo: []model.InvolvingSchemaInfo{ + { + Database: dbInfo.Name.L, + Table: tblInfo.Name.L, + }, + }, + } + ctx.SetValue(sessionctx.QueryString, "skip") + err := d.DoDDLJobWrapper(ctx, ddl.NewJobWrapperWithArgs(job, args, true)) + require.NoError(t, err) + + v := getSchemaVer(t, ctx) + checkHistoryJobArgs(t, ctx, job.ID, &historyJobArgs{ver: v, tbl: tblInfo}) + return job +} + +func checkTableModeTest(t *testing.T, store kv.Storage, dbInfo *model.DBInfo, tblInfo *model.TableInfo, mode model.TableModeState) { + ctx := kv.WithInternalSourceType(context.Background(), kv.InternalTxnDDL) + err := kv.RunInNewTxn(ctx, store, false, func(ctx context.Context, txn kv.Transaction) error { + tt := meta.NewMutator(txn) + info, err := tt.GetTable(dbInfo.ID, tblInfo.ID) + require.NoError(t, err) + require.NotNil(t, info) + require.Equal(t, mode, info.TableMode) + return nil + }) + require.NoError(t, err) +} + func testTruncateTable(t *testing.T, ctx sessionctx.Context, store kv.Storage, d ddl.ExecutorForTest, dbInfo *model.DBInfo, tblInfo *model.TableInfo) *model.Job { genIDs, err := genGlobalIDs(store, 1) require.NoError(t, err) @@ -270,6 +318,12 @@ func TestTable(t *testing.T) { testCheckTableState(t, store, dbInfo1, tblInfo, model.StatePublic) testCheckJobDone(t, store, job.ID, true) + // for alter table mode + job = testAlterTableMode(t, ctx, de, dbInfo1, tblInfo, model.TableModeImport) + testCheckTableState(t, store, dbInfo1, tblInfo, model.StatePublic) + testCheckJobDone(t, store, job.ID, true) + checkTableModeTest(t, store, dbInfo1, tblInfo, model.TableModeImport) + job = testLockTable(t, ctx, de, d.GetID(), dbInfo1.ID, dbInfo1.Name, tblInfo, ast.TableLockWrite) testCheckTableState(t, store, dbInfo1, tblInfo, model.StatePublic) testCheckJobDone(t, store, job.ID, true) From 3a03551cb41879b8c1abeb17b2e57cd84d8f5754 Mon Sep 17 00:00:00 2001 From: Xiaoyuan Jin Date: Fri, 24 Jan 2025 06:18:50 +0800 Subject: [PATCH 04/27] AlterTableMode directly use JobVersion2 --- pkg/ddl/executor.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/ddl/executor.go b/pkg/ddl/executor.go index 75e5f65956a38..66c2be06bb720 100644 --- a/pkg/ddl/executor.go +++ b/pkg/ddl/executor.go @@ -5671,7 +5671,7 @@ func (e *executor) AlterTableMode(ctx sessionctx.Context, args *model.AlterTable } job := &model.Job{ - Version: model.GetJobVerInUse(), + Version: model.JobVersion2, SchemaID: args.SchemaID, TableID: args.TableID, Type: model.ActionAlterTableMode, From 56555c81bbac6e7a5eeeb8b07967161f0678fa78 Mon Sep 17 00:00:00 2001 From: Xiaoyuan Jin Date: Fri, 24 Jan 2025 08:33:12 +0800 Subject: [PATCH 05/27] part of test, will have more. and will refine code later --- pkg/ddl/table_mode_test.go | 154 +++++++++++++++++++++++++++++++++++++ pkg/ddl/table_test.go | 2 +- 2 files changed, 155 insertions(+), 1 deletion(-) create mode 100644 pkg/ddl/table_mode_test.go diff --git a/pkg/ddl/table_mode_test.go b/pkg/ddl/table_mode_test.go new file mode 100644 index 0000000000000..6ec0094d3f0f5 --- /dev/null +++ b/pkg/ddl/table_mode_test.go @@ -0,0 +1,154 @@ +// Copyright 2019 PingCAP, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package ddl_test + +import ( + "context" + "github.com/pingcap/errors" + "github.com/pingcap/tidb/pkg/parser/terror" + "github.com/pingcap/tidb/pkg/sessionctx" + "testing" + + "github.com/pingcap/tidb/pkg/ddl" + "github.com/pingcap/tidb/pkg/domain" + "github.com/pingcap/tidb/pkg/meta/model" + "github.com/pingcap/tidb/pkg/parser/ast" + "github.com/pingcap/tidb/pkg/testkit" + "github.com/stretchr/testify/require" +) + +func getClonedTableInfoFromDomain(dbName string, tableName string, dom *domain.Domain) (*model.TableInfo, error) { + tbl, err := dom.InfoSchema().TableByName(context.Background(), ast.NewCIStr(dbName), ast.NewCIStr(tableName)) + if err != nil { + return nil, err + } + return tbl.Meta().Clone(), nil +} + +func TestCreateTableWithModeInfo(t *testing.T) { + store, domain := testkit.CreateMockStoreAndDomain(t) + de := domain.DDLExecutor() + tk := testkit.NewTestKit(t, store) + + ctx := testkit.NewTestKit(t, store).Session() + + // init test + tk.MustExec("use test") + tk.MustExec("create table t1(id int)") + + // get cloned table info for creating new table t1_restore + tblInfo, err := getClonedTableInfoFromDomain("test", "t1", domain) + require.NoError(t, err) + + // For testing create table as ModeRestore + tblInfo.Name = ast.NewCIStr("t1_restore") + tblInfo.TableMode = model.TableModeRestore + err = de.CreateTableWithInfo(tk.Session(), ast.NewCIStr("test"), tblInfo, nil, ddl.WithOnExist(ddl.OnExistIgnore)) + require.NoError(t, err) + dbInfo, ok := domain.InfoSchema().SchemaByName(ast.NewCIStr("test")) + require.True(t, ok) + checkTableModeTest(t, store, dbInfo, tblInfo, model.TableModeRestore) + + // For testing select is not allowed when table is in ModeImport + // TODO(xiaoyuan): currently the error is very ugly like: + // [schema:8020]Table 't1_restore' was locked in %!s(MISSING) by %!v(MISSING) + // this is because infoschema.ErrTableModeRestore is based on mysql.ErrTableLocked, + // how to make it look nice? or shoule I define a new mysql error? + tk.MustGetErrCode("select * from t1_restore", 8020) + + // For testing insert is not allowed when table is in ModeImport + tk.MustGetErrCode("insert into t1_restore values(1)", 8020) + + // TODO(xiaoyuan): extract as function and reuse the code + // For testing AlterTable from ModeRestore to ModeImport is not allowed + args := &model.AlterTableModeArgs{ + TableMode: model.TableModeImport, + SchemaID: dbInfo.ID, + TableID: tblInfo.ID, + } + job := &model.Job{ + Version: model.JobVersion2, + SchemaID: dbInfo.ID, + TableID: tblInfo.ID, + Type: model.ActionAlterTableMode, + BinlogInfo: &model.HistoryInfo{}, + InvolvingSchemaInfo: []model.InvolvingSchemaInfo{ + { + Database: dbInfo.Name.L, + Table: tblInfo.Name.L, + }, + }, + } + ctx.SetValue(sessionctx.QueryString, "skip") + err = de.(ddl.ExecutorForTest).DoDDLJobWrapper(ctx, ddl.NewJobWrapperWithArgs(job, args, true)) + // TODO(xiaoyuan): consider use different error code for transition error and not-accessible error + originErr := errors.Cause(err) + tErr, ok := originErr.(*terror.Error) + require.True(t, ok) + sqlErr := terror.ToSQLError(tErr) + require.Equal(t, 8020, int(sqlErr.Code)) + + // For testing AlterTableMode from ModeRestore to ModeNormal + args = &model.AlterTableModeArgs{ + TableMode: model.TableModeNormal, + SchemaID: dbInfo.ID, + TableID: tblInfo.ID, + } + job = &model.Job{ + Version: model.JobVersion2, + SchemaID: dbInfo.ID, + TableID: tblInfo.ID, + Type: model.ActionAlterTableMode, + BinlogInfo: &model.HistoryInfo{}, + InvolvingSchemaInfo: []model.InvolvingSchemaInfo{ + { + Database: dbInfo.Name.L, + Table: tblInfo.Name.L, + }, + }, + } + ctx.SetValue(sessionctx.QueryString, "skip") + err = de.(ddl.ExecutorForTest).DoDDLJobWrapper(ctx, ddl.NewJobWrapperWithArgs(job, args, true)) + testCheckTableState(t, store, dbInfo, tblInfo, model.StatePublic) + testCheckJobDone(t, store, job.ID, true) + checkTableModeTest(t, store, dbInfo, tblInfo, model.TableModeNormal) + + // For testing AlterTableMode from ModeNormal to ModeRestore + args = &model.AlterTableModeArgs{ + TableMode: model.TableModeImport, + SchemaID: dbInfo.ID, + TableID: tblInfo.ID, + } + job = &model.Job{ + Version: model.JobVersion2, + SchemaID: dbInfo.ID, + TableID: tblInfo.ID, + Type: model.ActionAlterTableMode, + BinlogInfo: &model.HistoryInfo{}, + InvolvingSchemaInfo: []model.InvolvingSchemaInfo{ + { + Database: dbInfo.Name.L, + Table: tblInfo.Name.L, + }, + }, + } + ctx.SetValue(sessionctx.QueryString, "skip") + err = de.(ddl.ExecutorForTest).DoDDLJobWrapper(ctx, ddl.NewJobWrapperWithArgs(job, args, true)) + testCheckTableState(t, store, dbInfo, tblInfo, model.StatePublic) + testCheckJobDone(t, store, job.ID, true) + checkTableModeTest(t, store, dbInfo, tblInfo, model.TableModeImport) + + // TODO: batch create tables with ModeRestore test +} diff --git a/pkg/ddl/table_test.go b/pkg/ddl/table_test.go index 3e1d85d17586c..064bb184127fb 100644 --- a/pkg/ddl/table_test.go +++ b/pkg/ddl/table_test.go @@ -177,7 +177,7 @@ func testAlterTableMode( TableID: tblInfo.ID, } job := &model.Job{ - Version: model.GetJobVerInUse(), + Version: model.JobVersion2, SchemaID: dbInfo.ID, TableID: tblInfo.ID, Type: model.ActionAlterTableMode, From 7143444c3a323426fced0d06bffd771f83b0fbfa Mon Sep 17 00:00:00 2001 From: Xiaoyuan Jin Date: Thu, 6 Feb 2025 09:01:22 +0800 Subject: [PATCH 06/27] TestCreateTableWithModeInfo --- pkg/ddl/table_mode_test.go | 149 ++++++++++++++++++------------------- 1 file changed, 71 insertions(+), 78 deletions(-) diff --git a/pkg/ddl/table_mode_test.go b/pkg/ddl/table_mode_test.go index 6ec0094d3f0f5..b45ae706bba39 100644 --- a/pkg/ddl/table_mode_test.go +++ b/pkg/ddl/table_mode_test.go @@ -17,6 +17,7 @@ package ddl_test import ( "context" "github.com/pingcap/errors" + "github.com/pingcap/tidb/pkg/kv" "github.com/pingcap/tidb/pkg/parser/terror" "github.com/pingcap/tidb/pkg/sessionctx" "testing" @@ -37,6 +38,46 @@ func getClonedTableInfoFromDomain(dbName string, tableName string, dom *domain.D return tbl.Meta().Clone(), nil } +func setTableModeTest(ctx sessionctx.Context, t *testing.T, store kv.Storage, de ddl.ExecutorForTest, dbInfo *model.DBInfo, tblInfo *model.TableInfo, mode model.TableModeState) error { + args := &model.AlterTableModeArgs{ + TableMode: mode, + SchemaID: dbInfo.ID, + TableID: tblInfo.ID, + } + job := &model.Job{ + Version: model.JobVersion2, + SchemaID: dbInfo.ID, + TableID: tblInfo.ID, + Type: model.ActionAlterTableMode, + BinlogInfo: &model.HistoryInfo{}, + InvolvingSchemaInfo: []model.InvolvingSchemaInfo{ + { + Database: dbInfo.Name.L, + Table: tblInfo.Name.L, + }, + }, + } + ctx.SetValue(sessionctx.QueryString, "skip") + err := de.DoDDLJobWrapper(ctx, ddl.NewJobWrapperWithArgs(job, args, true)) + + if err == nil { + testCheckTableState(t, store, dbInfo, tblInfo, model.StatePublic) + testCheckJobDone(t, store, job.ID, true) + checkTableModeTest(t, store, dbInfo, tblInfo, mode) + } + + return err +} + +// TODO(xiaoyuan): consider use different error code for transition error and not-accessible error +func checkErrorCode(t *testing.T, err error, expected int) { + originErr := errors.Cause(err) + tErr, ok := originErr.(*terror.Error) + require.True(t, ok) + sqlErr := terror.ToSQLError(tErr) + require.Equal(t, expected, int(sqlErr.Code)) +} + func TestCreateTableWithModeInfo(t *testing.T) { store, domain := testkit.CreateMockStoreAndDomain(t) de := domain.DDLExecutor() @@ -71,84 +112,36 @@ func TestCreateTableWithModeInfo(t *testing.T) { // For testing insert is not allowed when table is in ModeImport tk.MustGetErrCode("insert into t1_restore values(1)", 8020) - // TODO(xiaoyuan): extract as function and reuse the code - // For testing AlterTable from ModeRestore to ModeImport is not allowed - args := &model.AlterTableModeArgs{ - TableMode: model.TableModeImport, - SchemaID: dbInfo.ID, - TableID: tblInfo.ID, - } - job := &model.Job{ - Version: model.JobVersion2, - SchemaID: dbInfo.ID, - TableID: tblInfo.ID, - Type: model.ActionAlterTableMode, - BinlogInfo: &model.HistoryInfo{}, - InvolvingSchemaInfo: []model.InvolvingSchemaInfo{ - { - Database: dbInfo.Name.L, - Table: tblInfo.Name.L, - }, - }, - } - ctx.SetValue(sessionctx.QueryString, "skip") - err = de.(ddl.ExecutorForTest).DoDDLJobWrapper(ctx, ddl.NewJobWrapperWithArgs(job, args, true)) - // TODO(xiaoyuan): consider use different error code for transition error and not-accessible error - originErr := errors.Cause(err) - tErr, ok := originErr.(*terror.Error) - require.True(t, ok) - sqlErr := terror.ToSQLError(tErr) - require.Equal(t, 8020, int(sqlErr.Code)) + // For testing AlterTable ModeRestore -> ModeImport is not allowed + err = setTableModeTest(ctx, t, store, de.(ddl.ExecutorForTest), dbInfo, tblInfo, model.TableModeImport) + checkErrorCode(t, err, 8020) - // For testing AlterTableMode from ModeRestore to ModeNormal - args = &model.AlterTableModeArgs{ - TableMode: model.TableModeNormal, - SchemaID: dbInfo.ID, - TableID: tblInfo.ID, - } - job = &model.Job{ - Version: model.JobVersion2, - SchemaID: dbInfo.ID, - TableID: tblInfo.ID, - Type: model.ActionAlterTableMode, - BinlogInfo: &model.HistoryInfo{}, - InvolvingSchemaInfo: []model.InvolvingSchemaInfo{ - { - Database: dbInfo.Name.L, - Table: tblInfo.Name.L, - }, - }, - } - ctx.SetValue(sessionctx.QueryString, "skip") - err = de.(ddl.ExecutorForTest).DoDDLJobWrapper(ctx, ddl.NewJobWrapperWithArgs(job, args, true)) - testCheckTableState(t, store, dbInfo, tblInfo, model.StatePublic) - testCheckJobDone(t, store, job.ID, true) - checkTableModeTest(t, store, dbInfo, tblInfo, model.TableModeNormal) - - // For testing AlterTableMode from ModeNormal to ModeRestore - args = &model.AlterTableModeArgs{ - TableMode: model.TableModeImport, - SchemaID: dbInfo.ID, - TableID: tblInfo.ID, - } - job = &model.Job{ - Version: model.JobVersion2, - SchemaID: dbInfo.ID, - TableID: tblInfo.ID, - Type: model.ActionAlterTableMode, - BinlogInfo: &model.HistoryInfo{}, - InvolvingSchemaInfo: []model.InvolvingSchemaInfo{ - { - Database: dbInfo.Name.L, - Table: tblInfo.Name.L, - }, - }, - } - ctx.SetValue(sessionctx.QueryString, "skip") - err = de.(ddl.ExecutorForTest).DoDDLJobWrapper(ctx, ddl.NewJobWrapperWithArgs(job, args, true)) - testCheckTableState(t, store, dbInfo, tblInfo, model.StatePublic) - testCheckJobDone(t, store, job.ID, true) - checkTableModeTest(t, store, dbInfo, tblInfo, model.TableModeImport) + // For testing AlterTableMode ModeRestore -> ModeNormal + err = setTableModeTest(ctx, t, store, de.(ddl.ExecutorForTest), dbInfo, tblInfo, model.TableModeNormal) + require.NoError(t, err) + + // For testing AlterTableMode ModeNormal -> ModeRestore + err = setTableModeTest(ctx, t, store, de.(ddl.ExecutorForTest), dbInfo, tblInfo, model.TableModeRestore) + require.NoError(t, err) - // TODO: batch create tables with ModeRestore test + // For testing batch create tables with info + var tblInfo1, tblInfo2, tblInfo3 *model.TableInfo + tblInfo1, err = getClonedTableInfoFromDomain("test", "t1", domain) + tblInfo1.Name = ast.NewCIStr("t1_1") + tblInfo1.TableMode = model.TableModeNormal + tblInfo2, err = getClonedTableInfoFromDomain("test", "t1", domain) + tblInfo2.Name = ast.NewCIStr("t1_2") + tblInfo2.TableMode = model.TableModeImport + tblInfo3, err = getClonedTableInfoFromDomain("test", "t1", domain) + tblInfo3.Name = ast.NewCIStr("t1_3") + tblInfo3.TableMode = model.TableModeRestore + err = de.BatchCreateTableWithInfo( + ctx, + ast.NewCIStr("test"), + []*model.TableInfo{tblInfo1, tblInfo2, tblInfo3}, + ddl.WithOnExist(ddl.OnExistIgnore), + ) + checkTableModeTest(t, store, dbInfo, tblInfo1, model.TableModeNormal) + checkTableModeTest(t, store, dbInfo, tblInfo2, model.TableModeImport) + checkTableModeTest(t, store, dbInfo, tblInfo3, model.TableModeRestore) } From 8d8b540cad20dc4155983fdf4c8a6a2236363cad Mon Sep 17 00:00:00 2001 From: Xiaoyuan Jin Date: Mon, 10 Feb 2025 02:55:04 +0800 Subject: [PATCH 07/27] add new proper error code --- pkg/ddl/executor.go | 4 +--- pkg/ddl/table_mode.go | 9 ++------- pkg/ddl/table_mode_test.go | 15 ++++++++------- pkg/errno/errcode.go | 2 ++ pkg/errno/errname.go | 2 ++ pkg/infoschema/error.go | 11 ++++------- pkg/planner/core/optimizer.go | 13 ++++++++++--- pkg/planner/optimize.go | 2 +- 8 files changed, 30 insertions(+), 28 deletions(-) diff --git a/pkg/ddl/executor.go b/pkg/ddl/executor.go index 66c2be06bb720..537ff04799f80 100644 --- a/pkg/ddl/executor.go +++ b/pkg/ddl/executor.go @@ -1064,9 +1064,7 @@ func (e *executor) createTableWithInfoJob( if tbInfo.TableMode == model.TableModeRestore { oldTableMode := oldTable.Meta().TableMode if oldTableMode != model.TableModeRestore { - return nil, infoschema.ErrTableModeInvalidTransition.GenWithStackByArgs( - fmt.Sprintf("invalid transition from '%s' to '%s'", oldTableMode, tbInfo.TableMode), - ) + return nil, infoschema.ErrInvalidTableModeConversion.GenWithStackByArgs(oldTableMode, tbInfo.TableMode) } } return nil, nil diff --git a/pkg/ddl/table_mode.go b/pkg/ddl/table_mode.go index 1f9da21541bf9..3d026d4a6ac42 100644 --- a/pkg/ddl/table_mode.go +++ b/pkg/ddl/table_mode.go @@ -15,7 +15,6 @@ package ddl import ( - "fmt" "github.com/pingcap/tidb/pkg/infoschema" "github.com/pingcap/tidb/pkg/meta/model" ) @@ -43,9 +42,7 @@ func onAlterTableMode(jobCtx *jobContext, job *model.Job) (ver int64, err error) } default: job.State = model.JobStateCancelled - err = infoschema.ErrTableModeInvalidTransition.GenWithStackByArgs( - fmt.Sprintf("invalid transition from '%s' to '%s'", tbInfo.TableMode, args.TableMode), - ) + err = infoschema.ErrInvalidTableModeConversion.GenWithStackByArgs(tbInfo.TableMode, args.TableMode, tbInfo.Name.O) } return ver, err @@ -57,9 +54,7 @@ func alterTableMode(tbInfo *model.TableInfo, args *model.AlterTableModeArgs) err if args.TableMode == model.TableModeImport { // only transition from ModeNormal to ModeImport is allowed if tbInfo.TableMode != model.TableModeNormal { - return infoschema.ErrTableModeInvalidTransition.GenWithStackByArgs( - fmt.Sprintf("invalid transition from '%s' to '%s'", tbInfo.TableMode, args.TableMode), - ) + return infoschema.ErrInvalidTableModeConversion.GenWithStackByArgs(tbInfo.TableMode, args.TableMode, tbInfo.Name.O) } } tbInfo.TableMode = args.TableMode diff --git a/pkg/ddl/table_mode_test.go b/pkg/ddl/table_mode_test.go index b45ae706bba39..8c822164b0629 100644 --- a/pkg/ddl/table_mode_test.go +++ b/pkg/ddl/table_mode_test.go @@ -17,6 +17,7 @@ package ddl_test import ( "context" "github.com/pingcap/errors" + "github.com/pingcap/tidb/pkg/errno" "github.com/pingcap/tidb/pkg/kv" "github.com/pingcap/tidb/pkg/parser/terror" "github.com/pingcap/tidb/pkg/sessionctx" @@ -103,18 +104,18 @@ func TestCreateTableWithModeInfo(t *testing.T) { checkTableModeTest(t, store, dbInfo, tblInfo, model.TableModeRestore) // For testing select is not allowed when table is in ModeImport - // TODO(xiaoyuan): currently the error is very ugly like: - // [schema:8020]Table 't1_restore' was locked in %!s(MISSING) by %!v(MISSING) - // this is because infoschema.ErrTableModeRestore is based on mysql.ErrTableLocked, - // how to make it look nice? or shoule I define a new mysql error? - tk.MustGetErrCode("select * from t1_restore", 8020) + tk.MustGetErrCode("select * from t1_restore", errno.ErrProtectedTableMode) // For testing insert is not allowed when table is in ModeImport - tk.MustGetErrCode("insert into t1_restore values(1)", 8020) + tk.MustGetErrCode("insert into t1_restore values(1)", errno.ErrProtectedTableMode) + + // For testing accessing table metadata is allowed when table is in ModeRestore + tk.MustExec("show create table t1_restore") + tk.MustExec("describe t1_restore") // For testing AlterTable ModeRestore -> ModeImport is not allowed err = setTableModeTest(ctx, t, store, de.(ddl.ExecutorForTest), dbInfo, tblInfo, model.TableModeImport) - checkErrorCode(t, err, 8020) + checkErrorCode(t, err, errno.ErrInvalidTableModeConversion) // For testing AlterTableMode ModeRestore -> ModeNormal err = setTableModeTest(ctx, t, store, de.(ddl.ExecutorForTest), dbInfo, tblInfo, model.TableModeNormal) diff --git a/pkg/errno/errcode.go b/pkg/errno/errcode.go index 044eda4acf284..95ea1712702b0 100644 --- a/pkg/errno/errcode.go +++ b/pkg/errno/errcode.go @@ -1136,6 +1136,8 @@ const ( ErrDDLSetting = 8246 ErrIngestFailed = 8247 ErrIngestCheckEnvFailed = 8256 + ErrProtectedTableMode = 8258 + ErrInvalidTableModeConversion = 8259 ErrCannotPauseDDLJob = 8260 ErrCannotResumeDDLJob = 8261 diff --git a/pkg/errno/errname.go b/pkg/errno/errname.go index eedd8c562bf4f..46f998af6d504 100644 --- a/pkg/errno/errname.go +++ b/pkg/errno/errname.go @@ -1079,6 +1079,8 @@ var MySQLErrName = map[uint16]*mysql.ErrMessage{ ErrMemoryExceedForInstance: mysql.Message("Your query has been cancelled due to exceeding the allowed memory limit for the tidb-server instance and this query is currently using the most memory. Please try narrowing your query scope or increase the tidb_server_memory_limit and try again.[conn=%d]", nil), ErrDeleteNotFoundColumn: mysql.Message("Delete can not find column %s for table %s", nil), ErrKeyTooLarge: mysql.Message("key is too large, the size of given key is %d", nil), + ErrProtectedTableMode: mysql.Message("Table %s is in mode %s", nil), + ErrInvalidTableModeConversion: mysql.Message("Invalid mode conversion from %s to %s for table %s", nil), ErrHTTPServiceError: mysql.Message("HTTP request failed with status %s", nil), diff --git a/pkg/infoschema/error.go b/pkg/infoschema/error.go index 0aa359ee0cdc1..847e20a63471f 100644 --- a/pkg/infoschema/error.go +++ b/pkg/infoschema/error.go @@ -108,11 +108,8 @@ var ( ErrResourceGroupSupportDisabled = dbterror.ClassSchema.NewStd(mysql.ErrResourceGroupSupportDisabled) // ErrCheckConstraintDupName returns for duplicate constraint names. ErrCheckConstraintDupName = dbterror.ClassSchema.NewStd(mysql.ErrCheckConstraintDupName) - // TODO(xiaoyuan): what is the proper mysql error I should use for TableMode? - // ErrTableModeImport returns for accessing table in import mode. - ErrTableModeImport = dbterror.ClassSchema.NewStd(mysql.ErrTableLocked) - // ErrTableModeRestore returns for accessing table in restore mode. - ErrTableModeRestore = dbterror.ClassSchema.NewStd(mysql.ErrTableLocked) - // ErrTableModeInvalidTransition returns for invalid TableMode transition. - ErrTableModeInvalidTransition = dbterror.ClassSchema.NewStd(mysql.ErrTableLocked) + // ErrProtectedTableMode returns for accessing table in import/restore mode. + ErrProtectedTableMode = dbterror.ClassSchema.NewStd(mysql.ErrProtectedTableMode) + // ErrInvalidTableModeConversion returns for invalid TableMode conversion. + ErrInvalidTableModeConversion = dbterror.ClassSchema.NewStd(mysql.ErrInvalidTableModeConversion) ) diff --git a/pkg/planner/core/optimizer.go b/pkg/planner/core/optimizer.go index fc64149ae7504..37eec63255dab 100644 --- a/pkg/planner/core/optimizer.go +++ b/pkg/planner/core/optimizer.go @@ -248,7 +248,14 @@ func CheckTableLock(ctx tablelock.TableLockReadContext, is infoschema.InfoSchema } // CheckTableMode checks if the table is accessible by table mode. -func CheckTableMode(is infoschema.InfoSchema, vs []visitInfo) error { +func CheckTableMode(is infoschema.InfoSchema, node ast.Node, vs []visitInfo) error { + // First make exceptions for stmt that only visit table meta; + // For example, `describe ` and `show create table `; + // These exceptions can be simply categorized as `ast.ShowStmt`; + if _, ok := node.(*ast.ShowStmt); ok { + return nil + } + for i := range vs { tb, err := is.TableByName(context.Background(), ast.NewCIStr(vs[i].db), ast.NewCIStr(vs[i].table)) if infoschema.ErrTableNotExists.Equal(err) { @@ -258,9 +265,9 @@ func CheckTableMode(is infoschema.InfoSchema, vs []visitInfo) error { return err } if tb.Meta().TableMode == model.TableModeImport { - return infoschema.ErrTableModeImport.GenWithStackByArgs(tb.Meta().Name.O) + return infoschema.ErrProtectedTableMode.GenWithStackByArgs(tb.Meta().Name.O, model.TableModeImport) } else if tb.Meta().TableMode == model.TableModeRestore { - return infoschema.ErrTableModeRestore.GenWithStackByArgs(tb.Meta().Name.O) + return infoschema.ErrProtectedTableMode.GenWithStackByArgs(tb.Meta().Name.O, model.TableModeRestore) } } return nil diff --git a/pkg/planner/optimize.go b/pkg/planner/optimize.go index 7aa2aba8318c6..7cf70c1951504 100644 --- a/pkg/planner/optimize.go +++ b/pkg/planner/optimize.go @@ -474,7 +474,7 @@ func optimize(ctx context.Context, sctx planctx.PlanContext, node *resolve.NodeW return nil, nil, 0, err } - if err := core.CheckTableMode(is, builder.GetVisitInfo()); err != nil { + if err := core.CheckTableMode(is, node.Node, builder.GetVisitInfo()); err != nil { return nil, nil, 0, err } From 107017bfe99304151845887cbff755cac3482672 Mon Sep 17 00:00:00 2001 From: Xiaoyuan Jin Date: Mon, 10 Feb 2025 03:25:42 +0800 Subject: [PATCH 08/27] fix error message missing param --- pkg/ddl/executor.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/ddl/executor.go b/pkg/ddl/executor.go index 537ff04799f80..dfbc92a32e21b 100644 --- a/pkg/ddl/executor.go +++ b/pkg/ddl/executor.go @@ -5660,12 +5660,12 @@ func (e *executor) AlterTableMode(ctx sessionctx.Context, args *model.AlterTable schema, ok := is.SchemaByID(args.SchemaID) if !ok { - return infoschema.ErrDatabaseNotExists.GenWithStackByArgs() + return infoschema.ErrDatabaseNotExists.GenWithStackByArgs(fmt.Sprintf("SchemaID: %v", args.SchemaID)) } t, ok := is.TableByID(e.ctx, args.TableID) if !ok { - return infoschema.ErrTableNotExists.GenWithStackByArgs() + return infoschema.ErrTableNotExists.GenWithStackByArgs(schema.Name, args.TableID) } job := &model.Job{ From 996e5dd5c1a69e40026861daf34053cbe73415eb Mon Sep 17 00:00:00 2001 From: Xiaoyuan Jin Date: Mon, 10 Feb 2025 03:33:15 +0800 Subject: [PATCH 09/27] remove an idiot todo --- pkg/ddl/executor.go | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/pkg/ddl/executor.go b/pkg/ddl/executor.go index dfbc92a32e21b..e304a4bf7f6bc 100644 --- a/pkg/ddl/executor.go +++ b/pkg/ddl/executor.go @@ -5663,7 +5663,7 @@ func (e *executor) AlterTableMode(ctx sessionctx.Context, args *model.AlterTable return infoschema.ErrDatabaseNotExists.GenWithStackByArgs(fmt.Sprintf("SchemaID: %v", args.SchemaID)) } - t, ok := is.TableByID(e.ctx, args.TableID) + _, ok = is.TableByID(e.ctx, args.TableID) if !ok { return infoschema.ErrTableNotExists.GenWithStackByArgs(schema.Name, args.TableID) } @@ -5675,13 +5675,6 @@ func (e *executor) AlterTableMode(ctx sessionctx.Context, args *model.AlterTable Type: model.ActionAlterTableMode, BinlogInfo: &model.HistoryInfo{}, CDCWriteSource: ctx.GetSessionVars().CDCWriteSource, - // TODO(xiaoyuan): when is this information needed? - InvolvingSchemaInfo: []model.InvolvingSchemaInfo{ - { - Database: schema.Name.L, - Table: t.Meta().Name.L, - }, - }, } err := e.doDDLJob2(ctx, job, args) return errors.Trace(err) From db34e54a70976ed201a1d2bc70949bb5ed6e89ac Mon Sep 17 00:00:00 2001 From: Xiaoyuan Jin Date: Mon, 10 Feb 2025 03:53:44 +0800 Subject: [PATCH 10/27] modify createTable comments about mode consistency problem --- pkg/ddl/executor.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/ddl/executor.go b/pkg/ddl/executor.go index e304a4bf7f6bc..b7b8505fd3066 100644 --- a/pkg/ddl/executor.go +++ b/pkg/ddl/executor.go @@ -1060,13 +1060,16 @@ func (e *executor) createTableWithInfoJob( switch cfg.OnExist { case OnExistIgnore: ctx.GetSessionVars().StmtCtx.AppendNote(err) - // If table exists, changing from Normal/Import to Restore is not allowed. + // If table exists: + // and if target TableMode is ModeRestore, we check if the existing table is consistent if tbInfo.TableMode == model.TableModeRestore { oldTableMode := oldTable.Meta().TableMode if oldTableMode != model.TableModeRestore { + // Indeed this is not a conversion problem but an inconsistency problem. return nil, infoschema.ErrInvalidTableModeConversion.GenWithStackByArgs(oldTableMode, tbInfo.TableMode) } } + // target TableMode will not be ModeImport because ImportInto does not use this function return nil, nil case OnExistReplace: // only CREATE OR REPLACE VIEW is supported at the moment. From 30310fb4572269efd1b5f5fe754a94f6ed7cbd4c Mon Sep 17 00:00:00 2001 From: Xiaoyuan Jin Date: Wed, 12 Feb 2025 01:54:06 +0800 Subject: [PATCH 11/27] typo --- pkg/meta/model/job.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/meta/model/job.go b/pkg/meta/model/job.go index 05e899953ff82..524285948a156 100644 --- a/pkg/meta/model/job.go +++ b/pkg/meta/model/job.go @@ -113,7 +113,7 @@ const ( ActionRemovePartitioning ActionType = 72 ActionAddVectorIndex ActionType = 73 ActionModifyEngineAttribute ActionType = 74 - ActionAlterTableMode ActionType = 75 + ActionAlterTableMode ActionType = 75 ) // ActionMap is the map of DDL ActionType to string. @@ -187,6 +187,7 @@ var ActionMap = map[ActionType]string{ ActionRemovePartitioning: "alter table remove partitioning", ActionAddVectorIndex: "add vector index", ActionModifyEngineAttribute: "modify engine attribute", + ActionAlterTableMode: "alter table mode", // `ActionAlterTableAlterPartition` is removed and will never be used. // Just left a tombstone here for compatibility. From 1bf6658b217dccb29a66700cdec9777aeb8fe837 Mon Sep 17 00:00:00 2001 From: Xiaoyuan Jin Date: Wed, 12 Feb 2025 02:04:49 +0800 Subject: [PATCH 12/27] fix bazel --- pkg/ddl/BUILD.bazel | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/ddl/BUILD.bazel b/pkg/ddl/BUILD.bazel index 2dc23911008ce..82ab3005840a4 100644 --- a/pkg/ddl/BUILD.bazel +++ b/pkg/ddl/BUILD.bazel @@ -66,6 +66,7 @@ go_library( "stat.go", "table.go", "table_lock.go", + "table_mode.go", "ttl.go", ], importpath = "github.com/pingcap/tidb/pkg/ddl", @@ -269,6 +270,7 @@ go_test( "schema_test.go", "sequence_test.go", "stat_test.go", + "table_mode_test.go", "table_modify_test.go", "table_split_test.go", "table_test.go", From 2cc27f67d314350474d9f85f57969ec6b346e5ee Mon Sep 17 00:00:00 2001 From: Xiaoyuan Jin Date: Wed, 12 Feb 2025 02:12:54 +0800 Subject: [PATCH 13/27] fix bdr.go --- pkg/meta/model/bdr.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/meta/model/bdr.go b/pkg/meta/model/bdr.go index 6103c5345c437..4965cc800139a 100644 --- a/pkg/meta/model/bdr.go +++ b/pkg/meta/model/bdr.go @@ -103,6 +103,7 @@ var BDRActionMap = map[DDLBDRType][]ActionType{ ActionRemovePartitioning, ActionAddVectorIndex, ActionModifyEngineAttribute, + ActionAlterTableMode, }, UnmanagementDDL: { ActionCreatePlacementPolicy, From d412aec64fbe4e47250b8fe96d54945b9760e6ef Mon Sep 17 00:00:00 2001 From: Xiaoyuan Jin Date: Wed, 12 Feb 2025 02:21:14 +0800 Subject: [PATCH 14/27] fix variable comment --- pkg/meta/model/job_args.go | 1 + pkg/meta/model/table.go | 28 ++++++++++++++++------------ 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/pkg/meta/model/job_args.go b/pkg/meta/model/job_args.go index 4676d1c651cb8..cc8d1fb1e16ab 100644 --- a/pkg/meta/model/job_args.go +++ b/pkg/meta/model/job_args.go @@ -1083,6 +1083,7 @@ func GetLockTablesArgs(job *Job) (*LockTablesArgs, error) { return getOrDecodeArgs[*LockTablesArgs](&LockTablesArgs{}, job) } +// AlterTableModeArgs is the argument for AlterTableMode. type AlterTableModeArgs struct { TableMode TableModeState SchemaID int64 diff --git a/pkg/meta/model/table.go b/pkg/meta/model/table.go index c247dc1915f2a..0c2c575250417 100644 --- a/pkg/meta/model/table.go +++ b/pkg/meta/model/table.go @@ -650,11 +650,27 @@ const ( TableLockStatePublic ) +// String implements fmt.Stringer interface. +func (t TableLockState) String() string { + switch t { + case TableLockStatePreLock: + return "pre-lock" + case TableLockStatePublic: + return "public" + default: + return "none" + } +} + +// TableModeState is the state for table mode. type TableModeState byte const ( + // TableModeNormal means the table is in normal mode. TableModeNormal TableModeState = iota + // TableModeImport means the table is in import mode. TableModeImport + // TableModeRestore means the table is in restore mode. TableModeRestore ) @@ -672,18 +688,6 @@ func (t TableModeState) String() string { } } -// String implements fmt.Stringer interface. -func (t TableLockState) String() string { - switch t { - case TableLockStatePreLock: - return "pre-lock" - case TableLockStatePublic: - return "public" - default: - return "none" - } -} - // TiFlashReplicaInfo means the flash replica info. type TiFlashReplicaInfo struct { Count uint64 From d9cc5c74a91997b71952863880b249c172b9511f Mon Sep 17 00:00:00 2001 From: Xiaoyuan Jin Date: Wed, 12 Feb 2025 02:25:53 +0800 Subject: [PATCH 15/27] move some tests from table_test.go into table_mode_test.go --- pkg/ddl/table_mode_test.go | 28 ++++++++++++++++++-- pkg/ddl/table_test.go | 54 -------------------------------------- 2 files changed, 26 insertions(+), 56 deletions(-) diff --git a/pkg/ddl/table_mode_test.go b/pkg/ddl/table_mode_test.go index 8c822164b0629..87ef004ba87e8 100644 --- a/pkg/ddl/table_mode_test.go +++ b/pkg/ddl/table_mode_test.go @@ -19,6 +19,7 @@ import ( "github.com/pingcap/errors" "github.com/pingcap/tidb/pkg/errno" "github.com/pingcap/tidb/pkg/kv" + "github.com/pingcap/tidb/pkg/meta" "github.com/pingcap/tidb/pkg/parser/terror" "github.com/pingcap/tidb/pkg/sessionctx" "testing" @@ -39,7 +40,15 @@ func getClonedTableInfoFromDomain(dbName string, tableName string, dom *domain.D return tbl.Meta().Clone(), nil } -func setTableModeTest(ctx sessionctx.Context, t *testing.T, store kv.Storage, de ddl.ExecutorForTest, dbInfo *model.DBInfo, tblInfo *model.TableInfo, mode model.TableModeState) error { +func setTableModeTest( + ctx sessionctx.Context, + t *testing.T, + store kv.Storage, + de ddl.ExecutorForTest, + dbInfo *model.DBInfo, + tblInfo *model.TableInfo, + mode model.TableModeState, +) error { args := &model.AlterTableModeArgs{ TableMode: mode, SchemaID: dbInfo.ID, @@ -62,6 +71,9 @@ func setTableModeTest(ctx sessionctx.Context, t *testing.T, store kv.Storage, de err := de.DoDDLJobWrapper(ctx, ddl.NewJobWrapperWithArgs(job, args, true)) if err == nil { + v := getSchemaVer(t, ctx) + checkHistoryJobArgs(t, ctx, job.ID, &historyJobArgs{ver: v, tbl: tblInfo}) + testCheckTableState(t, store, dbInfo, tblInfo, model.StatePublic) testCheckJobDone(t, store, job.ID, true) checkTableModeTest(t, store, dbInfo, tblInfo, mode) @@ -70,7 +82,19 @@ func setTableModeTest(ctx sessionctx.Context, t *testing.T, store kv.Storage, de return err } -// TODO(xiaoyuan): consider use different error code for transition error and not-accessible error +func checkTableModeTest(t *testing.T, store kv.Storage, dbInfo *model.DBInfo, tblInfo *model.TableInfo, mode model.TableModeState) { + ctx := kv.WithInternalSourceType(context.Background(), kv.InternalTxnDDL) + err := kv.RunInNewTxn(ctx, store, false, func(ctx context.Context, txn kv.Transaction) error { + tt := meta.NewMutator(txn) + info, err := tt.GetTable(dbInfo.ID, tblInfo.ID) + require.NoError(t, err) + require.NotNil(t, info) + require.Equal(t, mode, info.TableMode) + return nil + }) + require.NoError(t, err) +} + func checkErrorCode(t *testing.T, err error, expected int) { originErr := errors.Cause(err) tErr, ok := originErr.(*terror.Error) diff --git a/pkg/ddl/table_test.go b/pkg/ddl/table_test.go index 0e98b740f8fd3..81739556223f8 100644 --- a/pkg/ddl/table_test.go +++ b/pkg/ddl/table_test.go @@ -163,54 +163,6 @@ func checkTableLockedTest(t *testing.T, store kv.Storage, dbInfo *model.DBInfo, require.NoError(t, err) } -func testAlterTableMode( - t *testing.T, - ctx sessionctx.Context, - d ddl.ExecutorForTest, - dbInfo *model.DBInfo, - tblInfo *model.TableInfo, - mode model.TableModeState, -) *model.Job { - args := &model.AlterTableModeArgs{ - TableMode: mode, - SchemaID: dbInfo.ID, - TableID: tblInfo.ID, - } - job := &model.Job{ - Version: model.JobVersion2, - SchemaID: dbInfo.ID, - TableID: tblInfo.ID, - Type: model.ActionAlterTableMode, - BinlogInfo: &model.HistoryInfo{}, - InvolvingSchemaInfo: []model.InvolvingSchemaInfo{ - { - Database: dbInfo.Name.L, - Table: tblInfo.Name.L, - }, - }, - } - ctx.SetValue(sessionctx.QueryString, "skip") - err := d.DoDDLJobWrapper(ctx, ddl.NewJobWrapperWithArgs(job, args, true)) - require.NoError(t, err) - - v := getSchemaVer(t, ctx) - checkHistoryJobArgs(t, ctx, job.ID, &historyJobArgs{ver: v, tbl: tblInfo}) - return job -} - -func checkTableModeTest(t *testing.T, store kv.Storage, dbInfo *model.DBInfo, tblInfo *model.TableInfo, mode model.TableModeState) { - ctx := kv.WithInternalSourceType(context.Background(), kv.InternalTxnDDL) - err := kv.RunInNewTxn(ctx, store, false, func(ctx context.Context, txn kv.Transaction) error { - tt := meta.NewMutator(txn) - info, err := tt.GetTable(dbInfo.ID, tblInfo.ID) - require.NoError(t, err) - require.NotNil(t, info) - require.Equal(t, mode, info.TableMode) - return nil - }) - require.NoError(t, err) -} - func testTruncateTable(t *testing.T, ctx sessionctx.Context, store kv.Storage, d ddl.ExecutorForTest, dbInfo *model.DBInfo, tblInfo *model.TableInfo) *model.Job { genIDs, err := genGlobalIDs(store, 1) require.NoError(t, err) @@ -318,12 +270,6 @@ func TestTable(t *testing.T) { testCheckTableState(t, store, dbInfo1, tblInfo, model.StatePublic) testCheckJobDone(t, store, job.ID, true) - // for alter table mode - job = testAlterTableMode(t, ctx, de, dbInfo1, tblInfo, model.TableModeImport) - testCheckTableState(t, store, dbInfo1, tblInfo, model.StatePublic) - testCheckJobDone(t, store, job.ID, true) - checkTableModeTest(t, store, dbInfo1, tblInfo, model.TableModeImport) - job = testLockTable(t, ctx, de, d.GetID(), dbInfo1.ID, dbInfo1.Name, tblInfo, ast.TableLockWrite) testCheckTableState(t, store, dbInfo1, tblInfo, model.StatePublic) testCheckJobDone(t, store, job.ID, true) From 457331d119b6023541e3ac743dfc53277052fba0 Mon Sep 17 00:00:00 2001 From: Xiaoyuan Jin Date: Wed, 12 Feb 2025 02:27:17 +0800 Subject: [PATCH 16/27] fix ineffassign --- pkg/ddl/table_mode.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/ddl/table_mode.go b/pkg/ddl/table_mode.go index 3d026d4a6ac42..d1582e53a9cc7 100644 --- a/pkg/ddl/table_mode.go +++ b/pkg/ddl/table_mode.go @@ -21,7 +21,8 @@ import ( // onAlterTableMode should only be called by alterTableMode, will call updateVersionAndTableInfo func onAlterTableMode(jobCtx *jobContext, job *model.Job) (ver int64, err error) { - args, err := model.GetAlterTableModeArgs(job) + var args *model.AlterTableModeArgs + args, err = model.GetAlterTableModeArgs(job) var tbInfo *model.TableInfo metaMut := jobCtx.metaMut tbInfo, err = GetTableInfoAndCancelFaultJob(metaMut, job, job.SchemaID) From d4d4a1f250e036af615cf8eae26490f632c57952 Mon Sep 17 00:00:00 2001 From: Xiaoyuan Jin Date: Wed, 12 Feb 2025 02:31:31 +0800 Subject: [PATCH 17/27] fix ineffassign --- pkg/ddl/table_mode.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/pkg/ddl/table_mode.go b/pkg/ddl/table_mode.go index d1582e53a9cc7..25a513a2414bf 100644 --- a/pkg/ddl/table_mode.go +++ b/pkg/ddl/table_mode.go @@ -21,8 +21,11 @@ import ( // onAlterTableMode should only be called by alterTableMode, will call updateVersionAndTableInfo func onAlterTableMode(jobCtx *jobContext, job *model.Job) (ver int64, err error) { - var args *model.AlterTableModeArgs - args, err = model.GetAlterTableModeArgs(job) + args, err := model.GetAlterTableModeArgs(job) + if err != nil { + return ver, err + } + var tbInfo *model.TableInfo metaMut := jobCtx.metaMut tbInfo, err = GetTableInfoAndCancelFaultJob(metaMut, job, job.SchemaID) From 68f15045fd6f48540ef50be2fba21567c6d00116 Mon Sep 17 00:00:00 2001 From: Xiaoyuan Jin Date: Wed, 12 Feb 2025 02:45:31 +0800 Subject: [PATCH 18/27] better error info and comment, fix several ineffassign --- pkg/ddl/executor.go | 8 ++++---- pkg/ddl/table_mode.go | 4 ++-- pkg/ddl/table_mode_test.go | 26 ++++++++++++++------------ pkg/errno/errcode.go | 2 +- pkg/errno/errname.go | 2 +- pkg/infoschema/error.go | 4 ++-- 6 files changed, 24 insertions(+), 22 deletions(-) diff --git a/pkg/ddl/executor.go b/pkg/ddl/executor.go index e072dcac506a9..5b9e51afd44e1 100644 --- a/pkg/ddl/executor.go +++ b/pkg/ddl/executor.go @@ -1060,18 +1060,18 @@ func (e *executor) createTableWithInfoJob( if oldTable, err := is.TableByName(e.ctx, schema.Name, tbInfo.Name); err == nil { err = infoschema.ErrTableExists.GenWithStackByArgs(ast.Ident{Schema: schema.Name, Name: tbInfo.Name}) switch cfg.OnExist { + // If table not exists, we will set tableMode as specified in tbInfo. If table exists: case OnExistIgnore: ctx.GetSessionVars().StmtCtx.AppendNote(err) - // If table exists: - // and if target TableMode is ModeRestore, we check if the existing table is consistent + // If table exists, and if target TableMode is ModeRestore, we check if the existing table is consistent if tbInfo.TableMode == model.TableModeRestore { oldTableMode := oldTable.Meta().TableMode if oldTableMode != model.TableModeRestore { // Indeed this is not a conversion problem but an inconsistency problem. - return nil, infoschema.ErrInvalidTableModeConversion.GenWithStackByArgs(oldTableMode, tbInfo.TableMode) + return nil, infoschema.ErrInvalidTableModeSet.GenWithStackByArgs(oldTableMode, tbInfo.TableMode) } } - // target TableMode will not be ModeImport because ImportInto does not use this function + // Currently, target TableMode will NEVER be ModeImport because ImportInto does not use this function return nil, nil case OnExistReplace: // only CREATE OR REPLACE VIEW is supported at the moment. diff --git a/pkg/ddl/table_mode.go b/pkg/ddl/table_mode.go index 25a513a2414bf..0d024f01f44f3 100644 --- a/pkg/ddl/table_mode.go +++ b/pkg/ddl/table_mode.go @@ -46,7 +46,7 @@ func onAlterTableMode(jobCtx *jobContext, job *model.Job) (ver int64, err error) } default: job.State = model.JobStateCancelled - err = infoschema.ErrInvalidTableModeConversion.GenWithStackByArgs(tbInfo.TableMode, args.TableMode, tbInfo.Name.O) + err = infoschema.ErrInvalidTableModeSet.GenWithStackByArgs(tbInfo.TableMode, args.TableMode, tbInfo.Name.O) } return ver, err @@ -58,7 +58,7 @@ func alterTableMode(tbInfo *model.TableInfo, args *model.AlterTableModeArgs) err if args.TableMode == model.TableModeImport { // only transition from ModeNormal to ModeImport is allowed if tbInfo.TableMode != model.TableModeNormal { - return infoschema.ErrInvalidTableModeConversion.GenWithStackByArgs(tbInfo.TableMode, args.TableMode, tbInfo.Name.O) + return infoschema.ErrInvalidTableModeSet.GenWithStackByArgs(tbInfo.TableMode, args.TableMode, tbInfo.Name.O) } } tbInfo.TableMode = args.TableMode diff --git a/pkg/ddl/table_mode_test.go b/pkg/ddl/table_mode_test.go index 87ef004ba87e8..c4fef068023c6 100644 --- a/pkg/ddl/table_mode_test.go +++ b/pkg/ddl/table_mode_test.go @@ -32,12 +32,15 @@ import ( "github.com/stretchr/testify/require" ) -func getClonedTableInfoFromDomain(dbName string, tableName string, dom *domain.Domain) (*model.TableInfo, error) { +func getClonedTableInfoFromDomain( + t *testing.T, + dbName string, + tableName string, + dom *domain.Domain, +) *model.TableInfo { tbl, err := dom.InfoSchema().TableByName(context.Background(), ast.NewCIStr(dbName), ast.NewCIStr(tableName)) - if err != nil { - return nil, err - } - return tbl.Meta().Clone(), nil + require.NoError(t, err) + return tbl.Meta().Clone() } func setTableModeTest( @@ -115,13 +118,12 @@ func TestCreateTableWithModeInfo(t *testing.T) { tk.MustExec("create table t1(id int)") // get cloned table info for creating new table t1_restore - tblInfo, err := getClonedTableInfoFromDomain("test", "t1", domain) - require.NoError(t, err) + tblInfo := getClonedTableInfoFromDomain(t, "test", "t1", domain) // For testing create table as ModeRestore tblInfo.Name = ast.NewCIStr("t1_restore") tblInfo.TableMode = model.TableModeRestore - err = de.CreateTableWithInfo(tk.Session(), ast.NewCIStr("test"), tblInfo, nil, ddl.WithOnExist(ddl.OnExistIgnore)) + err := de.CreateTableWithInfo(tk.Session(), ast.NewCIStr("test"), tblInfo, nil, ddl.WithOnExist(ddl.OnExistIgnore)) require.NoError(t, err) dbInfo, ok := domain.InfoSchema().SchemaByName(ast.NewCIStr("test")) require.True(t, ok) @@ -139,7 +141,7 @@ func TestCreateTableWithModeInfo(t *testing.T) { // For testing AlterTable ModeRestore -> ModeImport is not allowed err = setTableModeTest(ctx, t, store, de.(ddl.ExecutorForTest), dbInfo, tblInfo, model.TableModeImport) - checkErrorCode(t, err, errno.ErrInvalidTableModeConversion) + checkErrorCode(t, err, errno.ErrInvalidTableModeSet) // For testing AlterTableMode ModeRestore -> ModeNormal err = setTableModeTest(ctx, t, store, de.(ddl.ExecutorForTest), dbInfo, tblInfo, model.TableModeNormal) @@ -151,13 +153,13 @@ func TestCreateTableWithModeInfo(t *testing.T) { // For testing batch create tables with info var tblInfo1, tblInfo2, tblInfo3 *model.TableInfo - tblInfo1, err = getClonedTableInfoFromDomain("test", "t1", domain) + tblInfo1 = getClonedTableInfoFromDomain(t, "test", "t1", domain) tblInfo1.Name = ast.NewCIStr("t1_1") tblInfo1.TableMode = model.TableModeNormal - tblInfo2, err = getClonedTableInfoFromDomain("test", "t1", domain) + tblInfo2 = getClonedTableInfoFromDomain(t, "test", "t1", domain) tblInfo2.Name = ast.NewCIStr("t1_2") tblInfo2.TableMode = model.TableModeImport - tblInfo3, err = getClonedTableInfoFromDomain("test", "t1", domain) + tblInfo3 = getClonedTableInfoFromDomain(t, "test", "t1", domain) tblInfo3.Name = ast.NewCIStr("t1_3") tblInfo3.TableMode = model.TableModeRestore err = de.BatchCreateTableWithInfo( diff --git a/pkg/errno/errcode.go b/pkg/errno/errcode.go index 4bd96a7219a36..2622729a3faf6 100644 --- a/pkg/errno/errcode.go +++ b/pkg/errno/errcode.go @@ -1138,7 +1138,7 @@ const ( ErrIngestFailed = 8247 ErrIngestCheckEnvFailed = 8256 ErrProtectedTableMode = 8258 - ErrInvalidTableModeConversion = 8259 + ErrInvalidTableModeSet = 8259 ErrCannotPauseDDLJob = 8260 ErrCannotResumeDDLJob = 8261 diff --git a/pkg/errno/errname.go b/pkg/errno/errname.go index 2055c5698d7bb..6b6aaf5065dd7 100644 --- a/pkg/errno/errname.go +++ b/pkg/errno/errname.go @@ -1081,7 +1081,7 @@ var MySQLErrName = map[uint16]*mysql.ErrMessage{ ErrDeleteNotFoundColumn: mysql.Message("Delete can not find column %s for table %s", nil), ErrKeyTooLarge: mysql.Message("key is too large, the size of given key is %d", nil), ErrProtectedTableMode: mysql.Message("Table %s is in mode %s", nil), - ErrInvalidTableModeConversion: mysql.Message("Invalid mode conversion from %s to %s for table %s", nil), + ErrInvalidTableModeSet: mysql.Message("Invalid mode set from (or by default) %s to %s for table %s", nil), ErrHTTPServiceError: mysql.Message("HTTP request failed with status %s", nil), diff --git a/pkg/infoschema/error.go b/pkg/infoschema/error.go index 847e20a63471f..807f2ea694154 100644 --- a/pkg/infoschema/error.go +++ b/pkg/infoschema/error.go @@ -110,6 +110,6 @@ var ( ErrCheckConstraintDupName = dbterror.ClassSchema.NewStd(mysql.ErrCheckConstraintDupName) // ErrProtectedTableMode returns for accessing table in import/restore mode. ErrProtectedTableMode = dbterror.ClassSchema.NewStd(mysql.ErrProtectedTableMode) - // ErrInvalidTableModeConversion returns for invalid TableMode conversion. - ErrInvalidTableModeConversion = dbterror.ClassSchema.NewStd(mysql.ErrInvalidTableModeConversion) + // ErrInvalidTableModeSet returns for invalid TableMode conversion. + ErrInvalidTableModeSet = dbterror.ClassSchema.NewStd(mysql.ErrInvalidTableModeSet) ) From 801ad5872b0f8875c14f61f15db903fb509fc010 Mon Sep 17 00:00:00 2001 From: Xiaoyuan Jin Date: Wed, 12 Feb 2025 02:52:50 +0800 Subject: [PATCH 19/27] fix ineffassign and go import --- pkg/ddl/table_mode.go | 7 +++++-- pkg/ddl/table_mode_test.go | 13 +++++++------ 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/pkg/ddl/table_mode.go b/pkg/ddl/table_mode.go index 0d024f01f44f3..97660dfb222e1 100644 --- a/pkg/ddl/table_mode.go +++ b/pkg/ddl/table_mode.go @@ -42,7 +42,7 @@ func onAlterTableMode(jobCtx *jobContext, job *model.Job) (ver int64, err error) } else { // update table info and schema version ver, err = updateVersionAndTableInfo(jobCtx, job, tbInfo, true) - job.FinishTableJob(model.JobStateDone, model.StatePublic, ver, tbInfo) // TODO: change of schema state + job.FinishTableJob(model.JobStateDone, model.StatePublic, ver, tbInfo) } default: job.State = model.JobStateCancelled @@ -54,7 +54,10 @@ func onAlterTableMode(jobCtx *jobContext, job *model.Job) (ver int64, err error) // alterTableMode first checks if the change is valid and changes table mode to target mode func alterTableMode(tbInfo *model.TableInfo, args *model.AlterTableModeArgs) error { - // currently we can assume args.TableMode will not be model.TableModeRestore + // Currently we can assume args.TableMode will NEVER be model.TableModeRestore. + // Because BR will NOT use this function to set a table into ModeRestore, + // instead BR will use (batch)CreateTableWithInfo. + if args.TableMode == model.TableModeImport { // only transition from ModeNormal to ModeImport is allowed if tbInfo.TableMode != model.TableModeNormal { diff --git a/pkg/ddl/table_mode_test.go b/pkg/ddl/table_mode_test.go index c4fef068023c6..c7a0ff6c42afb 100644 --- a/pkg/ddl/table_mode_test.go +++ b/pkg/ddl/table_mode_test.go @@ -16,18 +16,18 @@ package ddl_test import ( "context" - "github.com/pingcap/errors" - "github.com/pingcap/tidb/pkg/errno" - "github.com/pingcap/tidb/pkg/kv" - "github.com/pingcap/tidb/pkg/meta" - "github.com/pingcap/tidb/pkg/parser/terror" - "github.com/pingcap/tidb/pkg/sessionctx" "testing" + "github.com/pingcap/errors" "github.com/pingcap/tidb/pkg/ddl" "github.com/pingcap/tidb/pkg/domain" + "github.com/pingcap/tidb/pkg/errno" + "github.com/pingcap/tidb/pkg/kv" + "github.com/pingcap/tidb/pkg/meta" "github.com/pingcap/tidb/pkg/meta/model" "github.com/pingcap/tidb/pkg/parser/ast" + "github.com/pingcap/tidb/pkg/parser/terror" + "github.com/pingcap/tidb/pkg/sessionctx" "github.com/pingcap/tidb/pkg/testkit" "github.com/stretchr/testify/require" ) @@ -168,6 +168,7 @@ func TestCreateTableWithModeInfo(t *testing.T) { []*model.TableInfo{tblInfo1, tblInfo2, tblInfo3}, ddl.WithOnExist(ddl.OnExistIgnore), ) + require.NoError(t, err) checkTableModeTest(t, store, dbInfo, tblInfo1, model.TableModeNormal) checkTableModeTest(t, store, dbInfo, tblInfo2, model.TableModeImport) checkTableModeTest(t, store, dbInfo, tblInfo3, model.TableModeRestore) From fb868b31685c23ef63f3e0b449c7253c472b02cd Mon Sep 17 00:00:00 2001 From: Xiaoyuan Jin Date: Wed, 12 Feb 2025 20:40:08 +0800 Subject: [PATCH 20/27] fix meta test --- pkg/meta/meta_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/meta/meta_test.go b/pkg/meta/meta_test.go index 6b2c1c0068697..c6aac82ad3b20 100644 --- a/pkg/meta/meta_test.go +++ b/pkg/meta/meta_test.go @@ -738,7 +738,7 @@ func TestIsTableInfoMustLoadSubStringsOrder(t *testing.T) { tableInfo := &model.TableInfo{} b, err := json.Marshal(tableInfo) require.NoError(t, err) - expect := `{"id":0,"name":{"O":"","L":""},"charset":"","collate":"","cols":null,"index_info":null,"constraint_info":null,"fk_info":null,"state":0,"pk_is_handle":false,"is_common_handle":false,"common_handle_version":0,"comment":"","auto_inc_id":0,"auto_id_cache":0,"auto_rand_id":0,"max_col_id":0,"max_idx_id":0,"max_fk_id":0,"max_cst_id":0,"update_timestamp":0,"ShardRowIDBits":0,"max_shard_row_id_bits":0,"auto_random_bits":0,"auto_random_range_bits":0,"pre_split_regions":0,"partition":null,"compression":"","view":null,"sequence":null,"Lock":null,"version":0,"tiflash_replica":null,"is_columnar":false,"temp_table_type":0,"cache_table_status":0,"policy_ref_info":null,"stats_options":null,"exchange_partition_info":null,"ttl_info":null,"revision":0}` + expect := `{"id":0,"name":{"O":"","L":""},"charset":"","collate":"","cols":null,"index_info":null,"constraint_info":null,"fk_info":null,"state":0,"pk_is_handle":false,"is_common_handle":false,"common_handle_version":0,"comment":"","auto_inc_id":0,"auto_id_cache":0,"auto_rand_id":0,"max_col_id":0,"max_idx_id":0,"max_fk_id":0,"max_cst_id":0,"update_timestamp":0,"ShardRowIDBits":0,"max_shard_row_id_bits":0,"auto_random_bits":0,"auto_random_range_bits":0,"pre_split_regions":0,"partition":null,"compression":"","view":null,"sequence":null,"Lock":null,"version":0,"tiflash_replica":null,"is_columnar":false,"temp_table_type":0,"cache_table_status":0,"policy_ref_info":null,"stats_options":null,"exchange_partition_info":null,"ttl_info":null,"revision":0,"table_mode":0}` require.Equal(t, string(b), expect) } From 591079824092d2e94c47489544605e558a2875da Mon Sep 17 00:00:00 2001 From: Xiaoyuan Jin Date: Thu, 13 Feb 2025 02:10:24 +0800 Subject: [PATCH 21/27] fix schemainfo integration test result (column 26->27) --- tests/integrationtest/r/infoschema/infoschema.result | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integrationtest/r/infoschema/infoschema.result b/tests/integrationtest/r/infoschema/infoschema.result index c2e4ad2c96155..7fc320d26cea6 100644 --- a/tests/integrationtest/r/infoschema/infoschema.result +++ b/tests/integrationtest/r/infoschema/infoschema.result @@ -54,11 +54,11 @@ count(*) 3 desc format='brief' SELECT count(*) FROM information_schema.TABLES WHERE (TABLE_SCHEMA= 'mysql' or TABLE_SCHEMA = 'test') and (TABLE_NAME = 't1' or TABLE_NAME = 't2'); id estRows task access object operator info -HashAgg 1.00 root funcs:count(1)->Column#26 +HashAgg 1.00 root funcs:count(1)->Column#27 └─MemTableScan 10000.00 root table:TABLES table_name:["t1","t2"], table_schema:["mysql","test"] desc format='brief' SELECT count(*) FROM information_schema.TABLES WHERE TABLE_SCHEMA in ('mysql', 'test') and TABLE_NAME in ('t1', 't2'); id estRows task access object operator info -HashAgg 1.00 root funcs:count(1)->Column#26 +HashAgg 1.00 root funcs:count(1)->Column#27 └─MemTableScan 10000.00 root table:TABLES table_name:["t1","t2"], table_schema:["mysql","test"] SELECT count(*) FROM information_schema.TABLES WHERE TABLE_NAME in ('t1', 't2') and TABLE_SCHEMA = 'mysql'; count(*) From 639c92f4fd26ed900ab89ce94c6bf3975f09e9f9 Mon Sep 17 00:00:00 2001 From: Xiaoyuan Jin Date: Thu, 13 Feb 2025 03:13:38 +0800 Subject: [PATCH 22/27] add concurrency test --- pkg/ddl/table_mode.go | 9 ++++ pkg/ddl/table_mode_test.go | 101 ++++++++++++++++++++++++++++++++++++- 2 files changed, 109 insertions(+), 1 deletion(-) diff --git a/pkg/ddl/table_mode.go b/pkg/ddl/table_mode.go index 97660dfb222e1..22b1d3fd3d0ec 100644 --- a/pkg/ddl/table_mode.go +++ b/pkg/ddl/table_mode.go @@ -64,6 +64,15 @@ func alterTableMode(tbInfo *model.TableInfo, args *model.AlterTableModeArgs) err return infoschema.ErrInvalidTableModeSet.GenWithStackByArgs(tbInfo.TableMode, args.TableMode, tbInfo.Name.O) } } + + if args.TableMode == model.TableModeRestore { + // Currently this branch will never be executed except for testing. + // only transition from ModeNormal to ModeRestore is allowed + if tbInfo.TableMode != model.TableModeNormal { + return infoschema.ErrInvalidTableModeSet.GenWithStackByArgs(tbInfo.TableMode, args.TableMode, tbInfo.Name.O) + } + } + tbInfo.TableMode = args.TableMode return nil } diff --git a/pkg/ddl/table_mode_test.go b/pkg/ddl/table_mode_test.go index c7a0ff6c42afb..55194c01a22e3 100644 --- a/pkg/ddl/table_mode_test.go +++ b/pkg/ddl/table_mode_test.go @@ -16,6 +16,7 @@ package ddl_test import ( "context" + "sync" "testing" "github.com/pingcap/errors" @@ -106,7 +107,7 @@ func checkErrorCode(t *testing.T, err error, expected int) { require.Equal(t, expected, int(sqlErr.Code)) } -func TestCreateTableWithModeInfo(t *testing.T) { +func TestTableModeBasic(t *testing.T) { store, domain := testkit.CreateMockStoreAndDomain(t) de := domain.DDLExecutor() tk := testkit.NewTestKit(t, store) @@ -173,3 +174,101 @@ func TestCreateTableWithModeInfo(t *testing.T) { checkTableModeTest(t, store, dbInfo, tblInfo2, model.TableModeImport) checkTableModeTest(t, store, dbInfo, tblInfo3, model.TableModeRestore) } + +func TestTableModeConcurrent(t *testing.T) { + // Setup a fresh test environment. + store, domain := testkit.CreateMockStoreAndDomain(t) + de := domain.DDLExecutor() + tk := testkit.NewTestKit(t, store) + ctx := testkit.NewTestKit(t, store).Session() + + tk.MustExec("use test") + tk.MustExec("create table t1(id int)") + dbInfo, ok := domain.InfoSchema().SchemaByName(ast.NewCIStr("test")) + require.True(t, ok) + + // Concurrency test1: concurrently alter t1 to ModeImport, expecting one success, one failure. + t1Infos := []*model.TableInfo{ + getClonedTableInfoFromDomain(t, "test", "t1", domain), + getClonedTableInfoFromDomain(t, "test", "t1", domain), + } + var wg sync.WaitGroup + wg.Add(len(t1Infos)) + errs := make(chan error, len(t1Infos)) + for _, info := range t1Infos { + go func(info *model.TableInfo) { + defer wg.Done() + errs <- setTableModeTest(ctx, t, store, de.(ddl.ExecutorForTest), dbInfo, info, model.TableModeImport) + }(info) + } + wg.Wait() + close(errs) + var successCount int + var failedErr error + for e := range errs { + if e == nil { + successCount++ + } else { + failedErr = e + } + } + require.Equal(t, 1, successCount) + require.NotNil(t, failedErr) + checkErrorCode(t, failedErr, errno.ErrInvalidTableModeSet) + checkTableModeTest(t, store, dbInfo, t1Infos[0], model.TableModeImport) + + // Concurrency test2: concurrently alter t1 to ModeNormal, expecting both success. + t1NormalInfos := []*model.TableInfo{ + getClonedTableInfoFromDomain(t, "test", "t1", domain), + getClonedTableInfoFromDomain(t, "test", "t1", domain), + } + var wg2 sync.WaitGroup + wg2.Add(len(t1NormalInfos)) + errs2 := make(chan error, len(t1NormalInfos)) + for _, info := range t1NormalInfos { + go func(info *model.TableInfo) { + defer wg2.Done() + errs2 <- setTableModeTest(ctx, t, store, de.(ddl.ExecutorForTest), dbInfo, info, model.TableModeNormal) + }(info) + } + wg2.Wait() + close(errs2) + for e := range errs2 { + require.NoError(t, e) + } + // Verify t1 is now in ModeNormal. + checkTableModeTest(t, store, dbInfo, t1NormalInfos[0], model.TableModeNormal) + + // Concurrency test3: concurrently alter t1 to ModeRestore and ModeImport, expecting one success, one failure. + modes := []model.TableModeState{ + model.TableModeRestore, // allowed transition from ModeNormal + model.TableModeImport, // disallowed transition from ModeNormal + } + clones := make([]*model.TableInfo, len(modes)) + for i, _ := range modes { + clones[i] = getClonedTableInfoFromDomain(t, "test", "t1", domain) + } + var wg3 sync.WaitGroup + wg3.Add(len(modes)) + errs3 := make(chan error, len(modes)) + for i, mode := range modes { + go func(clone *model.TableInfo, m model.TableModeState) { + defer wg3.Done() + errs3 <- setTableModeTest(ctx, t, store, de.(ddl.ExecutorForTest), dbInfo, clone, m) + }(clones[i], mode) + } + wg3.Wait() + close(errs3) + var successCount3 int + var failedErr3 error + for e := range errs3 { + if e == nil { + successCount3++ + } else { + failedErr3 = e + } + } + require.Equal(t, 1, successCount3) + require.NotNil(t, failedErr3) + checkErrorCode(t, failedErr3, errno.ErrInvalidTableModeSet) +} From ffa0f25b38248e62e172796c9932cf068b5bfda9 Mon Sep 17 00:00:00 2001 From: Xiaoyuan Date: Thu, 13 Feb 2025 09:30:57 +0800 Subject: [PATCH 23/27] fix format --- pkg/ddl/table_mode_test.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/pkg/ddl/table_mode_test.go b/pkg/ddl/table_mode_test.go index 55194c01a22e3..f7f5f4115a400 100644 --- a/pkg/ddl/table_mode_test.go +++ b/pkg/ddl/table_mode_test.go @@ -176,7 +176,6 @@ func TestTableModeBasic(t *testing.T) { } func TestTableModeConcurrent(t *testing.T) { - // Setup a fresh test environment. store, domain := testkit.CreateMockStoreAndDomain(t) de := domain.DDLExecutor() tk := testkit.NewTestKit(t, store) @@ -236,16 +235,15 @@ func TestTableModeConcurrent(t *testing.T) { for e := range errs2 { require.NoError(t, e) } - // Verify t1 is now in ModeNormal. checkTableModeTest(t, store, dbInfo, t1NormalInfos[0], model.TableModeNormal) // Concurrency test3: concurrently alter t1 to ModeRestore and ModeImport, expecting one success, one failure. modes := []model.TableModeState{ - model.TableModeRestore, // allowed transition from ModeNormal - model.TableModeImport, // disallowed transition from ModeNormal + model.TableModeRestore, + model.TableModeImport, } clones := make([]*model.TableInfo, len(modes)) - for i, _ := range modes { + for i := range modes { clones[i] = getClonedTableInfoFromDomain(t, "test", "t1", domain) } var wg3 sync.WaitGroup From 564ee921ce9c7ba11e4ca47cc5d34b7f40f785eb Mon Sep 17 00:00:00 2001 From: Xiaoyuan Jin Date: Thu, 13 Feb 2025 18:20:52 +0800 Subject: [PATCH 24/27] fix integration test result related to infoschema --- tests/integrationtest/r/planner/core/integration.result | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integrationtest/r/planner/core/integration.result b/tests/integrationtest/r/planner/core/integration.result index f090176e415b0..c5b9c8f70388c 100644 --- a/tests/integrationtest/r/planner/core/integration.result +++ b/tests/integrationtest/r/planner/core/integration.result @@ -3595,9 +3595,9 @@ Projection_16 10000.00 root planner__core__integration.t1.a └─TableFullScan_24 20000000.00 cop[tikv] table:two keep order:false, stats:pseudo explain select rank() over (partition by table_name) from information_schema.tables; id estRows task access object operator info -Projection_7 10000.00 root Column#27->Column#28 +Projection_7 10000.00 root Column#28->Column#29 └─Shuffle_11 10000.00 root execution info: concurrency:5, data sources:[MemTableScan_9] - └─Window_8 10000.00 root rank()->Column#27 over(partition by Column#3) + └─Window_8 10000.00 root rank()->Column#28 over(partition by Column#3) └─Sort_10 10000.00 root Column#3 └─ShuffleReceiver_12 10000.00 root └─MemTableScan_9 10000.00 root table:TABLES From 68fd4e0d3a91af14bdea3b22e6831e7edde8c616 Mon Sep 17 00:00:00 2001 From: Xiaoyuan Jin Date: Thu, 13 Feb 2025 22:19:48 +0800 Subject: [PATCH 25/27] update errors.toml --- errors.toml | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/errors.toml b/errors.toml index cab50305f90be..2553e73407b1e 100644 --- a/errors.toml +++ b/errors.toml @@ -2946,6 +2946,16 @@ error = ''' Cannot set resource group for a role ''' +["schema:8258"] +error = ''' +Table %s is in mode %s +''' + +["schema:8259"] +error = ''' +Invalid mode set from (or by default) %s to %s for table %s +''' + ["server:1040"] error = ''' Too many connections From 4472686f821bd91af25e9d3a44e0d36702ce60e1 Mon Sep 17 00:00:00 2001 From: Xiaoyuan Date: Fri, 14 Feb 2025 09:23:55 +0800 Subject: [PATCH 26/27] remove todo --- pkg/ddl/executor.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/ddl/executor.go b/pkg/ddl/executor.go index 5b9e51afd44e1..9bfa1fbc134ed 100644 --- a/pkg/ddl/executor.go +++ b/pkg/ddl/executor.go @@ -1215,7 +1215,6 @@ func (e *executor) CreateTableWithInfo( if err != nil { // table exists, but if_not_exists flags is true, so we ignore this error. if c.OnExist == OnExistIgnore && infoschema.ErrTableExists.Equal(err) { - // TODO(xiaoyuan): I think this branch will never be reached, please see e.createTableWithInfoJob ctx.GetSessionVars().StmtCtx.AppendNote(err) err = nil } From 572e3f8a372f55e599c9dd35aa0474a2f0c974fa Mon Sep 17 00:00:00 2001 From: Xiaoyuan Jin Date: Thu, 6 Mar 2025 02:41:34 +0800 Subject: [PATCH 27/27] Copyright typo --- pkg/ddl/table_mode.go | 2 +- pkg/ddl/table_mode_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/ddl/table_mode.go b/pkg/ddl/table_mode.go index 22b1d3fd3d0ec..79642f5432894 100644 --- a/pkg/ddl/table_mode.go +++ b/pkg/ddl/table_mode.go @@ -1,4 +1,4 @@ -// Copyright 2019 PingCAP, Inc. +// Copyright 2025 PingCAP, Inc. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. diff --git a/pkg/ddl/table_mode_test.go b/pkg/ddl/table_mode_test.go index f7f5f4115a400..7f16f829a4159 100644 --- a/pkg/ddl/table_mode_test.go +++ b/pkg/ddl/table_mode_test.go @@ -1,4 +1,4 @@ -// Copyright 2019 PingCAP, Inc. +// Copyright 2025 PingCAP, Inc. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License.