schemastore: fix old table metadata for rename table - #5943
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
do-not-merge/work-in-progress
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe rename-table handler reconstructs the old schema and table identity from rename metadata, SQL, and database mappings. It stores the recovered identity in persisted event fields and adds unit and integration coverage for rename handling. ChangesRename Metadata Recovery
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
ti-chi-bot
Bot
added
size/L
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
|
@coderabbitai review |
|
![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 (3)
logservice/schemastore/persist_storage_ddl_handlers.go (2)
863-885: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrack the query-provided schema name with an explicit flag.
Line 879 reads
queryInfo.oldSchemaNameoutside theparsedguard. This works today becauseparseRenameTableQueryInforeturns a zero-value struct withfalse. The coupling is implicit. If that function later returns partial data withfalse, the capitalization branch changes behavior silently.♻️ Proposed refactor
queryInfo, parsed := parseRenameTableQueryInfo(args.job.Query) + schemaNameFromQuery := false if parsed { if queryInfo.oldTableName != "" { oldTableName = queryInfo.oldTableName } if queryInfo.oldSchemaName != "" { oldSchemaName = queryInfo.oldSchemaName oldSchemaSource = "query" + schemaNameFromQuery = true } } // Complete a missing old schema ID or name through databaseMap. if oldSchemaID == 0 && oldSchemaName != "" { oldSchemaID, _ = findSchemaIDByName(args.databaseMap, oldSchemaName) } - if queryInfo.oldSchemaName == "" { + if !schemaNameFromQuery { if oldSchema, ok := args.databaseMap[oldSchemaID]; ok {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@logservice/schemastore/persist_storage_ddl_handlers.go` around lines 863 - 885, Track whether the parsed query explicitly provided an old schema using a local boolean set only when parseRenameTableQueryInfo returns parsed data with a non-empty oldSchemaName. Use that flag in the databaseMap capitalization fallback instead of reading queryInfo.oldSchemaName outside the parsed guard, while preserving the existing oldSchemaName and oldSchemaSource assignments.
903-925: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReconsider the log level for the query rebuild.
Line 910 emits
log.Infofor every rename-table job. SchemaStore replays the full DDL history at bootstrap. A cluster with many historical renames produces one info line per rename. Considerlog.Debug, or keeplog.Infoonly when the recovered identity differs from the snapshot identity.The repository guidelines ask to consult
docs/agents/logging.mdbefore adding logs. Confirm the wanted level there.As per coding guidelines: "Logs are operational signals; see docs/agents/logging.md before adding, removing, or rewriting logs."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@logservice/schemastore/persist_storage_ddl_handlers.go` around lines 903 - 925, Review docs/agents/logging.md and adjust the log level in the rename-query rebuild block around the log call, reducing the per-job message from log.Info to the repository-approved level (likely log.Debug) unless the recovered identity differs from the snapshot identity and warrants Info. Preserve the existing structured fields and query-rebuild behavior.Source: Coding guidelines
logservice/schemastore/persist_storage_test.go (1)
3233-3285: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
tableMapfallback branch.Both subtests exercise paths where the job supplies a complete old identity, so the fallback block at
persist_storage_ddl_handlers.golines 887-901 never runs. That block contains thelog.Panicpaths I flagged on the handler.Add a subtest with an unparsable
job.Query, noInvolvingSchemaInfo, and no decodable args. Assert that theExtra*fields come fromtableMapanddatabaseMap. Add a second case wheredatabaseMapomits the old schema ID, to pin the wanted behavior instead of a panic.As per coding guidelines: "Prefer focused deterministic tests; see docs/agents/testing.md before adding or changing tests."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@logservice/schemastore/persist_storage_test.go` around lines 3233 - 3285, Add focused subtests for the tableMap fallback used by buildPersistedDDLEventForRenameTable: use an unparsable job.Query with no InvolvingSchemaInfo and undecodable args, then assert ExtraSchemaID, ExtraSchemaName, and ExtraTableName are derived from tableMap and databaseMap. Add a second case with the old schema ID absent from databaseMap and assert the handler completes with the intended non-panicking fallback behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@logservice/schemastore/persist_storage_ddl_handlers.go`:
- Around line 887-901: Update the fallback block around oldSchemaID,
oldSchemaName, and oldTableName so getSchemaID runs only when oldSchemaID is
zero and getTableName runs only when oldTableName is empty, avoiding helper
calls when tableMap lacks event.TableID. Resolve a missing oldSchemaName
directly from args.databaseMap[oldSchemaID], preserving an empty name for
untracked schema IDs and avoiding getSchemaName; ensure event.Query remains
unchanged when the old schema name is unavailable.
---
Nitpick comments:
In `@logservice/schemastore/persist_storage_ddl_handlers.go`:
- Around line 863-885: Track whether the parsed query explicitly provided an old
schema using a local boolean set only when parseRenameTableQueryInfo returns
parsed data with a non-empty oldSchemaName. Use that flag in the databaseMap
capitalization fallback instead of reading queryInfo.oldSchemaName outside the
parsed guard, while preserving the existing oldSchemaName and oldSchemaSource
assignments.
- Around line 903-925: Review docs/agents/logging.md and adjust the log level in
the rename-query rebuild block around the log call, reducing the per-job message
from log.Info to the repository-approved level (likely log.Debug) unless the
recovered identity differs from the snapshot identity and warrants Info.
Preserve the existing structured fields and query-rebuild behavior.
In `@logservice/schemastore/persist_storage_test.go`:
- Around line 3233-3285: Add focused subtests for the tableMap fallback used by
buildPersistedDDLEventForRenameTable: use an unparsable job.Query with no
InvolvingSchemaInfo and undecodable args, then assert ExtraSchemaID,
ExtraSchemaName, and ExtraTableName are derived from tableMap and databaseMap.
Add a second case with the old schema ID absent from databaseMap and assert the
handler completes with the intended non-panicking fallback behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a6e0e724-9295-4fd0-8f10-682e3254091c
📒 Files selected for processing (2)
logservice/schemastore/persist_storage_ddl_handlers.gologservice/schemastore/persist_storage_test.go
ti-chi-bot
Bot
added
size/XL
|
/test pull-error-log-review |
|
/retest |
|
/test pull-error-log-review |
|
/retest |
|
/test pull-error-log-review |
|
Can we make an integration test case to prove it? |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 3AceShowHand The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration_tests/rename_table_start_ts/run.sh`:
- Around line 65-66: After the successful gc_worker safepoint update in run.sh,
register an EXIT trap that restores gc_worker_value before invoking stop_test.
Ensure the cleanup runs when run_cdc_server or ensure exits the script, while
preserving the existing stop_test behavior and avoiding duplicate trap
registration.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 45b02fd1-d882-4a9b-9e88-087d317f0135
📒 Files selected for processing (5)
logservice/schemastore/persist_storage_ddl_handlers.gotests/integration_tests/rename_table_start_ts/conf/changefeed.tomltests/integration_tests/rename_table_start_ts/run.shtests/integration_tests/rename_table_start_ts/set_gc_safepoint.gotests/integration_tests/run_light_it_in_ci.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- logservice/schemastore/persist_storage_ddl_handlers.go
We cannot add a reliable integration test because SchemaStore initializes from the GC safepoint, whose exact value is difficult to control. |
4 participants
What problem does this PR solve?
Issue Number: close #5946
What is changed and how it works?
ExtraSchemaID,ExtraSchemaName, andExtraTableName.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