*: make table attribute labels keyspace-aware#68410
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-tests-checked
release-note-none
|
@ChangRui-Ryan I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details. ⏳ This process typically takes 10-30 minutes depending on the complexity of the changes. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
the
size/XL
|
Hi @ChangRui-Ryan. 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThreads a tikv.Codec into label rule ID generation and Rule.Reset, enabling optional keyspace-prefixed rule IDs and codec-driven region boundary encoding; updates DDL partition/table flows, backup/restore, infosync filtering, executor info-schema parsing, tests, and BUILD files accordingly. ChangesKeyspace-Aware Label Rule Support
Estimated code review effort: Suggested labels: Suggested reviewers:
🚥 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/ddl/label/attributes.go (1)
102-123:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winComma logic bug when first label is skipped.
The comma is inserted based on the loop index
i, not the count of actually written labels. If the first label in the slice is skipped (e.g., akeyspacelabel at index 0 in NextGen mode, or adb/table/partitionlabel), the next non-skipped label at index 1 will havei > 0true and insert a leading comma, producing output like,"attr=val"instead of"attr=val".In practice,
Resetappends internal labels (db,table,partition,keyspace) after user-defined attributes, so this bug is unlikely to manifest. However, if labels are manually constructed in a different order (as the test inattributes_test.golines 237-243 does), the bug could surface.🐛 Proposed fix
Track the number of written labels instead of using the loop index:
func RestoreRegionLabels(labels *[]pd.RegionLabel) string { var sb strings.Builder + written := 0 for i, label := range *labels { switch label.Key { case dbKey, tableKey, partitionKey: continue case keyspaceKey: if kerneltype.IsNextGen() { continue } default: } - if i > 0 { + if written > 0 { sb.WriteByte(',') } sb.WriteByte('"') sb.WriteString(RestoreRegionLabel(&label)) sb.WriteByte('"') + written++ } return sb.String() }🤖 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/attributes.go` around lines 102 - 123, RestoreRegionLabels incorrectly uses the loop index `i` to decide when to write commas, causing a leading comma if early labels are skipped; change it to track the number of actually written labels (e.g., a `written` counter) and use `written > 0` to decide when to write a comma, incrementing `written` only when you append a label (still honoring the existing skip logic for dbKey/tableKey/partitionKey and keyspaceKey with kerneltype.IsNextGen) and continue to call RestoreRegionLabel(&label) for the appended entries.pkg/ddl/table.go (1)
1633-1653:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass the destination schema into
updateLabelRules.This helper rebuilds rule IDs from
job.SchemaName, butcheckAndRenameTablesis also used byRENAME TABLES, where each entry carries its own destination schema. In a multi-table rename that targets different schemas, later iterations can recreate PD label rules under the wrong schema name. Thread the new schema name through this helper instead of reading it from the shared job object.🤖 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/table.go` around lines 1633 - 1653, The updateLabelRules helper currently reads job.SchemaName when rebuilding PD label rules, which breaks multi-table RENAMEs where each table may have a different destination schema; change updateLabelRules to accept a destination schema parameter (e.g. destSchema string) and use that instead of job.SchemaName in calls to label.Rule.Clone().Reset(...); update all callers (notably checkAndRenameTables and any other places that call updateLabelRules) to pass the correct per-table destination schema from the rename entry rather than the shared job.SchemaName so rules are created under the intended schema.pkg/ddl/partition.go (1)
3003-3028:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the two schema names distinct during
EXCHANGE PARTITIONlabel updates.This function already fetches
ntDbInfoandptDbInfo, so the two tables are not guaranteed to live in the same schema. The rule lookup/reset still usesjob.SchemaNamefor both sides, which will miss the partition rule or recreate it under the wrong schema when the exchange crosses schemas. Build/resetntrwithntDbInfo.Name.LandptrwithptDbInfo.Name.L.🤖 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/partition.go` around lines 3003 - 3028, The code builds and resets label rule IDs and rules using job.SchemaName for both tables, which fails when nt and pt live in different schemas; change the code that creates ntrID and ptrID (label.NewRuleID) to use ntDbInfo.Name.L for the nt rule and ptDbInfo.Name.L for the pt rule, and likewise update the ntr.Clone().Reset(...) and ptr.Clone().Reset(...) calls to pass the matching schema name (ntDbInfo.Name.L for ntr resets, ptDbInfo.Name.L for ptr resets) so lookups and resets target the correct schemas.
🧹 Nitpick comments (3)
pkg/executor/infoschema_reader_keyspace_test.go (1)
46-81: ⚡ Quick winAdd negative ID-format cases to prevent parser regressions.
Please add invalid IDs (e.g. wrong root prefix, malformed keyspace prefix) to ensure
checkRulefails fast when the ID shape is not table-attribute-compatible.✅ Suggested test extension
tests := []struct { name string ruleID string wantDB string wantTable string wantPartition string + wantErr bool }{ + { + name: "invalid non-keyspace prefix", + ruleID: "foo/test/t1", + wantErr: true, + }, { name: "non-keyspace table", ruleID: fmt.Sprintf("%s/test/t1", label.IDPrefix), wantDB: "test", wantTable: "t1", }, + { + name: "invalid keyspace inner prefix", + ruleID: fmt.Sprintf("%s/42/foo/test/t2", label.KeyspacePrefix), + wantErr: true, + }, // ... } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { dbName, tableName, partitionName, err := checkRule(newRule(tc.ruleID)) + if tc.wantErr { + require.Error(t, err) + return + } require.NoError(t, err) require.Equal(t, tc.wantDB, dbName) require.Equal(t, tc.wantTable, tableName) require.Equal(t, tc.wantPartition, partitionName) }) }🤖 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/executor/infoschema_reader_keyspace_test.go` around lines 46 - 81, Add negative test cases to pkg/executor/infoschema_reader_keyspace_test.go to assert checkRule returns an error for malformed IDs: extend the tests slice (near the existing entries that use newRule and checkRule) with cases like wrong root prefix, malformed keyspace prefix, missing ID parts, and non-table shapes (e.g., malformed "%s/test" or "%s/42/%s/test" variants) and in the t.Run for those cases call checkRule(newRule(tc.ruleID)) and require.Error(t, err) (and optionally require.Empty for db/table/partition) so the parser fails fast on invalid ID shapes.pkg/ddl/label/attributes_test.go (1)
237-243: ⚡ Quick winMissing test coverage for reversed label ordering.
The test only validates the case where
merge_option=allowprecedeskeyspace=42. Consider adding a test case with reversed ordering (keyspace=42, thenmerge_option=allow) to verify the comma logic handles skipped-first-label scenarios correctly. This would catch the comma insertion bug flagged inRestoreRegionLabels.📋 Suggested additional test case
// Test reversed ordering to verify comma logic if kerneltype.IsNextGen() { output := RestoreRegionLabels(&[]pd.RegionLabel{input6, input1}) require.Equal(t, `"merge_option=allow"`, output) } else { output := RestoreRegionLabels(&[]pd.RegionLabel{input6, input1}) require.Equal(t, `"keyspace=42","merge_option=allow"`, output) }🤖 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/attributes_test.go` around lines 237 - 243, Add a test that covers reversed label ordering to ensure RestoreRegionLabels handles commas when the first label is skipped: call RestoreRegionLabels with the slice order swapped (use the existing pd.RegionLabel fixtures input6 then input1) and assert the expected strings—when kerneltype.IsNextGen() expect `"merge_option=allow"` and otherwise expect `"keyspace=42","merge_option=allow"`; place this alongside the existing test so it exercises RestoreRegionLabels with the reversed input order.pkg/ddl/label/rule.go (1)
173-179: 💤 Low valueMinor inefficiency: Classic-mode keys are computed then discarded in keyspace mode.
Lines 173-174 unconditionally compute
startKey/endKeyusing classic encoding, then lines 175-179 immediately overwrite them whenuseKeyspaceis true. Consider restructuring with an if-else to avoid the wasted computation.♻️ Optional refactor to eliminate redundant encoding
- startKey := codec.EncodeBytes(nil, tablecodec.GenTablePrefix(ids[i])) - endKey := codec.EncodeBytes(nil, tablecodec.GenTablePrefix(ids[i]+1)) + var startKey, endKey []byte if useKeyspace { - // Label rules are consumed as region boundary keys, so V2 must encode - // the whole outer key instead of prefixing a mem-encoded table key. startKey, endKey = tikvCodec.EncodeRegionRange(tablecodec.GenTablePrefix(ids[i]), tablecodec.GenTablePrefix(ids[i]+1)) + } else { + startKey = codec.EncodeBytes(nil, tablecodec.GenTablePrefix(ids[i])) + endKey = codec.EncodeBytes(nil, tablecodec.GenTablePrefix(ids[i]+1)) }🤖 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.go` around lines 173 - 179, The current code unconditionally computes classic-mode keys with codec.EncodeBytes into startKey/endKey and then overwrites them when useKeyspace is true; change to an if-else so you only compute the classic keys when useKeyspace is false and only call tikvCodec.EncodeRegionRange(tablecodec.GenTablePrefix(ids[i]), tablecodec.GenTablePrefix(ids[i]+1)) when useKeyspace is true, referencing the same symbols (startKey, endKey, codec.EncodeBytes, tikvCodec.EncodeRegionRange, tablecodec.GenTablePrefix, ids[i]) to avoid wasted encoding work.
🤖 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.
Inline comments:
In `@pkg/executor/infoschema_reader.go`:
- Around line 4166-4172: The validation in checkRule incorrectly gates the
label.IDPrefix check behind idOffset > 0, allowing IDs like "foo/db/t" to be
parsed as table rules; change the guard so the prefix and minimum length are
validated regardless of idOffset (i.e., ensure s[idOffset] == label.IDPrefix and
len(s) >= idOffset+3 for table rules), and keep setting dbName = s[idOffset+1]
and tableName = s[idOffset+2] only after this stricter check; reference
checkRule, idOffset, s, label.IDPrefix, rule.ID, dbName and tableName when
making the fix.
---
Outside diff comments:
In `@pkg/ddl/label/attributes.go`:
- Around line 102-123: RestoreRegionLabels incorrectly uses the loop index `i`
to decide when to write commas, causing a leading comma if early labels are
skipped; change it to track the number of actually written labels (e.g., a
`written` counter) and use `written > 0` to decide when to write a comma,
incrementing `written` only when you append a label (still honoring the existing
skip logic for dbKey/tableKey/partitionKey and keyspaceKey with
kerneltype.IsNextGen) and continue to call RestoreRegionLabel(&label) for the
appended entries.
In `@pkg/ddl/partition.go`:
- Around line 3003-3028: The code builds and resets label rule IDs and rules
using job.SchemaName for both tables, which fails when nt and pt live in
different schemas; change the code that creates ntrID and ptrID
(label.NewRuleID) to use ntDbInfo.Name.L for the nt rule and ptDbInfo.Name.L for
the pt rule, and likewise update the ntr.Clone().Reset(...) and
ptr.Clone().Reset(...) calls to pass the matching schema name (ntDbInfo.Name.L
for ntr resets, ptDbInfo.Name.L for ptr resets) so lookups and resets target the
correct schemas.
In `@pkg/ddl/table.go`:
- Around line 1633-1653: The updateLabelRules helper currently reads
job.SchemaName when rebuilding PD label rules, which breaks multi-table RENAMEs
where each table may have a different destination schema; change
updateLabelRules to accept a destination schema parameter (e.g. destSchema
string) and use that instead of job.SchemaName in calls to
label.Rule.Clone().Reset(...); update all callers (notably checkAndRenameTables
and any other places that call updateLabelRules) to pass the correct per-table
destination schema from the rename entry rather than the shared job.SchemaName
so rules are created under the intended schema.
---
Nitpick comments:
In `@pkg/ddl/label/attributes_test.go`:
- Around line 237-243: Add a test that covers reversed label ordering to ensure
RestoreRegionLabels handles commas when the first label is skipped: call
RestoreRegionLabels with the slice order swapped (use the existing
pd.RegionLabel fixtures input6 then input1) and assert the expected strings—when
kerneltype.IsNextGen() expect `"merge_option=allow"` and otherwise expect
`"keyspace=42","merge_option=allow"`; place this alongside the existing test so
it exercises RestoreRegionLabels with the reversed input order.
In `@pkg/ddl/label/rule.go`:
- Around line 173-179: The current code unconditionally computes classic-mode
keys with codec.EncodeBytes into startKey/endKey and then overwrites them when
useKeyspace is true; change to an if-else so you only compute the classic keys
when useKeyspace is false and only call
tikvCodec.EncodeRegionRange(tablecodec.GenTablePrefix(ids[i]),
tablecodec.GenTablePrefix(ids[i]+1)) when useKeyspace is true, referencing the
same symbols (startKey, endKey, codec.EncodeBytes, tikvCodec.EncodeRegionRange,
tablecodec.GenTablePrefix, ids[i]) to avoid wasted encoding work.
In `@pkg/executor/infoschema_reader_keyspace_test.go`:
- Around line 46-81: Add negative test cases to
pkg/executor/infoschema_reader_keyspace_test.go to assert checkRule returns an
error for malformed IDs: extend the tests slice (near the existing entries that
use newRule and checkRule) with cases like wrong root prefix, malformed keyspace
prefix, missing ID parts, and non-table shapes (e.g., malformed "%s/test" or
"%s/42/%s/test" variants) and in the t.Run for those cases call
checkRule(newRule(tc.ruleID)) and require.Error(t, err) (and optionally
require.Empty for db/table/partition) so the parser fails fast on invalid ID
shapes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 45c66364-70b3-4a37-97dc-f557507c3cb6
📒 Files selected for processing (20)
br/pkg/backup/BUILD.bazelbr/pkg/backup/schema.gobr/pkg/backup/schema_merge_option_test.gobr/pkg/restore/snap_client/client.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/infoschema_reader.gopkg/executor/infoschema_reader_keyspace_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #68410 +/- ##
================================================
- Coverage 77.2803% 76.7175% -0.5628%
================================================
Files 2010 2046 +36
Lines 555624 575910 +20286
================================================
+ Hits 429388 441824 +12436
- Misses 125316 131440 +6124
- Partials 920 2646 +1726
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ChangRui-Ryan
force-pushed
the
changrui_cse_1968
branch
from
b81e920 to
19953cd
Compare
ChangRui-Ryan
force-pushed
the
changrui_cse_1968
branch
from
19953cd to
8d52045
Compare
ti-chi-bot
Bot
added
size/XXL
| case dbKey, tableKey, partitionKey: | ||
| continue | ||
| case keyspaceKey: | ||
| if kerneltype.IsNextGen() { |
There was a problem hiding this comment.
keyspace is an internal label injected by TiDB for keyspace-aware table attribute label rules. It is not part of the user-specified table attributes, so information_schema.attributes should hide it in NextGen, similar to how we already hide the internal db/table/partition labels.
| r = append(r, (*label.Rule)(labelRule)) | ||
| } | ||
| return r, nil | ||
| return filterRulesByKeyspace(r, codec), nil |
There was a problem hiding this comment.
this feature would be better be implemented at PD side where we make those rules keyspace scoped, instead of difference them by prefix, and encoding/filtering at client side
There was a problem hiding this comment.
Agree this would be cleaner in PD long term.
For this PR, the current PD label-rule API is still global and has no keyspace context. Since TiDB already generates rule IDs and encoded region ranges with the store codec, keyspace/<id>/ prefixing plus client-side filtering is the minimal compatible fix. Native PD-side scoping can be a follow-up.
|
|
||
| dbName = s[idOffset+1] | ||
| tableName = s[idOffset+2] | ||
| if len(s) > idOffset+3 { |
There was a problem hiding this comment.
| if len(s) > idOffset+3 { | |
| if idOffset+3 < len(s) { |
There was a problem hiding this comment.
Good point. I will take the suggestion.
|
/hold please fix existing comments |
ti-chi-bot
Bot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, YuJuncen 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
|
/unhold |
ti-chi-bot
Bot
removed
the
do-not-merge/hold
|
/retest |
|
@ChangRui-Ryan: 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. |
|
@ChangRui-Ryan: 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. |
3 participants
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
Improvements
Tests