From c3f649157090a89f810306e47bd23a00d01f3665 Mon Sep 17 00:00:00 2001 From: Jai Dhyani Date: Thu, 28 May 2026 23:20:57 -0700 Subject: [PATCH 1/2] perf(migration): compute search text once in FTS backfill The Postgres FTS backfill in 014_add_session_search_fts.sql called _extract_event_search_text(payload) twice per row -- once in the UPDATE SET clause and once in the WHERE filter -- doubling the per-row work over the whole conversation_events table. Compute it once in a subquery and reuse the result for both the tsvector value and the NULL filter. Results are identical: only rows with non-NULL extracted text get a search_vector. The SQLite 014 backfill already computes content once (subquery + outer WHERE content != ''), so this is a Postgres-only change. Added a regression test asserting the backfill UPDATE evaluates the helper exactly once. Card 6a0f94f1 (PR #614 review follow-up, GH #763). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../postgres/014_add_session_search_fts.sql | 16 ++++++++--- .../unit_tests/utils/test_search.py | 27 +++++++++++++++++++ 2 files changed, 39 insertions(+), 4 deletions(-) diff --git a/migrations/postgres/014_add_session_search_fts.sql b/migrations/postgres/014_add_session_search_fts.sql index 5c65bd94f..27783d11b 100644 --- a/migrations/postgres/014_add_session_search_fts.sql +++ b/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) diff --git a/tests/luthien_proxy/unit_tests/utils/test_search.py b/tests/luthien_proxy/unit_tests/utils/test_search.py index 08107b04c..3de024e9f 100644 --- a/tests/luthien_proxy/unit_tests/utils/test_search.py +++ b/tests/luthien_proxy/unit_tests/utils/test_search.py @@ -12,6 +12,33 @@ 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", From 21842b745fd971a30645c7b8a23f9aa396998010 Mon Sep 17 00:00:00 2001 From: Jai Dhyani Date: Thu, 28 May 2026 23:21:23 -0700 Subject: [PATCH 2/2] docs(changelog): add fragment for PR #614 review follow-ups Co-Authored-By: Claude Opus 4.8 (1M context) --- changelog.d/pr614-review-followups.md | 5 +++++ tests/luthien_proxy/unit_tests/utils/test_search.py | 1 + 2 files changed, 6 insertions(+) create mode 100644 changelog.d/pr614-review-followups.md diff --git a/changelog.d/pr614-review-followups.md b/changelog.d/pr614-review-followups.md new file mode 100644 index 000000000..219dbb057 --- /dev/null +++ b/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) diff --git a/tests/luthien_proxy/unit_tests/utils/test_search.py b/tests/luthien_proxy/unit_tests/utils/test_search.py index 3de024e9f..aae884f61 100644 --- a/tests/luthien_proxy/unit_tests/utils/test_search.py +++ b/tests/luthien_proxy/unit_tests/utils/test_search.py @@ -40,6 +40,7 @@ def test_postgres_backfill_calls_extract_helper_once_per_row() -> None: update_sql = _postgres_fts_backfill_update() assert update_sql.count("_extract_event_search_text(") == 1 + REQUIRED_MIGRATIONS = ( "003_add_conversation_tables.sql", "006_add_session_id.sql",