test(plan): cover prefix-index range correctness - #26852
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
XuPeng-SH
left a comment
There was a problem hiding this comment.
I found two correctness blockers. Both can still let a lossy prefix key exclude valid rows before the base-table predicate is evaluated.
- P1 — scoped FORCE hints bypass the new guards.
applyForceIndexHintsruns before ordinary filter index selection. ForFORCE INDEX FOR ORDER BY/GROUP BY,applyForceIndexHintToScancallstryHintedIndexAccess, andtryHintedCoveringIndexScantreats a prefix part as a complete covered column and rewrites the original filter ontoserial_extract(hiddenKey, ...). It then protects the base scan, so the guard added intryIndexOnlyScanis never reached. I reproduced this at the exact PR head with:
CREATE TABLE t(id INT PRIMARY KEY, a VARCHAR(32), KEY idx_a(a(3)));
SELECT id FROM t FORCE INDEX FOR ORDER BY(idx_a)
WHERE a BETWEEN 'abcX' AND 'abdA' ORDER BY a;The planner still produces an idx_a covering scan carrying the range on the truncated key. Besides under-fetching, a prefix key cannot provide full-value ORDER/GROUP ordering among values sharing the same prefix. Please centralize the lossy-prefix eligibility contract (or reject these hinted covering/order/group paths) and add scoped-hint regressions with same-prefix values.
- P1 — semantically stale prefix metadata fails open.
indexHasLossyLeadingPrefixonly fails closed on parse errors; it returnsprefixLengths[ResolveAlias(Parts[0])] > 0. On the current base,ALTER TABLE ... RENAME COLUMNis INPLACE and updatesIndexDef.Parts/mo_indexes.column_name, but does not rename the key insidealgo_params.prefix_lengths. A valid state such asParts=["new_name", ...]plus{"prefix_lengths":"old_name:3"}therefore returns false here, while the existing physical index rows are still truncated. The new range maps can then select that index and reproduce the same wrong-result bug after a normal DDL operation. This also contradicts the PR claim that malformed metadata fails closed. Please keep prefix metadata synchronized on rename and treat orphan/mismatched prefix keys conservatively; cover rename + range before/after flush and subsequent DML.
The focused tests added by this PR pass locally, and git diff --check is clean, but they do not exercise either bypass.
6e5a9fe to
ab637b0
Compare
|
Addressed review 4890910135 in the rebased head Both P1 findings were reproduced on the previous head before changing production code:
The updated contract now rejects lossy prefix indexes from scoped hint, index-only, IN, and range paths; stale metadata makes optional reads fall back. Equality remains usable only as a candidate lookup with a truncated probe and the original base-table residual. RENAME and CHANGE now rewrite persisted prefix metadata, with an all-index preflight before mutation. Stale metadata is rejected for all four regular-index DML binders. The BVT also verifies rename followed by INSERT, UPDATE, DELETE, and flush. During self-review, the first recorded BVT exposed an empty renamed-prefix equality result. I did not accept that generated expectation; the lookup truncation was fixed, and the final expected INSERT/UPDATE row now returns as id 9. Validation on current main
|
ab637b0 to
06bd75e
Compare
|
Rebased onto current upstream main and resolved the conflicts. The production prefix-index correction is already in main through #26829, so I intentionally did not replay the older conflicting implementation. This PR now contains only the additional regression coverage for #26841: scoped FORCE ORDER/GROUP hints, non-equality candidate selection, complete-index control, and end-to-end rename/DML/flush behavior. Validated with complete controlled-CGo |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Reviewed the complete current diff at c7e56fcf00956e540d2522bf94b695f6faefad41 (base df49b097935ccd650d9f30016d5219228e053c5e).
No blocking correctness issue found. The added coverage exercises the previously unsafe declared-prefix range paths across FORCE/IGNORE, scalar and paired ranges, OR/IN, prepared statements, unique indexes, flush, rename/DML, scoped ORDER/GROUP hints, malformed metadata, and a complete-index control. Expected plans and result sets are consistent with the fail-closed prefix-index contract. The test-only change adds no production ownership, wait, or lifecycle edge.
I also integrated this head with current latest main f9a3184c3ae251810265aef90baa7aaa8047fe23 (the regular-index cost refactor): merge was conflict-free, focused tests passed 10x, and go test ./pkg/sql/plan -count=1 -timeout=300s passed.
What type of PR is this?
Which issue(s) this PR covers:
Regression coverage for #26841. The production correction merged independently in #26829 while this PR was under review.
What this PR does / why we need it:
This branch is rebased on current
mainand intentionally retains only coverage not already present upstream:During conflict resolution, the old production implementation was deliberately not replayed: current main contains the stronger #26829 implementation, including V2 prefix-metadata support. Reintroducing the old code would duplicate validation and reject valid delimiter-bearing column names.
Validation
./pkg/sql/plantests;go vet ./pkg/sql/planwith the project CGo include paths;make build;prefix_index_rangeBVT: 51/51;prefix_index_non_equalityBVT: 52/52;git diff --check.