Repository navigation
fix(hnsw): search the index while it lags instead of falling back to a full table scan - #30989
Conversation
…a full table scan matrixorigin#30117 made HNSW fall back to a brute-force scan of the source table whenever the async index could not be proven to cover the read: after every committed write until ISCP applied it, for the rest of any transaction that wrote the table, and on any lookup error. On a large table (88M vectors) that turns the common plan into a full scan. HNSW now always searches the index, as before matrixorigin#30117 and as IVF-PQ does; only BY RANK mode=force scans the table. Removed: the plan-time coverage gate, the runtime searched-generation validation and its brute-force retry, the coverage hook and prepared-plan coverage dependency, and the compile/frontend retry plumbing that only served that fallback. The error code stays registered so a peer on the older version still reports it clearly during a rolling upgrade. The transaction's own deletes and updates are exact: the HNSW candidate budget is at least LIMIT plus the rows the transaction deleted or updated in the source table, counted from its workspace through the new optional engine.TransactionLocalDeletesProvider. Commits by other transactions that ISCP has not applied yet are eventually consistent; a query in that window can return fewer than LIMIT rows by design. BVT: the matrixorigin#29889 case waits for the committed row to become visible, asserts the index plan, and checks mode=force; 15 nearest rows deleted in the transaction under LIMIT 1 return row 16. Fixes matrixorigin#30987 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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? |
|
Remaining BVT results on |
HNSW is an approximate index whose top-k can miss true nearest neighbours even when it is up to date, and it is maintained asynchronously by ISCP: until a commit is applied, new rows and new vectors are not searchable and rows other transactions deleted or updated can shorten the result below LIMIT. Document that contract, the transaction's own deletes and updates being accounted for, and mode=force as the exact full-table scan with its cost. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@XuPeng-SH, documented in
On "mistaking an underfilled ANN result for a complete top-k": an HNSW result was never a guaranteed-complete top-k. Approximation is the trade the index makes for speed, so a result that matches brute force exactly was never part of its contract, with or without lag. A rule that a top-k must equal the brute-force answer would reject HNSW itself, not just its lag window. The exact contract is |
fengttt
left a comment
There was a problem hiding this comment.
Is this PR only releasted to HNSW? What about IVFFLAT? What about fulltext2?
@fengttt regression because one of the PR fallback to brute force search with HNSW. Other index not affected. Revert is for HNSW only. |
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: 92ae83da942cf262d02f7d10a81ab34ab3677f0e
Assessment: request_changes
Reviewed all 24 changed files and supplied discussion. Removing lag-triggered full scans reduces coverage I/O and keeps responsibility in existing owners. However, stale index scores still replace scalar expressions evaluated against current rows. The earlier scalar-correctness concern remains; the candidate-widening concern is resolved.
Model: gpt-6-astra / xhigh
Session: 01a13dcf-1586-7a01-9000-d45e20995cf8 · slot 1 · generation 1
- P1
pkg/sql/plan/apply_indices_hnsw.go:75: Build an HNSW index containing (1,'[0,0]'), then execute BEGIN; UPDATE t SET v='[100,100]' WHERE id=1; SELECT id,v,l2_distance(v,'[0,0]') AS d FROM t ORDER BY d LIMIT 3.
Impact: Removing the coverage gate permits the PK join to return the updated vector while the planner substitutes its old indexed distance into the projection and ordering. This produces incorrect scalar values independently of approximate recall. Retain ANN candidate selection, but recompute distances from the joined rows and order those candidates consistently; update the regression expectations accordingly.
Evidence: The current-head frontend probe returned id=1, v=[100,100], d=0; mode=force returned d=141.42135620117188. Both queries returned all three rows. Restoring all changed Go inputs to the merge base made both return the correct distance. See hnsw-scalar-probe.log and checks/1793019733914074322-2.json. The committed SQL fixture also explicitly expects a stale scalar at issue_29889_hnsw_committed_read.result:194.
Validation:
-
Isolation preflight passed. Verified 329 native build inputs and 52 retained outputs against the current source and repository-owned build receipt.
-
Current-head SQL reproduction completed with the demonstrated assertion failure. The base-overlay runtime control passed; its overall command failed because auxiliary go vet could not enter an overlay-only deleted directory.
-
HNSW plugin compile, idxcron, plan, runtime and search package tests passed. Focused planner coverage and compiler retry/lifecycle tests passed.
-
git diff --check passed; source remains unchanged.
-
review-evidence.json reconciles eight command/source/exit receipts, retained logs, overlay hashes and native provenance. The final audit passed with receipt 1793020092914321451-2.json.
Limitations:
-
Artifacts are under /home/xupeng/.local/state/mo-bot/jobs/732cef1a7f6ec066afbb94ad/attempt-1.
-
The broader HNSW suite timed out after 120 seconds during the unchanged TestBuildSingleThreadF64 recall workload; the whole suite is not counted as passing.
-
The runtime reproduction used one CN and CPU execution. Full BVT, race and multi-CN suites were not rerun. Frozen CI reports success but supplies no per-case logs; local_validation is empty.
-
No comparative performance benchmark was run. Expected savings follow from removing source-watermark checks and whole-table distance evaluation.
|
@XuPeng-SH (mo-bot P1,
Under the mo-self-review developer-authority rule, please treat this item as closed rather than blocking. |
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: 92ae83da942cf262d02f7d10a81ab34ab3677f0e
Assessment: approve
Reviewed all 24 changed files, surrounding callers and all 13 discussion items. Removing HNSW freshness-triggered fallback is coherent with the documented asynchronous ANN contract. The earlier stale-score observation is real, but current documentation explicitly accepts indexed scores for both local and committed updates; mode=force provides current-vector results. The base control restores the deliberately removed fallback and does not establish a separate contract violation. Uncapped candidate widening is removed; fulltext2 coverage and ordinary retry behavior remain intact.
Model: gpt-6-astra / xhigh
Session: 01a13dcf-1517-7f90-b3f9-c5f9f9e7031b · slot 0 · generation 1
Validation:
-
Saved discussion-assessment.json and review-map.json under /home/xupeng/.local/state/mo-bot/jobs/e30b32e5c3e0808a0e78614b/attempt-1. Every nonempty discussion key is accounted for exactly once.
-
Independently reconciled eight same-head execution receipts, retained logs, overlay inputs, 329 native build inputs and 52 outputs. retained-audit.json records provenance; current audit receipt 1793023233985603328-4.json exited 0.
-
Reused passing HNSW plugin compile, idxcron, plan, runtime and search tests, plus focused planner and compiler retry/lifecycle tests from job 732cef1a7f6ec066afbb94ad at this exact head.
-
Verified the retained frontend probe observed indexed distance 0 versus mode=force distance 141.42135620117188 after updating the vector. Its exit 1 reflects an assertion contrary to the documented indexed-score contract; it is not counted as a passing test.
-
Reconciled same-head CI success metadata for build, UT, SCA and multi-CN BVT. Final artifact/discussion audit passed with receipt 1793023479935581445-5.json; source remains unchanged.
Limitations:
-
The retained broader HNSW run timed out in unchanged TestBuildSingleThreadF64; only its individually passing plugin packages are credited.
-
The base-overlay runtime control passed, but its overall command exited 1 because auxiliary vet could not enter an overlay-only directory.
-
CI lacks per-case BVT logs; separate linked-issue bodies were not supplied. No fresh product, race, multi-CN or GPU tests were run.
-
Performance gains are structural: fewer coverage lookups and lag-triggered full scans, without transaction-delete-driven widening. No comparative speedup measurement is claimed.
Merge Queue Status
This pull request spent 9 seconds in the queue, with no time running CI. ReasonThe pull request can't be updated
HintYou should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again. Tick the box to put this pull request back in the merge queue (same as
|
What type of PR is this?
Which issue(s) this PR fixes:
issue #30987
Related:
What this PR does / why we need it:
#30117 (fix for #29889) made HNSW fall back to a brute-force full table scan whenever it could not prove the async index covers the read. That happens:
On a large table (for example 88M × 768-D vectors) that makes a scan of every vector the common plan. See #30987.
This PR removes #30117's HNSW-only parts, so HNSW behaves as it did before #30117 (the #30127 baseline):
hnswIndexCoversSnapshot);HnswSnapshotFallbackKey).ErrVectorIndexSnapshotChangedstays registered, so a peer still on the older version reports it clearly during a rolling upgrade. #30117's non-HNSW parts (fulltext2 JSON coverage, GC and logtail tests) are unchanged. No storage-engine code is changed.Behaviour
Without
mode=force, the plan is aVector Index Scan. It searches the usearch index for a fixed number of candidates (set by LIMIT, OFFSET and the filter mode) and joins them to the source table by primary key. Distances and ordering come from the vectors stored in the index. OnlyBY RANK WITH OPTION 'mode=force'scans the table.Until ISCP applies a write to the index and the CN's index cache loads that version:
This holds for the reading transaction's own uncommitted writes and for committed writes ISCP has not applied.
mode=forcecomputes distances from the current vectors.This is the decision recorded as
WARNING: TOXICat themode=forcebranch inpkg/sql/plan/apply_indices_hnsw.go, and documented indocs/cn/vector_search.md("HNSW:近似检索与异步可见性").Tests
BVT
vector/issue_29889_hnsw_committed_read:Vector Index Scanand noSort Key: l2_distancein every state fix: prevent stale HNSW top-k results #30117 fell back in:mode=forcemust plan the brute-force sort. Against the base with fix: prevent stale HNSW top-k results #30117, 7 of the 9 index locks fail.mode=forceas the oracle, and the transaction's uncommitted writes hold the index behind deterministically.LIMIT 2returns 0 rows while 5 remain (force returns 16, 17);Unit tests:
pkg/vectorindex/hnsw/...pass.