pkg/ddl: fix exchange partition metadata mismatch for shard_row_id_bits#66632
pkg/ddl: fix exchange partition metadata mismatch for shard_row_id_bits#66632ti-chi-bot[bot] merged 3 commits intopingcap:masterfrom
Conversation
ti-chi-bot
bot
added
release-note
|
Review Complete Findings: 0 issues |
ti-chi-bot
bot
added
the
size/M
|
Hi @fzzf678. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. 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. |
📝 WalkthroughWalkthroughSets Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #66632 +/- ##
================================================
+ Coverage 77.6807% 79.4907% +1.8100%
================================================
Files 2008 1956 -52
Lines 549265 537094 -12171
================================================
+ Hits 426673 426940 +267
+ Misses 120920 108691 -12229
+ Partials 1672 1463 -209
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wjhuang2016, YangKeao 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 |
ti-chi-bot
bot
added
approved
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/ddl/tests/partition/db_partition_test.go (1)
4179-4182: Make exchange-partition enablement explicit for test determinism.To avoid dependence on environment defaults, set
tidb_enable_exchange_partitionin this test explicitly.Suggested patch
func TestIssue66077ExchangePartitionDifferentDefinitionsWithShardRowIDBits(t *testing.T) { store := testkit.CreateMockStore(t) tk := testkit.NewTestKit(t, store) + tk.MustExec("set @@session.tidb_enable_exchange_partition = 1") + defer tk.MustExec("set @@session.tidb_enable_exchange_partition = 0") tk.MustExec("set @@session.tidb_enable_clustered_index = off") tk.MustExec("set @@session.tidb_shard_row_id_bits = 4")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/ddl/tests/partition/db_partition_test.go` around lines 4179 - 4182, The test relies on the session variable tidb_enable_exchange_partition but doesn't set it explicitly; update the test initialized via testkit.NewTestKit(t, store) to call tk.MustExec to set @@session.tidb_enable_exchange_partition (e.g., to ON) before the create database sbtest statement so the test is deterministic across environments—add the tk.MustExec("set @@session.tidb_enable_exchange_partition = on") call alongside the other tk.MustExec lines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/ddl/tests/partition/db_partition_test.go`:
- Around line 4179-4182: The test relies on the session variable
tidb_enable_exchange_partition but doesn't set it explicitly; update the test
initialized via testkit.NewTestKit(t, store) to call tk.MustExec to set
@@session.tidb_enable_exchange_partition (e.g., to ON) before the create
database sbtest statement so the test is deterministic across environments—add
the tk.MustExec("set @@session.tidb_enable_exchange_partition = on") call
alongside the other tk.MustExec lines.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/ddl/tests/partition/db_partition_test.go (1)
4182-4183: Make DB setup idempotent to avoid flaky reruns.Using a fixed DB name with
create database sbtestcan fail in shared-store/rerun scenarios. Consider dropping it first.♻️ Proposed change
- tk.MustExec("create database sbtest") + tk.MustExec("drop database if exists sbtest") + tk.MustExec("create database sbtest")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/ddl/tests/partition/db_partition_test.go` around lines 4182 - 4183, The test uses a fixed database name via tk.MustExec("create database sbtest") which can fail on reruns; make the DB setup idempotent by ensuring the database is removed or created conditionally before use: update the setup calls around tk.MustExec to either run a drop (e.g., DROP DATABASE IF EXISTS sbtest) prior to create or replace the create with a conditional form (CREATE DATABASE IF NOT EXISTS sbtest), then continue with tk.MustExec("use sbtest") so reruns won’t fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/ddl/tests/partition/db_partition_test.go`:
- Around line 4182-4183: The test uses a fixed database name via
tk.MustExec("create database sbtest") which can fail on reruns; make the DB
setup idempotent by ensuring the database is removed or created conditionally
before use: update the setup calls around tk.MustExec to either run a drop
(e.g., DROP DATABASE IF EXISTS sbtest) prior to create or replace the create
with a conditional form (CREATE DATABASE IF NOT EXISTS sbtest), then continue
with tk.MustExec("use sbtest") so reruns won’t fail.
|
/retest |
|
@fzzf678: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
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. |
|
/retest |
1 similar comment
|
/retest |
|
@fzzf678: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
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. |
|
@fzzf678: 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. |
4 participants
What problem does this PR solve?
Issue Number: close #66077
Problem Summary:
ALTER TABLE ... EXCHANGE PARTITIONcould incorrectly return[ddl:1736] Tables have different definitionswhen two tables have the same visible schema but differ in internalMaxShardRowIDBitsmetadata.What changed and how does it work?
When
tidb_shard_row_id_bitsis set in session/global variables,CREATE TABLEpreviously only populatedTableInfo.ShardRowIDBitsbut leftTableInfo.MaxShardRowIDBitsas0. This makes metadata inconsistent with/*T! SHARD_ROW_ID_BITS=... */which always sets both fields, and breaks the table definition compatibility check used byEXCHANGE PARTITION.This PR sets
MaxShardRowIDBits = ShardRowIDBitswhen applying default shard row id bits duringCREATE TABLE, and adds a regression unit test using the SQL from the issue.Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Bug Fixes
Tests