fix(vector-index): repair per-bank index coverage after restore/upgrade (#2645) - #2872
Merged
Merged
Conversation
…de (#2645) Per-(bank, fact_type) partial vector indexes are created only at fresh-bank creation. A bank populated outside that path (logical restore, cross-version upgrade, extension switch) never gets them, so its recall silently falls back to the global index + post-filter — slower and under-returning (~0.63-0.72 recall@10 measured by the reporter). Two fixes: - import-bank: create the per-bank indexes explicitly after restoring the banks row. The prior get_or_create_bank_profile call was a no-op here (the row already exists, so it takes the SELECT branch), leaving every restored bank uncovered. - hindsight-admin repair-bank (--bank ID | --all): re-runnable operator escape hatch for the out-of-app routes (raw pg_dump restore, extension switch) that a one-time migration can't cover (a restore carries alembic_version at head, so the migration is already stamped). Detects missing OR invalid coverage (INVALID leftovers / drifted access method count as missing, unlike a name-only check) and rebuilds with CREATE INDEX CONCURRENTLY off any txn. Idempotent; concurrency handled by idempotency, not advisory locks. Deliberately excludes the boot/periodic background reconcile and retain-path self-heal: a bank restored and only ever read stays degraded until an operator runs repair-bank. That background layer can be a follow-up.
Collaborator
Author
|
Relationship to the other open PRs on #2645:
Happy to fold in the background reconcile as a follow-up if maintainers want zero-touch convergence for restored-then-read-only banks; this PR closes the leak and gives operators the escape hatch. Note: the pg0-backed integration tests could not run in my local worktree (embedded Postgres wouldn't boot here); they need CI to validate. The non-DB test and all lint/type/dead-code checks pass locally. |
This was referenced Jul 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2645.
Problem
Per-
(bank, fact_type)partial vector indexes (idx_mu_emb_{obsv,expr,worl}_<internal_id16>) are created only at fresh-bank-creation time (gated onINSERT ... ON CONFLICT DO NOTHING RETURNING). A bank that becomes populated outside that path never gets them, so its bank-scoped recall silently falls back to the global index + abank_id/fact_typepost-filter — which is both slower and under-returns results (the ANN candidate set is drawn across all banks, then filtered to the target bank afterward). As measured by the reporter on a live 40-bank deployment: recall@10 of ~0.63–0.72 — a silent ~30% top-10 miss — plus 14×–100× slower plans.A bank reaches this uncovered state via a logical restore, a cross-version upgrade, or a vector-extension switch (e.g. ScaNN→pgvector).
Two roots, two fixes
1. The one in-app leak:
import-bank.importer.pyrestores thebanksrow directly, then callsget_or_create_bank_profileintending to "ensure the indexes exist" — but because the row now exists, that call takes the SELECT branch (created=False) and skips index creation. So every imported/restored bank landed uncovered. Fixed by creating the per-bank indexes explicitly right after thebanksrow is restored, while the bank is still empty (facts are imported afterward, so the build is instant).2. The out-of-app routes: a re-runnable repair command. Raw
pg_dump/restore and extension switches never run application code, so there is no hook to plug — the escape hatch has to be an operator command. A one-time migration can't cover these: a logical restore carriesalembic_versionat head, so a backfill migration is already stamped "applied" and skips.hindsight-admin repair-bankis re-runnable — run it once post-upgrade, and again after each restore/switch.memory_units, on a supported access method, and to carry the partial predicate. A name-only check (pg_indexes+IF NOT EXISTS) would treat an INVALID leftover from an interrupted build, or an index whose type drifted after a backend switch, as "present" and never repair it — this rejects both and rebuilds them.CREATE INDEX CONCURRENTLYon a raw autocommit connection — never takesACCESS EXCLUSIVEon the sharedmemory_units, so repairing one bank can't stall the fleet.CREATE INDEX CONCURRENTLY IF NOT EXISTSgated by a valid/ready health check, so concurrency is handled by idempotency rather than a lock (per the project rule against advisory locks, which are unreliable behind poolers).--dry-runreports without touching anything, multi-tenant (base + discovered tenant schemas), no-op for global-index backends (ScaNN/Oracle).Deliberately NOT included
No boot/periodic background reconcile and no retain-path self-heal hook. Those add a maintenance-loop job, an advisory-lock-gated worker connection, a PgBouncer/transaction-pool guard, and a hot-path catalog check — a much larger surface. The accepted tradeoff: a bank restored and then only ever read stays degraded until an operator runs
repair-bank. For deployments that do routine bare-metal restores and want zero-touch convergence, that background layer can be a follow-up; this PR is the minimal, self-contained fix for the leak and the operator escape hatch.Tests
tests/test_repair_bank_vector_indexes.py(deterministic, pg0 — index presence/shape via the catalog, no LLM):import-bankround-trip (export → delete → import) leaves the restored bank with its per-bank partial indexes (the leak regression);repair-bank --bank/--allrebuild dropped indexes;--dry-runcreates nothing; re-run is idempotent (no dupes); exactly one of--bank/--allis required; no-op backend is a no-op.Plus admin-CLI docs for the new command.