From cd46eee3f1d00d6c78888e4a38daf80f29c69266 Mon Sep 17 00:00:00 2001 From: Ti Chi Robot Date: Wed, 29 Sep 2021 10:14:46 +0800 Subject: [PATCH] onlineddl: report an error when online ddl only matches only one regex (#2182) (#2185) --- _utils/terror_gen/errors_release.txt | 1 + dm/config/subtask.go | 7 +- errors.toml | 6 ++ pkg/terror/error_list.go | 3 + syncer/ddl.go | 6 ++ syncer/ddl_test.go | 94 ++++++++++++++++++++++--- syncer/online-ddl-tools/online_ddl.go | 99 +++++++++++++++++++++++++-- 7 files changed, 200 insertions(+), 16 deletions(-) diff --git a/_utils/terror_gen/errors_release.txt b/_utils/terror_gen/errors_release.txt index 6b0c9e634d..202d4cb5ab 100644 --- a/_utils/terror_gen/errors_release.txt +++ b/_utils/terror_gen/errors_release.txt @@ -177,6 +177,7 @@ ErrConfigGenTableRouter,[code=20045:class=config:scope=internal:level=high], "Me ErrConfigGenColumnMapping,[code=20046:class=config:scope=internal:level=high], "Message: generate column mapping error, Workaround: Please check the `column-mappings` config in task configuration file." ErrConfigInvalidChunkFileSize,[code=20047:class=config:scope=internal:level=high], "Message: invalid `chunk-filesize` %v, Workaround: Please check the `chunk-filesize` config in task configuration file." ErrConfigOnlineDDLInvalidRegex,[code=20048:class=config:scope=internal:level=high], "Message: config '%s' regex pattern '%s' invalid, reason: %s, Workaround: Please check if params is correctly in the configuration file." +ErrConfigOnlineDDLMistakeRegex,[code=20049:class=config:scope=internal:level=high], "Message: online ddl sql '%s' invalid, table %s fail to match '%s' online ddl regex, Workaround: Please update your `shadow-table-rules` or `trash-table-rules` in the configuration file." ErrBinlogExtractPosition,[code=22001:class=binlog-op:scope=internal:level=high] ErrBinlogInvalidFilename,[code=22002:class=binlog-op:scope=internal:level=high], "Message: invalid binlog filename" ErrBinlogParsePosFromStr,[code=22003:class=binlog-op:scope=internal:level=high] diff --git a/dm/config/subtask.go b/dm/config/subtask.go index 57faad3f79..a8a33e85ac 100644 --- a/dm/config/subtask.go +++ b/dm/config/subtask.go @@ -42,6 +42,9 @@ const ( DefaultShadowTableRules = "^_(.+)_(?:new|gho)$" DefaultTrashTableRules = "^_(.+)_(?:ghc|del|old)$" + + ShadowTableRules = "shadow-table-rules" + TrashTableRules = "trash-table-rules" ) var defaultMaxIdleConns = 2 @@ -309,7 +312,7 @@ func (c *SubTaskConfig) Adjust(verifyDecryptPassword bool) error { if len(c.ShadowTableRules) == 0 { c.ShadowTableRules = []string{DefaultShadowTableRules} } else { - shadowTableRule, err := adjustOnlineTableRules("shadow-table-rules", c.ShadowTableRules) + shadowTableRule, err := adjustOnlineTableRules(ShadowTableRules, c.ShadowTableRules) if err != nil { return err } @@ -319,7 +322,7 @@ func (c *SubTaskConfig) Adjust(verifyDecryptPassword bool) error { if len(c.TrashTableRules) == 0 { c.TrashTableRules = []string{DefaultTrashTableRules} } else { - trashTableRule, err := adjustOnlineTableRules("trash-table-rules", c.TrashTableRules) + trashTableRule, err := adjustOnlineTableRules(TrashTableRules, c.TrashTableRules) if err != nil { return err } diff --git a/errors.toml b/errors.toml index b0fd7d6b64..7211df2fb4 100644 --- a/errors.toml +++ b/errors.toml @@ -1072,6 +1072,12 @@ description = "" workaround = "Please check if params is correctly in the configuration file." tags = ["internal", "high"] +[error.DM-config-20049] +message = "online ddl sql '%s' invalid, table %s fail to match '%s' online ddl regex" +description = "" +workaround = "Please update your `shadow-table-rules` or `trash-table-rules` in the configuration file." +tags = ["internal", "high"] + [error.DM-binlog-op-22001] message = "" description = "" diff --git a/pkg/terror/error_list.go b/pkg/terror/error_list.go index 9d652089bf..ddfa0381f1 100644 --- a/pkg/terror/error_list.go +++ b/pkg/terror/error_list.go @@ -240,6 +240,7 @@ const ( codeConfigGenColumnMapping codeConfigInvalidChunkFileSize codeConfigOnlineDDLInvalidRegex + codeConfigOnlineDDLMistakeRegex ) // Binlog operation error code list. @@ -865,6 +866,8 @@ var ( ErrConfigInvalidChunkFileSize = New(codeConfigInvalidChunkFileSize, ClassConfig, ScopeInternal, LevelHigh, "invalid `chunk-filesize` %v", "Please check the `chunk-filesize` config in task configuration file.") ErrConfigOnlineDDLInvalidRegex = New(codeConfigOnlineDDLInvalidRegex, ClassConfig, ScopeInternal, LevelHigh, "config '%s' regex pattern '%s' invalid, reason: %s", "Please check if params is correctly in the configuration file.") + ErrConfigOnlineDDLMistakeRegex = New(codeConfigOnlineDDLMistakeRegex, ClassConfig, ScopeInternal, LevelHigh, + "online ddl sql '%s' invalid, table %s fail to match '%s' online ddl regex", "Please update your `shadow-table-rules` or `trash-table-rules` in the configuration file.") // Binlog operation error. ErrBinlogExtractPosition = New(codeBinlogExtractPosition, ClassBinlogOp, ScopeInternal, LevelHigh, "", "") diff --git a/syncer/ddl.go b/syncer/ddl.go index 88325a56bc..3ace3b49b5 100644 --- a/syncer/ddl.go +++ b/syncer/ddl.go @@ -133,6 +133,12 @@ func (s *Syncer) splitAndFilterDDL( statements := make([]string, 0, len(sqls)) tables = make(map[string]*filter.Table) + + if s.onlineDDL != nil { + if err = s.onlineDDL.CheckRegex(stmt, schema, s.SourceTableNamesFlavor); err != nil { + return nil, nil, err + } + } for _, sql := range sqls { stmt2, err2 := p.ParseOneStmt(sql, "", "") if err2 != nil { diff --git a/syncer/ddl_test.go b/syncer/ddl_test.go index 6c6c207c34..33a51ead55 100644 --- a/syncer/ddl_test.go +++ b/syncer/ddl_test.go @@ -15,23 +15,24 @@ package syncer import ( "context" - "errors" "fmt" - "github.com/pingcap/dm/dm/config" - tcontext "github.com/pingcap/dm/pkg/context" - "github.com/pingcap/dm/pkg/log" - parserpkg "github.com/pingcap/dm/pkg/parser" - "github.com/pingcap/dm/pkg/utils" - onlineddl "github.com/pingcap/dm/syncer/online-ddl-tools" - "github.com/DATA-DOG/go-sqlmock" . "github.com/pingcap/check" + "github.com/pingcap/errors" "github.com/pingcap/parser" "github.com/pingcap/parser/ast" "github.com/pingcap/tidb-tools/pkg/filter" router "github.com/pingcap/tidb-tools/pkg/table-router" "go.uber.org/zap" + + "github.com/pingcap/dm/dm/config" + tcontext "github.com/pingcap/dm/pkg/context" + "github.com/pingcap/dm/pkg/log" + parserpkg "github.com/pingcap/dm/pkg/parser" + "github.com/pingcap/dm/pkg/terror" + "github.com/pingcap/dm/pkg/utils" + onlineddl "github.com/pingcap/dm/syncer/online-ddl-tools" ) func (s *testSyncerSuite) TestAnsiQuotes(c *C) { @@ -451,7 +452,7 @@ func (s *testSyncerSuite) TestResolveOnlineDDL(c *C) { c.Assert(err, IsNil) c.Assert(sqls, HasLen, 0) c.Assert(tables, HasLen, 0) - sql = fmt.Sprintf("RENAME TABLE `test`.`%s` TO `test`.`t1`", ca.ghostname) + sql = fmt.Sprintf("RENAME TABLE `test`.`t1` TO `test`.`%s`, `test`.`%s` TO `test`.`t1`", ca.trashName, ca.ghostname) stmt, err = p.ParseOneStmt(sql, "", "") c.Assert(err, IsNil) sqls, tables, err = syncer.splitAndFilterDDL(ec, p, stmt, "test") @@ -463,6 +464,77 @@ func (s *testSyncerSuite) TestResolveOnlineDDL(c *C) { } } +func (s *testSyncerSuite) TestMistakeOnlineDDLRegex(c *C) { + cases := []struct { + onlineType string + trashName string + ghostname string + matchGho bool + }{ + { + config.GHOST, + "_t1_del", + "_t1_gho_invalid", + false, + }, + { + config.GHOST, + "_t1_del_invalid", + "_t1_gho", + true, + }, + { + config.PT, + "_t1_old", + "_t1_new_invalid", + false, + }, + { + config.PT, + "_t1_old_invalid", + "_t1_new", + true, + }, + } + tctx := tcontext.Background().WithLogger(log.With(zap.String("test", "TestMistakeOnlineDDLRegex"))) + p := parser.New() + + ec := eventContext{tctx: tctx} + for _, ca := range cases { + plugin, err := onlineddl.NewRealOnlinePlugin(tctx, s.cfg) + c.Assert(err, IsNil) + syncer := NewSyncer(s.cfg, nil) + syncer.onlineDDL = plugin + c.Assert(plugin.Clear(tctx), IsNil) + + // ghost table + sql := fmt.Sprintf("ALTER TABLE `test`.`%s` ADD COLUMN `n` INT", ca.ghostname) + stmt, err := p.ParseOneStmt(sql, "", "") + c.Assert(err, IsNil) + sqls, tables, err := syncer.splitAndFilterDDL(ec, p, stmt, "test") + c.Assert(err, IsNil) + c.Assert(tables, HasLen, 0) + table := ca.ghostname + matchRules := config.ShadowTableRules + if ca.matchGho { + c.Assert(sqls, HasLen, 0) + table = ca.trashName + matchRules = config.TrashTableRules + } else { + c.Assert(sqls, HasLen, 1) + c.Assert(sqls[0], Equals, sql) + } + sql = fmt.Sprintf("RENAME TABLE `test`.`t1` TO `test`.`%s`, `test`.`%s` TO `test`.`t1`", ca.trashName, ca.ghostname) + stmt, err = p.ParseOneStmt(sql, "", "") + c.Assert(err, IsNil) + sqls, tables, err = syncer.splitAndFilterDDL(ec, p, stmt, "test") + c.Assert(terror.ErrConfigOnlineDDLMistakeRegex.Equal(err), IsTrue) + c.Assert(sqls, HasLen, 0) + c.Assert(tables, HasLen, 0) + c.Assert(err, ErrorMatches, ".*"+sql+".*"+table+".*"+matchRules+".*") + } +} + func (s *testSyncerSuite) TestDropSchemaInSharding(c *C) { var ( targetDB = "target_db" @@ -526,6 +598,10 @@ func (m mockOnlinePlugin) CheckAndUpdate(tctx *tcontext.Context, schemas map[str return nil } +func (m mockOnlinePlugin) CheckRegex(stmt ast.StmtNode, schema string, flavor utils.LowerCaseTableNamesFlavor) error { + return nil +} + func (s *testSyncerSuite) TestClearOnlineDDL(c *C) { var ( targetDB = "target_db" diff --git a/syncer/online-ddl-tools/online_ddl.go b/syncer/online-ddl-tools/online_ddl.go index 37eac22cb4..f2ec30ec31 100644 --- a/syncer/online-ddl-tools/online_ddl.go +++ b/syncer/online-ddl-tools/online_ddl.go @@ -19,17 +19,18 @@ import ( "regexp" "sync" - "github.com/pingcap/failpoint" - "github.com/pingcap/dm/dm/config" "github.com/pingcap/dm/pkg/conn" tcontext "github.com/pingcap/dm/pkg/context" "github.com/pingcap/dm/pkg/cputil" parserpkg "github.com/pingcap/dm/pkg/parser" "github.com/pingcap/dm/pkg/terror" + "github.com/pingcap/dm/pkg/utils" "github.com/pingcap/dm/syncer/dbconn" + "github.com/pingcap/failpoint" "github.com/pingcap/parser/ast" + "github.com/pingcap/parser/model" "github.com/pingcap/tidb-tools/pkg/dbutil" "github.com/pingcap/tidb-tools/pkg/filter" "go.uber.org/zap" @@ -50,7 +51,7 @@ type OnlinePlugin interface { Apply(tctx *tcontext.Context, tables []*filter.Table, statement string, stmt ast.StmtNode) ([]string, string, string, error) // Finish would delete online ddl from memory and storage Finish(tctx *tcontext.Context, schema, table string) error - // TableType returns ghhost/real table + // TableType returns ghost/real table TableType(table string) TableType // RealName returns real table name that removed ghost suffix and handled by table router RealName(table string) string @@ -63,6 +64,8 @@ type OnlinePlugin interface { Close() // CheckAndUpdate try to check and fix the schema/table case-sensitive issue CheckAndUpdate(tctx *tcontext.Context, schemas map[string]string, tables map[string]map[string]string) error + // CheckRegex checks the regex of shadow/trash table rules and reports an error if a ddl event matches only either of the rules + CheckRegex(stmt ast.StmtNode, schema string, flavor utils.LowerCaseTableNamesFlavor) error } // TableType is type of table. @@ -75,6 +78,12 @@ const ( TrashTable TableType = "trash table" // means we should ignore these tables ) +const ( + shadowTable int = iota + trashTable + allTable +) + // GhostDDLInfo stores ghost information and ddls. type GhostDDLInfo struct { Schema string `json:"schema"` @@ -391,14 +400,14 @@ func NewRealOnlinePlugin(tctx *tcontext.Context, cfg *config.SubTaskConfig) (Onl for _, sg := range cfg.ShadowTableRules { shadowReg, err := regexp.Compile(sg) if err != nil { - return nil, terror.ErrConfigOnlineDDLInvalidRegex.Generate("shadow-table-rules", sg, "fail to compile: "+err.Error()) + return nil, terror.ErrConfigOnlineDDLInvalidRegex.Generate(config.ShadowTableRules, sg, "fail to compile: "+err.Error()) } shadowRegs = append(shadowRegs, shadowReg) } for _, tg := range cfg.TrashTableRules { trashReg, err := regexp.Compile(tg) if err != nil { - return nil, terror.ErrConfigOnlineDDLInvalidRegex.Generate("trash-table-rules", tg, "fail to compile: "+err.Error()) + return nil, terror.ErrConfigOnlineDDLInvalidRegex.Generate(config.TrashTableRules, tg, "fail to compile: "+err.Error()) } trashRegs = append(trashRegs, trashReg) } @@ -558,3 +567,83 @@ func (r *RealOnlinePlugin) ResetConn(tctx *tcontext.Context) error { func (r *RealOnlinePlugin) CheckAndUpdate(tctx *tcontext.Context, schemas map[string]string, tables map[string]map[string]string) error { return r.storage.CheckAndUpdate(tctx, schemas, tables, r.RealName) } + +// CheckRegex checks the regex of shadow/trash table rules and reports an error if a ddl event matches only either of the rules. +func (r *RealOnlinePlugin) CheckRegex(stmt ast.StmtNode, schema string, flavor utils.LowerCaseTableNamesFlavor) error { + var ( + v *ast.RenameTableStmt + ok bool + ) + if v, ok = stmt.(*ast.RenameTableStmt); !ok { + return nil + } + t2ts := v.TableToTables + if len(t2ts) != 2 { + return nil + } + onlineDDLMatched := allTable + tableRecords := make([]*filter.Table, 2) + schemaName := model.NewCIStr(schema) // fill schema name + + // Online DDL sql example: RENAME TABLE `test`.`t1` TO `test`.`_t1_old`, `test`.`_t1_new` TO `test`.`t1` + // We should parse two rename DDL from this DDL: + // tables[0] tables[1] + // DDL 0 real table ───► trash table + // DDL 1 shadow table ───► real table + // If we only have one of them, that means users may configure a wrong trash/shadow table regex + for i, t2t := range t2ts { + if t2t.OldTable.Schema.O == "" { + t2t.OldTable.Schema = schemaName + } + if t2t.NewTable.Schema.O == "" { + t2t.NewTable.Schema = schemaName + } + + v.TableToTables = []*ast.TableToTable{t2t} + + if i == 0 { + tableRecords[trashTable] = fetchTable(t2t.NewTable, flavor) + if r.TableType(t2t.OldTable.Name.String()) == RealTable && + r.TableType(t2t.NewTable.Name.String()) == TrashTable { + onlineDDLMatched = trashTable + } + } else { + tableRecords[shadowTable] = fetchTable(t2t.OldTable, flavor) + if r.TableType(t2t.OldTable.Name.String()) == GhostTable && + r.TableType(t2t.NewTable.Name.String()) == RealTable { + // if no trash table is not matched before, we should record that shadow table is matched here + // if shadow table is matched before, we just return all tables are matched and a nil error + if onlineDDLMatched != trashTable { + onlineDDLMatched = shadowTable + } else { + onlineDDLMatched = allTable + } + } + } + } + if onlineDDLMatched != allTable { + return terror.ErrConfigOnlineDDLMistakeRegex.Generate(stmt.Text(), tableRecords[onlineDDLMatched^1], unmatchedOnlineDDLRules(onlineDDLMatched)) + } + return nil +} + +func unmatchedOnlineDDLRules(match int) string { + switch match { + case shadowTable: + return config.TrashTableRules + case trashTable: + return config.ShadowTableRules + default: + return "" + } +} + +func fetchTable(t *ast.TableName, flavor utils.LowerCaseTableNamesFlavor) *filter.Table { + var tb *filter.Table + if flavor == utils.LCTableNamesSensitive { + tb = &filter.Table{Schema: t.Schema.O, Name: t.Name.O} + } else { + tb = &filter.Table{Schema: t.Schema.L, Name: t.Name.L} + } + return tb +}