*: make table attribute labels keyspace-aware (#68410)#69762
Conversation
ti-chi-bot
added
do-not-merge/hold
📝 WalkthroughWalkthroughTiKV codec context is threaded through label-rule creation, reset, filtering, backup, restore, and DDL flows. Next-generation keyspace-aware IDs, labels, region ranges, and information-schema parsing are added with expanded tests and Bazel wiring. ChangesKeyspace-aware label contracts
DDL codec propagation
Backup and restore rule updates
Filtering and parsing label rules
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DDL
participant TiKVCodec
participant LabelRule
participant PD
participant InfoSchema
DDL->>TiKVCodec: GetCodec()
DDL->>LabelRule: NewRuleID(codec) and Reset(codec)
LabelRule->>PD: Store keyspace-aware rule
InfoSchema->>PD: GetAllLabelRules(codec)
PD-->>InfoSchema: Filter rules by keyspace
InfoSchema-->>InfoSchema: Restore and parse public rule IDs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: context deadline exceeded" 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 |
ChangRui-Ryan
force-pushed
the
cherry-pick-68410-to-release-nextgen-202603
branch
from
f41ba81 to
97f1aec
Compare
|
Cherry-pick conflicts appear resolved; removing the |
|
[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 |
ti-chi-bot
removed
the
do-not-merge/hold
|
/retest |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
br/pkg/backup/schema.go (1)
328-375: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deduplicating partition rule ID computation.
label.NewRuleID(codec, dbName, tableName, def.Name.L)is computed identically in both the collection loop (line 341) and the lookup loop (line 373). Building amap[string]stringfrom partition name to rule ID in the first loop would eliminate the redundant recomputation in the second.♻️ Optional: reuse partition rule IDs
// Collect all rule IDs to check (table + partitions if any) ruleIDs := []string{ruleID} +partitionRuleIDs := make(map[string]string) if s.tableInfo.Partition != nil && len(s.tableInfo.Partition.Definitions) > 0 { for _, def := range s.tableInfo.Partition.Definitions { partitionRuleID := label.NewRuleID(codec, dbName, tableName, def.Name.L) ruleIDs = append(ruleIDs, partitionRuleID) + partitionRuleIDs[def.Name.O] = partitionRuleID } } // ... after fetching rules ... if s.tableInfo.Partition != nil && len(s.tableInfo.Partition.Definitions) > 0 { for _, def := range s.tableInfo.Partition.Definitions { - partitionRuleID := label.NewRuleID(codec, dbName, tableName, def.Name.L) + partitionRuleID := partitionRuleIDs[def.Name.O] if rule, exists := rules[partitionRuleID]; exists {🤖 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 `@br/pkg/backup/schema.go` around lines 328 - 375, Deduplicate partition rule ID computation in checkMergeOptionAllowed by storing each partition’s computed ID during the initial ruleIDs collection, keyed by partition name or definition. Reuse those stored IDs when checking partition rules instead of calling label.NewRuleID again in the lookup loop.pkg/ddl/label/rule_test.go (1)
118-157: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGood test coverage for keyspace-aware codec behavior.
The test correctly validates V1 (classic), V2 in Classic mode, and V2 in NextGen mode, including partition rules and
RestoreRuleIDround-tripping. Validating region range keys againstcodecV2.EncodeRegionRangeoutput rather than hardcoded hex is a solid approach.One minor observation: the partition rule test at line 156 only checks
partitionKeylabel but doesn't verify thatdbKey/tableKeylabels were updated to"db2"/"t2"after the reset. Consider adding those assertions for completeness.🤖 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 `@pkg/ddl/label/rule_test.go` around lines 118 - 157, Extend TestResetWithKeyspaceCodec to verify that partitionRule.Labels contains dbKey with value "db2" and tableKey with value "t2", alongside the existing partitionKey assertion, confirming Reset updates all identifying labels.
🤖 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 `@br/pkg/backup/schema.go`:
- Around line 328-375: Deduplicate partition rule ID computation in
checkMergeOptionAllowed by storing each partition’s computed ID during the
initial ruleIDs collection, keyed by partition name or definition. Reuse those
stored IDs when checking partition rules instead of calling label.NewRuleID
again in the lookup loop.
In `@pkg/ddl/label/rule_test.go`:
- Around line 118-157: Extend TestResetWithKeyspaceCodec to verify that
partitionRule.Labels contains dbKey with value "db2" and tableKey with value
"t2", alongside the existing partitionKey assertion, confirming Reset updates
all identifying labels.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 15f747da-6cac-4a24-a618-46849550fec6
📒 Files selected for processing (24)
br/pkg/backup/BUILD.bazelbr/pkg/backup/schema.gobr/pkg/backup/schema_merge_option_test.gobr/pkg/restore/snap_client/client.gopkg/ddl/attributes_sql_test.gopkg/ddl/executor.gopkg/ddl/label/BUILD.bazelpkg/ddl/label/attributes.gopkg/ddl/label/attributes_test.gopkg/ddl/label/rule.gopkg/ddl/label/rule_test.gopkg/ddl/partition.gopkg/ddl/schema.gopkg/ddl/table.gopkg/domain/infosync/BUILD.bazelpkg/domain/infosync/info.gopkg/domain/infosync/label_manager.gopkg/domain/infosync/label_manager_test.gopkg/executor/BUILD.bazelpkg/executor/importer/BUILD.bazelpkg/executor/infoschema_reader.gopkg/executor/infoschema_reader_keyspace_test.gopkg/infoschema/BUILD.bazelpkg/planner/core/issuetest/BUILD.bazel
💤 Files with no reviewable changes (2)
- pkg/infoschema/BUILD.bazel
- pkg/planner/core/issuetest/BUILD.bazel
2 participants
This is an automated cherry-pick of #68410
What problem does this PR solve?
Issue Number: ref #67765
Problem Summary:
In NextGen deployments, TiDB can run with a keyspace-aware TiKV codec. Table attributes are persisted as PD region label rules, but the existing rule IDs are generated only from database/table/partition names, for example
schema/db/tableorschema/db/table/partition.This is not sufficient in keyspace-aware deployments. Different keyspaces may contain tables with the same database, table, and partition names. Without keyspace information in the table attribute label rule ID, label rules from different keyspaces can collide or be read back together. In addition, the rule key ranges must be encoded with the keyspace-aware TiKV codec so that PD applies the rule to the correct keyspace range.
As a result, table attribute label rules need to be keyspace-aware whenever TiDB is running in NextGen with keyspace metadata.
What changed and how does it work?
This PR makes table attribute label rules keyspace-aware in NextGen deployments when the TiKV codec contains keyspace metadata.
The core change is to centralize table attribute label rule ID generation in
pkg/ddl/label:label.UseKeyspaceAwareRules(codec).label.NewRuleID(codec, dbName, tableName, partName).schema/db/tableschema/db/table/partitionkeyspace/<keyspace-id>/schema/db/tablekeyspace/<keyspace-id>/schema/db/table/partitionlabel.Rule.Resetnow takes the TiKV codec and uses the centralized rule ID helper. When keyspace-aware rules are enabled, it also adds or updates an internalkeyspace=<id>label and encodes rule ranges withcodec.EncodeRegionRange(...), so the PD rule points to the correct keyspace range. When keyspace-aware rules are not enabled, it keeps the old rule ID format and the old table prefix range encoding.All DDL table attribute lifecycle paths now pass the store codec and use the centralized rule ID helper, including:
ALTER TABLE ... ATTRIBUTESALTER TABLE ... PARTITION ... ATTRIBUTESThe information schema path is updated accordingly:
infosync.GetAllLabelRulesnow passes the current TiKV codec to the label rule manager.GetAllLabelRulesfilters PD label rules by the current keyspace prefix, soinformation_schema.attributesonly shows table attribute rules for the current keyspace.checkRulecan parse keyspace-prefixed rule IDs in NextGen.BR backup/restore merge-option logic now also uses the same centralized rule ID helper, so DDL, backup, and restore use consistent table attribute label rule IDs in both classic and keyspace-aware modes.
For restored user-visible attributes, the internal
keyspacelabel is hidden in NextGen, while classic mode preserves the existing behavior.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
New Features
Bug Fixes