schemastore: fix rename tables when tidb version <= v8.1.x#4388
schemastore: fix rename tables when tidb version <= v8.1.x#4388ti-chi-bot[bot] merged 19 commits intomasterfrom
Conversation
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
release-note
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 an issue with 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
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
|
ti-chi-bot
bot
added
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:
📝 WalkthroughWalkthroughAdds parsing helpers and resilient handling for RENAME TABLE jobs: extracts rename mappings from SQL when job args are incomplete, reconciles identifiers with schema-store metadata, logs warnings on fallbacks/mismatches, and updates persisted DDL assembly. Tests for fallback and cyclic rename cases are added. Changes
Sequence Diagram(s)sequenceDiagram
participant Job as JobArgs
participant Handler as DDLHandler
participant Parser as SQLParser
participant Store as SchemaStore
participant Logger as Logger
participant Emitter as EventEmitter
Job->>Handler: provide job (Args, maybe Query)
Handler->>Handler: safe decode rename args
alt decoded has complete old identifiers
Handler->>Handler: use decoded old/new identifiers
else decoded incomplete
Handler->>Parser: parse Query (parseRenameTablesQueryInfos)
Parser-->>Handler: rename pairs
Handler->>Store: lookup schema IDs/names (getSchemaIDByName / metadata)
Store-->>Handler: metadata (table info)
Handler->>Handler: match parsed info to entries (matchRenameQueryInfoByNewTable)
Handler->>Logger: warn about fallbacks/mismatches
end
Handler->>Emitter: emit persisted DDL event (resolved old/new identifiers, RENAME TABLE SQL)
Emitter-->>Handler: ack
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![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 introduces a fallback mechanism in buildPersistedDDLEventForRenameTables to handle cases where DDL job arguments for RENAME TABLES might be incomplete, using metadata from the schema store to fill in missing identifiers and including a unit test for this new logic. However, the manual reconstruction of the RENAME TABLE SQL query at line 850 remains vulnerable to SQL injection because it does not escape backticks in schema or table names, which could allow an attacker with DDL privileges to execute arbitrary SQL; it is recommended to use the common.QuoteSchema utility for safe reconstruction. Additionally, a potential inconsistency in the fallback logic regarding the schema ID used for looking up the schema name should be addressed to ensure it aligns with the schema ID being used for the event, preventing potential discrepancies.
![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 (2)
logservice/schemastore/persist_storage_ddl_handlers.go (2)
848-849: RenameSchemaNametoschemaName.Uppercase local names read like exported identifiers and break the repo's Go naming rule.
As per coding guidelines, "Use lowerCamelCase for variable names in Go (e.g.,
flushInterval, notflush_interval)".🤖 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 848 - 849, The local variable name SchemaName should be renamed to lowerCamelCase schemaName to follow Go naming conventions; update the assignment from getSchemaName(args.databaseMap, info.NewSchemaID) to assign to schemaName and then append schemaName to event.SchemaNames so references around getSchemaName, args.databaseMap, info.NewSchemaID, and event.SchemaNames use the new variable name.
820-846: Resolve fallback old identifiers as one tuple.Once fallback is needed, this block only patches the missing fields. That can leave
ExtraSchemaIDscoming fromRenameTableInfowhileExtraSchemaNames/ExtraTableNamescome from schema-store metadata, and those are consumed by different downstream paths in this file. Prefer rebuilding the full old(schemaID, schemaName, tableName)tuple from schema-store state once fallback is triggered, and fail fast ifinfo.TableIDcannot be found.Suggested hardening
oldSchemaID := info.OldSchemaID oldSchemaName := info.OldSchemaName.O oldTableName := info.OldTableName.O if oldSchemaName == "" || oldTableName == "" || oldSchemaID == 0 { - if tableInfo, ok := args.tableMap[info.TableID]; ok { - if oldSchemaID == 0 { - oldSchemaID = tableInfo.SchemaID - } - if oldSchemaName == "" { - oldSchemaName = getSchemaName(args.databaseMap, tableInfo.SchemaID) - } - if oldTableName == "" { - oldTableName = tableInfo.Name - } - log.Warn("rename tables args miss old table identifiers fallback to schema store metadata", - zap.Int64("tableID", info.TableID), - zap.Int64("oldSchemaIDInArgs", info.OldSchemaID), - zap.String("oldSchemaNameInArgs", info.OldSchemaName.O), - zap.String("oldTableNameInArgs", info.OldTableName.O), - zap.Int64("oldSchemaIDInStore", tableInfo.SchemaID), - zap.String("oldTableNameInStore", tableInfo.Name)) - } + tableInfo, ok := args.tableMap[info.TableID] + if !ok { + log.Panic("rename tables old table not found in schema store", + zap.Int64("tableID", info.TableID), + zap.Int64("oldSchemaIDInArgs", info.OldSchemaID), + zap.String("oldSchemaNameInArgs", info.OldSchemaName.O), + zap.String("oldTableNameInArgs", info.OldTableName.O)) + } + oldSchemaID = tableInfo.SchemaID + oldSchemaName = getSchemaName(args.databaseMap, tableInfo.SchemaID) + oldTableName = tableInfo.Name + log.Warn("rename tables args miss old table identifiers fallback to schema store metadata", + zap.Int64("tableID", info.TableID), + zap.Int64("oldSchemaIDInArgs", info.OldSchemaID), + zap.String("oldSchemaNameInArgs", info.OldSchemaName.O), + zap.String("oldTableNameInArgs", info.OldTableName.O), + zap.Int64("oldSchemaIDInStore", tableInfo.SchemaID), + zap.String("oldSchemaNameInStore", oldSchemaName), + zap.String("oldTableNameInStore", tableInfo.Name)) }🤖 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 820 - 846, When any of info.OldSchemaID, info.OldSchemaName.O, or info.OldTableName.O is missing, don't patch fields individually; instead require lookup of args.tableMap[info.TableID], fail fast if not found, and rebuild the full old tuple (schemaID := tableInfo.SchemaID; schemaName := getSchemaName(args.databaseMap, schemaID); tableName := tableInfo.Name) so event.ExtraSchemaIDs, event.ExtraSchemaNames and event.ExtraTableNames are all appended from the same source. Replace the existing partial-field-fallback logic around those symbols (info, args.tableMap, tableInfo, getSchemaName, event.ExtraSchemaIDs/Names/TableNames) with this atomic replacement and adjust the log to reflect that a full tuple fallback occurred.
🤖 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_test.go`:
- Around line 3134-3162: The test for rename fallback is non-discriminative
because both source and destination names are identical; update the test data in
buildRenameTablesJobForTest/buildPersistedDDLEventForRenameTables to use
different destination table/schema names (e.g., "orders_new", "users_new" and
corresponding "sc_predeleted_test_predelete_new") so the expected ddl.Query,
ddl.ExtraTableNames, ddl.ExtraSchemaNames and ddl.SchemaNames assertions will
fail if the code erroneously reads NewTableName instead of the old one, and
replace assert.Equal calls with require.Equal from testify/require to make each
assertion a prerequisite (referencing ddl.Query, ddl.ExtraTableNames,
ddl.ExtraSchemaNames, ddl.SchemaNames and the helper builders
buildRenameTablesJobForTest and buildPersistedDDLEventForRenameTables).
---
Nitpick comments:
In `@logservice/schemastore/persist_storage_ddl_handlers.go`:
- Around line 848-849: The local variable name SchemaName should be renamed to
lowerCamelCase schemaName to follow Go naming conventions; update the assignment
from getSchemaName(args.databaseMap, info.NewSchemaID) to assign to schemaName
and then append schemaName to event.SchemaNames so references around
getSchemaName, args.databaseMap, info.NewSchemaID, and event.SchemaNames use the
new variable name.
- Around line 820-846: When any of info.OldSchemaID, info.OldSchemaName.O, or
info.OldTableName.O is missing, don't patch fields individually; instead require
lookup of args.tableMap[info.TableID], fail fast if not found, and rebuild the
full old tuple (schemaID := tableInfo.SchemaID; schemaName :=
getSchemaName(args.databaseMap, schemaID); tableName := tableInfo.Name) so
event.ExtraSchemaIDs, event.ExtraSchemaNames and event.ExtraTableNames are all
appended from the same source. Replace the existing partial-field-fallback logic
around those symbols (info, args.tableMap, tableInfo, getSchemaName,
event.ExtraSchemaIDs/Names/TableNames) with this atomic replacement and adjust
the log to reflect that a full tuple fallback occurred.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2566de92-5fca-4b53-bff4-720d7f61c285
📒 Files selected for processing (2)
logservice/schemastore/persist_storage_ddl_handlers.gologservice/schemastore/persist_storage_test.go
ti-chi-bot
bot
added
size/XL
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 1023-1035: The current code silently truncates renameTableInfos
and multipleTableInfos to minLen which can partially apply a committed
multi-table RENAME; instead, detect the length mismatch (the if block comparing
len(renameTableInfos) and len(multipleTableInfos)) and fail fast: log an error
with context (use zap fields like renameArgsLen, multipleTableInfosLen, query
from args.job.Query), and return an error (or mark the job as failed) rather
than slicing the slices; do this in the same scope where renameTableInfos and
multipleTableInfos are compared so no partial application occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab8d3e9f-0f4c-4533-818a-6561f463cb0f
📒 Files selected for processing (2)
logservice/schemastore/persist_storage_ddl_handlers.gologservice/schemastore/persist_storage_test.go
lidezhu
changed the title
ti-chi-bot
bot
added
do-not-merge/needs-triage-completed
and removed
do-not-merge/needs-triage-completed
labels
ti-chi-bot
bot
added
size/L
There was a problem hiding this comment.
🧹 Nitpick comments (3)
logservice/schemastore/persist_storage_test.go (2)
3165-3194: Userequire.Equalinstead ofassert.Equalfor consistency.Same concern as the previous test function. Use
require.Equalfor fail-fast behavior.♻️ Suggested fix
- assert.Equal(t, + require.Equal(t, "RENAME TABLE `source_db`.`source_t1` TO `target_db`.`target_t1`;"+ "RENAME TABLE `source_db`.`source_t2` TO `target_db`.`target_t2`;", ddl.Query) - assert.Equal(t, []string{"source_t1", "source_t2"}, ddl.ExtraTableNames) - assert.Equal(t, []string{"source_db", "source_db"}, ddl.ExtraSchemaNames) + require.Equal(t, []string{"source_t1", "source_t2"}, ddl.ExtraTableNames) + require.Equal(t, []string{"source_db", "source_db"}, ddl.ExtraSchemaNames)As per coding guidelines, "
**/*_test.go: Use unit test files named*_test.goin Go; favor deterministic tests and usetestify/require".🤖 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 3165 - 3194, The test TestBuildPersistedDDLEventForRenameTablesFallbackQueryOldTableName is using assert.Equal which is inconsistent with project guidelines; replace the three assert.Equal calls for ddl.Query, ddl.ExtraTableNames, and ddl.ExtraSchemaNames with require.Equal to make the test fail fast (and ensure the test file imports testify/require if not already). Locate the assertions in TestBuildPersistedDDLEventForRenameTablesFallbackQueryOldTableName and change assert.Equal(...) to require.Equal(...), adding or updating the require import as needed.
3133-3163: Userequire.Equalinstead ofassert.Equalper coding guidelines.The test correctly discriminates between old and new table names by using distinct values. However, per coding guidelines, tests should use
testify/requirefor assertions to fail fast on the first mismatch.♻️ Suggested fix
- assert.Equal(t, + require.Equal(t, "RENAME TABLE `source_db`.`source_t1` TO `target_db`.`target_t1`;"+ "RENAME TABLE `source_db`.`source_t2` TO `target_db`.`target_t2`;", ddl.Query) - assert.Equal(t, []string{"source_t1", "source_t2"}, ddl.ExtraTableNames) - assert.Equal(t, []string{"source_db", "source_db"}, ddl.ExtraSchemaNames) - assert.Equal(t, []string{"target_db", "target_db"}, ddl.SchemaNames) + require.Equal(t, []string{"source_t1", "source_t2"}, ddl.ExtraTableNames) + require.Equal(t, []string{"source_db", "source_db"}, ddl.ExtraSchemaNames) + require.Equal(t, []string{"target_db", "target_db"}, ddl.SchemaNames)As per coding guidelines, "
**/*_test.go: Use unit test files named*_test.goin Go; favor deterministic tests and usetestify/require".🤖 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 3133 - 3163, The test TestBuildPersistedDDLEventForRenameTablesFallbackOldTableName uses assert.Equal which should be changed to require.Equal to fail fast; update the three assert.Equal calls (comparing ddl.Query, ddl.ExtraTableNames, ddl.ExtraSchemaNames, ddl.SchemaNames) to require.Equal and add the require import ("github.com/stretchr/testify/require") in the test file (removing or keeping the assert import only as needed) so the test fails immediately on the first mismatch.logservice/schemastore/persist_storage_ddl_handlers.go (1)
880-887:getSchemaIDByNamereturns 0 on not-found, which may be ambiguous.Returning
0when the schema is not found could be confused with a valid (though unlikely) schema ID. Consider returning(int64, bool)to explicitly signal whether the schema was found, similar to the existingfindSchemaIDByNamefunction defined at lines 521-528.♻️ Suggested signature change
-func getSchemaIDByName(databaseMap map[int64]*BasicDatabaseInfo, schemaName string) int64 { +func getSchemaIDByName(databaseMap map[int64]*BasicDatabaseInfo, schemaName string) (int64, bool) { for schemaID, databaseInfo := range databaseMap { if strings.EqualFold(databaseInfo.Name, schemaName) { - return schemaID + return schemaID, true } } - return 0 + return 0, false }Then update call sites to handle the boolean return value.
🤖 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 880 - 887, Change getSchemaIDByName to return (int64, bool) instead of int64 and mirror the behavior of findSchemaIDByName: iterate databaseMap (map[int64]*BasicDatabaseInfo) and return (schemaID, true) when strings.EqualFold matches, otherwise return (0, false); then update all call sites of getSchemaIDByName to handle the boolean (check the found flag and handle the not-found case explicitly rather than assuming 0 is invalid).
🤖 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 880-887: Change getSchemaIDByName to return (int64, bool) instead
of int64 and mirror the behavior of findSchemaIDByName: iterate databaseMap
(map[int64]*BasicDatabaseInfo) and return (schemaID, true) when
strings.EqualFold matches, otherwise return (0, false); then update all call
sites of getSchemaIDByName to handle the boolean (check the found flag and
handle the not-found case explicitly rather than assuming 0 is invalid).
In `@logservice/schemastore/persist_storage_test.go`:
- Around line 3165-3194: The test
TestBuildPersistedDDLEventForRenameTablesFallbackQueryOldTableName is using
assert.Equal which is inconsistent with project guidelines; replace the three
assert.Equal calls for ddl.Query, ddl.ExtraTableNames, and ddl.ExtraSchemaNames
with require.Equal to make the test fail fast (and ensure the test file imports
testify/require if not already). Locate the assertions in
TestBuildPersistedDDLEventForRenameTablesFallbackQueryOldTableName and change
assert.Equal(...) to require.Equal(...), adding or updating the require import
as needed.
- Around line 3133-3163: The test
TestBuildPersistedDDLEventForRenameTablesFallbackOldTableName uses assert.Equal
which should be changed to require.Equal to fail fast; update the three
assert.Equal calls (comparing ddl.Query, ddl.ExtraTableNames,
ddl.ExtraSchemaNames, ddl.SchemaNames) to require.Equal and add the require
import ("github.com/stretchr/testify/require") in the test file (removing or
keeping the assert import only as needed) so the test fails immediately on the
first mismatch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 29757d1e-f8bf-435d-9458-a6afc5b6ef14
📒 Files selected for processing (2)
logservice/schemastore/persist_storage_ddl_handlers.gologservice/schemastore/persist_storage_test.go
lidezhu
changed the title
ti-chi-bot
bot
added
size/XL
There was a problem hiding this comment.
🧹 Nitpick comments (2)
logservice/schemastore/persist_storage_ddl_handlers.go (2)
976-984: Consider adding debug/warning logs for final fallback assignments.When old identifiers cannot be determined from any source (args, parsed query, or schema store), the code silently falls back to using new identifiers as old identifiers. This could mask underlying issues and make debugging difficult.
Suggested logging addition
if oldSchemaID == 0 { oldSchemaID = info.NewSchemaID + log.Warn("rename tables cannot determine old schema ID fallback to new schema ID", + zap.Int64("tableID", info.TableID), + zap.Int64("newSchemaID", info.NewSchemaID)) } if oldSchemaName == "" { oldSchemaName = newSchemaName + log.Warn("rename tables cannot determine old schema name fallback to new schema name", + zap.Int64("tableID", info.TableID), + zap.String("newSchemaName", newSchemaName)) } if oldTableName == "" { oldTableName = info.NewTableName.O + log.Warn("rename tables cannot determine old table name fallback to new table name", + zap.Int64("tableID", info.TableID), + zap.String("newTableName", info.NewTableName.O)) }🤖 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 976 - 984, The code silently falls back to new identifiers when old ones are missing (the blocks setting oldSchemaID = info.NewSchemaID, oldSchemaName = newSchemaName, oldTableName = info.NewTableName.O); add logging to surface these fallback events by emitting a debug or warning log that includes which identifier was missing and the fallback value (e.g., log oldSchemaID fallback with info.NewSchemaID, oldSchemaName fallback with newSchemaName, and oldTableName fallback with info.NewTableName.O) so callers can trace unexpected fallbacks; ensure logs reference the context (operation/statement id or function) and use the existing logger used elsewhere in this file.
880-887: Consider consolidating withfindSchemaIDByNamefor consistency.There's a similar function
findSchemaIDByNameat line 521 that returns(int64, bool)and uses exact string comparison. This new function uses case-insensitive comparison and returns0for not found.While the case-insensitive matching aligns with the
matchRenameQueryInfoByNewTablelogic, having two similar functions with different behaviors could be confusing. Consider:
- Reusing
findSchemaIDByNameand handling case-insensitivity at the call site if needed, or- Adding a comment explaining why case-insensitive matching is required here.
🤖 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 880 - 887, There are two similar helpers—getSchemaIDByName (case-insensitive, returns 0) and findSchemaIDByName (exact match, returns (int64,bool)) which is confusing; either consolidate by changing findSchemaIDByName to accept a caseInsensitive bool (or add an overloaded helper) and update callers to use the unified function, or keep the current case-insensitive behavior but add a clear comment above getSchemaIDByName explaining why case-insensitive matching is required (and document the 0-not-found sentinel) so callers understand the difference; reference the functions findSchemaIDByName and getSchemaIDByName when making the change.
🤖 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 976-984: The code silently falls back to new identifiers when old
ones are missing (the blocks setting oldSchemaID = info.NewSchemaID,
oldSchemaName = newSchemaName, oldTableName = info.NewTableName.O); add logging
to surface these fallback events by emitting a debug or warning log that
includes which identifier was missing and the fallback value (e.g., log
oldSchemaID fallback with info.NewSchemaID, oldSchemaName fallback with
newSchemaName, and oldTableName fallback with info.NewTableName.O) so callers
can trace unexpected fallbacks; ensure logs reference the context
(operation/statement id or function) and use the existing logger used elsewhere
in this file.
- Around line 880-887: There are two similar helpers—getSchemaIDByName
(case-insensitive, returns 0) and findSchemaIDByName (exact match, returns
(int64,bool)) which is confusing; either consolidate by changing
findSchemaIDByName to accept a caseInsensitive bool (or add an overloaded
helper) and update callers to use the unified function, or keep the current
case-insensitive behavior but add a clear comment above getSchemaIDByName
explaining why case-insensitive matching is required (and document the
0-not-found sentinel) so callers understand the difference; reference the
functions findSchemaIDByName and getSchemaIDByName when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ad0e37fd-7f17-4012-a575-e047dc9f0077
📒 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
ti-chi-bot
bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wk989898, wlwilliamx 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:
|
|
/test all-v7-5 |
|
/test all |
|
/test all-v7-5 |
|
/test all-v8-1 |
ti-chi-bot
bot
added
size/XXL
|
/test all-v8-1 |
|
/test all-v8-1 |
|
/test all |
|
@lidezhu: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/unhold |
ti-chi-bot
bot
removed
the
do-not-merge/hold
lidezhu
added
the
needs-cherry-pick-release-8.5
|
/run-cherry-picker |
|
In response to a cherrypick label: new pull request created to branch |
5 participants
What problem does this PR solve?
Issue Number: close #4392
What is changed and how it works?
This pull request significantly improves the robustness and correctness of DDL event handling within the schema store, particularly for
RENAME TABLESoperations. It addresses a critical issue where older TiDB versions might provide incomplete old table information, by introducing a fallback mechanism that parses this data directly from the DDL query. Furthermore, it fortifies the system against potential SQL injection by ensuring all generated DDL queries properly quote schema and table identifiers. These changes enhance the reliability of schema synchronization and DDL application.Highlights
RENAME TABLESDDL events to derive missing old table names from the original SQL query, specifically addressing issues in TiDB versions <= v8.1.x where this information might be incomplete.DROP VIEW,DROP TABLE,RENAME TABLE,EXCHANGE PARTITION) to consistently usecommon.QuoteSchemaandcommon.QuoteNamefor proper identifier escaping, mitigating potential SQL injection vulnerabilities.findSchemaIDByName,findTableIDByName) to perform case-insensitive comparisons, improving flexibility and correctness.RENAME TABLESoperations, ensuring that multi-table DDLs are applied atomically and preventing partial schema updates.RENAME TABLES, cyclic rename scenarios, identifier escaping, and case-insensitive name matching.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
Bug Fixes
Tests