Repository navigation
fix: prevent stale HNSW top-k results - #30117
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? |
matrix-meow
added
size/L
Thanks for asking. The persisted-delete component test itself cannot run unchanged on main, because it depends on HNSW coverage code introduced by this PR. The old-provider overlay failure (sourceTS=1 instead of 150) validates the earlier provider fix in #29798; it is not a pre-PR failure for this HNSW change. I verified the SQL regression case for #29889 on mo@10.222.1.51 with separately built services. On current main (8e315d1), the query after the insert returned the wrong id=1 in 4/4 runs; on the PR (d09e411), it returned the expected id=3 in 4/4 runs. I also added TestHnswLaggingGenerationFallsBackToSource in 6e918ad. It fixes the index build timestamp at 99, the latest source commit at 100, and the read snapshot at 101. It fails on main because the planner still selects the stale HNSW index, and passes on the PR because the planner retains the source scan. |
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-xupeng · review
Commit: 6e918ad05794cfb5e7e68352c8aa1cca49d4ce26
Assessment: approve
Reviewed the complete 26-file delta, callers, ownership, failure paths and supplied review concerns. HNSW coverage checks, prepared-plan rebuilding, searched-generation validation and checkpoint cleanup/rollback are consistent. No actionable unresolved findings.
Model: gpt-6-astra / xhigh
Session: 01a15209-2c6d-71e2-b8f3-aa17387fa283 · slot 2 · generation 0
Validation:
-
The new source-lag regression passed both cases on the candidate. The exact merge-base Go overlay plus this test failed only the lagging-generation case, confirming the regression.
-
Reconciled retained evidence for byte-identical production and existing tests: 36 focused tests, five focused race tests, 402 race repetitions and six complete owner-package race suites. Two RPC fixture failures caused by read-only cwd passed their retained writable-cwd rerun.
-
Isolation preflight passed. Verified 192 native inputs, 56 native artifacts, clean frozen source, command/source/exit receipts and retained log hashes.
-
Detailed evidence: /home/xupeng/.local/state/mo-bot/jobs/1db5e75df6de14c46984b2dc/attempt-1/validation-report.json and review-coverage.json.
Limitations:
-
No fresh live SQL/BVT or end-to-end native HNSW execution in this attempt; the new regression exercises planner behavior, and retained persisted-delete coverage uses a synthetic ANN backend.
-
Linked issue body, author live-SQL raw logs and per-case CI logs were unavailable. Summaries and green statuses were not treated as independent behavioral verification.
-
No throughput, p99 or large-catalog benchmark; additional watermark checks, prepared-plan rebuilding and metadata-copy costs remain unquantified.
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-xupeng · review
Commit: 8895442cd2493072d3e3cd5140d6d20b6e0cb505
Assessment: approve
Reviewed the complete 19-file delta, surrounding owners and callers, and supplied review concerns. The change reuses existing coverage, cache-generation, retry, and checkpoint owners. No actionable defects remain.
Model: gpt-6-astra / xhigh
Session: 01a15209-2c6d-71e2-b8f3-aa17387fa283 · slot 2 · generation 0
Validation:
-
Isolation preflight passed. Native prerequisites were built through make cgo and verified against 192 current-head inputs and 56 artifact hashes.
-
Complete normal tests passed for six packages: SQL planner, HNSW plugin, coverage, search, GC v3, and logtail. Go 1.27.1, Linux/amd64, matrixone_test, short mode; two existing planner subtests skipped.
-
The merge-base plus the new source-lag regression failed behind_source and passed covers_source. Both cases passed on the candidate.
-
Two initial build timeouts remain recorded as incomplete checks; subsequent runs completed the six changed Go owners.
-
Source remained clean and git diff --check passed. Commands, source fingerprints, exit receipts, and retained logs were reconciled in attempt-1/validation-report.json; final-evidence-audit.log records successful verification.
Limitations:
-
No local race, live SQL/BVT, multi-CN, or performance rerun. Frozen CI reports success, but its raw logs and the author's current-head artifacts were unavailable locally.
-
The linked issue body was not supplied; its behavioral contract was assessed through the PR description, comments, regression cases, and production paths.
aunjgr
left a comment
aunjgr
left a comment
There was a problem hiding this comment.
Request changes on 8895442 for one architectural boundary violation.
[P2] Keep prepared-plan coverage policy behind the index plugin
PreparedPlanDependsOnIndexCoverage in pkg/sql/plan/apply_indices_fulltext_json.go adds a new catalog.IsHnswIndexAlgo(...) branch. This makes the common prepared-plan owner enumerate another algorithm to decide whether execution must rebuild. The repository index-plugin contract explicitly prohibits adding this algorithm-specific SQL-layer dispatch: the algorithm's capability/policy must be supplied through a hook or descriptor. This PR already installs HNSW's coverage hook; please also route the prepared-plan dependency decision through a narrow plugin-owned policy, preserving the existing fulltext JSON-probe distinction. This is a concrete architecture gate, not a claim that the present two-algorithm classifier returns the wrong result.
The freshness invariant itself is coherent: retain the full-HLC source lower bound, require physical build time strictly before the read snapshot, and validate the cache's under-lock receipt for the generation actually searched. I traced cold/historical metadata, exact/pre-mode fallback, prepared rebuilding, result closure and registration, and reviewed the persisted-tombstone/control evidence. No additional verified functional or cleanup finding from those paths. Passing exact-head CI is reused; synthetic ANN output is not being presented as native ANN or throughput qualification. No new local service run is claimed.
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-xupeng · review
Commit: f93e55191d6037bfec9a46ef6fc114f045002757
Assessment: approve
Reviewed the complete 21-file delta and supplied concerns. Plugin-owned prepared-plan policy preserves HNSW and fulltext2 behavior; freshness checks and regression coverage address prior findings. No actionable defects remain.
Model: gpt-6-astra / xhigh
Session: 01a14d34-4e51-79a3-a62d-6c9c16279dc8 · slot 0 · generation 0
Validation:
-
Current-head planner, indexplugin, fulltext2 plugin and HNSW plugin suites passed: 3,896 top-level tests; two existing subtests skipped.
-
Independently reconciled retained coverage, reader, GC and logtail checks against receipts, logs and unchanged relevant inputs.
-
Verified the retained merge-base regression fails behind_source and passes covers_source; both cases pass on the candidate.
-
Isolation preflight, repository-owned native build, provenance verification and 190 native-input comparisons passed. Frozen source remains clean.
-
Evidence reconciliation: /home/xupeng/.local/state/mo-bot/jobs/50970fe3feea2683589eeb53/attempt-1/validation-report.json. Initial toolchain and audit setup failures are recorded separately.
Limitations:
-
No fresh race, live SQL/BVT, native ANN, multi-CN or performance run; added watermark-check costs remain unquantified.
-
Linked issue body and author raw artifacts were unavailable; local_validation was empty. Aggregate CI statuses were not treated as case-level execution proof.
aunjgr
left a comment
aunjgr
left a comment
There was a problem hiding this comment.
APPROVE at f93e55191d6037bfec9a46ef6fc114f045002757. My previous plugin-boundary finding is resolved.
The SQL prepared-plan classifier now asks the registered plugin's narrow optional coverage policy. HNSW owns its always-rebuild decision; fulltext2 owns the JSONProbe-versus-user-MATCH distinction. The framework contains no algorithm-specific dispatch/import, existing registration/build-tag boundaries remain intact, and compile-time conformance plus negative/unaffected-mode tests cover the new policy.
Reconciled the complete current change map with my previous reviewed head 8895442cd2493072d3e3cd5140d6d20b6e0cb505: only five of the current feature's paths differ, comprising this policy extraction and its tests. The HNSW coverage kernel, physical reader, source/tombstone regressions, GC/replay tests and public cases retain byte-identical contents. This is a focused correctness repair using existing coverage/watermark/cache owners, not a new maintenance or execution framework.
Rechecked the functional closure: generation physical time must strictly precede the read snapshot, its conservative HLC lower bound must cover the full source watermark, and execution validates the generation actually searched using the existing under-lock cache receipt. Missing/unsafe provenance keeps the existing exact-scan or typed pre-mode retry behavior. Historical/cold metadata lookups and prepared rebuild ownership remain consistent; no duplicate watermark or lifecycle state is added.
Reused qualifying green exact-head CI and source-qualified planner/plugin/provider/cache/prepared/replay/race evidence from the unchanged closure. Existing SQL BVT assertions retain committed-result and catalog oracles without promising a particular legitimate asynchronous scan plan. No fresh native ANN execution, forced tombstone-flush SQL, mixed-binary deployment or throughput/p99 benchmark is claimed. No remaining blocking finding.
|
mo-bot review · Commit: Review belongs to an older commit; it is not a review of the current head. |
|
@Mergifyio queue |
☑️ This pull request is already queued |
Merge Queue Status
This pull request spent 2 hours 46 minutes 5 seconds in the queue, including 1 hour 23 minutes 5 seconds running CI. Required conditions to merge
|
This branch was successfully deployed
7 participants
What type of PR is this?
Which issue(s) this PR fixes:
Fixes #29889.
What this PR does / why we need it:
An asynchronously maintained HNSW generation can omit committed nearest rows or contain vector contents newer than the read snapshot. Planning and execution must prove that the generation actually searched covers the source and predates the read; otherwise the query uses the existing exact-scan or typed pre-mode retry path.
Reuse the coverage hook, source watermark provider and cache's under-lock searched-generation receipt. Coverage-dependent prepared plans rebuild at execution. Preserve the full-HLC source lower bound and require
buildPhysical < readPhysical, because generation provenance has only physical time. Reject invalid upper bounds before source lookup, and clone historical metadata transactions only on cache misses. Main's shared persisted-tombstone provider supplies DELETE coverage; no duplicate watermark state or scan path is added.Rebased onto main
d82e828b0a4df9ce635fe573c1e90c5cde978bf9. Use upstream #29685's checkpoint error propagation, unpublished-object ownership, standalone Update rollback and outer GC-scan Clone/RestoreFrom handling. The overlapping checkpoint/backup/logtail/RPC production changes are fully supplied by main. Preserve the real GC failure/retry/replay regression, consolidate V12 preextended-column allocation failures into the upstream test, and extend the existing clone/restore test with alias and independence assertions. No duplicate savepoint API remains. The final delta contains 19 files; 17 retain byte-identical original PR contents.Regression coverage includes a lagging-generation planner control, actual persisted TN tombstone replay through the provider/coverage/cache/reader boundary, historical snapshots, prepared rebuilding and searched-generation validation. The persisted-delete component uses synthetic ANN output. ALTER COPY retains catalog/result assertions while allowing either legitimate asynchronous scan plan.
Validation
Go 1.27.1, macOS arm64 CPU, matching provenance-verified native artifacts and the repository CGo runner:
ddl/alter_copy_add_index.sql(78/78) andvector/issue_29889_hnsw_committed_read.sql(12/12), twice each on the same isolated instance. Database and catalog teardown checks are zero after every run. SQL and ISCP readiness were verified before execution.gc/v3/window_test.goalso reproduces with all PR Go changes overlaid back to exact clean main; it is not introduced by this PR.~/.codex/issue-to-pr/matrixone/30117-rebase-main-20261019/. Setup failures did not supply behavioral validation.No forced tombstone-flush SQL, forced native ANN execution, throughput/p99 qualification or heavy full-CI/performance rerun is claimed. Additional source-watermark checks and metadata copying retain their existing correctness costs. GitHub CI for the updated head is pending.