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/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..aae884f61 100644 --- a/tests/luthien_proxy/unit_tests/utils/test_search.py +++ b/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",