planner: discount equality-bound clustered-key prefix in index pruning#69726
planner: discount equality-bound clustered-key prefix in index pruning#69726terry1purcell wants to merge 4 commits into
Conversation
In multi-tenant schemas every secondary index leads with the tenant column (also the clustered-key prefix), so absolute coverage scoring keeps many redundant indexes: each one scores for covering the tenant column even though the table path serves it directly and it cannot differentiate one index from another. Index pruning now discounts leading clustered-key columns bound by equality/IN constants: they still extend an index's usable consecutive prefix but contribute nothing to its score, so prefix-only indexes fall to the existing zero-score prune. In addition, the phase-2 ordering-key diversity check becomes a full coverage signature (consecutive prefix plus remaining covered columns) enforced from the first selection slot, deduplicating indexes with identical interesting-column coverage. For a 54-index tenant table this reduces kept paths from 11 to 6 for a typical filtered ORDER BY LIMIT query, and from 11 to 2 when only the tenant column is bound, cutting index stats sync-load, range building, and skyline work per query. Forced (use/force index) and IndexMerge- hinted paths still bypass pruning. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ti-chi-bot
Bot
added
release-note-none
|
[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 |
|
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)
📝 WalkthroughWalkthroughIndex pruning now discounts clustered-key prefix columns bound by constant equality or IN predicates. The pruning rule’s scoring and selection logic change to use those discounted columns, and new test and benchmark coverage was added for shared-prefix index layouts. ChangesIndex pruning discount logic
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![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/planner/core/rule/rule_prune_indexes.go (1)
625-655: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winApply coverage-key deduplication to non-consecutive coverage too.
Line 631 only checks duplicate coverage keys when
hasConsecutiveis true, and Line 654 only records keys for consecutive prefixes. An index withconsecutiveColumnIDs == nilbutcoveredColumnIDs == [c]still has a meaningfulbuildCoverageKey("|c"), so duplicate non-consecutive coverage can consume phase-1 slots and keep extra paths.♻️ Proposed adjustment
func shouldAddIndex(entry scoredIndex, path *util.AccessPath, req columnRequirements, state *indexSelectionState) bool { hasConsecutive := len(entry.info.consecutiveColumnIDs) > 0 + hasCoverage := len(entry.info.coveredColumnIDs) > 0 if state.phase1Count < state.phase1Limit { @@ - if hasConsecutive { + if hasCoverage { if _, seen := state.seenCoverageKeys[buildCoverageKey(entry.info)]; seen { return false } } @@ func recordCoverage(info indexWithScore, state *indexSelectionState) { @@ - if len(info.consecutiveColumnIDs) > 0 { + if len(info.coveredColumnIDs) > 0 { state.seenCoverageKeys[buildCoverageKey(info)] = struct{}{} } }🤖 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/planner/core/rule/rule_prune_indexes.go` around lines 625 - 655, Phase 1 in the index pruning logic is only deduplicating coverage keys for indexes with consecutive columns, so non-consecutive coverage can still be selected multiple times. Update the phase-1 path in the selection logic around shouldAddIndexWithConsecutive/shouldAddIndexWithoutConsecutive to check buildCoverageKey(entry.info) for all indexes, not just when hasConsecutive is true. Also adjust recordCoverage so it always stores the coverage key for any index that has a meaningful covered-column set, while still tracking consecutiveColumnIDs separately.
🤖 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 `@pkg/planner/core/rule/rule_prune_indexes.go`:
- Around line 625-655: Phase 1 in the index pruning logic is only deduplicating
coverage keys for indexes with consecutive columns, so non-consecutive coverage
can still be selected multiple times. Update the phase-1 path in the selection
logic around shouldAddIndexWithConsecutive/shouldAddIndexWithoutConsecutive to
check buildCoverageKey(entry.info) for all indexes, not just when hasConsecutive
is true. Also adjust recordCoverage so it always stores the coverage key for any
index that has a meaningful covered-column set, while still tracking
consecutiveColumnIDs separately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 04370861-599e-4fef-ae3c-ddef21a10e39
📒 Files selected for processing (3)
pkg/planner/core/casetest/index/BUILD.bazelpkg/planner/core/casetest/index/index_test.gopkg/planner/core/rule/rule_prune_indexes.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69726 +/- ##
================================================
- Coverage 76.3227% 74.2118% -2.1110%
================================================
Files 2041 2050 +9
Lines 560399 578285 +17886
================================================
+ Hits 427712 429156 +1444
- Misses 131786 148403 +16617
+ Partials 901 726 -175
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest-required |
…width tiebreak Two refinements to the clustered-prefix discount: The coverage-signature dedup now includes discounted columns at their declared chain position. idx(ws, a) and idx(a) on a clustered (ws, id) table score identically, but ranger builds ranges only over declared index columns: idx(ws, a) reaches a two-column range while idx(a) ranges on a alone with ws as an index filter, scanning every tenant's matching entries. Conflating them could prune the strictly better index; both now survive to skyline. Indexes that differ only in unused trailing columns (idx(a) vs idx(a, b)) still deduplicate. The fewer-columns tiebreak no longer requires a single consecutive column. When indexes tie on score, chain, and covering, the narrower index wins at any chain length instead of falling through to index-ID order: its entries are narrower and, on clustered tables, the PK suffix (usable for index-side filtering and ordering) starts earlier. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…uning Benchmark the 54-index multi-tenant schema that motivated the clustered-prefix discount, planning a filtered ORDER BY LIMIT query and an order-only variant at the default prune threshold and with pruning disabled (threshold=-1). Measured medians on this change (vs master, 6 runs x 30 plans, mockstore with analyzed stats): full query 1084us -> 889us (-18%), order-only 552us -> 324us (-41%); the disabled-pruning variants are unchanged, confirming the discount costs nothing when inactive. Relative to no stage-1 pruning at all, the default threshold saves 43% and 67% respectively on this schema. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ti-chi-bot
Bot
added
size/XXL
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/planner/core/casetest/index/index_prune_bench_test.go (1)
178-187: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the redundant
rowsvariable.
rowsis always equal toisince the loop starts at 0, making it a redundant tracker. Usei > 0directly for the comma-separator check.♻️ Proposed simplification
- rows := 0 for i := range 900 { ws := tenants[i%len(tenants)] - if rows > 0 { + if i > 0 { sb.WriteString(",") } fmt.Fprintf(&sb, "(UUID_TO_BIN(UUID()), '%s', %d, 'label-%d', UUID_TO_BIN('%s'), UUID_TO_BIN(UUID()), %d, %d)", ws, i, i%50, typeUUIDs[i%len(typeUUIDs)], i%3, 15+(i%4)) - rows++ }🤖 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/planner/core/casetest/index/index_prune_bench_test.go` around lines 178 - 187, The loop in the benchmark setup uses a redundant rows counter in the index_prune bench test. Simplify the separator logic in the for loop by removing rows entirely and using i > 0 for the comma check, while keeping the fmt.Fprintf row generation unchanged; this is in the code that builds the VALUES string for the test input.
🤖 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 `@pkg/planner/core/casetest/index/index_prune_bench_test.go`:
- Around line 178-187: The loop in the benchmark setup uses a redundant rows
counter in the index_prune bench test. Simplify the separator logic in the for
loop by removing rows entirely and using i > 0 for the comma check, while
keeping the fmt.Fprintf row generation unchanged; this is in the code that
builds the VALUES string for the test input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5f82b369-db9c-4674-9500-fcf9318810e7
📒 Files selected for processing (2)
pkg/planner/core/casetest/index/BUILD.bazelpkg/planner/core/casetest/index/index_prune_bench_test.go
There was a problem hiding this comment.
Pull request overview
This PR refines TiDB planner index-pruning heuristics to reduce planning overhead in schemas with many secondary indexes that share a clustered-key (tenant-style) leading prefix, while also deduplicating indexes that provide equivalent “interesting-column” coverage for a given query.
Changes:
- Discount equality/IN-bound leading clustered-key prefix columns from index coverage scoring (while still treating them as extending the usable prefix for range-building identity).
- Replace phase-2 “ordering diversity” with a full coverage signature (usable prefix chain + remaining covered interesting columns) and apply deduplication starting from the first selection slot.
- Add planner casetests and a benchmark to validate and measure pruning behavior on shared-prefix tenant schemas.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/planner/core/rule/rule_prune_indexes.go | Implements discounted clustered-prefix scoring and coverage-signature-based dedup during index selection. |
| pkg/planner/core/casetest/index/index_test.go | Adds casetests covering shared clustered-prefix discounting, dedup behavior, and hint bypass behavior. |
| pkg/planner/core/casetest/index/index_prune_bench_test.go | Adds benchmarks to measure planning-time impact for shared-prefix schemas with many indexes. |
| pkg/planner/core/casetest/index/BUILD.bazel | Registers the new benchmark file in the Bazel test target and adjusts sharding. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Selection could rebuild the coverage key (with a clone and sort of the non-consecutive covered columns) up to twice per candidate across the phase-1 duplicate check, recordCoverage, and phase-2 checks. Compute it once when the candidate is scored, and skip the clone/sort when the non-consecutive remainder has at most one element. Addresses a Copilot review comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
In multi-tenant schemas every secondary index leads with the tenant column (also the clustered-key prefix), so absolute coverage scoring keeps many redundant indexes: each one scores for covering the tenant column even though the table path serves it directly and it cannot differentiate one index from another.
Index pruning now discounts leading clustered-key columns bound by equality/IN constants: they still extend an index's usable consecutive prefix but contribute nothing to its score, so prefix-only indexes fall to the existing zero-score prune. In addition, the phase-2 ordering-key diversity check becomes a full coverage signature (consecutive prefix plus remaining covered columns) enforced from the first selection slot, deduplicating indexes with identical interesting-column coverage.
For a 54-index tenant table this reduces kept paths from 11 to 6 for a typical filtered ORDER BY LIMIT query, and from 11 to 2 when only the tenant column is bound, cutting index stats sync-load, range building, and skyline work per query. Forced (use/force index) and IndexMerge- hinted paths still bypass pruning.
What problem does this PR solve?
Issue Number: ref #63856
Problem Summary:
What changed and how does it work?
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
force indexbehavior.Tests / Benchmarks