sync: return an error for rename table statement in optimistic mode #1512
Conversation
GMHDBJD
added
status/PTAL
| @@ -998,6 +999,7 @@ var ( | |||
| ErrSyncerOperatorNotExist = New(codeSyncerOperatorNotExist, ClassSyncUnit, ScopeInternal, LevelLow, "error operator not exist, position: %s", "") | |||
| ErrSyncerReplaceEventNotExist = New(codeSyncerReplaceEventNotExist, ClassSyncUnit, ScopeInternal, LevelHigh, "replace event not exist, location: %s", "") | |||
| ErrSyncerParseDDL = New(codeSyncerParseDDL, ClassSyncUnit, ScopeInternal, LevelHigh, "parse DDL: %s", "Please confirm your DDL statement is correct and needed. For TiDB compatible DDL, see https://docs.pingcap.com/tidb/stable/mysql-compatibility#ddl. You can use `handle-error` command to skip or replace the DDL or add a binlog filter rule to ignore it if the DDL is not needed.") | |||
| ErrSyncerUnsupportedStmt = New(codeSyncerUnsupportedStmt, ClassSyncUnit, ScopeInternal, LevelHigh, "`%s` statement not supported in %s mode", "") | |||
There was a problem hiding this comment.
we could add a workaround 🤔
There was a problem hiding this comment.
I can't find out a solution. Maybe we should support rename to the same group now. 😥
|
need to tell user how to handle this situation. rest LGTM |
GMHDBJD
added
status/DNM
GMHDBJD
changed the title
GMHDBJD
added
status/PTAL
| @@ -1856,6 +1856,8 @@ func (s *Syncer) handleQueryEvent(ev *replication.QueryEvent, ec eventContext, o | |||
| case *ast.TruncateTableStmt: | |||
| ec.tctx.L().Info("ignore truncate table statement in shard group", zap.String("event", "query"), zap.String("statement", sqlDDL)) | |||
| continue | |||
| case *ast.RenameTableStmt: | |||
There was a problem hiding this comment.
we heavily rely on "an optimistic sharding task should sync no other tables than table-route", if user both sync sharding tables and non sharding tables in one task, non sharding tables will not permitted to RENAME.
How about we give an error when user both specify block-allow list and table routes, to ensure user add a minimal sharding task that will not sync non-sharding tables? we might talk to PM if you think that's reasonable. @GMHDBJD @lichunzhu
could join us after your vacation @gozssky
There was a problem hiding this comment.
(but #1517 is still not solved)
There was a problem hiding this comment.
yes, what do you think "this PR also forbid user RENAME a non sharding table in a optimistic sharding task", could we raise an error when user both specify block-allow list and table routes (use table routes as allow list)?
There was a problem hiding this comment.
We have other stmts not supported for a non sharding table in shard mode(truncate statement). So it's not a problem of Rename.
raise an error when user both specify block-allow list and table routes
We can support it in another pr or #1517? Maybe talk to PM will be better.
lance6716
added
the
needs-update-docs
|
/lgtm |
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by writing |
Signed-off-by: ti-srebot <[email protected]>
|
cherry pick to release-2.0 in PR #1535 |
ti-srebot
added
already-cherry-pick-2.0
What problem does this PR solve?
we do not support rename table now, simply return an error in optimistic mode.
For pessimistic mode, though shardddl group was wrong, dm works well and data are still consistent 🙈.
TODO: support rename table for all mode.
What is changed and how it works?
prefilter before handle statement in optimistic mode.
Check List
Tests
TODO: emphasize don't mixed sharding tables with normal tables in one task, in other words,
sharding-modemeans applying to all tables in this task, not only table-routes tables