perf(plan): cost encoded regular index access - #26850
Conversation
|
Evidence handoff for
The shared service was not switched during #26847 validation. CI was not monitored. |
|
Final PR-head refresh: Mergify merged current Fresh validation on
No CI checks were inspected or monitored. |
|
Triple-candidate acceptance found a remaining prepared-parameter ranking defect on the Round 34 300k-row DDL-order matrix. With both
SQL PREPARE EXPLAIN confirms the split: literal uses |
|
Integration acceptance findings are resolved in 0278375:
Fresh #26850 evidence: 32 focused planner tests PASS; full pkg/sql/plan PASS (4.712s); go list/build/vet PASS; controlled make build PASS; benchmark 19.62-19.94 us/op, 14680 B/op, 49 allocs/op. Updated triple validation with exact heads #26848 b4c2d1b, #26850 0278375, and #26851 18091e7: all 38 added planner tests PASS, full pkg/sql/plan PASS (4.552s), controlled build/vet and make build PASS. No integration incompatibility found. |
|
55 acceptance on the updated three-PR integration candidate (
The follow-up ParamRef neutral range estimate and FORCE covering-shape priority are therefore accepted on the live workload. A separate binary-protocol |
|
Five-PR live combination acceptance passed on 55:
Important interaction found during staging: the four-way candidate without #26806 made the base/shadow DECIMAL path correct via #26815 while the secondary-index range path remained wrong ( |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Post-push deep review completed at f25c17802cc50efc6e1b8e64bc76d060d2cf01d8 on top of main@54303518dca38693d2610189a688921eeece652b.
Verdict: no blocking findings remain.
Root cause and closure:
- The global regular-index scorer compared covering scans with backfill joins even when statistics were intentionally ignored for a small table. A required exact CHAR/VARCHAR prefix residual increased the covering score enough to select an INDEX join for
t14, regressing the stable small-table access shape. - Small tables and FORCE INDEX now preserve the established shape priority (covering before backfill), while cost continues to rank candidates within a shape. Large automatic tables still use cross-shape global costing.
- The rebase reconciliation preserves upstream byte-string and nullable strict-upper-bound residual semantics, and candidate costing now charges exactly the residual positions materialized by the final index-only plan.
Review matrix:
- all regular candidates across covering/backfill and point/range shapes; DDL-order independence, equal-score stability, valid/unscorable fallback, and base-scan rejection;
- FORCE/USE/IGNORE hints, small-table threshold, fake/compound/simple PKs, duplicate parts, empty metadata, spatial priority;
- CHAR/VARCHAR/BINARY prefix ambiguity, nullable/prepared predicates, range bounds, LIMIT/OFFSET safety, and residual selectivity;
- planning hot path remains bounded by existing candidate/part/filter traversal and adds no execution-time state or resource lifecycle.
Local validation:
- focused regression matrix passed, including the pre-fix failing small-table covering-vs-backfill counterexample;
- full
./pkg/sql/plantest suite passed with the controlled CGo wrapper; go buildandgo vetfor./pkg/sql/planpassed;BenchmarkEncodedIndexCostWideTableMultiIndex: 15.1 us/op, 14,680 B/op, 49 allocs/op (100 iterations, darwin/arm64).
The GitHub head and base match the reviewed SHAs and the PR is mergeable. CI was intentionally not awaited.
## Summary - let forced ORDER BY access skip a contiguous leading index prefix fixed by single-value equality - preserve plain `FORCE INDEX` as an access-path constraint when the named index cannot provide ordering, leaving `Sort` to enforce order - keep `FORCE INDEX FOR ORDER BY` order-scoped and leave ordinary optimization unchanged ## Root cause The force-index ORDER BY prepass required ORDER columns to match the physical index from part zero. For an index such as `(status, amount, id)`, it therefore rejected a fixed status prefix followed by `ORDER BY amount, id` even though that prefix is order-neutral. When the order match failed, base-scan protection also caused an unscoped plain `FORCE INDEX` to be silently discarded. The matcher now skips only a contiguous prefix fixed by `column = runtime_constant`. IN, range, and unconstrained or gapped parts remain order-incompatible. If an unscoped plain force cannot satisfy order, the planner still builds the named covering scan or index backfill join without marking it ordered, so the existing Sort remains authoritative. ## Validation Fresh validation from isolated worktree `/home/xupeng/github/matrixone-issue-26814`, based on `origin/main` `0676200da47052659cf46ad9e22d67ab9654e393`: - regression reproduced before the production change: equality-prefix and plain-force fallback cases failed to retain the index - focused force/order planner tests: pass - full `pkg/sql/plan` suite: pass - package `go build` and `go vet` with repository-local CGo paths: pass - whole `make build`: pass - isolated candidate-binary BVT `optimizer/index_hint.sql`: 53/53 statements pass - pairwise merge plus focused/full planner suites with draft #26848: pass - pairwise merge plus focused/full planner suites with draft #26850 (for #26803): pass The regression matrix covers ASC/DESC, LIMIT/no LIMIT, equality/IN/range/gapped leading parts, covering/backfill access, invalid hints, unhinted optimization, and exact/distinct row checks. No row-path benchmark was added: the new work is planner-only and bounded by index parts times scan filters plus ORDER columns. Fixes #26814
## Summary - stop dropped-table auto-increment metadata GC from requesting a committed-logtail visibility wait - keep the fail-closed wait unchanged for allocation and offset update paths - release the account lookup result before executing the metadata delete - assert transaction/nested SQL options and mpool release in regression tests ## Root cause `TestHashBuildSharedBudgetRecoverySQL` completed its main workload, but CN shutdown joined the auto-increment cleanup worker while it was deleting metadata for a dropped table. The delete inherited `WithWaitCommittedLogApplied` into the nested SQL execution. After the transaction committed, `SyncLatestCommitTS` waited with its own five-minute background timeout and did not observe the cleanup worker context, so teardown ended in the five-minute fatal timeout seen in both PR #26850 and PR #26806. A second deep-review pass found that the successful account lookup result was overwritten by the subsequent delete result when the tenant existed. Its batches therefore never reached `Result.Close`, retaining their mpool allocation for every cleaned table. The lookup result is now closed immediately after reading it, and the test uses a real allocated result to verify that `CurrNB` returns to zero. ## Safety This delete is lazy GC for a globally allocated table ID that is not reused. Commit durability is sufficient; no later operation requires the same CN to observe the deletion immediately. Failed deletes remain in the destroyed-table set and are retried. Allocation and offset mutation paths retain their committed-logtail waits. ## Validation - focused delete tests plus the background `TestDelete` - `go test ./pkg/incrservice` - `go test -race ./pkg/incrservice` - each of the three delete tests with adaptive race stress: `T=0/0/0.06s`, `B=30s`, `N=100` - `TestHashBuildSharedBudgetRecoverySQL` normally and under race; race `T=18.28s`, `B=30s`, `N=1` - `go build ./pkg/incrservice` - `go vet ./pkg/incrservice` - `golangci-lint run ./pkg/incrservice/...` (0 issues) Failure examples: - https://github.com/matrixorigin/matrixone/actions/runs/31321448737/job/93265096191 - https://github.com/matrixorigin/matrixone/actions/runs/31321617474/job/93265404854 --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
0bcf549 to
d58b20e
Compare
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review completed on the exact range 8be242b25bf9a44d73a3b1cc1db75fd0264a293c..df8bb8f6d24b2987262d79ad00418a7118830c32.
Verdict: two blocking cost/physical-plan closure defects remain. The existing green tests and CI do not exercise either crossover.
The governing invariant is: before comparing a regular-index candidate with a sibling or the base scan, every predicate classified as hidden-index pushdown and every serial_extract charged by the score must match the operations materialized by applyExtraFiltersOnIndex / applyRegularIndexOnlyScan.
Confirmed findings:
-
[P1] Lossy prefix-part residuals are scored as pushed although execution deliberately leaves them on the base scan.
applyExtraFiltersOnIndexskips a full-value predicate whenprefixLengths[partName] > 0, but the backfill score treats every direct indexed part as pushable and multiplies its selectivity intohiddenRows. A valid(category, event_time(4), PK)counterexample producedbaseWork=17,600,000, current score15,400,993(accepted), and physical no-pushdown work22,800,253(must reject). -
[P1] A non-unique index's appended simple-PK residual executes
serial_extractbut the score suppresses its extraction work. The plan builder checks index parts before the direct physical-PK fallback, so ordinary(category, PK)access emitsserial_extract(__mo_index_idx_col, ..., PK_TYPE); the score drops the work solely becausecolPos == pkIdx. The focused counterexample producedbaseWork=14,400,000, current score10,801,349(accepted), and20,801,349after charging the emitted extract (must reject). The publicexplain_indexBVT already demonstrates this reachable shape fort2i2: its simple-PK residual isserial_extract(__mo_index_idx_col, 2, INT) BETWEEN ....
Both defects can reintroduce the same class of regression this PR is intended to close: an automatic candidate appears cheaper only because the scorer models a different physical plan. The systematic fix is to derive pushability and extraction source from one shared physical mapping (lossy prefix / encoded part / direct physical PK / compound-PK component) and use it both for scoring and plan materialization.
Fresh validation on the unchanged PR head:
GOWORK=off go list/build/vet -mod=readonly ./pkg/sql/plan: PASS- full controlled CGo
pkg/sql/plansuite,-count=1 -timeout=120s: PASS (5.697s) - focused benchmark, 3 x 100 iterations on darwin/arm64: 15.725-19.368 us/op, 14,680 B/op, 49 allocs/op
git diff --check: PASS- two counterexample tests above: FAIL for the stated semantic reasons; diagnostic fixtures were removed afterward
Unhappy-path audit:
| Q | Scope | Verdict |
|---|---|---|
| Q1 | Query-scoped cost context, scratch slices, candidate handoff | Closed: no external resource owner or double cleanup; state remains query-owned |
| Q2 | Wait/termination graph | Closed: no goroutine, lock, channel, RPC, or blocking edge is introduced |
| Q3 | Columns, filters, parts, candidates, scratch reuse | Closed: growth is bounded by query/catalog metadata and scratch marks are reset between candidates |
I am the PR author, so GitHub only permits a COMMENTED self-review; the two P1 findings above should be treated as merge blockers.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Post-push deep review completed on the exact range 8be242b25bf9a44d73a3b1cc1db75fd0264a293c..82c6169cc9f3228d212545519bfb643d6dc9e72b.
Verdict: no blocking correctness, performance, or unhappy-path findings remain.
The two prior P1 closure defects are resolved in 82c6169cc9:
- lossy prefix-part residuals now resolve as unavailable to both the scorer and materializer, so their selectivity is never credited as hidden pushdown;
- an appended simple PK resolves as an encoded key part, and the scorer charges the same
serial_extractfrom index-table column 0 that the physical plan emits.
The implementation uses one physical residual mapping for exact index parts, direct physical PKs, compound-PK components, and unavailable/lossy sources. The scorer uses its precomputed part-position map for O(1) hot-path lookup; equivalence with the materializer path is covered by the table-driven source matrix. Invalid prefix metadata remains fail-safe and scratch state is reset between candidates.
Fresh local gates on the pushed head:
- focused physical-source and crossover regressions: PASS;
- full controlled-CGo
pkg/sql/plansuite: PASS (-count=1 -timeout=120s); go vet ./pkg/sql/plan: PASS;golangci-lint --new-from-revforpkg/sql/plan: 0 issues;git diff --check: PASS;- benchmark after the hot-path correction: 14.33-14.45 us/op, 13,592 B/op, 48 allocs/op, versus PR-head baseline 13.37-13.66 us/op, 14,680 B/op, 49 allocs/op.
Q1-Q3 audit: no external resource ownership, goroutine, lock, channel, wait edge, or execution-time state was added; query-scoped storage stays bounded by columns, filters, parts, and candidates. Both original review threads have objective fix replies and are resolved. CI was not awaited.
3 participants
Summary
Root cause
Regular-index planning selected the first usable covering or backfill shape and did not compare encoded key width, candidate cardinality, repeated
serial_extractconsumers, hidden residual work, and base-table rechecks across sibling candidates. Rejecting only an index-only shape could therefore move a query to an even more expensive INDEX join, while catalog order could hide a profitable narrow sibling.The fix builds one query-scoped cost context, scores covering/point/range/backfill alternatives, and mutates the plan only after selecting the global winner. Invalid statistics or malformed candidate metadata fail open without allowing an unscorable candidate to beat any scorable candidate.
Validation
Fresh local validation on Linux/amd64 with Go 1.26.4 and repository-local CGo artifacts:
GOWORK=off go list -mod=readonly ./pkg/sql/planGOWORK=off go build -mod=readonly ./pkg/sql/planGOWORK=off go vet -mod=readonly ./pkg/sql/planpkg/sql/plansuite: pass (-count=1 -timeout=120s)BenchmarkEncodedIndexCostWideTableMultiIndex, 5 runs: 20.125–20.546 µs/op, 14,680 B/op, 49 allocs/opAccepted service evidence on the 120k-row workload:
No shared service switch was performed during #26847 validation.
Compatibility and interaction
#26848 has one manual merge conflict in
pkg/sql/plan/apply_indices.go: its nullable strict-upper-bound trigger overlaps this PR's deduplicated,Selectivity=1residual construction. Its tests merge automatically. The combined resolution should retain #26848's trigger and this PR's one-copy/non-selective residual accounting.Fixes #26803