onlineddl: report an error when online ddl only matches only one regex (#2182) (#2185)

Author: ti-chi-bot

_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]

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
 		}

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 = ""

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, "", "")

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 {

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"

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
+}