schemastore: fix rename tables when tidb version <= v8.1.x (#4388)#4410
Conversation
ti-chi-bot
added
lgtm
release-note
|
This cherry pick PR is for a release branch and has not yet been approved by triage owners. To merge this cherry pick:
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. |
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 significantly enhances the schema store's handling of DDL events, particularly for 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
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 significant improvements to DDL event handling, particularly for RENAME TABLES operations, by adding a fallback to parse the original SQL query. This addresses issues with incomplete information from older TiDB versions. The changes also enhance security by consistently quoting identifiers in generated DDL queries, mitigating potential SQL injection risks. Additionally, lookups for schema and table names are now case-insensitive, and error handling for multi-table renames has been made more robust. The new logic is well-supported by extensive unit tests. My review found one minor issue with unquoting partition names that could lead to incorrect DDL generation in some edge cases.
| @@ -787,8 +792,10 @@ func buildPersistedDDLEventForExchangePartition(args buildPersistedDDLEventFuncA | |||
| // Note that partition name should be parsed from original query, not the upperQuery. | |||
| partName := strings.TrimSpace(event.Query[idx1:idx2]) | |||
| partName = strings.Replace(partName, "`", "", -1) | |||
There was a problem hiding this comment.
The use of strings.Replace(partName, "", "", -1)` to unquote the partition name is incorrect as it removes all backticks, which can corrupt partition names that legitimately contain a backtick. A proper unquoting logic should be used that handles surrounding backticks and escaped backticks.
if len(partName) > 1 && partName[0] == '`' && partName[len(partName)-1] == '`' {
partName = strings.ReplaceAll(partName[1:len(partName)-1], "``", "`")
}
|
/test all |
|
[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 |
|
/test all |
|
@ti-chi-bot: The following test 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. |
2 participants
This is an automated cherry-pick of #4388
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