schemastore: fix rename table when source database is not specified#4416
schemastore: fix rename table when source database is not specified#4416
Conversation
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
bot
added
the
size/M
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReworks rename-table handling to reconstruct old schema/table names from parsed query, RenameTableArgs (with compatibility for older job versions), and InvolvingSchemaInfo; defers rebuilding persisted event.Query until both old names are known; adds parsing/compatibility helpers and cross-database rename tests. Changes
Sequence Diagram(s)sequenceDiagram
participant JobProducer as JobProducer
participant Handler as PersistHandler
participant Parser as SQLParser
participant Storage as SchemaStore
JobProducer->>Handler: submit RenameTable job (RawArgs / InvolvingSchemaInfo)
Handler->>Parser: parse query via parseRenameTableQueryInfo
Parser-->>Handler: parsed old/new schema.table info
Handler->>Handler: getRenameTableArgsCompatible(RenameTableArgs)
Handler->>Handler: reconcile old names (parsed, args, involvingInfo)
alt old schema/table both present
Handler->>Storage: build persisted DDL with updated event.Query
else incomplete old names
Handler->>Storage: persist event without modifying event.Query
end
Storage-->>Handler: ack
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
logservice/schemastore/persist_storage_test.go (1)
3137-3154: Exercise both supported job versions.The production branch now special-cases
model.JobVersion1andmodel.JobVersion2, but this test only coversmodel.JobVersion2. Please add aJobVersion1case, or table-drive the version, so the older format can't regress unnoticed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@logservice/schemastore/persist_storage_test.go` around lines 3137 - 3154, Test only exercises model.JobVersion2; add coverage for model.JobVersion1 (or convert to a table-driven subtest iterating over []model.JobVersion{model.JobVersion1, model.JobVersion2}) so both formats are validated. For each iteration set job.Version accordingly, call job.FillArgs(...) and buildPersistedDDLEventForRenameTable(buildPersistedDDLEventFuncArgs{...}) using the same databaseMap/tableMap, then assert the expected ddl.Query for that version (referencing job.Version, FillArgs, buildPersistedDDLEventForRenameTable, buildPersistedDDLEventFuncArgs, and ddl.Query); ensure test names reflect the version for easier debug.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@logservice/schemastore/persist_storage_ddl_handlers.go`:
- Around line 733-738: model.GetRenameTableArgs(args.job) failures are currently
ignored causing silent fallback to InvolvingSchemaInfo; change this by catching
the error from model.GetRenameTableArgs (while still preserving the existing
behavior) and log the failure with context (include args.job identifier/version
and the error) before falling back. Update the block that calls
model.GetRenameTableArgs (around renameArgs) to log the error when err != nil,
referencing renameArgs, args.job and InvolvingSchemaInfo so the diagnostic
contains which job/version failed and the error message.
---
Nitpick comments:
In `@logservice/schemastore/persist_storage_test.go`:
- Around line 3137-3154: Test only exercises model.JobVersion2; add coverage for
model.JobVersion1 (or convert to a table-driven subtest iterating over
[]model.JobVersion{model.JobVersion1, model.JobVersion2}) so both formats are
validated. For each iteration set job.Version accordingly, call
job.FillArgs(...) and
buildPersistedDDLEventForRenameTable(buildPersistedDDLEventFuncArgs{...}) using
the same databaseMap/tableMap, then assert the expected ddl.Query for that
version (referencing job.Version, FillArgs,
buildPersistedDDLEventForRenameTable, buildPersistedDDLEventFuncArgs, and
ddl.Query); ensure test names reflect the version for easier debug.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ec8027be-f37c-48e9-a37e-b8226868bbf5
📒 Files selected for processing (2)
logservice/schemastore/persist_storage_ddl_handlers.gologservice/schemastore/persist_storage_test.go
ti-chi-bot
bot
added
size/L
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
logservice/schemastore/persist_storage_ddl_handlers.go (1)
736-755:⚠️ Potential issue | 🟠 MajorApply the recovered old schema/table back to the persisted rename fields.
This block only uses
RenameTableArgs/ parsed SQL to rebuildevent.Query.event.ExtraSchemaID,event.ExtraSchemaName, andevent.ExtraTableNamestill come fromtableMap, but later code uses those fields to detect cross-schema renames and to populateBlockedTableNames,UpdatedSchemas, andTableNameChange.DropName. In the stale-snapshot case described above, the query becomes correct while the persisted old side of the rename is still wrong.💡 Suggested fix
// RenameTableArgs keeps the old schema name even when the query omits it. if args.job.Version == model.JobVersion1 || args.job.Version == model.JobVersion2 { renameArgs, err := model.GetRenameTableArgs(args.job) - if err == nil && renameArgs.OldSchemaName.O != "" { - oldSchemaName = renameArgs.OldSchemaName.O + if err == nil { + if renameArgs.OldSchemaID != 0 { + event.ExtraSchemaID = renameArgs.OldSchemaID + } + if renameArgs.OldSchemaName.O != "" { + oldSchemaName = renameArgs.OldSchemaName.O + } } } if queryInfo, parsed := parseRenameTableQueryInfo(args.job.Query); parsed { if queryInfo.oldTableName != "" { oldTableName = queryInfo.oldTableName @@ if queryInfo.oldSchemaName != "" { oldSchemaName = queryInfo.oldSchemaName } } - if oldSchemaName != "" && oldTableName != "" { + if oldSchemaName != "" { + event.ExtraSchemaName = oldSchemaName + } + if oldTableName != "" { + event.ExtraTableName = oldTableName + } + if event.ExtraSchemaName != "" && event.ExtraTableName != "" { event.Query = fmt.Sprintf("RENAME TABLE %s TO %s", - common.QuoteSchema(oldSchemaName, oldTableName), + common.QuoteSchema(event.ExtraSchemaName, event.ExtraTableName), common.QuoteSchema(event.SchemaName, event.TableName)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@logservice/schemastore/persist_storage_ddl_handlers.go` around lines 736 - 755, The persisted rename handling builds event.Query from RenameTableArgs / parseRenameTableQueryInfo but fails to update the persisted rename fields (event.ExtraSchemaID, event.ExtraSchemaName, event.ExtraTableName) causing downstream logic to see stale old-side values; after you compute oldSchemaName and oldTableName (from model.GetRenameTableArgs and parseRenameTableQueryInfo), update event.ExtraSchemaName and event.ExtraTableName to those recovered values and set event.ExtraSchemaID appropriately (lookup the schema ID like tableMap did or clear it if unknown) so the persisted event fields match the reconstructed query and downstream cross-schema detection (BlockedTableNames, UpdatedSchemas, TableNameChange.DropName) uses the corrected old side.
♻️ Duplicate comments (1)
logservice/schemastore/persist_storage_ddl_handlers.go (1)
736-742:⚠️ Potential issue | 🟡 MinorDon't silently swallow
GetRenameTableArgsfailures.If decoding rename args fails here, we quietly fall back to
InvolvingSchemaInfo, which is exactly the normalized-name path this change is trying to avoid. Please at least emit a warning with the job id/version before falling back.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@logservice/schemastore/persist_storage_ddl_handlers.go` around lines 736 - 742, The code currently swallows errors from model.GetRenameTableArgs when extracting RenameTableArgs for args.job.Version 1 or 2, which can mask decoding failures and cause silent fallback to InvolvingSchemaInfo; update the block that calls model.GetRenameTableArgs (and handles renameArgs/OldSchemaName) to log a warning including the job identifier and version when err != nil (use args.job.ID or equivalent and args.job.Version), then continue the existing fallback behavior—do not change control flow, just emit a clear processLogger.Warn/processLogger.Warning (or the project's logger) message referencing GetRenameTableArgs, args.job.ID, and args.job.Version so failures aren’t silent.
🧹 Nitpick comments (1)
logservice/schemastore/persist_storage_test.go (1)
3132-3174: These new cases still don't reproduce the stale-snapshot bug.
tableMap[101].SchemaIDis left as200, so the old schema is already available from the store even ifRenameTableArgsare ignored. The regression described in the handler comment only appears when the snapshot already shows the table inArchiveDB; please set the table-map entry to the new schema and assertddl.ExtraSchemaID/ddl.ExtraSchemaNameas well.🧪 Suggested test tightening
databaseMap: map[int64]*BasicDatabaseInfo{ }, tableMap: map[int64]*BasicTableInfo{ - 101: {SchemaID: 200, Name: "t1"}, + 101: {SchemaID: 100, Name: "t1"}, }, }) + assert.Equal(t, int64(200), ddl.ExtraSchemaID) + assert.Equal(t, "SalesDB", ddl.ExtraSchemaName) + assert.Equal(t, "t1", ddl.ExtraTableName) assert.Equal(t, "RENAME TABLE `SalesDB`.`t1` TO `ArchiveDB`.`t1`", ddl.Query)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@logservice/schemastore/persist_storage_test.go` around lines 3132 - 3174, The failing test doesn't reproduce the stale-snapshot bug because tableMap[101].SchemaID remains 200 (old schema) so the old schema is present; change the second case's tableMap entry for table id 101 to have SchemaID: 100 (the new ArchiveDB schema) and then add assertions that ddl.ExtraSchemaID == 100 and ddl.ExtraSchemaName == "ArchiveDB" (use the persisted DDL from buildPersistedDDLEventForRenameTable and check ddl.ExtraSchemaID / ddl.ExtraSchemaName) so the test covers the scenario where the snapshot already shows the table in ArchiveDB; reference: buildRenameTableJobForTest, RenameTableArgs, buildPersistedDDLEventForRenameTable, BasicTableInfo, ddl.ExtraSchemaID, ddl.ExtraSchemaName.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@logservice/schemastore/persist_storage_ddl_handlers.go`:
- Around line 736-755: The persisted rename handling builds event.Query from
RenameTableArgs / parseRenameTableQueryInfo but fails to update the persisted
rename fields (event.ExtraSchemaID, event.ExtraSchemaName, event.ExtraTableName)
causing downstream logic to see stale old-side values; after you compute
oldSchemaName and oldTableName (from model.GetRenameTableArgs and
parseRenameTableQueryInfo), update event.ExtraSchemaName and
event.ExtraTableName to those recovered values and set event.ExtraSchemaID
appropriately (lookup the schema ID like tableMap did or clear it if unknown) so
the persisted event fields match the reconstructed query and downstream
cross-schema detection (BlockedTableNames, UpdatedSchemas,
TableNameChange.DropName) uses the corrected old side.
---
Duplicate comments:
In `@logservice/schemastore/persist_storage_ddl_handlers.go`:
- Around line 736-742: The code currently swallows errors from
model.GetRenameTableArgs when extracting RenameTableArgs for args.job.Version 1
or 2, which can mask decoding failures and cause silent fallback to
InvolvingSchemaInfo; update the block that calls model.GetRenameTableArgs (and
handles renameArgs/OldSchemaName) to log a warning including the job identifier
and version when err != nil (use args.job.ID or equivalent and
args.job.Version), then continue the existing fallback behavior—do not change
control flow, just emit a clear processLogger.Warn/processLogger.Warning (or the
project's logger) message referencing GetRenameTableArgs, args.job.ID, and
args.job.Version so failures aren’t silent.
---
Nitpick comments:
In `@logservice/schemastore/persist_storage_test.go`:
- Around line 3132-3174: The failing test doesn't reproduce the stale-snapshot
bug because tableMap[101].SchemaID remains 200 (old schema) so the old schema is
present; change the second case's tableMap entry for table id 101 to have
SchemaID: 100 (the new ArchiveDB schema) and then add assertions that
ddl.ExtraSchemaID == 100 and ddl.ExtraSchemaName == "ArchiveDB" (use the
persisted DDL from buildPersistedDDLEventForRenameTable and check
ddl.ExtraSchemaID / ddl.ExtraSchemaName) so the test covers the scenario where
the snapshot already shows the table in ArchiveDB; reference:
buildRenameTableJobForTest, RenameTableArgs,
buildPersistedDDLEventForRenameTable, BasicTableInfo, ddl.ExtraSchemaID,
ddl.ExtraSchemaName.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3eff6e30-1c19-40ed-8665-738606e98a49
📒 Files selected for processing (2)
logservice/schemastore/persist_storage_ddl_handlers.gologservice/schemastore/persist_storage_test.go
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a bug in the schema store's handling of Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a bug in handling RENAME TABLE DDLs, ensuring the original schema name is correctly determined, especially in cross-database rename scenarios where the schema is not explicit in the query. The fix correctly prioritizes RenameTableArgs from the DDL job as a source for the old schema name, which is more reliable than InvolvingSchemaInfo. The change is accompanied by a targeted unit test that validates the fix. The overall approach is sound. I have one suggestion to improve error logging for better observability.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
logservice/schemastore/persist_storage_ddl_handlers.go (1)
754-767: Consider wrapping errors from library calls.Per coding guidelines, errors from library calls should be wrapped with stack traces. The errors from
model.GetRenameTableArgsat lines 757 and 763 are returned unwrapped.Suggested fix
func getRenameTableArgsCompatible(job *model.Job) (*model.RenameTableArgs, error) { switch job.Version { case model.JobVersion1, model.JobVersion2: - return model.GetRenameTableArgs(job) + args, err := model.GetRenameTableArgs(job) + if err != nil { + return nil, cerror.WrapError(cerror.ErrDecodeFailed, err) + } + return args, nil case 0: // TiDB v7.5.0 historical jobs still encode rename-table args in v1 layout, // but the version field is left as zero. jobClone := *job jobClone.Version = model.JobVersion1 - return model.GetRenameTableArgs(&jobClone) + args, err := model.GetRenameTableArgs(&jobClone) + if err != nil { + return nil, cerror.WrapError(cerror.ErrDecodeFailed, err) + } + return args, nil default: - return nil, errors.New("unsupported rename table job version") + return nil, cerror.ErrDecodeFailed.GenWithStackByArgs("unsupported rename table job version") } }As per coding guidelines: "When an error comes from a third-party or library call in Go, wrap it immediately with
errors.Trace(err)orerrors.WrapError(...)to attach a stack trace."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@logservice/schemastore/persist_storage_ddl_handlers.go` around lines 754 - 767, The function getRenameTableArgsCompatible returns errors straight from model.GetRenameTableArgs; wrap those errors with a stack trace before returning (e.g., call errors.Trace(err) or the project's wrap helper) so callers get traceable errors. Update both call sites inside getRenameTableArgsCompatible (the branch returning model.GetRenameTableArgs(job) and the branch returning model.GetRenameTableArgs(&jobClone)) to capture the returned args and err, and if err != nil return nil, errors.Trace(err) (or errors.WrapError(err, "...") if your project prefers) instead of returning the raw err.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@logservice/schemastore/persist_storage_ddl_handlers.go`:
- Around line 754-767: The function getRenameTableArgsCompatible returns errors
straight from model.GetRenameTableArgs; wrap those errors with a stack trace
before returning (e.g., call errors.Trace(err) or the project's wrap helper) so
callers get traceable errors. Update both call sites inside
getRenameTableArgsCompatible (the branch returning model.GetRenameTableArgs(job)
and the branch returning model.GetRenameTableArgs(&jobClone)) to capture the
returned args and err, and if err != nil return nil, errors.Trace(err) (or
errors.WrapError(err, "...") if your project prefers) instead of returning the
raw err.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b626536b-a272-4ada-b931-9e3215f28ae9
📒 Files selected for processing (2)
logservice/schemastore/persist_storage_ddl_handlers.gologservice/schemastore/persist_storage_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- logservice/schemastore/persist_storage_test.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
logservice/schemastore/persist_storage_ddl_handlers.go (1)
703-767:⚠️ Potential issue | 🔴 CriticalThis only fixes the SQL text, not the old rename identity.
Line 751 rewrites
event.Query, but Line 1361 and Line 2239 later still consumeevent.ExtraSchemaID,event.ExtraSchemaName, andevent.ExtraTableNameas the pre-rename identity. In the stale-snapshot case you call out at Line 703, those fields stay on the post-rename state, so cross-database moves can still miss the schema change and rename filtering /TableNameChangecan still target the new name as if it were the old one. Please persist the recovered old schema ID/name/table name back intoevent.Extra*before rebuilding the query.🐛 Suggested fix
- oldSchemaName := "" - oldTableName := "" + oldSchemaID := event.ExtraSchemaID + oldSchemaName := "" + oldTableName := ""if renameArgs, err := getRenameTableArgsCompatible(args.job); err == nil { + if renameArgs.OldSchemaID != 0 { + oldSchemaID = renameArgs.OldSchemaID + } if renameArgs.OldSchemaName.O != "" { oldSchemaName = renameArgs.OldSchemaName.O } } ... + if oldSchemaName != "" { + if resolvedID, ok := findSchemaIDByName(args.databaseMap, oldSchemaName); ok { + oldSchemaID = resolvedID + } + } + if oldSchemaID != 0 { + event.ExtraSchemaID = oldSchemaID + } + if oldSchemaName != "" { + event.ExtraSchemaName = oldSchemaName + } + if oldTableName != "" { + event.ExtraTableName = oldTableName + } - if oldSchemaName != "" && oldTableName != "" { + if event.ExtraSchemaName != "" && event.ExtraTableName != "" { event.Query = fmt.Sprintf("RENAME TABLE %s TO %s", - common.QuoteSchema(oldSchemaName, oldTableName), + common.QuoteSchema(event.ExtraSchemaName, event.ExtraTableName), common.QuoteSchema(event.SchemaName, event.TableName)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@logservice/schemastore/persist_storage_ddl_handlers.go` around lines 703 - 767, The rebuild fixes only event.Query but not the pre-rename identity fields; update event.ExtraSchemaName, event.ExtraTableName (and event.ExtraSchemaID when available) with the recovered old values before rewriting the query. Concretely: after you compute oldSchemaName/oldTableName (using getRenameTableArgsCompatible, args.job.InvolvingSchemaInfo, and parseRenameTableQueryInfo) assign event.ExtraSchemaName = oldSchemaName and event.ExtraTableName = oldTableName, and if renameArgs.OldSchemaID is set assign event.ExtraSchemaID = renameArgs.OldSchemaID, then proceed to set event.Query = fmt.Sprintf(...). This ensures later consumers that read event.Extra* see the true pre-rename identity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@logservice/schemastore/persist_storage_ddl_handlers.go`:
- Around line 703-767: The rebuild fixes only event.Query but not the pre-rename
identity fields; update event.ExtraSchemaName, event.ExtraTableName (and
event.ExtraSchemaID when available) with the recovered old values before
rewriting the query. Concretely: after you compute oldSchemaName/oldTableName
(using getRenameTableArgsCompatible, args.job.InvolvingSchemaInfo, and
parseRenameTableQueryInfo) assign event.ExtraSchemaName = oldSchemaName and
event.ExtraTableName = oldTableName, and if renameArgs.OldSchemaID is set assign
event.ExtraSchemaID = renameArgs.OldSchemaID, then proceed to set event.Query =
fmt.Sprintf(...). This ensures later consumers that read event.Extra* see the
true pre-rename identity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bc57fdac-5a99-409c-abba-e1126a78c775
📒 Files selected for processing (1)
logservice/schemastore/persist_storage_ddl_handlers.go
|
/gemini summary |
lidezhu
changed the title
1 participant
What problem does this PR solve?
Issue Number: close #4424
What is changed and how it works?
This pull request addresses a bug in the schema store's handling of
RENAME TABLEDDL events, particularly when dealing with cross-database renames. The changes enhance the accuracy of identifying the original schema and table names, ensuring that the persisted DDL events correctly reflect the source and target qualifiers. This leads to more reliable DDL event generation and improved data consistency across different database operations.Highlights
RENAME TABLEDDL event processing has been enhanced to ensure accurate identification, especially for cross-database renames.parseRenameTableQueryInfo, was introduced to robustly extract old schema and table names from DDL queries, centralizing and improving parsing logic.Check List
Tests
Questions
Will it cause performance regression or break compatibility?
Do you need to update user documentation, design documentation or monitoring documentation?
Release note
Summary by CodeRabbit