perf(index): add cosine HNSW indexes and text continuation (#173) - #219
Conversation
…lk#173 Create migration 0025 HNSW indexes on embeddings_text/visual/face with vector_cosine_ops. Text search now walks ORDER BY distance LIMIT n so the planner can use the index, then falls back to exact DISTINCT ON when a bounded scan underfills after per-file deduplication. Visual already matched that shape. Face clustering stays in-process. Do not use CREATE INDEX CONCURRENTLY: a failed concurrent build leaves an INVALID index that IF NOT EXISTS will skip. Wrong dimensions fail at the vector(N) column. Recall is not claimed; see bytefolk#175. Refs: bytefolk#173
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The 3-minute context deadline still uses time.Minute after the unused import cleanup. Restore the import so vet and integration compile. Refs: bytefolk#173
JOIN + parameterized ANY(exclude) made PostgreSQL seq-scan a 2k-row embeddings_text table. The shipping distance-order round now ANN-scans embeddings_text first, then joins files for owner/path/MIME/time filters. Empty exclude lists omit ANY() so the first round stays planner-friendly. Refs: bytefolk#173
TestHNSWMigrationPostgres already recorded Index Scan using both HNSW indexes. The follow-up psql script failed on sslmode in the pgx URL. Refs: bytefolk#173
CodeQL flagged q.Limit as user-controlled. Apply the same 100 cap Search() already uses so ANN continuation slices cannot be sized from the raw query.
sun-970
left a comment
sun-970
left a comment
There was a problem hiding this comment.
Review — Approve
Scope: #173 cosine HNSW indexes on embedding columns via migration 0025.
Analysis
-
Migration design ✅ — Migration 0025 creates
vector_cosine_opsHNSW indexes onembeddings_text(768),embeddings_visual(512), andembeddings_face(512). Round-trip 24→25→24→25 preserves vectors and produces VALID indexes. -
Text search integration ✅ — Uses
ORDER BY embedding <=> query LIMIT nfor planner-usable cosine-order candidates, with exact per-fileDISTINCT ONfallback when bounded scan underfills after deduplication. Honest about not claiming recall. -
Scope boundaries ✅ — Face clustering remains in-process; face index is DDL only.
index_generation_vectorsis not indexed. Recall measurement deferred to #175 (the live harness). The design doc explicitly states this does not waive a review gate or authorize deployment. -
Test coverage ✅ —
TestHNSWMigrationPostgrescovers populated migration, rollback, re-apply, and EXPLAIN ANALYZE. Verification script provided for manual validation. -
CI ✅ — Green across all required checks.
Non-blocking observation:
- Face index is created but not used for queries. This is documented and intentional.
No blocking issues found.
CodeQL still treats a sanitized q.Limit as user-controlled allocation. Use maxTextANNLimit as the make capacity so the three high alerts on runTextANN go away.
|
Follow-up |
Bindy-lbb
left a comment
Bindy-lbb
left a comment
There was a problem hiding this comment.
CODEOWNERS Review: Approved
Summary of Changes
- Migration 0025: Implements
vector_cosine_opsHNSW indexes forembeddings_text(768d),embeddings_visual(512d), andembeddings_face(512d) with idempotent down migration. - Search & Relator Optimization: Replaces bottleneck
DISTINCT ONscans with HNSW-friendly chunk iteration (queryTextDistanceOrder) and adds robust fallback (queryTextExactRemaining) for candidate underfills. - Safety & Boundary Guarantees: Caps query limits against CodeQL taint issues and fails closed on invalid path allowlists.
- Verification & Tests: Comprehensive test coverage with
TestHNSWMigrationPostgrescovering migration/rollback roundtrips and EXPLAIN ANALYZE index usage verification.
All CI pipelines and CodeQL checks have passed. Approved for merge.
4 participants
Summary
Completes the work #213 left undone against #173.
embeddings_text(768),embeddings_visual(512), andembeddings_face(512) withvector_cosine_ops.DISTINCT ON (f.id) ORDER BY f.id, distanceas the primary plan. That shape cannot use HNSW. It now walksORDER BY embedding <=> $1 LIMIT n, keeps the first sighting of each file, excludes selected files, and falls back to the original exactDISTINCT ONquery if a bounded scan underfills (one file owning many near chunks).index_generation_vectorsis not indexed (scope boundary).CREATE INDEX(notCONCURRENTLY): a failed concurrent build leaves anINVALIDindex thatIF NOT EXISTSwill skip.vector(N)column. Recall is not claimed; harness is #175.Does not reopen #213. #197 remains the earlier HOLD attempt; this branch is based on current
main(schema 24) and takes 0025.Changes
server/internal/db/migrations/0025_ann_hnsw_indexes.sqlserver/internal/search/search.go— text continuation + exact fallbackserver/internal/relator/relator.go— same policyTestHNSWMigrationPostgres— populated 24→25→24→25, ingest, dimension rejection, EXPLAINTestTextANNFileSemanticsPostgres— 101-chunk file still returns 10 eligible filesscripts/verify_hnsw_indexes.sh+scripts/verify.shhead 25docs/VALIDATION_HNSW.md, SPEC, CHANGELOGValidation ledger
make test-servermake test-workermake test-webmake test-racemake test-integrationEXPECTED_MIGRATION_HEAD=25;TestHNSWMigrationPostgres+verify_hnsw_indexes.shmake test-integration-racemake test-recallTestTextANNFileSemanticsPostgresTestHNSWMigrationPostgres+scripts/verify_hnsw_indexes.shenable_seqscan=off.Local sandbox could not run Docker or download Go 1.25, so EXPLAIN was not executed here. CI
memory-validation.yml/verify.sh integrationis the evidence path.Design notes
m=16,ef_construction=64. No iterative-scan GUC.uuid[]exclude lists are never sent as SQL NULL (ANY(NULL)would drop all rows).Fixes #173