Skip to content

ticdc: unquote exchange partition names when rebuilding ddl - #12762

Open
haiboumich wants to merge 1 commit into
pingcap:masterfrom
haiboumich:fix-exchange-partition-quote
Open

ticdc: unquote exchange partition names when rebuilding ddl#12762
haiboumich wants to merge 1 commit into
pingcap:masterfrom
haiboumich:fix-exchange-partition-quote

Conversation

@haiboumich

@haiboumich haiboumich commented Jul 22, 2026

Copy link
Copy Markdown

What problem does this PR solve?

Issue Number: close #12761

What is changed and how it works?

EXCHANGE PARTITION DDLs may already contain quoted partition names in job.Query. When TiCDC rebuilds the DDL directly from that SQL fragment, it may wrap an already-quoted partition name again, producing doubled backticks and invalid downstream DDL.

This PR fixes the issue by unquoting the parsed partition name before quoting it again during DDL reconstruction.

Changes in this PR:

  • add a helper to remove one layer of MySQL identifier quoting
  • unquote the parsed partition name before rebuilding EXCHANGE PARTITION DDL
  • add unit tests for the identifier helper
  • add unit tests for the EXCHANGE PARTITION DDL path, including quoted partition names

Check List

Tests

  • Unit test

Questions

Will it cause performance regression or break compatibility?

No. This change only affects the DDL reconstruction path for EXCHANGE PARTITION and does not change normal replication behavior for other DDLs.

Do you need to update user documentation, design documentation or monitoring documentation?

No.

Release note

Fix a bug where TiCDC could generate invalid `EXCHANGE PARTITION` DDL with doubled backticks when the partition name in the upstream DDL was already quoted.

<!-- This is an auto-generated comment: release notes by coderabbit.ai -->

## Summary by CodeRabbit

- **Bug Fixes**
  - Fixed table-partition exchange statements involving quoted or escaped partition names.
  - Improved identifier handling to ensure generated SQL preserves names correctly and safely.
  - Ensured exchange statements retain the `WITHOUT VALIDATION` option where applicable.

- **Tests**
  - Added coverage for quoted partition names, escaped identifiers, and identifier unquoting behavior.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Exchange partition DDLs may already contain quoted partition names in job.Query.
Rebuilding the SQL without removing that outer quoting produces doubled
backticks and invalid downstream DDL.

Unquote the parsed partition name before re-quoting it, and add unit
tests for both identifier helpers and the exchange partition path.
@ti-chi-bot

ti-chi-bot Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Details

Instructions 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.

@ti-chi-bot ti-chi-bot Bot added first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. do-not-merge/needs-triage-completed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jul 22, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign leavrth for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 22, 2026
@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

Exchange partition quoting

Layer / File(s) Summary
Identifier unquoting contract
pkg/quotes/quotes.go, pkg/quotes/quotes_test.go
Adds UnquoteName and tests handling of quoted, escaped, raw, and empty identifiers.
EXCHANGE PARTITION SQL reconstruction
cdc/model/sink.go, cdc/model/sink_test.go
Rebuilds EXCHANGE PARTITION DDL with quote helpers and tests quoted partition output, event type, and WITHOUT VALIDATION.

Estimated code review effort: 2 (Simple) | ~10 minutes

Poem

I’m a rabbit hopping through quoted names,
Doubling backticks no longer plays games.
Partitions emerge neat, escaped, and bright,
DDL hops safely through the night.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: unquoting exchange partition names during DDL rebuild.
Description check ✅ Passed The description follows the template with issue number, change summary, tests, questions, and a release note.
Linked Issues check ✅ Passed The code addresses #12761 by preserving quoted partition identifiers during EXCHANGE PARTITION DDL reconstruction.
Out of Scope Changes check ✅ Passed The changes are scoped to the reported DDL reconstruction bug and related unit tests, with no unrelated additions.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions
The command is terminated due to an error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
cdc/model/sink_test.go (1)

463-463: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Pass the expected value first to require.Equal.

Line 463 currently passes event.Type as the expected value and the constant as the actual value, which makes failure diagnostics misleading. Use require.Equal(t, timodel.ActionExchangeTablePartition, event.Type) instead.

🤖 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 `@cdc/model/sink_test.go` at line 463, Update the require.Equal assertion in
the sink test to pass timodel.ActionExchangeTablePartition as the expected value
and event.Type as the actual value, preserving the existing assertion.
🤖 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.

Nitpick comments:
In `@cdc/model/sink_test.go`:
- Line 463: Update the require.Equal assertion in the sink test to pass
timodel.ActionExchangeTablePartition as the expected value and event.Type as the
actual value, preserving the existing assertion.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 49dd0dda-fd96-4048-903a-ef37d72b3c83

📥 Commits

Reviewing files that changed from the base of the PR and between 4a8a84d and 86e4cb0.

📒 Files selected for processing (4)
  • cdc/model/sink.go
  • cdc/model/sink_test.go
  • pkg/quotes/quotes.go
  • pkg/quotes/quotes_test.go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/needs-triage-completed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TiCDC may rebuild EXCHANGE PARTITION DDL with doubled backticks in partition name, causing ErrConvertDDLToEventTypeFailed

1 participant