feat(backends): observable maintenance hooks + pgvector indexed-build path (RFC 001) - #1732
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements backend maintenance hooks (RFC 001) for the pgvector and sqlite_exact backends, introducing maintenance_state() and run_maintenance(kind) to handle tasks like database analysis, compaction, and index rebuilding. The review feedback highlights two important issues in the pgvector implementation: a critical bug where _hnsw_index_name can generate an index name identical to the table name for 63-character table names, causing a namespace conflict, and a robustness issue where maintenance_state lacks comprehensive exception handling, potentially violating its non-raising contract.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def _hnsw_index_name(table: str) -> str: | ||
| """Deterministic index name for ``table``, clamped to Postgres' 63-char limit.""" | ||
| return f"{table}_hnsw_idx"[:63] |
There was a problem hiding this comment.
Critical Bug: If the table name is exactly 63 characters long (which is the exact length returned by _pg_identifier for any long table name), _hnsw_index_name will return the table name itself because f"{table}_hnsw_idx"[:63] slices the string back to the original table name.
In Postgres, tables and indexes share the same namespace (pg_class). Attempting to create an index with the same name as the table will fail with a relation already exists error. Using _pg_identifier to safely clamp and hash the index name guarantees uniqueness and avoids this conflict.
| def _hnsw_index_name(table: str) -> str: | |
| """Deterministic index name for ``table``, clamped to Postgres' 63-char limit.""" | |
| return f"{table}_hnsw_idx"[:63] | |
| def _hnsw_index_name(table: str) -> str: | |
| """Deterministic index name for ``table``, clamped to Postgres' 63-char limit.""" | |
| return _pg_identifier(f"{table}_hnsw_idx") |
| def maintenance_state(self) -> dict: | ||
| self._ensure_open() | ||
| if not self._table_exists(): | ||
| return {"row_count": 0, "vector_index": None, "index_build_complete": False} | ||
| rows = self._client.count_rows(self._table) | ||
| has_index = False | ||
| try: | ||
| has_index = self._client.has_vector_index(self._table) | ||
| except Exception: # noqa: BLE001 - state report must not raise | ||
| logger.debug("pgvector index probe failed", exc_info=True) | ||
| return { | ||
| "row_count": rows, | ||
| "vector_index": "hnsw" if has_index else None, | ||
| "index_build_complete": has_index, | ||
| } |
There was a problem hiding this comment.
Robustness Issue: The maintenance_state method does not wrap self._table_exists() and self._client.count_rows(self._table) in a try...except block. If there is a database connection issue or operational failure, this method will raise an exception, violating the "must not raise" contract of maintenance_state (as done in sqlite_exact.py). Wrapping all database calls ensures the method safely returns a default state dictionary.
| def maintenance_state(self) -> dict: | |
| self._ensure_open() | |
| if not self._table_exists(): | |
| return {"row_count": 0, "vector_index": None, "index_build_complete": False} | |
| rows = self._client.count_rows(self._table) | |
| has_index = False | |
| try: | |
| has_index = self._client.has_vector_index(self._table) | |
| except Exception: # noqa: BLE001 - state report must not raise | |
| logger.debug("pgvector index probe failed", exc_info=True) | |
| return { | |
| "row_count": rows, | |
| "vector_index": "hnsw" if has_index else None, | |
| "index_build_complete": has_index, | |
| } | |
| def maintenance_state(self) -> dict: | |
| self._ensure_open() | |
| try: | |
| if not self._table_exists(): | |
| return {"row_count": 0, "vector_index": None, "index_build_complete": False} | |
| rows = self._client.count_rows(self._table) | |
| has_index = self._client.has_vector_index(self._table) | |
| except Exception: # noqa: BLE001 - state report must not raise | |
| logger.debug("pgvector maintenance state probe failed", exc_info=True) | |
| return {"row_count": 0, "vector_index": None, "index_build_complete": False} | |
| return { | |
| "row_count": rows, | |
| "vector_index": "hnsw" if has_index else None, | |
| "index_build_complete": has_index, | |
| } |
There was a problem hiding this comment.
Pull request overview
This PR implements RFC 001’s observable backend maintenance hooks across the backend contract and in-tree backends, including an operator-invoked pgvector HNSW index build path serialized by advisory locks to prevent concurrent index-build wedges.
Changes:
- Adds
MaintenanceResult,UnsupportedMaintenanceKindError,BaseBackend.maintenance_kinds, and defaultBaseCollection.maintenance_state()/run_maintenance()behaviors to the backend contract. - Implements maintenance hooks for
sqlite_exact(ANALYZE + VACUUM/compact with reclaimed-pages stats) andpgvector(ANALYZE + advisory-lock-serialized HNSW “reindex”). - Adds delegation in
EmbeddingCollectionand comprehensive contract/backend tests for maintenance hooks (including a fake pgvector client for advisory-lock flow).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
mempalace/backends/base.py |
Introduces the maintenance hook contract types and defaults on the base interfaces. |
mempalace/backends/sqlite_exact.py |
Implements sqlite maintenance state reporting plus analyze/compact maintenance kinds. |
mempalace/backends/pgvector.py |
Adds advisory-lock helpers and implements analyze + lock-serialized HNSW index build (reindex). |
mempalace/backends/embedding_wrapper.py |
Ensures maintenance hook calls delegate to the wrapped collection (avoids BaseCollection method shadowing). |
mempalace/backends/__init__.py |
Re-exports MaintenanceResult and UnsupportedMaintenanceKindError from the public backends package surface. |
tests/test_maintenance_hooks.py |
Adds test coverage for the new contract surface and sqlite/pgvector maintenance behaviors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _hnsw_index_name(table: str) -> str: | ||
| """Deterministic index name for ``table``, clamped to Postgres' 63-char limit.""" | ||
| return f"{table}_hnsw_idx"[:63] |
| self._ensure_open() | ||
| if kind == "analyze": | ||
| self._client.analyze_table(self._table) | ||
| return MaintenanceResult(kind="analyze", status="ran") | ||
|
|
| # across daemon writers (RFC 001). classid is a fixed mempalace constant; | ||
| # objid is a stable per-table key. Both must fit a signed int4, which | ||
| # ``pg_advisory_lock(int4, int4)`` requires. | ||
| _MAINTENANCE_LOCK_CLASSID = 0x4D454D50 # "MEMP" → 1296256848, a valid int4 |
… path (RFC 001, #1725) Adds the maintenance contract RFC 001 specifies but #1679 deferred, and gives pgvector an opt-in HNSW index path with concurrency-safe builds. - base.py: MaintenanceResult (status ran/already_running/noop + free-form stats), UnsupportedMaintenanceKindError, BaseBackend.maintenance_kinds ClassVar (reserved: analyze/compact/reindex; a backend with no analogue MUST omit, not no-op), and BaseCollection.maintenance_state()/run_maintenance() defaults. EmbeddingCollection forwards both (BaseCollection methods shadow __getattr__). - sqlite_exact: analyze (ANALYZE) + compact (VACUUM, autocommit + page stats); omits reindex (exact scan, no ANN index). maintenance_state reports row/page counts. - pgvector: reindex builds the optional HNSW index, serialized by a session-level pg_advisory_lock so concurrent daemon writers learn "already_running" instead of each stacking an ACCESS EXCLUSIVE build (the production wedge). It is opt-in: the default exact `<=>` scan is the 100%-recall path; an HNSW index trades exact recall for scale, so an operator invokes it deliberately. Also analyze; omits compact (autovacuum). Advertises supports_server_side_indexes. maintenance_state reports index presence. - qdrant/chroma: empty maintenance_kinds (qdrant self-optimizes; chroma maintenance is the separate repair CLI) — the faithful "omit" default. Tests: contract + sqlite (real, CI-runnable) + pgvector advisory-lock flow via a fake client (ran/noop/already_running, no live Postgres). Full suite green: 2488 passed, 82.47% coverage. Benchmark three-phase wiring is deferred — the existing benchmarks/ are task-benchmarks, not backend-comparison harnesses, so there is nothing to wire into yet. Closes #1725. Refs #743.
a65c065 to
fdbafe5
Compare
Close the last open items on #743 before merge: - Conformance: document two isolation arms (cross-id for all backends; same-id/different-namespace for supports_namespace_isolation advertisers). - No silent drop: non-advertising backends must raise UnsupportedCapabilityError when PalaceRef.namespace is set, rather than accept-and-ignore. - Wire require_namespace_support() into chroma/sqlite_exact; add conformance test. - Refresh implementation-status banner now that #1727/#1731/#1732/#1734 landed.
Summary
Implements the backend maintenance hooks from RFC 001 (#743) — the last of the deferred follow-ups (#1725) — and gives pgvector an opt-in HNSW index path with concurrency-safe builds. This closes the loop on the "is pgvector doing real vector search?" thread: it stays exact-by-default (100% recall), and an operator at scale can opt into an indexed path safely.
What changed
backends/base.py):MaintenanceResult(status∈ran/already_running/noop, plus free-formstats),UnsupportedMaintenanceKindError,BaseBackend.maintenance_kindsClassVar (reservedanalyze/compact/reindex; a backend with no analogue MUST omit a kind, not no-op it), andBaseCollection.maintenance_state()/run_maintenance(kind)defaults.EmbeddingCollectionforwards both (they're concrete onBaseCollection, so__getattr__would otherwise shadow them).analyze(ANALYZE) +compact(VACUUM with autocommit, reporting pages reclaimed). Omitsreindex— exact cosine over every row, no ANN index.maintenance_statereports row/page/freelist counts.reindexbuilds the optional HNSW index (USING hnsw (embedding vector_cosine_ops)), serialized by a session-levelpg_advisory_lockso concurrent daemon writers learnalready_runninginstead of each stacking anACCESS EXCLUSIVEbuild — the production wedge this fixes. It is opt-in: the default exact<=>scan is the 100%-recall path; an HNSW index makes search approximate (trades recall for scale), so an operator invokes it deliberately. Alsoanalyze; omitscompact(Postgres autovacuum). Advertisessupports_server_side_indexes.maintenance_statereports index presence.maintenance_kinds— qdrant self-optimizes, chroma's maintenance is the separaterepairCLI. The faithful "omit" default.Recall posture (intentional)
Building an HNSW index makes pgvector's query approximate, which is why
reindexis an explicit operator action rather than automatic. The default path (no index, exact scan) preserves MemPalace's 100%-recall requirement; the hook is the "let operators control scaling characteristics" lever the RFC added.maintenance_state()reports which mode a collection is in.Tests
tests/test_maintenance_hooks.py(16 tests): contract surface; sqlite_exact against a real backend (analyze/compact/state, reindex-omitted, unknown-kind — all CI-runnable); and the pgvector advisory-lockreindexflow via a fake client coveringran/noop/already_runningand lock release, plus advisory-key bounds — no live Postgres needed. Full suite: 2488 passed, 82.47% coverage,ruff check+format --checkclean.Deferred
The §7.3 benchmark three-phase publishing is not wired: the existing
benchmarks/are task-benchmarks (LoCoMo, LongMemEval, …), not backend-comparison harnesses, so there's no harness to recordmaintenance_state()into yet. Noted for when such a harness exists.Closes #1725. Refs #743.