perf(search): AGE edge-endpoint indexes for hybrid/age-fused graph-walk latency (Cat 7b) - #206
Conversation
…h-walk latency Cat 7b: /search/hybrid p50 was ~3-5x /search and /search/keyword (prod familiar 2026-05-30: vector 626ms, union 429ms, hybrid 2064ms, p95 5.6s). The entire delta is AGE graph-walk Cypher, not rerank, pool size, or the vector/BM25 fusion. Each per-entity edge lookup the hybrid candidate-merger and /search/age-fused's _age_lookup issue does a full Parallel Seq Scan of the edge backing table (MENTIONS 6.69M rows, RELATION 1.92M rows) because AGE only btree-indexes a label table's own id, never the start_id/end_id graphid columns the walks join on. ~5.8s cold for a hot entity, x N entities. Add POST /backfill-age/indexes: an operator-triggered route that installs the four missing edge-endpoint indexes (idx_mentions_end_id, idx_mentions_start_id, idx_relation_start_id, idx_relation_end_id) with CREATE INDEX CONCURRENTLY IF NOT EXISTS — no table lock against live reads, idempotent, auth-gated, postgres-only (503 on chroma). Mirrors the existing /backfill-age operator pattern; no silent startup DDL. Validated on a synthetic 3.17M-edge AGE graph (apache/age PG16_1.6.0): the MENTIONS seq scan flips to a Bitmap Index Scan (planner cost 13855 -> 831; cold wall-clock 320ms -> 206ms at half-prod scale, widening at prod's 2x). Negative results recorded in the docs note: parallelizing the per-entity loop is *slower* (contention on the seq scans), and a direct-SQL Cypher rewrite compiles to the same seq-scan plan — only the index changes the plan. Companion scripts/age_graph_indexes.sql for offline application; full profile in docs/perf/2026-05-30-hybrid-graph-walk-latency.md. These indexes belong in mempalace.backfill_age long-term (upstream follow-up); this route is the bridge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses significant latency issues in hybrid and age-fused graph retrieval by introducing missing edge-endpoint indexes. The current implementation of AGE graph-walks was performing full sequential scans on large edge tables for every entity lookup, leading to substantial performance degradation. By providing a safe, operator-triggered mechanism to install these indexes concurrently, the changes significantly reduce query latency and improve scalability under load. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![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 introduces a new operator-triggered endpoint POST /backfill-age/indexes to concurrently install missing edge-endpoint indexes on the AGE graph, resolving a significant latency bottleneck in hybrid and age-fused graph-walk queries. It also includes comprehensive performance documentation, an offline SQL script, and robust unit tests. The review feedback highlights two important improvements: first, checking indisvalid = true on pg_index to ensure that invalid indexes from previously failed concurrent builds are not incorrectly treated as already present; second, restructuring the connection lifecycle handling to prevent potential connection leaks if setting autocommit fails.
| def _existing_age_indexes(conn) -> set[str]: | ||
| """Names of the edge-endpoint indexes already present on mempalace_kg.*.""" | ||
| wanted = tuple(name for name, _ in _AGE_INDEX_DDL) | ||
| with conn.cursor() as cur: | ||
| cur.execute( | ||
| "SELECT indexname FROM pg_indexes " | ||
| "WHERE schemaname = 'mempalace_kg' AND indexname = ANY(%s)", | ||
| (list(wanted),), | ||
| ) | ||
| return {r[0] for r in cur.fetchall()} |
There was a problem hiding this comment.
In PostgreSQL, if a CREATE INDEX CONCURRENTLY statement fails, it leaves behind an INVALID index in the catalog. The pg_indexes view includes invalid indexes, meaning that if a previous index creation failed, _existing_age_indexes will find the invalid index, report it as already_present, and skip recreating it. This results in a silent performance degradation because the query planner cannot use invalid indexes.
To prevent this, query pg_index directly and check indisvalid = true to ensure only valid indexes are considered present. If an invalid index exists, the route will attempt to recreate it and fail with a clear error (e.g., relation already exists), alerting the operator to drop the invalid index.
def _existing_age_indexes(conn) -> set[str]:
"""Names of the valid edge-endpoint indexes already present on mempalace_kg.*."""
wanted = tuple(name for name, _ in _AGE_INDEX_DDL)
with conn.cursor() as cur:
cur.execute(
"SELECT c.relname "
"FROM pg_index i "
"JOIN pg_class c ON c.oid = i.indexrelid "
"JOIN pg_namespace n ON n.oid = c.relnamespace "
"WHERE n.nspname = 'mempalace_kg' "
" AND c.relname = ANY(%s) "
" AND i.indisvalid = true",
(list(wanted),),
)
return {r[0] for r in cur.fetchall()}| try: | ||
| conn = psycopg2.connect(dsn, connect_timeout=5) | ||
| except psycopg2.OperationalError as e: | ||
| _record_db_error(e) | ||
| raise | ||
| # CONCURRENTLY forbids an open transaction; autocommit each DDL. | ||
| conn.autocommit = True | ||
| try: | ||
| with conn.cursor() as cur: | ||
| cur.execute("LOAD 'age'") | ||
| cur.execute('SET search_path = ag_catalog, "$user", public') | ||
| present = _existing_age_indexes(conn) | ||
| for name, ddl in _AGE_INDEX_DDL: | ||
| if name in present: | ||
| already.append(name) | ||
| continue | ||
| try: | ||
| with conn.cursor() as cur: | ||
| cur.execute(ddl) | ||
| created.append(name) | ||
| except Exception as e: # one bad index shouldn't sink the rest | ||
| errors[name] = f"{type(e).__name__}: {e}" | ||
| logging.getLogger("palace-daemon").warning( | ||
| "backfill-age/indexes: %s failed: %s", name, e | ||
| ) | ||
| finally: | ||
| try: | ||
| conn.close() | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
If psycopg2.connect succeeds but setting conn.autocommit = True raises an exception, the connection is leaked because the second try block is never entered, bypassing the finally block.
Initializing conn = None and wrapping the entire connection setup and execution in a single try...finally block ensures that the connection is always closed cleanly if it was successfully opened.
conn = None
try:
try:
conn = psycopg2.connect(dsn, connect_timeout=5)
except psycopg2.OperationalError as e:
_record_db_error(e)
raise
# CONCURRENTLY forbids an open transaction; autocommit each DDL.
conn.autocommit = True
with conn.cursor() as cur:
cur.execute("LOAD 'age'")
cur.execute('SET search_path = ag_catalog, "$user", public')
present = _existing_age_indexes(conn)
for name, ddl in _AGE_INDEX_DDL:
if name in present:
already.append(name)
continue
try:
with conn.cursor() as cur:
cur.execute(ddl)
created.append(name)
except Exception as e: # one bad index shouldn't sink the rest
errors[name] = f"{type(e).__name__}: {e}"
logging.getLogger("palace-daemon").warning(
"backfill-age/indexes: %s failed: %s", name, e
)
finally:
if conn is not None:
try:
conn.close()
except Exception:
pass
Two correctness improvements from the PR #206 review: 1. Presence probe now checks pg_index.indisvalid instead of the pg_indexes view. A CREATE INDEX CONCURRENTLY that fails partway leaves an INVALID index in the catalog — pg_indexes lists it but the planner can't use it. Counting it as already_present would silently skip the rebuild and leave the latency bug unfixed. Filtering on indisvalid means an invalid index reads as absent, so the route re-attempts it (and the rebuild's clear "already exists" error alerts the operator to DROP the invalid one). 2. Connection-leak hardening: init conn=None and wrap the whole connect + autocommit + execute sequence in a single try/finally. Previously if connect() succeeded but `conn.autocommit = True` raised, the finally (in a separate nested try) was never entered and the connection leaked. New tests: probe queries pg_index/indisvalid (not the view); connection is closed even when setting autocommit raises. indisvalid probe SQL verified valid against a real apache/age PG16 graph. 9/9 index-route tests green. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cat 7b — hybrid retrieval is slow
Reported: vector p50 626ms / union 429ms / hybrid 2064ms (p95 5.6s) — hybrid ~3-5× the others.
Root cause (profiled read-only on prod familiar:8085, 2026-05-30)
The entire hybrid-vs-union delta is AGE graph-walk Cypher — not rerank (~14ms), not candidate-pool size (
n_input=10both), not the vector/BM25 fusion. union (vector ∪ BM25) is ~13× faster than hybrid at the same retrieval; the only difference is hybrid's graph candidate-merger.Each per-entity edge lookup the hybrid merger (
mempalace _graph_expand_from_*) and this daemon's/search/age-fused_age_lookupissue does a fullParallel Seq Scanof the edge backing table — MENTIONS (6.69M rows) or RELATION (1.92M rows) — because AGE only btree-indexes a label table's ownid, never thestart_id/end_idgraphid columns the edge walks join on.EXPLAIN ANALYZEshowed the full MENTIONS seq scan even when the Entity filter matched a single row (~5.8s cold for a hot entity, × N query entities). The hybrid RELATION walk is worse — the anonymous-[r:RELATION]->()target makes AGE materialize all 1.58M vertices and nested-loop, spilling to the container's 64MB/dev/shm.Fix
POST /backfill-age/indexes— operator-triggered route that installs the four missing edge-endpoint indexes withCREATE INDEX CONCURRENTLY IF NOT EXISTS(no table lock against live reads, idempotent). Auth-gated, postgres-only (503 on chroma). Mirrors the existing/backfill-ageoperator pattern — no silent startup DDL on prod.idx_mentions_end_id,idx_mentions_start_ididx_relation_start_id,idx_relation_end_idThe Entity side is already covered by the existing
idx_entity_nameGIN index.Validation (synthetic 3.17M-edge AGE graph, apache/age PG16_1.6.0, mirroring prod's index gap)
Decisive plan flip — the seq scan disappears:
Cold-cache wall clock (container restarted between runs): 320ms → 206ms (1.6×) at half-prod scale. The seq scan grows linearly with the table; the index scan grows with the result set — so the delta widens at prod's 2× scale and under the cold/contended conditions that produced the observed p95 5.6s.
Route DDL verified valid + idempotent against a fresh AGE graph (all four created, re-run is a clean no-op).
Recall impact
None — these are pure read-path access-method indexes. The graph-walk returns the same drawer IDs; only the plan changes. (Observed: the graph source surfaced 0 useful candidates on most of the 12 probe queries anyway — the cost was pure waste.)
Negative results (recorded so they aren't re-tried)
Follow-up (mempalace, separate repo — not in this PR)
mempalace.backfill_age(_ensure_edge_endpoint_indexes()alongside_ensure_drawer_unique_index()). This route is the bridge until that lands.-[r:RELATION]->()target insearcher._graph_expand_from_*should bind/drop the target so AGE stops materializing all vertices.mempalace-db/dev/shmis the Docker default 64MB — raise--shm-size(infra, separate).Tests / lint
tests/test_backfill_age_indexes.py— 7 tests (create-all, idempotent-skip, partial, per-index error isolation, all-errors-500, chroma-503, DDL-completeness guard). All green via the daemon venv TestClient.test_kg_predicate_norm.pyfailures are pre-existing onmain(verified with this branch stashed) and untouched by this change.py_compile+ast.parseclean.Full profile:
docs/perf/2026-05-30-hybrid-graph-walk-latency.md. Offline SQL:scripts/age_graph_indexes.sql.🤖 Generated with Claude Code