From 9fc40949bf2d3a97a8911a53ad9ba1956e7138c3 Mon Sep 17 00:00:00 2001 From: lance6716 Date: Mon, 27 Jun 2022 11:43:32 +0800 Subject: [PATCH 1/4] ddl: DROP TABLE/VIEW/SEQUENCE now use XXXStmt as parameter Signed-off-by: lance6716 --- ddl/ddl.go | 6 +- ddl/ddl_api.go | 232 +++++++++++++++++++++++++++++++----------------- executor/ddl.go | 108 +--------------------- 3 files changed, 157 insertions(+), 189 deletions(-) diff --git a/ddl/ddl.go b/ddl/ddl.go index e77a9da01e4a9..f0a48cee3058a 100644 --- a/ddl/ddl.go +++ b/ddl/ddl.go @@ -106,9 +106,9 @@ type DDL interface { DropSchema(ctx sessionctx.Context, schema model.CIStr) error CreateTable(ctx sessionctx.Context, stmt *ast.CreateTableStmt) error CreateView(ctx sessionctx.Context, stmt *ast.CreateViewStmt) error - DropTable(ctx sessionctx.Context, tableIdent ast.Ident) (err error) + DropTable(ctx sessionctx.Context, stmt *ast.DropTableStmt) (err error) RecoverTable(ctx sessionctx.Context, recoverInfo *RecoverInfo) (err error) - DropView(ctx sessionctx.Context, tableIdent ast.Ident) (err error) + DropView(ctx sessionctx.Context, stmt *ast.DropTableStmt) (err error) CreateIndex(ctx sessionctx.Context, tableIdent ast.Ident, keyType ast.IndexKeyType, indexName model.CIStr, columnNames []*ast.IndexPartSpecification, indexOption *ast.IndexOption, ifNotExists bool) error DropIndex(ctx sessionctx.Context, tableIdent ast.Ident, indexName model.CIStr, ifExists bool) error @@ -122,7 +122,7 @@ type DDL interface { UpdateTableReplicaInfo(ctx sessionctx.Context, physicalID int64, available bool) error RepairTable(ctx sessionctx.Context, table *ast.TableName, createStmt *ast.CreateTableStmt) error CreateSequence(ctx sessionctx.Context, stmt *ast.CreateSequenceStmt) error - DropSequence(ctx sessionctx.Context, tableIdent ast.Ident, ifExists bool) (err error) + DropSequence(ctx sessionctx.Context, stmt *ast.DropSequenceStmt) (err error) AlterSequence(ctx sessionctx.Context, stmt *ast.AlterSequenceStmt) error CreatePlacementPolicy(ctx sessionctx.Context, stmt *ast.CreatePlacementPolicyStmt) error DropPlacementPolicy(ctx sessionctx.Context, stmt *ast.DropPlacementPolicyStmt) error diff --git a/ddl/ddl_api.go b/ddl/ddl_api.go index 5a7758a36ba88..f7bb0fd9e762a 100644 --- a/ddl/ddl_api.go +++ b/ddl/ddl_api.go @@ -5256,71 +5256,167 @@ func (d *ddl) RenameIndex(ctx sessionctx.Context, ident ast.Ident, spec *ast.Alt return errors.Trace(err) } -// DropTable will proceed even if some table in the list does not exists. -func (d *ddl) DropTable(ctx sessionctx.Context, ti ast.Ident) (err error) { - schema, tb, err := d.getSchemaAndTableByIdent(ctx, ti) - if err != nil { - return errors.Trace(err) - } +// If one drop those tables by mistake, it's difficult to recover. +// In the worst case, the whole TiDB cluster fails to bootstrap, so we prevent user from dropping them. +var systemTables = map[string]struct{}{ + "tidb": {}, + "gc_delete_range": {}, + "gc_delete_range_done": {}, +} - if tb.Meta().IsView() { - return infoschema.ErrTableNotExists.GenWithStackByArgs(ti.Schema, ti.Name) - } - if tb.Meta().IsSequence() { - return infoschema.ErrTableNotExists.GenWithStackByArgs(ti.Schema, ti.Name) +func isSystemTable(schema, table string) bool { + if schema != "mysql" { + return false } - if tb.Meta().TableCacheStatusType != model.TableCacheStatusDisable { - return dbterror.ErrOptOnCacheTable.GenWithStackByArgs("Drop Table") + if _, ok := systemTables[table]; ok { + return true } + return false +} - job := &model.Job{ - SchemaID: schema.ID, - TableID: tb.Meta().ID, - SchemaName: schema.Name.L, - SchemaState: schema.State, - TableName: tb.Meta().Name.L, - Type: model.ActionDropTable, - BinlogInfo: &model.HistoryInfo{}, - } +type objectType int + +const ( + tableObject objectType = iota + viewObject + sequenceObject +) + +// dropTableObject provides common logic to DROP TABLE/VIEW/SEQUENCE. +func (d *ddl) dropTableObject( + ctx sessionctx.Context, + objects []*ast.TableName, + ifExists bool, + tableObjectType objectType, +) error { + var ( + notExistTables []string + sessVars = ctx.GetSessionVars() + is = d.GetInfoSchemaWithInterceptor(ctx) + dropExistErr *terror.Error + jobType model.ActionType + ) + + switch tableObjectType { + case tableObject: + dropExistErr = infoschema.ErrTableDropExists + jobType = model.ActionDropTable + case viewObject: + dropExistErr = infoschema.ErrTableDropExists + jobType = model.ActionDropView + case sequenceObject: + dropExistErr = infoschema.ErrSequenceDropExists + jobType = model.ActionDropSequence + } + + for _, tn := range objects { + fullti := ast.Ident{Schema: tn.Schema, Name: tn.Name} + schema, ok := is.SchemaByName(tn.Schema) + if !ok { + // TODO: we should return special error for table not exist, checking "not exist" is not enough, + // because some other errors may contain this error string too. + notExistTables = append(notExistTables, fullti.String()) + continue + } + tableInfo, err := is.TableByName(tn.Schema, tn.Name) + if err != nil && infoschema.ErrTableNotExists.Equal(err) { + notExistTables = append(notExistTables, fullti.String()) + continue + } else if err != nil { + return err + } + + // precheck before build DDL job + switch tableObjectType { + case tableObject: + if !tableInfo.Meta().IsBaseTable() { + notExistTables = append(notExistTables, fullti.String()) + continue + } + + tempTableType := tableInfo.Meta().TempTableType + if config.CheckTableBeforeDrop && tempTableType == model.TempTableNone { + logutil.BgLogger().Warn("admin check table before drop", + zap.String("database", fullti.Schema.O), + zap.String("table", fullti.Name.O), + ) + exec := ctx.(sqlexec.RestrictedSQLExecutor) + _, _, err := exec.ExecRestrictedSQL(context.TODO(), nil, "admin check table %n.%n", fullti.Schema.O, fullti.Name.O) + if err != nil { + return err + } + } + + if tableInfo.Meta().TableCacheStatusType != model.TableCacheStatusDisable { + return dbterror.ErrOptOnCacheTable.GenWithStackByArgs("Drop Table") + } + case viewObject: + if !tableInfo.Meta().IsView() { + return dbterror.ErrWrongObject.GenWithStackByArgs(fullti.Schema, fullti.Name, "VIEW") + } + case sequenceObject: + if !tableInfo.Meta().IsSequence() { + err = dbterror.ErrWrongObject.GenWithStackByArgs(fullti.Schema, fullti.Name, "SEQUENCE") + if ifExists { + ctx.GetSessionVars().StmtCtx.AppendNote(err) + continue + } + return err + } + } + + // Protect important system table from been dropped by a mistake. + // I can hardly find a case that a user really need to do this. + if isSystemTable(tn.Schema.L, tn.Name.L) { + return errors.Errorf("Drop tidb system table '%s.%s' is forbidden", tn.Schema.L, tn.Name.L) + } + + job := &model.Job{ + SchemaID: schema.ID, + TableID: tableInfo.Meta().ID, + SchemaName: schema.Name.L, + SchemaState: schema.State, + TableName: tableInfo.Meta().Name.L, + Type: jobType, + BinlogInfo: &model.HistoryInfo{}, + } + + err = d.DoDDLJob(ctx, job) + err = d.callHookOnChanged(job, err) + if infoschema.ErrDatabaseNotExists.Equal(err) || infoschema.ErrTableNotExists.Equal(err) { + notExistTables = append(notExistTables, fullti.String()) + } else if err != nil { + return errors.Trace(err) + } + + if !config.TableLockEnabled() { + return nil + } + if ok, _ := ctx.CheckTableLocked(tableInfo.Meta().ID); ok { + ctx.ReleaseTableLockByTableIDs([]int64{tableInfo.Meta().ID}) + } - err = d.DoDDLJob(ctx, job) - err = d.callHookOnChanged(job, err) - if err != nil { - return errors.Trace(err) } - if !config.TableLockEnabled() { - return nil + if len(notExistTables) > 0 && !ifExists { + return dropExistErr.GenWithStackByArgs(strings.Join(notExistTables, ",")) } - if ok, _ := ctx.CheckTableLocked(tb.Meta().ID); ok { - ctx.ReleaseTableLockByTableIDs([]int64{tb.Meta().ID}) + // We need add warning when use if exists. + if len(notExistTables) > 0 && ifExists { + for _, table := range notExistTables { + sessVars.StmtCtx.AppendNote(dropExistErr.GenWithStackByArgs(table)) + } } return nil } -// DropView will proceed even if some view in the list does not exists. -func (d *ddl) DropView(ctx sessionctx.Context, ti ast.Ident) (err error) { - schema, tb, err := d.getSchemaAndTableByIdent(ctx, ti) - if err != nil { - return errors.Trace(err) - } - - if !tb.Meta().IsView() { - return dbterror.ErrWrongObject.GenWithStackByArgs(ti.Schema, ti.Name, "VIEW") - } - - job := &model.Job{ - SchemaID: schema.ID, - TableID: tb.Meta().ID, - SchemaName: schema.Name.L, - SchemaState: tb.Meta().State, - TableName: tb.Meta().Name.L, - Type: model.ActionDropView, - BinlogInfo: &model.HistoryInfo{}, - } +// DropTable will proceed even if some table in the list does not exists. +func (d *ddl) DropTable(ctx sessionctx.Context, stmt *ast.DropTableStmt) (err error) { + return d.dropTableObject(ctx, stmt.Tables, stmt.IfExists, tableObject) +} - err = d.DoDDLJob(ctx, job) - err = d.callHookOnChanged(job, err) - return errors.Trace(err) +// DropView will proceed even if some view in the list does not exists. +func (d *ddl) DropView(ctx sessionctx.Context, stmt *ast.DropTableStmt) (err error) { + return d.dropTableObject(ctx, stmt.Tables, stmt.IfExists, viewObject) } func (d *ddl) TruncateTable(ctx sessionctx.Context, ti ast.Ident) error { @@ -6606,34 +6702,8 @@ func (d *ddl) AlterSequence(ctx sessionctx.Context, stmt *ast.AlterSequenceStmt) return errors.Trace(err) } -func (d *ddl) DropSequence(ctx sessionctx.Context, ti ast.Ident, ifExists bool) (err error) { - schema, tbl, err := d.getSchemaAndTableByIdent(ctx, ti) - if err != nil { - return errors.Trace(err) - } - - if !tbl.Meta().IsSequence() { - err = dbterror.ErrWrongObject.GenWithStackByArgs(ti.Schema, ti.Name, "SEQUENCE") - if ifExists { - ctx.GetSessionVars().StmtCtx.AppendNote(err) - return nil - } - return err - } - - job := &model.Job{ - SchemaID: schema.ID, - TableID: tbl.Meta().ID, - SchemaName: schema.Name.L, - SchemaState: tbl.Meta().State, - TableName: tbl.Meta().Name.L, - Type: model.ActionDropSequence, - BinlogInfo: &model.HistoryInfo{}, - } - - err = d.DoDDLJob(ctx, job) - err = d.callHookOnChanged(job, err) - return errors.Trace(err) +func (d *ddl) DropSequence(ctx sessionctx.Context, stmt *ast.DropSequenceStmt) (err error) { + return d.dropTableObject(ctx, stmt.Sequences, stmt.IfExists, sequenceObject) } func (d *ddl) AlterIndexVisibility(ctx sessionctx.Context, ident ast.Ident, indexName model.CIStr, visibility ast.IndexVisibility) error { diff --git a/executor/ddl.go b/executor/ddl.go index af8742fb1f901..5a6510e1a05da 100644 --- a/executor/ddl.go +++ b/executor/ddl.go @@ -38,7 +38,6 @@ import ( "github.com/pingcap/tidb/util/dbterror" "github.com/pingcap/tidb/util/gcutil" "github.com/pingcap/tidb/util/logutil" - "github.com/pingcap/tidb/util/sqlexec" "go.uber.org/zap" ) @@ -406,117 +405,16 @@ func (e *DDLExec) executeDropDatabase(s *ast.DropDatabaseStmt) error { return err } -// If one drop those tables by mistake, it's difficult to recover. -// In the worst case, the whole TiDB cluster fails to bootstrap, so we prevent user from dropping them. -var systemTables = map[string]struct{}{ - "tidb": {}, - "gc_delete_range": {}, - "gc_delete_range_done": {}, -} - -func isSystemTable(schema, table string) bool { - if schema != "mysql" { - return false - } - if _, ok := systemTables[table]; ok { - return true - } - return false -} - -type objectType int - -const ( - tableObject objectType = iota - viewObject - sequenceObject -) - func (e *DDLExec) executeDropTable(s *ast.DropTableStmt) error { - return e.dropTableObject(s.Tables, tableObject, s.IfExists) + return domain.GetDomain(e.ctx).DDL().DropTable(e.ctx, s) } func (e *DDLExec) executeDropView(s *ast.DropTableStmt) error { - return e.dropTableObject(s.Tables, viewObject, s.IfExists) + return domain.GetDomain(e.ctx).DDL().DropView(e.ctx, s) } func (e *DDLExec) executeDropSequence(s *ast.DropSequenceStmt) error { - return e.dropTableObject(s.Sequences, sequenceObject, s.IfExists) -} - -// dropTableObject actually applies to `tableObject`, `viewObject` and `sequenceObject`. -func (e *DDLExec) dropTableObject(objects []*ast.TableName, obt objectType, ifExists bool) error { - var notExistTables []string - sessVars := e.ctx.GetSessionVars() - for _, tn := range objects { - fullti := ast.Ident{Schema: tn.Schema, Name: tn.Name} - _, ok := e.is.SchemaByName(tn.Schema) - if !ok { - // TODO: we should return special error for table not exist, checking "not exist" is not enough, - // because some other errors may contain this error string too. - notExistTables = append(notExistTables, fullti.String()) - continue - } - _, err := e.is.TableByName(tn.Schema, tn.Name) - if err != nil && infoschema.ErrTableNotExists.Equal(err) { - notExistTables = append(notExistTables, fullti.String()) - continue - } else if err != nil { - return err - } - - // Protect important system table from been dropped by a mistake. - // I can hardly find a case that a user really need to do this. - if isSystemTable(tn.Schema.L, tn.Name.L) { - return errors.Errorf("Drop tidb system table '%s.%s' is forbidden", tn.Schema.L, tn.Name.L) - } - tableInfo, err := e.is.TableByName(tn.Schema, tn.Name) - if err != nil { - return err - } - tempTableType := tableInfo.Meta().TempTableType - if obt == tableObject && config.CheckTableBeforeDrop && tempTableType == model.TempTableNone { - logutil.BgLogger().Warn("admin check table before drop", - zap.String("database", fullti.Schema.O), - zap.String("table", fullti.Name.O), - ) - exec := e.ctx.(sqlexec.RestrictedSQLExecutor) - _, _, err := exec.ExecRestrictedSQL(context.TODO(), nil, "admin check table %n.%n", fullti.Schema.O, fullti.Name.O) - if err != nil { - return err - } - } - switch obt { - case tableObject: - err = domain.GetDomain(e.ctx).DDL().DropTable(e.ctx, fullti) - case viewObject: - err = domain.GetDomain(e.ctx).DDL().DropView(e.ctx, fullti) - case sequenceObject: - err = domain.GetDomain(e.ctx).DDL().DropSequence(e.ctx, fullti, ifExists) - } - if infoschema.ErrDatabaseNotExists.Equal(err) || infoschema.ErrTableNotExists.Equal(err) { - notExistTables = append(notExistTables, fullti.String()) - } else if err != nil { - return err - } - } - if len(notExistTables) > 0 && !ifExists { - if obt == sequenceObject { - return infoschema.ErrSequenceDropExists.GenWithStackByArgs(strings.Join(notExistTables, ",")) - } - return infoschema.ErrTableDropExists.GenWithStackByArgs(strings.Join(notExistTables, ",")) - } - // We need add warning when use if exists. - if len(notExistTables) > 0 && ifExists { - for _, table := range notExistTables { - if obt == sequenceObject { - sessVars.StmtCtx.AppendNote(infoschema.ErrSequenceDropExists.GenWithStackByArgs(table)) - } else { - sessVars.StmtCtx.AppendNote(infoschema.ErrTableDropExists.GenWithStackByArgs(table)) - } - } - } - return nil + return domain.GetDomain(e.ctx).DDL().DropSequence(e.ctx, s) } func (e *DDLExec) dropLocalTemporaryTables(localTempTables []*ast.TableName) error { From 622660065ab9a30a2a2e66930d88994a6563177a Mon Sep 17 00:00:00 2001 From: lance6716 Date: Mon, 27 Jun 2022 13:05:24 +0800 Subject: [PATCH 2/4] fix wrong control flow Signed-off-by: lance6716 --- ddl/ddl_api.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ddl/ddl_api.go b/ddl/ddl_api.go index f7bb0fd9e762a..552e3b0c166ff 100644 --- a/ddl/ddl_api.go +++ b/ddl/ddl_api.go @@ -5385,12 +5385,13 @@ func (d *ddl) dropTableObject( err = d.callHookOnChanged(job, err) if infoschema.ErrDatabaseNotExists.Equal(err) || infoschema.ErrTableNotExists.Equal(err) { notExistTables = append(notExistTables, fullti.String()) + continue } else if err != nil { return errors.Trace(err) } if !config.TableLockEnabled() { - return nil + continue } if ok, _ := ctx.CheckTableLocked(tableInfo.Meta().ID); ok { ctx.ReleaseTableLockByTableIDs([]int64{tableInfo.Meta().ID}) From e17f7e021f911189add9276eeee7b034cad6c323 Mon Sep 17 00:00:00 2001 From: lance6716 Date: Mon, 27 Jun 2022 13:19:24 +0800 Subject: [PATCH 3/4] fix CI Signed-off-by: lance6716 --- ddl/ddl_api.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/ddl/ddl_api.go b/ddl/ddl_api.go index 552e3b0c166ff..a3514c850a0e9 100644 --- a/ddl/ddl_api.go +++ b/ddl/ddl_api.go @@ -5326,7 +5326,13 @@ func (d *ddl) dropTableObject( return err } - // precheck before build DDL job + // prechecks before build DDL job + + // Protect important system table from been dropped by a mistake. + // I can hardly find a case that a user really need to do this. + if isSystemTable(tn.Schema.L, tn.Name.L) { + return errors.Errorf("Drop tidb system table '%s.%s' is forbidden", tn.Schema.L, tn.Name.L) + } switch tableObjectType { case tableObject: if !tableInfo.Meta().IsBaseTable() { @@ -5365,12 +5371,6 @@ func (d *ddl) dropTableObject( } } - // Protect important system table from been dropped by a mistake. - // I can hardly find a case that a user really need to do this. - if isSystemTable(tn.Schema.L, tn.Name.L) { - return errors.Errorf("Drop tidb system table '%s.%s' is forbidden", tn.Schema.L, tn.Name.L) - } - job := &model.Job{ SchemaID: schema.ID, TableID: tableInfo.Meta().ID, From a46f0e38d0c77f98d0515fd38084beb657400893 Mon Sep 17 00:00:00 2001 From: lance6716 Date: Mon, 27 Jun 2022 19:02:21 +0800 Subject: [PATCH 4/4] fix CI :think: Signed-off-by: lance6716 --- ddl/ddl_api.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ddl/ddl_api.go b/ddl/ddl_api.go index a3514c850a0e9..ee3e27fe7e060 100644 --- a/ddl/ddl_api.go +++ b/ddl/ddl_api.go @@ -5390,6 +5390,10 @@ func (d *ddl) dropTableObject( return errors.Trace(err) } + // unlock table after drop + if tableObjectType != tableObject { + continue + } if !config.TableLockEnabled() { continue }