PR #614 review follow-ups

changelog.d/pr614-review-followups.md

@@ -0,0 +1,5 @@
+---
+category: Fixes
+---
+
+**FTS backfill computes search text once per row**: The Postgres `014_add_session_search_fts.sql` backfill called `_extract_event_search_text(payload)` twice per row (once in the `UPDATE SET` clause, once in the `WHERE` filter), doubling the per-row work over the whole `conversation_events` table. It now computes the value once in a subquery and reuses it for both the tsvector and the NULL filter; results are unchanged. (PR #614 review follow-up, GH #763)

migrations/postgres/014_add_session_search_fts.sql

@@ -49,10 +49,18 @@ $$ LANGUAGE plpgsql IMMUTABLE;
 ALTER TABLE conversation_events
     ADD COLUMN IF NOT EXISTS search_vector TSVECTOR;
 
-UPDATE conversation_events
-SET search_vector = to_tsvector('english', COALESCE(_extract_event_search_text(payload), ''))
-WHERE event_type = 'transaction.request_recorded'
-  AND _extract_event_search_text(payload) IS NOT NULL;
+-- Backfill existing rows. Compute _extract_event_search_text(payload) ONCE per
+-- row via a subquery, then both set the tsvector and filter on the result --
+-- calling the function twice (once in SET, once in WHERE) doubled the work.
+UPDATE conversation_events AS ce
+SET search_vector = to_tsvector('english', extracted.search_text)
+FROM (
+    SELECT id, _extract_event_search_text(payload) AS search_text
+    FROM conversation_events
+    WHERE event_type = 'transaction.request_recorded'
+) AS extracted
+WHERE ce.id = extracted.id
+  AND extracted.search_text IS NOT NULL;
 
 CREATE INDEX IF NOT EXISTS idx_conversation_events_search_vector
     ON conversation_events USING GIN (search_vector)

tests/luthien_proxy/unit_tests/utils/test_search.py

@@ -12,6 +12,34 @@
 from luthien_proxy.utils.search import session_fts_filter_sql
 
 MIGRATIONS_DIR = Path(__file__).resolve().parents[4] / "migrations" / "sqlite"
+POSTGRES_MIGRATIONS_DIR = Path(__file__).resolve().parents[4] / "migrations" / "postgres"
+
+
+def _postgres_fts_backfill_update() -> str:
+    """Return the backfill UPDATE statement from the Postgres FTS migration.
+
+    Naive `;`-splitting is unsafe here -- the migration contains `$$`-quoted
+    PL/pgSQL function bodies with embedded semicolons. Instead, slice from the
+    first top-level `UPDATE conversation_events` to its terminating `;`, which
+    sits outside any function body.
+    """
+    sql = (POSTGRES_MIGRATIONS_DIR / "014_add_session_search_fts.sql").read_text()
+    start = sql.index("UPDATE conversation_events")
+    end = sql.index(";", start)
+    return sql[start:end]
+
+
+def test_postgres_backfill_calls_extract_helper_once_per_row() -> None:
+    """The Postgres backfill must evaluate _extract_event_search_text once per row.
+
+    Regression guard for the PR #614 review finding: the original backfill called
+    `_extract_event_search_text(payload)` twice (once in SET, once in WHERE),
+    doubling the work. The corrected statement computes it once in a subquery and
+    reuses the result for both the value and the NULL filter.
+    """
+    update_sql = _postgres_fts_backfill_update()
+    assert update_sql.count("_extract_event_search_text(") == 1
+
 
 REQUIRED_MIGRATIONS = (
     "003_add_conversation_tables.sql",