diff --git a/AGENTS.md b/AGENTS.md index ee5525e8..65e66b57 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -146,6 +146,30 @@ Worker-crash recovery (`radis.core`): Labeling uses the shared core LLM client (`radis.core.utils.llm_client`); its timeout, rate-limit gate, and transient-retry knobs are the global `LLM_REQUEST_TIMEOUT_SECONDS`, `LLM_RATE_LIMIT_*`, and `LLM_TRANSIENT_RETRY_*` settings. +PostgreSQL parallelism (compose-only overrides, not in `example.env` — set in `.env` +only to override the compose file's default): + +- `POSTGRES_MAX_PARALLEL_WORKERS_PER_GATHER`: Parallel workers per query node scanning + the report index (default `4`, raised from PostgreSQL's own default of `2`) +- `POSTGRES_MAX_PARALLEL_WORKERS`, `POSTGRES_MAX_WORKER_PROCESSES`: Cluster-wide worker + pool sizes (each defaults to `8`, PostgreSQL's own default). Raise together with + `POSTGRES_MAX_PARALLEL_WORKERS_PER_GATHER` on a larger host +- `POSTGRES_SHARED_BUFFERS`: PostgreSQL's shared memory buffer (default `128MB`, + PostgreSQL's own default) +- `POSTGRES_MAINTENANCE_WORK_MEM`: memory for VACUUM and index builds (default + `64MB`, PostgreSQL's own default). Raise for the migration window on a large + embedded archive so the pgsearch 0005 HNSW rebuild assembles its graph in + memory (a 1.7M-vector corpus built in 7m36s with `16GB` and 7 workers), and + for the manual rebuilds in the admin guide's compaction section +- `POSTGRES_MAX_PARALLEL_MAINTENANCE_WORKERS`: parallel workers per index build + (default `2`, PostgreSQL's own default); also capped by + `POSTGRES_MAX_PARALLEL_WORKERS` +- `POSTGRES_SHM_SIZE_BYTES`: tmpfs size of the container's `/dev/shm` in bytes + (default `1073741824` = 1 GiB — a cap, not a reservation; Docker's own 64 MB + default is too small for parallel queries). Parallel maintenance + pre-allocates its whole `maintenance_work_mem` budget here, so keep it at + least as large as `POSTGRES_MAINTENANCE_WORK_MEM` + ## Code Standards - **Style Guide**: Google Python Style Guide diff --git a/docker-compose.base.yml b/docker-compose.base.yml index f080d7e3..fa0845df 100644 --- a/docker-compose.base.yml +++ b/docker-compose.base.yml @@ -40,8 +40,33 @@ services: postgres: image: pgvector/pgvector:pg17@sha256:cf134a767f474095eeba57e0117be8e568e011a63f33fbf252f14c9b760f8e6f hostname: postgres.local + command: + - postgres + - -c + - max_parallel_workers_per_gather=${POSTGRES_MAX_PARALLEL_WORKERS_PER_GATHER:-4} + - -c + - max_parallel_workers=${POSTGRES_MAX_PARALLEL_WORKERS:-8} + - -c + - max_worker_processes=${POSTGRES_MAX_WORKER_PROCESSES:-8} + - -c + - shared_buffers=${POSTGRES_SHARED_BUFFERS:-128MB} + - -c + - maintenance_work_mem=${POSTGRES_MAINTENANCE_WORK_MEM:-64MB} + - -c + - max_parallel_maintenance_workers=${POSTGRES_MAX_PARALLEL_MAINTENANCE_WORKERS:-2} volumes: - postgres_data:/var/lib/postgresql/data + # PostgreSQL's parallel workers exchange data through dynamic shared + # memory in /dev/shm, and Docker's default segment is only 64 MB. + # Parallel maintenance (index builds, e.g. the HNSW rebuild in pgsearch + # migration 0005) pre-allocates its whole maintenance_work_mem budget + # there, so size this at least as large as POSTGRES_MAINTENANCE_WORK_MEM. + # A tmpfs size is a cap, not a reservation. Bytes, because + # `docker stack deploy` rejects unit suffixes here (default: 1 GiB). + - type: tmpfs + target: /dev/shm + tmpfs: + size: ${POSTGRES_SHM_SIZE_BYTES:-1073741824} volumes: postgres_data: diff --git a/docker-compose.dev.yml b/docker-compose.dev.yml index 8c55bb38..7e76a155 100644 --- a/docker-compose.dev.yml +++ b/docker-compose.dev.yml @@ -26,6 +26,9 @@ services: profiles: - never + # In development `web` runs the migrations the workers below wait for, so + # their WAIT_INIT_TIMEOUT must outlast the longest migration too. Dev has no + # restart policy, so a timeout leaves the worker containers exited. web: <<: *default-app image: radis_dev-web:latest @@ -52,7 +55,7 @@ services: command: > bash -c " wait-for-it -s postgres.local:5432 -t ${WAIT_POSTGRES_TIMEOUT:-180} && - wait-for-it -s web.local:8000 -t 300 && + wait-for-it -s web.local:8000 -t ${WAIT_INIT_TIMEOUT:-3600} && ./manage.py sweep_stale_tasks && ./manage.py bg_worker -l debug -q default --autoreload " @@ -63,7 +66,7 @@ services: command: > bash -c " wait-for-it -s postgres.local:5432 -t ${WAIT_POSTGRES_TIMEOUT:-180} && - wait-for-it -s web.local:8000 -t 300 && + wait-for-it -s web.local:8000 -t ${WAIT_INIT_TIMEOUT:-3600} && ./manage.py sweep_stale_tasks && ./manage.py bg_worker -l debug -q llm --autoreload " diff --git a/docker-compose.prod.yml b/docker-compose.prod.yml index ed709d49..f8e516c5 100644 --- a/docker-compose.prod.yml +++ b/docker-compose.prod.yml @@ -18,6 +18,12 @@ services: # We can't use those manage commands inside the web container in production because # the web service may have multiple replicas. So we make sure to only run them once # and wait for it to be finished by the web service containers. + # + # init opens its ok_server port only after `migrate` finishes, and every + # service below waits for that port, so WAIT_INIT_TIMEOUT must stay above the + # longest migration a deploy runs -- a data migration over a large archive can + # take many minutes, and a timeout aborts the `&&` chain and exits the + # container. init: <<: *default-app hostname: init.local @@ -43,7 +49,7 @@ services: - ${WEB_HTTPS_PORT:-443}:443 command: > bash -c " - wait-for-it -s init.local:8000 -t 300 && + wait-for-it -s init.local:8000 -t ${WAIT_INIT_TIMEOUT:-3600} && echo 'Starting web server ...' && daphne -b 0.0.0.0 -p 80 \\ -e ssl:443:privateKey=/etc/web/ssl/key.pem:certKey=/etc/web/ssl/cert.pem \\ @@ -60,7 +66,7 @@ services: command: > bash -c " wait-for-it -s postgres.local:5432 -t ${WAIT_POSTGRES_TIMEOUT:-180} && - wait-for-it -s init.local:8000 -t 300 && + wait-for-it -s init.local:8000 -t ${WAIT_INIT_TIMEOUT:-3600} && ./manage.py sweep_stale_tasks && ./manage.py bg_worker -q default " @@ -72,7 +78,7 @@ services: command: > bash -c " wait-for-it -s postgres.local:5432 -t ${WAIT_POSTGRES_TIMEOUT:-180} && - wait-for-it -s init.local:8000 -t 300 && + wait-for-it -s init.local:8000 -t ${WAIT_INIT_TIMEOUT:-3600} && ./manage.py sweep_stale_tasks && ./manage.py bg_worker -q llm " diff --git a/docs/superpowers/plans/2026-08-30-fts-search-projection.md b/docs/superpowers/plans/2026-08-30-fts-search-projection.md new file mode 100644 index 00000000..e72bc8f7 --- /dev/null +++ b/docs/superpowers/plans/2026-08-30-fts-search-projection.md @@ -0,0 +1,1379 @@ +# FTS Search Projection Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make the full-text search candidate query single-table by completing `ReportSearchIndex` as a search projection, so it stops joining three tables and applying `SELECT DISTINCT`. + +**Architecture:** `ReportSearchIndex` already mirrors derived data (`search_vector`, `embedding`). This adds ten more mirrored columns — the fields search *filters* on — maintained by statement-level PostgreSQL triggers rather than application signals, because `group_ids` is access-control data and must stay correct for every writer including raw SQL. `_build_filter_query` then becomes a single-table predicate builder and `.distinct()` disappears. + +**Tech Stack:** Django 6.0, PostgreSQL 17, pytest + pytest-django, factory-boy. + +**Spec:** `docs/superpowers/specs/2026-08-29-fts-query-shape-performance-design.md` + +## Global Constraints + +- **Design target: 8 million reports.** Measured there: `8,388 ms → 882 ms`. +- **Access control is the risk surface.** `group_ids` decides who may read a patient's report. Any change touching it needs a test. +- **`filters.group=None` is fail-closed** and must match only reports in no group — `Q(group_ids=[])`, never a `contains` variant. +- **Triggers must be `AFTER`, never `BEFORE`.** PostgreSQL does not expose stored generated column values to BEFORE triggers, so `patient_age` would mirror as NULL. Verified. +- **Migration order is load-bearing:** columns → triggers → backfill → indexes. Triggers before backfill so concurrent writes during a ~9-minute backfill still land. +- Line length 100 (Ruff), Google Python style, type checking pyright basic. +- Run tests with `uv run cli test -- `. + +**Deviation from the spec, deliberate:** §4.4 describes one migration. This plan splits it into four (`0003`–`0006`), one per ordered step. Same operations, same order — but each is independently applicable and testable, and no task has to edit a migration a previous task already applied. + +## File Structure + +| File | Responsibility | +| --- | --- | +| `radis/pgsearch/models.py` | Add the ten projection fields + two indexes to `ReportSearchIndex` | +| `radis/pgsearch/utils/projection.py` | **New.** The canonical projection SQL and `sync_projection(report_ids)`; single source of truth for the write path | +| `radis/pgsearch/migrations/0003_search_projection_columns.py` | **New.** `AddField` ×10 | +| `radis/pgsearch/migrations/0004_search_projection_triggers.py` | **New.** Three trigger functions + five triggers | +| `radis/pgsearch/migrations/0005_search_projection_backfill.py` | **New.** Chunked backfill, `atomic = False` | +| `radis/pgsearch/migrations/0006_search_projection_indexes.py` | **New.** Two indexes + GIN `fastupdate=off` | +| `radis/pgsearch/management/commands/check_search_projection.py` | **New.** Drift detection | +| `radis/pgsearch/providers.py` | `_build_filter_query`, `match_q`, `rank_expr`, `summary_expr`, `_exclude_negations` go single-table | +| `radis/pgsearch/signals.py` | Populate the projection when a row is created | +| `radis/pgsearch/utils/indexing.py` | Same, for the bulk path | +| `radis/pgsearch/tests/test_search_projection.py` | **New.** Columns, triggers, creation paths, backfill | +| `radis/pgsearch/tests/test_filter_query_equivalence.py` | **New.** Reference-oracle equivalence + plan shape | +| `docker-compose.base.yml` | Postgres tuning GUCs | +| `docs/user-docs/admin-guide.md` | Operator guidance for the GUCs | + +--- + +### Task 1: Projection columns + +**Files:** +- Modify: `radis/pgsearch/models.py` +- Create: `radis/pgsearch/migrations/0003_search_projection_columns.py` +- Test: `radis/pgsearch/tests/test_search_projection.py` + +**Interfaces:** +- Consumes: nothing. +- Produces: `ReportSearchIndex.group_ids: list[int]`, `.modality_codes: list[str]`, `.language_code: str | None`, `.patient_sex: str | None`, `.patient_age: int | None`, `.patient_id: str | None`, `.study_datetime: datetime | None`, `.study_description: str | None`, `.report_created_at: datetime | None`, `.report_updated_at: datetime | None`. + +- [ ] **Step 1: Write the failing test** + +Create `radis/pgsearch/tests/test_search_projection.py`: + +```python +"""Tests for the ReportSearchIndex search projection. + +The projection mirrors the Report fields that search filters on, so the FTS +candidate query can stay single-table. group_ids is access-control data, so +its correctness has its own tests here. +""" + +import pytest +from adit_radis_shared.accounts.factories import GroupFactory + +from radis.pgsearch.models import ReportSearchIndex +from radis.reports.factories import LanguageFactory, ReportFactory +from radis.reports.models import Report + +pytestmark = pytest.mark.django_db + + +def test_new_index_row_defaults_to_empty_arrays(): + report = ReportFactory.create(language=LanguageFactory.create(code="en")) + index = ReportSearchIndex.objects.get(report=report) + + assert index.group_ids == [] + assert index.modality_codes == [] +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `uv run cli test -- radis/pgsearch/tests/test_search_projection.py -v` +Expected: FAIL with `AttributeError: 'ReportSearchIndex' object has no attribute 'group_ids'` + +- [ ] **Step 3: Add the fields to the model** + +In `radis/pgsearch/models.py`, add to `ReportSearchIndex` after the `embedding` field: + +```python + # Search projection: mirrors of the Report fields the scan filters on, so + # the FTS candidate query stays single-table. Maintained by the triggers in + # migration 0004 and populated on creation by signals.py / indexing.py. + # Nullable so every AddField stays metadata-only on a large table; + # check_search_projection guards against drift instead of a NOT NULL scan. + group_ids = ArrayField(models.IntegerField(), default=list) + modality_codes = ArrayField(models.CharField(max_length=16), default=list) + language_code = models.CharField(max_length=10, null=True) + patient_sex = models.CharField(max_length=1, null=True) + patient_age = models.IntegerField(null=True) + patient_id = models.CharField(max_length=64, null=True) + study_datetime = models.DateTimeField(null=True) + study_description = models.CharField(max_length=64, blank=True, null=True) + report_created_at = models.DateTimeField(null=True) + report_updated_at = models.DateTimeField(null=True) +``` + +Add the import at the top of the file: + +```python +from django.contrib.postgres.fields import ArrayField +``` + +- [ ] **Step 4: Generate the migration** + +Run: `docker exec radis_dev-web-1 ./manage.py makemigrations pgsearch --name search_projection_columns` + +Verify the generated `radis/pgsearch/migrations/0003_search_projection_columns.py` contains ten `AddField` operations and no `AlterField`. Every field must have either a constant default or `null=True` — that is what keeps each `AddField` metadata-only on an 8M-row table. + +- [ ] **Step 5: Run test to verify it passes** + +Run: `uv run cli test -- radis/pgsearch/tests/test_search_projection.py -v` +Expected: PASS + +- [ ] **Step 6: Commit** + +```bash +git add radis/pgsearch/models.py radis/pgsearch/migrations/0003_search_projection_columns.py radis/pgsearch/tests/test_search_projection.py +git commit -m "Add search projection columns to ReportSearchIndex" +``` + +--- + +### Task 2: Triggers that keep the projection true + +**Files:** +- Create: `radis/pgsearch/migrations/0004_search_projection_triggers.py` +- Modify: `radis/pgsearch/tests/test_search_projection.py` + +**Interfaces:** +- Consumes: the columns from Task 1. +- Produces: SQL functions `pgsearch_sync_group_ids()`, `pgsearch_sync_modality_codes()`, `pgsearch_sync_report_fields()`; triggers `pgsearch_group_ids_ins`/`_del`, `pgsearch_modality_codes_ins`/`_del`, `pgsearch_report_fields_upd`. + +- [ ] **Step 1: Write the failing tests** + +Append to `radis/pgsearch/tests/test_search_projection.py`: + +```python +from django.db import connection + + +def test_adding_a_group_updates_the_projection(): + report = ReportFactory.create(language=LanguageFactory.create(code="en")) + group = GroupFactory.create() + + report.groups.add(group) + + index = ReportSearchIndex.objects.get(report=report) + assert index.group_ids == [group.pk] + + +def test_removing_a_group_updates_the_projection(): + """The leak direction: a report removed from a group must stop being visible.""" + report = ReportFactory.create(language=LanguageFactory.create(code="en")) + group = GroupFactory.create() + report.groups.add(group) + + report.groups.remove(group) + + index = ReportSearchIndex.objects.get(report=report) + assert index.group_ids == [] + + +def test_raw_sql_membership_write_updates_the_projection(): + """The whole reason for triggers over m2m_changed: writers that bypass the ORM.""" + report = ReportFactory.create(language=LanguageFactory.create(code="en")) + group = GroupFactory.create() + + with connection.cursor() as cursor: + cursor.execute( + "INSERT INTO reports_report_groups (report_id, group_id) VALUES (%s, %s)", + [report.pk, group.pk], + ) + + index = ReportSearchIndex.objects.get(report=report) + assert index.group_ids == [group.pk] + + +def test_bulk_membership_insert_updates_every_affected_row(): + """Statement-level triggers fail classically by processing only one transition row. + + This is the shape reports/api/viewsets.py:229 uses for bulk upsert. + """ + language = LanguageFactory.create(code="en") + reports = [ReportFactory.create(language=language) for _ in range(5)] + group = GroupFactory.create() + + through = Report.groups.through + through.objects.bulk_create( + [through(report_id=report.pk, group_id=group.pk) for report in reports] + ) + + for report in reports: + index = ReportSearchIndex.objects.get(report=report) + assert index.group_ids == [group.pk], f"report {report.pk} was not updated" + + +def test_deleting_a_report_does_not_error(): + """Deleting a Report cascades to both the membership rows and the index row, + firing the membership trigger against a row that may already be gone. The + spec calls that harmless in either cascade order; this pins it.""" + report = ReportFactory.create(language=LanguageFactory.create(code="en")) + report.groups.add(GroupFactory.create()) + report_pk = report.pk + + report.delete() + + assert not ReportSearchIndex.objects.filter(report_id=report_pk).exists() + + +def test_updating_a_report_updates_the_mirrored_scalars(): + report = ReportFactory.create(language=LanguageFactory.create(code="en")) + + report.patient_id = "CHANGED-123" + report.save() + + index = ReportSearchIndex.objects.get(report=report) + assert index.patient_id == "CHANGED-123" + + +def test_patient_age_is_mirrored(): + """patient_age is a stored generated column. A BEFORE trigger would read NULL + here; this test is what pins the triggers to AFTER.""" + report = ReportFactory.create(language=LanguageFactory.create(code="en")) + + report.patient_id = "TOUCH" + report.save() + + index = ReportSearchIndex.objects.get(report=report) + report.refresh_from_db() + assert index.patient_age == report.patient_age + assert index.patient_age is not None +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `uv run cli test -- radis/pgsearch/tests/test_search_projection.py -v` +Expected: the five new tests FAIL — `group_ids` stays `[]` and `patient_id` stays `None`, because nothing maintains them yet. + +- [ ] **Step 3: Write the trigger migration** + +Create `radis/pgsearch/migrations/0004_search_projection_triggers.py`: + +```python +"""Statement-level triggers maintaining the ReportSearchIndex search projection. + +Triggers rather than Django signals because group_ids is access-control data: +a signal fires only for ORM writes, missing management commands, the admin, +and raw SQL. A trigger holds inside the same transaction as the write, for +every writer. + +Statement-level rather than row-level because the bulk-upsert endpoint writes +membership with bulk_create (reports/api/viewsets.py:215,229), so one statement +can carry a whole batch. + +AFTER, never BEFORE: PostgreSQL does not expose stored generated column values +to BEFORE triggers, so patient_age would mirror as NULL. +""" + +from django.db import migrations + +SYNC_GROUP_IDS = """ +CREATE OR REPLACE FUNCTION pgsearch_sync_group_ids() RETURNS trigger +LANGUAGE plpgsql AS $$ +BEGIN + UPDATE pgsearch_reportsearchindex rsi + SET group_ids = COALESCE( + (SELECT array_agg(g.group_id ORDER BY g.group_id) + FROM reports_report_groups g WHERE g.report_id = rsi.report_id), + '{}') + WHERE rsi.report_id IN (SELECT DISTINCT report_id FROM changed); + RETURN NULL; +END $$; +""" + +SYNC_MODALITY_CODES = """ +CREATE OR REPLACE FUNCTION pgsearch_sync_modality_codes() RETURNS trigger +LANGUAGE plpgsql AS $$ +BEGIN + UPDATE pgsearch_reportsearchindex rsi + SET modality_codes = COALESCE( + (SELECT array_agg(m.code ORDER BY m.code) + FROM reports_report_modalities rm + JOIN reports_modality m ON m.id = rm.modality_id + WHERE rm.report_id = rsi.report_id), + '{}') + WHERE rsi.report_id IN (SELECT DISTINCT report_id FROM changed); + RETURN NULL; +END $$; +""" + +SYNC_REPORT_FIELDS = """ +CREATE OR REPLACE FUNCTION pgsearch_sync_report_fields() RETURNS trigger +LANGUAGE plpgsql AS $$ +BEGIN + UPDATE pgsearch_reportsearchindex rsi + SET language_code = l.code, + patient_sex = c.patient_sex, + patient_age = c.patient_age, + patient_id = c.patient_id, + study_datetime = c.study_datetime, + study_description = c.study_description, + report_created_at = c.created_at, + report_updated_at = c.updated_at + FROM changed c + LEFT JOIN reports_language l ON l.id = c.language_id + WHERE rsi.report_id = c.id + AND (rsi.language_code, rsi.patient_sex, rsi.patient_age, rsi.patient_id, + rsi.study_datetime, rsi.study_description, rsi.report_created_at, + rsi.report_updated_at) + IS DISTINCT FROM + (l.code, c.patient_sex, c.patient_age, c.patient_id, + c.study_datetime, c.study_description, c.created_at, c.updated_at); + RETURN NULL; +END $$; +""" + +CREATE_TRIGGERS = """ +CREATE TRIGGER pgsearch_group_ids_ins AFTER INSERT ON reports_report_groups +REFERENCING NEW TABLE AS changed +FOR EACH STATEMENT EXECUTE FUNCTION pgsearch_sync_group_ids(); + +CREATE TRIGGER pgsearch_group_ids_del AFTER DELETE ON reports_report_groups +REFERENCING OLD TABLE AS changed +FOR EACH STATEMENT EXECUTE FUNCTION pgsearch_sync_group_ids(); + +CREATE TRIGGER pgsearch_modality_codes_ins AFTER INSERT ON reports_report_modalities +REFERENCING NEW TABLE AS changed +FOR EACH STATEMENT EXECUTE FUNCTION pgsearch_sync_modality_codes(); + +CREATE TRIGGER pgsearch_modality_codes_del AFTER DELETE ON reports_report_modalities +REFERENCING OLD TABLE AS changed +FOR EACH STATEMENT EXECUTE FUNCTION pgsearch_sync_modality_codes(); + +CREATE TRIGGER pgsearch_report_fields_upd AFTER UPDATE ON reports_report +REFERENCING NEW TABLE AS changed +FOR EACH STATEMENT EXECUTE FUNCTION pgsearch_sync_report_fields(); +""" + +DROP_TRIGGERS = """ +DROP TRIGGER IF EXISTS pgsearch_group_ids_ins ON reports_report_groups; +DROP TRIGGER IF EXISTS pgsearch_group_ids_del ON reports_report_groups; +DROP TRIGGER IF EXISTS pgsearch_modality_codes_ins ON reports_report_modalities; +DROP TRIGGER IF EXISTS pgsearch_modality_codes_del ON reports_report_modalities; +DROP TRIGGER IF EXISTS pgsearch_report_fields_upd ON reports_report; +DROP FUNCTION IF EXISTS pgsearch_sync_group_ids(); +DROP FUNCTION IF EXISTS pgsearch_sync_modality_codes(); +DROP FUNCTION IF EXISTS pgsearch_sync_report_fields(); +""" + + +class Migration(migrations.Migration): + dependencies = [ + ("pgsearch", "0003_search_projection_columns"), + ("reports", "0013_alter_report_options"), + ] + + operations = [ + migrations.RunSQL(SYNC_GROUP_IDS, reverse_sql=migrations.RunSQL.noop), + migrations.RunSQL(SYNC_MODALITY_CODES, reverse_sql=migrations.RunSQL.noop), + migrations.RunSQL(SYNC_REPORT_FIELDS, reverse_sql=migrations.RunSQL.noop), + migrations.RunSQL(CREATE_TRIGGERS, reverse_sql=DROP_TRIGGERS), + ] +``` + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `uv run cli test -- radis/pgsearch/tests/test_search_projection.py -v` +Expected: PASS. If `test_patient_age_is_mirrored` fails with `None`, the triggers were written as BEFORE — fix, do not work around. + +- [ ] **Step 5: Commit** + +```bash +git add radis/pgsearch/migrations/0004_search_projection_triggers.py radis/pgsearch/tests/test_search_projection.py +git commit -m "Maintain the search projection with statement-level triggers" +``` + +--- + +### Task 3: Populate the projection when index rows are created + +**Files:** +- Create: `radis/pgsearch/utils/projection.py` +- Modify: `radis/pgsearch/signals.py` +- Modify: `radis/pgsearch/utils/indexing.py` +- Modify: `radis/pgsearch/tests/test_search_projection.py` + +**Interfaces:** +- Consumes: the columns from Task 1. +- Produces: `radis.pgsearch.utils.projection.sync_projection(report_ids: Iterable[int]) -> None` and the module constant `PROJECTION_UPDATE_SQL: str`. + +The triggers maintain the projection on *change*, but a `ReportSearchIndex` row is created before its groups are attached (the API serializer calls `report.groups.set()` after `Report.objects.create()`). This task gives both creation paths one shared way to fill the row. + +- [ ] **Step 1: Write the failing test** + +Append to `radis/pgsearch/tests/test_search_projection.py`: + +```python +from radis.pgsearch.utils.indexing import bulk_upsert_report_search_indexes + + +def test_creation_populates_the_mirrored_scalars(): + report = ReportFactory.create(language=LanguageFactory.create(code="de")) + + index = ReportSearchIndex.objects.get(report=report) + assert index.language_code == "de" + assert index.patient_id == report.patient_id + + +def test_bulk_upsert_populates_the_projection(): + language = LanguageFactory.create(code="en") + report = ReportFactory.create(language=language, modalities=["CT"]) + group = GroupFactory.create() + report.groups.add(group) + ReportSearchIndex.objects.filter(report=report).delete() + + bulk_upsert_report_search_indexes([report.pk]) + + index = ReportSearchIndex.objects.get(report=report) + assert index.language_code == "en" + assert index.group_ids == [group.pk] + assert index.modality_codes == ["CT"] +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `uv run cli test -- radis/pgsearch/tests/test_search_projection.py -v` +Expected: both FAIL — `language_code` is `None`. + +- [ ] **Step 3: Write the shared projection helper** + +Create `radis/pgsearch/utils/projection.py`: + +```python +"""The canonical SQL that fills the ReportSearchIndex search projection. + +Triggers (migration 0004) maintain the projection when its sources change. +This module fills it when an index row is first created, which happens before +the report's groups and modalities are attached. The backfill migration +duplicates this statement in SQL, the same way bulk_upsert_report_search_indexes +already duplicates the tsvector logic -- keep them in sync. +""" + +from collections.abc import Iterable + +from django.db import connection + +PROJECTION_UPDATE_SQL = """ +UPDATE pgsearch_reportsearchindex rsi +SET group_ids = COALESCE(g.ids, '{}'), + modality_codes = COALESCE(m.codes, '{}'), + language_code = l.code, + patient_sex = r.patient_sex, + patient_age = r.patient_age, + patient_id = r.patient_id, + study_datetime = r.study_datetime, + study_description = r.study_description, + report_created_at = r.created_at, + report_updated_at = r.updated_at +FROM reports_report r +LEFT JOIN reports_language l ON l.id = r.language_id +LEFT JOIN LATERAL ( + SELECT array_agg(rg.group_id ORDER BY rg.group_id) AS ids + FROM reports_report_groups rg WHERE rg.report_id = r.id +) g ON true +LEFT JOIN LATERAL ( + SELECT array_agg(mo.code ORDER BY mo.code) AS codes + FROM reports_report_modalities rm + JOIN reports_modality mo ON mo.id = rm.modality_id + WHERE rm.report_id = r.id +) m ON true +WHERE rsi.report_id = r.id AND rsi.report_id = ANY(%s) +""" + + +def sync_projection(report_ids: Iterable[int]) -> None: + """Fill the projection columns for the given reports from their sources.""" + ids = [int(report_id) for report_id in report_ids] + if not ids: + return + with connection.cursor() as cursor: + cursor.execute(PROJECTION_UPDATE_SQL, [ids]) +``` + +- [ ] **Step 4: Call it from both creation paths** + +In `radis/pgsearch/signals.py`, replace the body of the receiver: + +```python +from django.db.models.signals import post_save +from django.dispatch import receiver + +from radis.reports.models import Report + +from .models import ReportSearchIndex +from .utils.projection import sync_projection + + +@receiver(post_save, sender=Report) +def create_or_update_report_search_index(sender, instance, created, **kwargs): + if created: + ReportSearchIndex.objects.create(report=instance) + # Groups and modalities are attached after the Report is created, so + # they stay empty here and the migration 0004 triggers fill them. + sync_projection([instance.pk]) + return + + # update_fields is required, not an optimisation: OneToOneField caches the + # index on the report instance, so instance.search_index is often the stale + # object from creation time. A bare save() writes all of its fields back, + # clobbering the projection columns the AFTER UPDATE trigger just set. + instance.search_index.save(update_fields=["search_vector"]) +``` + +In `radis/pgsearch/utils/indexing.py`, add the import and call `sync_projection` once per chunk, immediately after the existing `cursor.execute(...)` block that sets `search_vector`: + +```python +from .projection import sync_projection +``` + +```python + sync_projection(chunk) +``` + +- [ ] **Step 5: Run tests to verify they pass** + +Run: `uv run cli test -- radis/pgsearch/tests/test_search_projection.py -v` +Expected: PASS + +- [ ] **Step 6: Run the whole pgsearch suite for regressions** + +Run: `uv run cli test -- radis/pgsearch -v` +Expected: PASS + +- [ ] **Step 7: Commit** + +```bash +git add radis/pgsearch/utils/projection.py radis/pgsearch/signals.py radis/pgsearch/utils/indexing.py radis/pgsearch/tests/test_search_projection.py +git commit -m "Populate the search projection when index rows are created" +``` + +--- + +### Task 4: Backfill existing rows + +**Files:** +- Create: `radis/pgsearch/migrations/0005_search_projection_backfill.py` +- Modify: `radis/pgsearch/tests/test_search_projection.py` + +**Interfaces:** +- Consumes: `PROJECTION_UPDATE_SQL` shape from Task 3 (duplicated inline — migrations stay self-contained). +- Produces: nothing importable. + +Measured at 8M: **9 minutes 20 seconds**, in 50,000-row chunks. This blocks the deploy, which is accepted for the target deployment. + +- [ ] **Step 1: Write the failing test** + +Append to `radis/pgsearch/tests/test_search_projection.py`: + +```python +def test_backfill_fills_rows_that_predate_the_projection(): + """Simulates a row written before the projection existed.""" + language = LanguageFactory.create(code="en") + report = ReportFactory.create(language=language, modalities=["MR"]) + group = GroupFactory.create() + report.groups.add(group) + + ReportSearchIndex.objects.filter(report=report).update( + group_ids=[], modality_codes=[], language_code=None, patient_id=None + ) + + from radis.pgsearch.utils.projection import sync_projection + + sync_projection([report.pk]) + + index = ReportSearchIndex.objects.get(report=report) + assert index.group_ids == [group.pk] + assert index.modality_codes == ["MR"] + assert index.language_code == "en" + assert index.patient_id == report.patient_id +``` + +- [ ] **Step 2: Run test to verify it passes already** + +Run: `uv run cli test -- radis/pgsearch/tests/test_search_projection.py::test_backfill_fills_rows_that_predate_the_projection -v` +Expected: PASS — Task 3 already provides the mechanism. This test pins the *semantics* the migration depends on, so a later refactor of `PROJECTION_UPDATE_SQL` cannot silently break the backfill. + +- [ ] **Step 3: Write the backfill migration** + +Create `radis/pgsearch/migrations/0005_search_projection_backfill.py`: + +```python +"""Backfill the ReportSearchIndex search projection for existing rows. + +atomic = False so each chunk commits on its own: at the 8M design target this +runs for about nine and a half minutes, and one transaction that long would +pin an equally long-lived snapshot. + +Runs after the triggers (0004) on purpose. A report edited during the backfill +is corrected by its trigger, and a chunk that later reprocesses the same row +simply rewrites the current values. + +The statement below duplicates utils/projection.PROJECTION_UPDATE_SQL, the same +way bulk_upsert_report_search_indexes duplicates the tsvector logic. Migrations +must not import app code, which drifts under them. +""" + +from django.db import migrations + +CHUNK_SIZE = 50_000 + +BACKFILL_SQL = """ +UPDATE pgsearch_reportsearchindex rsi +SET group_ids = COALESCE(g.ids, '{}'), + modality_codes = COALESCE(m.codes, '{}'), + language_code = l.code, + patient_sex = r.patient_sex, + patient_age = r.patient_age, + patient_id = r.patient_id, + study_datetime = r.study_datetime, + study_description = r.study_description, + report_created_at = r.created_at, + report_updated_at = r.updated_at +FROM reports_report r +LEFT JOIN reports_language l ON l.id = r.language_id +LEFT JOIN LATERAL ( + SELECT array_agg(rg.group_id ORDER BY rg.group_id) AS ids + FROM reports_report_groups rg WHERE rg.report_id = r.id +) g ON true +LEFT JOIN LATERAL ( + SELECT array_agg(mo.code ORDER BY mo.code) AS codes + FROM reports_report_modalities rm + JOIN reports_modality mo ON mo.id = rm.modality_id + WHERE rm.report_id = r.id +) m ON true +WHERE rsi.report_id = r.id AND rsi.report_id > %s AND rsi.report_id <= %s +""" + + +def backfill(apps, schema_editor): + connection = schema_editor.connection + with connection.cursor() as cursor: + cursor.execute("SELECT COALESCE(MAX(report_id), 0) FROM pgsearch_reportsearchindex") + max_id = cursor.fetchone()[0] + + low = 0 + while low < max_id: + high = low + CHUNK_SIZE + with connection.cursor() as cursor: + cursor.execute(BACKFILL_SQL, [low, high]) + low = high + + +class Migration(migrations.Migration): + atomic = False + + dependencies = [("pgsearch", "0004_search_projection_triggers")] + + operations = [migrations.RunPython(backfill, migrations.RunPython.noop)] +``` + +- [ ] **Step 4: Verify the migration applies cleanly** + +Run: `uv run cli test -- radis/pgsearch -v` +Expected: PASS — the test database is built by applying every migration, so a broken migration fails the whole suite. + +- [ ] **Step 5: Commit** + +```bash +git add radis/pgsearch/migrations/0005_search_projection_backfill.py radis/pgsearch/tests/test_search_projection.py +git commit -m "Backfill the search projection in 50k-row chunks" +``` + +--- + +### Task 5: Indexes, created last + +**Files:** +- Modify: `radis/pgsearch/models.py` +- Create: `radis/pgsearch/migrations/0006_search_projection_indexes.py` + +**Interfaces:** +- Consumes: the columns from Task 1. +- Produces: indexes `pgsearch_group_ids_gin`, `pgsearch_report_updated_at_idx`. + +Created after the backfill deliberately: while the new columns are unindexed the backfill can use HOT updates. Measured at 8M — GIN 3.7 s, btree 1.6 s. Two indexes only; `modality_codes`, `study_datetime`, `patient_id` and `report_created_at` are deliberately unindexed (spec §4.1). + +- [ ] **Step 1: Add the indexes to the model** + +In `radis/pgsearch/models.py`, add to `ReportSearchIndex.Meta.indexes`: + +```python + GinIndex(fields=["group_ids"], name="pgsearch_group_ids_gin"), + models.Index(fields=["report_updated_at"], name="pgsearch_report_updated_at_idx"), +``` + +- [ ] **Step 2: Generate the migration** + +Run: `docker exec radis_dev-web-1 ./manage.py makemigrations pgsearch --name search_projection_indexes` + +- [ ] **Step 3: Add the GIN fastupdate change** + +Append to the `operations` list in the generated `0006_search_projection_indexes.py`: + +```python + migrations.RunSQL( + # fastupdate leaves an unsorted pending list that every query must + # scan; RADIS writes in async batches and reads interactively, so + # predictable read latency is the better side of the trade. + "ALTER INDEX pgsearch_re_search__b0f715_gin SET (fastupdate = off);" + "SELECT gin_clean_pending_list('pgsearch_re_search__b0f715_gin');", + reverse_sql="ALTER INDEX pgsearch_re_search__b0f715_gin SET (fastupdate = on);", + ), +``` + +- [ ] **Step 4: Write the test** + +Append to `radis/pgsearch/tests/test_search_projection.py`: + +```python +def test_projection_indexes_exist(): + with connection.cursor() as cursor: + cursor.execute( + "SELECT indexname FROM pg_indexes WHERE tablename = 'pgsearch_reportsearchindex'" + ) + names = {row[0] for row in cursor.fetchall()} + + assert "pgsearch_group_ids_gin" in names + assert "pgsearch_report_updated_at_idx" in names +``` + +- [ ] **Step 5: Run tests to verify they pass** + +Run: `uv run cli test -- radis/pgsearch/tests/test_search_projection.py -v` +Expected: PASS + +- [ ] **Step 6: Commit** + +```bash +git add radis/pgsearch/models.py radis/pgsearch/migrations/0006_search_projection_indexes.py radis/pgsearch/tests/test_search_projection.py +git commit -m "Add the search projection indexes and disable GIN fastupdate" +``` + +--- + +### Task 6: check_search_projection command + +**Files:** +- Create: `radis/pgsearch/management/commands/check_search_projection.py` +- Create: `radis/pgsearch/tests/test_check_search_projection.py` + +**Interfaces:** +- Consumes: the columns from Task 1. +- Produces: management command `check_search_projection`, exiting non-zero when drift is found. + +- [ ] **Step 1: Write the failing test** + +Create `radis/pgsearch/tests/test_check_search_projection.py`: + +```python +"""The projection duplicates access-control data, so operators need a way to +prove it still matches its sources -- after a restore, a bulk import, or a +Language.code rename, which no trigger covers.""" + +import pytest +from adit_radis_shared.accounts.factories import GroupFactory +from django.core.management import call_command +from django.core.management.base import CommandError + +from radis.pgsearch.models import ReportSearchIndex +from radis.reports.factories import LanguageFactory, ReportFactory + +pytestmark = pytest.mark.django_db + + +def test_reports_no_drift_on_a_healthy_corpus(): + report = ReportFactory.create(language=LanguageFactory.create(code="en")) + report.groups.add(GroupFactory.create()) + + call_command("check_search_projection") + + +def test_detects_drifted_group_ids(): + report = ReportFactory.create(language=LanguageFactory.create(code="en")) + report.groups.add(GroupFactory.create()) + + ReportSearchIndex.objects.filter(report=report).update(group_ids=[9999]) + + with pytest.raises(CommandError, match="group_ids"): + call_command("check_search_projection") +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `uv run cli test -- radis/pgsearch/tests/test_check_search_projection.py -v` +Expected: FAIL with `Unknown command: 'check_search_projection'` + +- [ ] **Step 3: Write the command** + +Create `radis/pgsearch/management/commands/check_search_projection.py`: + +```python +"""Verify the ReportSearchIndex search projection against its sources.""" + +from django.core.management.base import BaseCommand, CommandError +from django.db import connection + +DRIFT_SQL = """ +SELECT + count(*) FILTER (WHERE rsi.group_ids IS DISTINCT FROM COALESCE(g.ids, '{}')) + AS group_ids, + count(*) FILTER (WHERE rsi.modality_codes IS DISTINCT FROM COALESCE(m.codes, '{}')) + AS modality_codes, + count(*) FILTER (WHERE rsi.language_code IS DISTINCT FROM l.code) + AS language_code, + count(*) FILTER (WHERE rsi.patient_sex IS DISTINCT FROM r.patient_sex) + AS patient_sex, + count(*) FILTER (WHERE rsi.patient_age IS DISTINCT FROM r.patient_age) + AS patient_age, + count(*) FILTER (WHERE rsi.patient_id IS DISTINCT FROM r.patient_id) + AS patient_id, + count(*) FILTER (WHERE rsi.study_datetime IS DISTINCT FROM r.study_datetime) + AS study_datetime, + count(*) FILTER (WHERE rsi.study_description IS DISTINCT FROM r.study_description) + AS study_description, + count(*) FILTER (WHERE rsi.report_created_at IS DISTINCT FROM r.created_at) + AS report_created_at, + count(*) FILTER (WHERE rsi.report_updated_at IS DISTINCT FROM r.updated_at) + AS report_updated_at +FROM pgsearch_reportsearchindex rsi +JOIN reports_report r ON r.id = rsi.report_id +LEFT JOIN reports_language l ON l.id = r.language_id +LEFT JOIN LATERAL ( + SELECT array_agg(rg.group_id ORDER BY rg.group_id) AS ids + FROM reports_report_groups rg WHERE rg.report_id = r.id +) g ON true +LEFT JOIN LATERAL ( + SELECT array_agg(mo.code ORDER BY mo.code) AS codes + FROM reports_report_modalities rm + JOIN reports_modality mo ON mo.id = rm.modality_id + WHERE rm.report_id = r.id +) m ON true +""" + + +class Command(BaseCommand): + help = "Verify the ReportSearchIndex search projection against its sources." + + def handle(self, *args, **options): + with connection.cursor() as cursor: + cursor.execute(DRIFT_SQL) + columns = [column.name for column in cursor.description] + counts = dict(zip(columns, cursor.fetchone(), strict=True)) + + drifted = {name: count for name, count in counts.items() if count} + if drifted: + details = ", ".join(f"{name}: {count}" for name, count in sorted(drifted.items())) + raise CommandError(f"Search projection drift detected -- {details}") + + self.stdout.write(self.style.SUCCESS("Search projection matches its sources.")) +``` + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `uv run cli test -- radis/pgsearch/tests/test_check_search_projection.py -v` +Expected: PASS + +- [ ] **Step 5: Commit** + +```bash +git add radis/pgsearch/management/commands/check_search_projection.py radis/pgsearch/tests/test_check_search_projection.py +git commit -m "Add check_search_projection drift detection command" +``` + +--- + +### Task 7: Single-table _build_filter_query + +**Files:** +- Modify: `radis/pgsearch/providers.py:180-244` +- Create: `radis/pgsearch/tests/test_filter_query_equivalence.py` + +**Interfaces:** +- Consumes: the projection columns from Task 1, populated by Tasks 2–4. +- Produces: `_build_filter_query(filters: SearchFilters) -> Q` returning predicates against `ReportSearchIndex`'s own columns only, with no `report__` traversal. + +This is the task that delivers the performance change. It is also the one that can leak reports between groups, so the equivalence test comes first. + +- [ ] **Step 1: Write the failing equivalence test** + +Create `radis/pgsearch/tests/test_filter_query_equivalence.py`: + +```python +"""The new single-table filter must select exactly what the joined one did. + +_build_filter_query_legacy is the pre-change implementation kept as a reference +oracle. Delete both it and this module once the change has settled in production. +""" + +from datetime import date, timedelta + +import pytest +from adit_radis_shared.accounts.factories import GroupFactory +from django.db.models import Q +from django.utils import timezone + +from radis.pgsearch.models import ReportSearchIndex +from radis.pgsearch.providers import _build_filter_query +from radis.reports.factories import LanguageFactory, ReportFactory +from radis.search.site import SearchFilters + +pytestmark = pytest.mark.django_db + + +def _build_filter_query_legacy(filters: SearchFilters) -> Q: + """The joined implementation this change replaces.""" + fq = Q(report__groups=filters.group) + if filters.patient_sex: + fq &= Q(report__patient_sex=filters.patient_sex) + if filters.language: + fq &= Q(report__language__code=filters.language) + if filters.modalities: + fq &= Q(report__modalities__code__in=filters.modalities) + if filters.study_date_from: + fq &= Q(report__study_datetime__date__gte=filters.study_date_from) + if filters.study_date_till: + fq &= Q(report__study_datetime__date__lte=filters.study_date_till) + if filters.study_description: + fq &= Q(report__study_description__icontains=filters.study_description) + if filters.patient_age_from is not None: + fq &= Q(report__patient_age__gte=filters.patient_age_from) + if filters.patient_age_till is not None: + fq &= Q(report__patient_age__lte=filters.patient_age_till) + if filters.patient_id: + fq &= Q(report__patient_id=filters.patient_id) + if filters.updated_after: + fq &= Q(report__updated_at__gte=filters.updated_after) + return fq + + +def _ids(fq: Q) -> set[int]: + return set( + ReportSearchIndex.objects.filter(fq).distinct().values_list("report_id", flat=True) + ) + + +@pytest.fixture +def corpus(): + language_en = LanguageFactory.create(code="en") + language_de = LanguageFactory.create(code="de") + group_a = GroupFactory.create(name="A") + group_b = GroupFactory.create(name="B") + now = timezone.now() + + first = ReportFactory.create( + language=language_en, patient_sex="M", patient_id="P1", + study_description="CT Thorax", study_datetime=now - timedelta(days=10), + modalities=["CT"], + ) + first.groups.add(group_a) + + second = ReportFactory.create( + language=language_de, patient_sex="F", patient_id="P2", + study_description="MR Head", study_datetime=now - timedelta(days=400), + modalities=["MR", "CT"], + ) + second.groups.add(group_a, group_b) + + orphan = ReportFactory.create(language=language_en, patient_sex="M", patient_id="P3") + + return {"group_a": group_a, "group_b": group_b, "orphan": orphan} + + +@pytest.mark.parametrize( + "make_filters", + [ + pytest.param(lambda c: SearchFilters(group=c["group_a"].pk), id="group-only"), + pytest.param(lambda c: SearchFilters(group=c["group_b"].pk), id="other-group"), + pytest.param( + lambda c: SearchFilters(group=c["group_a"].pk, language="en"), id="language" + ), + pytest.param( + lambda c: SearchFilters(group=c["group_a"].pk, modalities=["CT"]), id="modality" + ), + pytest.param( + lambda c: SearchFilters(group=c["group_a"].pk, modalities=["CT", "MR"]), + id="modalities-multi", + ), + pytest.param( + lambda c: SearchFilters(group=c["group_a"].pk, patient_sex="M"), id="sex" + ), + pytest.param( + lambda c: SearchFilters(group=c["group_a"].pk, patient_id="P1"), id="patient-id" + ), + pytest.param( + lambda c: SearchFilters(group=c["group_a"].pk, study_description="thorax"), + id="description-icontains", + ), + pytest.param( + lambda c: SearchFilters( + group=c["group_a"].pk, study_date_from=date.today() - timedelta(days=30) + ), + id="date-from", + ), + pytest.param( + lambda c: SearchFilters( + group=c["group_a"].pk, study_date_till=date.today() - timedelta(days=5) + ), + id="date-till", + ), + pytest.param( + lambda c: SearchFilters( + group=c["group_a"].pk, + study_date_from=date.today() - timedelta(days=30), + study_date_till=date.today(), + ), + id="date-range", + ), + pytest.param( + lambda c: SearchFilters(group=c["group_a"].pk, patient_age_from=0), id="age-from" + ), + pytest.param( + lambda c: SearchFilters(group=c["group_a"].pk, patient_age_till=200), id="age-till" + ), + pytest.param( + lambda c: SearchFilters( + group=c["group_a"].pk, updated_after=timezone.now() - timedelta(hours=1) + ), + id="updated-after", + ), + ], +) +def test_new_filter_matches_the_legacy_one(corpus, make_filters): + filters = make_filters(corpus) + + assert _ids(_build_filter_query(filters)) == _ids(_build_filter_query_legacy(filters)) + + +def test_group_none_is_fail_closed(corpus): + """group=None must match only reports in no group at all -- never everything.""" + result = _ids(_build_filter_query(SearchFilters(group=None))) + + assert result == {corpus["orphan"].pk} + + +def test_no_duplicates_without_distinct(corpus): + """A report matching on two modalities must appear once, which is what the + removed .distinct() used to guarantee.""" + filters = SearchFilters(group=corpus["group_a"].pk, modalities=["CT", "MR"]) + + rows = list( + ReportSearchIndex.objects.filter(_build_filter_query(filters)).values_list( + "report_id", flat=True + ) + ) + + assert len(rows) == len(set(rows)) +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `uv run cli test -- radis/pgsearch/tests/test_filter_query_equivalence.py -v` +Expected: `test_no_duplicates_without_distinct` FAILS (the current implementation duplicates), and `test_group_none_is_fail_closed` may pass incidentally. The parametrised cases pass trivially because both sides are still the same code. + +- [ ] **Step 3: Rewrite _build_filter_query** + +In `radis/pgsearch/providers.py`, replace the whole `_build_filter_query` function with: + +```python +def _local_day_start(day: date) -> datetime: + """Midnight of ``day`` in the active timezone, as an aware datetime.""" + return timezone.make_aware(datetime.combine(day, time.min)) + + +def _build_filter_query(filters: SearchFilters) -> Q: + # Every predicate here is a column on ReportSearchIndex itself -- the search + # projection (see migration 0003) mirrors the Report fields search filters + # on, so the candidate query stays single-table. Reintroducing a ``report__`` + # traversal restores the join and the multi-second query shape it caused; + # test_filter_query_plan_is_single_table guards against that. + # + # Group-scoped access control. ``SearchView`` supplies + # ``group=active_group.pk``; the extraction preview may pass ``group=None`` + # when the user has no active group, which is fail-closed: an empty + # ``group_ids`` matches only reports assigned to no group at all. + if filters.group is None: + fq = Q(group_ids=[]) + else: + fq = Q(group_ids__contains=[filters.group]) + + # Apply hard filter criteria + if filters.patient_sex: + fq &= Q(patient_sex=filters.patient_sex) + if filters.language: + fq &= Q(language_code=filters.language) + if filters.modalities: + fq &= Q(modality_codes__overlap=filters.modalities) + # Half-open ranges rather than ``__date``: Django compiles that lookup to + # ``(study_datetime AT TIME ZONE ...)::date``, a function over the column + # evaluated once per row -- millions of timezone conversions per query on + # the sequential scan, for a predicate equal to two timestamp comparisons. + if filters.study_date_from: + fq &= Q(study_datetime__gte=_local_day_start(filters.study_date_from)) + if filters.study_date_till: + fq &= Q(study_datetime__lt=_local_day_start(filters.study_date_till + timedelta(days=1))) + if filters.study_description: + fq &= Q(study_description__icontains=filters.study_description) + if filters.patient_age_from is not None: + fq &= Q(patient_age__gte=filters.patient_age_from) + if filters.patient_age_till is not None: + fq &= Q(patient_age__lte=filters.patient_age_till) + if filters.patient_id: + fq &= Q(patient_id=filters.patient_id) + if filters.created_after: + fq &= Q(report_created_at__gte=filters.created_after) + if filters.created_before: + fq &= Q(report_created_at__lte=filters.created_before) + if filters.labels: + from radis.labels.models import LabelResult + from radis.reports.models import Report + + surfacing_report_ids = Report.objects.filter( + label_results__label__name__in=filters.labels, + label_results__value__in=LabelResult.SURFACING_VALUES, + ).values("pk") + fq &= Q(report_id__in=surfacing_report_ids) + if filters.updated_after: + fq &= Q(report_updated_at__gte=filters.updated_after) + + return fq +``` + +Add these imports at the top of `providers.py`: + +```python +from datetime import date, datetime, time, timedelta + +from django.utils import timezone +``` + +- [ ] **Step 4: Remove the two `.distinct()` calls** + +In `_fuse_hybrid`, delete `.distinct()` from both the vector queryset (`providers.py:394`) and the FTS queryset (`providers.py:412`). With no joins there is nothing to deduplicate. + +- [ ] **Step 5: Run tests to verify they pass** + +Run: `uv run cli test -- radis/pgsearch/tests/test_filter_query_equivalence.py -v` +Expected: PASS, all parametrised cases plus both named tests. + +- [ ] **Step 6: Run the full suite for regressions** + +Run: `uv run cli test -- radis/pgsearch radis/search radis/extractions radis/subscriptions -v` +Expected: PASS. These are the four consumers of `_build_filter_query`. + +- [ ] **Step 7: Commit** + +```bash +git add radis/pgsearch/providers.py radis/pgsearch/tests/test_filter_query_equivalence.py +git commit -m "Make the FTS filter query single-table" +``` + +--- + +### Task 8: Language predicates off the join + +**Files:** +- Modify: `radis/pgsearch/providers.py` — `_exclude_negations`, `_fuse_hybrid`, `search` + +**Interfaces:** +- Consumes: `ReportSearchIndex.language_code` from Task 1. +- Produces: no signature change. + +Five `report__language__code__in` traversals remain after Task 7 and would each re-add the `reports_language` join. + +- [ ] **Step 1: Write the failing test** + +Append to `radis/pgsearch/tests/test_filter_query_equivalence.py`: + +```python +def test_no_report_traversal_remains_in_the_candidate_query(): + """Any report__ lookup re-adds a join and the multi-second query shape.""" + import inspect + + from radis.pgsearch import providers + + source = inspect.getsource(providers) + offending = [ + line.strip() + for line in source.splitlines() + if "report__" in line + and "report__body" not in line # hydration headline, deliberately joined + and "report__document_id" not in line # filter(), deliberately joined + and not line.strip().startswith("#") + ] + + assert offending == [], f"report__ traversals left in the hot path: {offending}" +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `uv run cli test -- radis/pgsearch/tests/test_filter_query_equivalence.py::test_no_report_traversal_remains_in_the_candidate_query -v` +Expected: FAIL listing the five `report__language__code__in` lines. + +- [ ] **Step 3: Replace the traversals** + +In `radis/pgsearch/providers.py`, replace every `report__language__code__in=codes` with `language_code__in=codes`. There are five occurrences: + +- `_exclude_negations`, in the `queryset.exclude(...)` call +- `_fuse_hybrid`, building `match_q` +- `_fuse_hybrid`, building `rank_expr` +- `search`, building `summary_expr` +- `search`, building its own `rank_expr` + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `uv run cli test -- radis/pgsearch -v` +Expected: PASS + +- [ ] **Step 5: Commit** + +```bash +git add radis/pgsearch/providers.py radis/pgsearch/tests/test_filter_query_equivalence.py +git commit -m "Read language_code from the projection instead of joining" +``` + +--- + +### Task 9: Plan-shape regression test + +**Files:** +- Modify: `radis/pgsearch/tests/test_filter_query_equivalence.py` + +**Interfaces:** +- Consumes: everything from Tasks 1–8. +- Produces: nothing importable. + +Wall-clock assertions are flaky in CI; plan structure is not. This is what catches a future change silently restoring the 8-second query. + +- [ ] **Step 1: Write the failing test** + +Append to `radis/pgsearch/tests/test_filter_query_equivalence.py`: + +```python +def test_filter_query_plan_is_single_table(corpus): + """The candidate query must not join the membership table or deduplicate.""" + from django.db import connection + + filters = SearchFilters(group=corpus["group_a"].pk, language="en") + queryset = ReportSearchIndex.objects.filter(_build_filter_query(filters)).values_list( + "report_id", flat=True + ) + sql, params = queryset.query.sql_with_params() + + with connection.cursor() as cursor: + cursor.execute(f"EXPLAIN {sql}", params) + plan = "\n".join(row[0] for row in cursor.fetchall()) + + assert "reports_report_groups" not in plan, plan + # Not `"Unique" not in plan`: Postgres implements SELECT DISTINCT as + # HashAggregate rather than Sort+Unique once the row count is non-trivial, + # so an operator-name check passes with the regression present at scale. + # Django emits the DISTINCT keyword unconditionally, so check the SQL. + assert "DISTINCT" not in sql, sql +``` + +- [ ] **Step 2: Run test to verify it passes** + +Run: `uv run cli test -- radis/pgsearch/tests/test_filter_query_equivalence.py::test_filter_query_plan_is_single_table -v` +Expected: PASS — Tasks 7 and 8 already removed the join. If it fails, one of them is incomplete. + +- [ ] **Step 3: Commit** + +```bash +git add radis/pgsearch/tests/test_filter_query_equivalence.py +git commit -m "Assert the candidate query plan stays single-table" +``` + +--- + +### Task 10: PostgreSQL tuning and operator documentation + +**Files:** +- Modify: `docker-compose.base.yml` +- Modify: `docs/user-docs/admin-guide.md` +- Modify: `CLAUDE.md` + +**Interfaces:** +- Consumes: nothing. +- Produces: env vars `POSTGRES_MAX_PARALLEL_WORKERS_PER_GATHER`, `POSTGRES_MAX_PARALLEL_WORKERS`, `POSTGRES_MAX_WORKER_PROCESSES`, `POSTGRES_SHARED_BUFFERS`. + +These are compose-only overrides with defaults, **not** `example.env` entries — matching how the project already treats `EMBEDDINGS_WORKER_CONCURRENCY` and `WAIT_POSTGRES_TIMEOUT`. Only the first differs from PostgreSQL's own default. + +- [ ] **Step 1: Add the GUCs to the compose file** + +In `docker-compose.base.yml`, replace the `postgres` service with: + +```yaml + postgres: + image: pgvector/pgvector:pg17@sha256:cf134a767f474095eeba57e0117be8e568e011a63f33fbf252f14c9b760f8e6f + hostname: postgres.local + command: + - postgres + - -c + - max_parallel_workers_per_gather=${POSTGRES_MAX_PARALLEL_WORKERS_PER_GATHER:-4} + - -c + - max_parallel_workers=${POSTGRES_MAX_PARALLEL_WORKERS:-8} + - -c + - max_worker_processes=${POSTGRES_MAX_WORKER_PROCESSES:-8} + - -c + - shared_buffers=${POSTGRES_SHARED_BUFFERS:-128MB} + volumes: + - postgres_data:/var/lib/postgresql/data +``` + +- [ ] **Step 2: Verify the stack starts and the setting took effect** + +```bash +uv run cli compose-up -- -d +docker exec radis_dev-postgres-1 psql -U postgres -c "SHOW max_parallel_workers_per_gather;" +``` + +Expected: `4` + +- [ ] **Step 3: Document them for operators** + +Add to `docs/user-docs/admin-guide.md`: + +```markdown +## Database tuning + +Search scans the report index table in parallel, so the number of parallel +workers is the one PostgreSQL setting worth revisiting. These are optional +overrides -- set them in `.env` only if the defaults do not suit your hardware. + +| Variable | Default | Guidance | +| --- | --- | --- | +| `POSTGRES_MAX_PARALLEL_WORKERS_PER_GATHER` | `4` | Measured on 8M reports: 606 ms at 2, 421 ms at 4, 343 ms at 8. Four captures most of the gain while leaving cores for concurrent searches. | +| `POSTGRES_MAX_PARALLEL_WORKERS` | `8` | PostgreSQL's default. Raise together with the two others on a larger host. | +| `POSTGRES_MAX_WORKER_PROCESSES` | `8` | As above. | +| `POSTGRES_SHARED_BUFFERS` | `128MB` | PostgreSQL's default. Around 25% of host RAM is the usual recommendation; a value larger than the container's memory will prevent PostgreSQL from starting. | +``` + +Add the same four variables to the environment-variable section of `CLAUDE.md`, noting they are compose-only overrides. + +- [ ] **Step 4: Commit** + +```bash +git add docker-compose.base.yml docs/user-docs/admin-guide.md CLAUDE.md +git commit -m "Tune PostgreSQL parallelism for the search scan" +``` + +--- + +## Verification after all tasks + +- [ ] `uv run cli test` — full suite green +- [ ] `uv run cli lint` — clean +- [ ] `docker exec radis_dev-web-1 ./manage.py check_search_projection` — reports no drift +- [ ] Search the dev stack for a common term and confirm results still render +- [ ] **Manual, against a large corpus:** confirm the spec's success criterion — + the FTS candidate query under 1,000 ms at the 8M design target with + `max_parallel_workers_per_gather = 4`. Not assertable in CI, which has no + corpus of that size; the plan-shape test in Task 9 is the CI proxy. diff --git a/docs/superpowers/specs/2026-08-29-fts-query-shape-performance-design.md b/docs/superpowers/specs/2026-08-29-fts-query-shape-performance-design.md new file mode 100644 index 00000000..dd384d5d --- /dev/null +++ b/docs/superpowers/specs/2026-08-29-fts-query-shape-performance-design.md @@ -0,0 +1,589 @@ +# Search performance: making the FTS scan single-table + +**Status:** design, not yet implemented +**Branch:** `fts-query-shape-performance` +**Supersedes:** parts of `hybrid-search.md` §7 (the FTS candidate query and its filter construction) +**Design target:** 8 million reports. The largest known deployment is ~15M; past +roughly 8M the working set stops fitting comfortably in page cache on ordinary +hardware and the conclusions in §2 need re-measuring (see §8). + +## 1. Problem + +Hybrid search takes seconds for any query term that matches a large share of the +corpus. Measured on a synthetic 5,001,000-report corpus (§8): + +| query | reports matched | page load | +| --- | --- | --- | +| `findings` | 5,001,000 | 6.5 s | +| `effusion` | 2,360,472 | 4.1 s | +| `pneumonia` | 520,104 | 3.8 s | +| `adenoma` | 5,001 | 0.17 s | + +A later run measured `pneumonia` at 1.9 s after an `ANALYZE` improved the +planner's estimates; the table above is one coherent run, and the conclusion is +unchanged either way. + +Cost tracks the number of *matching* reports, not corpus size. Paging is flat — +page 1 and page 400 both cost ~6.5 s — because every page re-ranks the whole +match set from scratch. + +## 2. Diagnosis + +`ORDER BY ts_rank(...)` has no top-k shortcut. A GIN posting list is ordered by +document id and carries no impact information, and `ts_rank` needs the whole +tsvector from the heap, so every match must be scored before `LIMIT` applies. +That part is inherent to PostgreSQL FTS and is not what this design changes. + +What *is* fixable is the constant factor, and it is dominated by the joins. +`_build_filter_query` builds every predicate as a `report__` traversal, so the +FTS candidate query joins `reports_report`, `reports_report_groups` and +`reports_language`, then applies `SELECT DISTINCT`. Two consequences: + +- The planner builds a hash table over the matched rows **carrying + `search_vector`**, because `ts_rank` above the join still needs it. On the 1M + rig (§8) with 730k matches that hash table was 1.2 GB, spilling into 256 + batches and writing ~1.2 GB of temp files at the default `work_mem`. +- `SELECT DISTINCT` blocks the top-N heapsort, forcing a full external merge sort + of every matching row before `LIMIT` can apply. + +Removing both makes the query a single-table parallel sequential scan with a +26 kB top-N heapsort. Measured at the 8M design target — same table, same four +workers on both sides — `findings` goes from **8,388 ms to 882 ms, about 9.5x**. + +Parallelism is a separate and additive lever worth a further ~1.4x (§4.5). An +earlier draft of this document claimed 33x by comparing the old shape at two +workers against the new shape at eight, crediting the query-shape change with a +gain that was partly parallelism and partly a smaller corpus. The controlled +comparison above is the one to trust. + +## 3. Ruled out, with measurements + +Recorded because each is the obvious question a reviewer will raise. + +| candidate | result | +| --- | --- | +| Lower `HYBRID_FTS_MAX_RESULTS` | No effect (1M rig). `LIMIT 100` and `LIMIT 10000` both 1.05 s. Under `ts_rank` the pool size is free; it only becomes load-bearing with a pruning index. | +| Raise `work_mem` | No effect on the 1M rig: 1.41 s at 256 MB vs 1.44 s at 4 MB. The spill is a symptom, not the cause. | +| Rewrite the joins as `EXISTS` | Worse: 1.55 s vs 1.05 s (1M rig). The planner still hashes. | +| Materialised id-only CTE, joins on narrow rows, then rank | Worse: 11,460 ms for `findings`, 3,089 ms for `pneumonia` at 5M. The joins are still over 5,001,000 rows. **This was the only way to avoid duplicating access-control data without changing the domain model (see §6), and it does not work.** | +| `strip()`ped second tsvector for ranking | Rejected (1M rig). 1.4x faster but collapses `ts_rank` to **1 distinct value** across 200,000 matching rows — `strip()` removes the positions that encode term frequency, so the ordering becomes a no-op. | +| Lowering `autovacuum_vacuum_scale_factor` during the backfill | Dropped, unmeasured. The intent was for one chunk's dead tuples to be reused by the next, but rows grow ~120 bytes so new versions rarely fit the freed slots, and a cost-throttled autovacuum cannot keep pace with a chunk every few seconds. The 10 GB → 21 GB doubling in §4.4 was measured without it and is expected regardless. A site that hits steady-state bloat can `ALTER TABLE ... SET` it without a migration. | +| Restricting recall ("rank only the most recent N", "refuse broad queries") | Withdrawn. No production search engine restricts recall; they restrict pagination depth and count precision. Not needed here anyway. | + +## 4. Design + +### 4.1 The projection + +`ReportSearchIndex` is already a derived projection — `search_vector` and +`embedding` are both derived from `report.body`. It is currently half-built: it +carries what search *ranks* on but not what search *filters* on, which is why +every query has to join. This design completes it. + +Governing rule: **denormalise what the scan touches, join for what the page +renders.** The scan visits millions of rows; hydration visits 25. + +```python +# search projection - mirrors of the Report fields the scan filters on +group_ids = ArrayField(IntegerField(), default=list) # Report.groups +modality_codes = ArrayField(CharField(16), default=list) # Report.modalities +language_code = CharField(max_length=10, null=True) +patient_sex = CharField(max_length=1, null=True) +patient_age = IntegerField(null=True) +patient_id = CharField(max_length=64, null=True) +study_datetime = DateTimeField(null=True) +study_description = CharField(max_length=64, blank=True, null=True) +report_created_at = DateTimeField(null=True) +report_updated_at = DateTimeField(null=True) +``` + +The mirrored scalars are nullable so every `AddField` stays metadata-only (§4.4); +tightening them to NOT NULL afterwards would need a validating table scan for no +benefit, and `check_search_projection` guards against drift instead. + +`report_created_at` / `report_updated_at` are named for their source; an +unqualified `created_at` on this table would be ambiguous about whose timestamp +it is. + +Indexes — two, chosen from which callers actually populate which filters: + +- **`GinIndex` on `group_ids` — measured.** On the 5M rig with the corpus split + 94.9% / 5% / 0.1% across three groups, comparing the planner's choice against + the same query with index scans forced off: + + | group holds | with index | forced scan | gain | + | --- | --- | --- | --- | + | 0.1% (5,001 reports) | 2–3 ms | 345 ms | ~170x | + | 5% (250,050) | 119–141 ms | 342–357 ms | ~2.9x | + | 94.9% (4,745,949) | 448–452 ms | 444–487 ms | none | + + It pays wherever a group holds a minority of the corpus, and costs nothing + where it does not — at 94.9% the planner abandons it unprompted. Carrying cost + is negligible: 6 MB, 1.2 s to build, because a handful of distinct group ids + means a handful of posting lists. +- **btree on `report_updated_at` — reasoned, not measured.** Subscriptions call + `filter()` with `updated_after` (`subscriptions/tasks.py`), which has no + tsquery to compete with, so a selective range over a timestamp is the textbook + index case; unindexed it is a full-table scan on every subscription refresh, + once per subscription. Worth confirming during implementation with the same + force-the-plan technique used for `group_ids` above. + +Deliberately **not** indexed: + +- **`modality_codes`.** With four or five modality codes a filter selects roughly + a quarter of the corpus, never selective enough to beat a sequential scan, and + GIN carries real write cost. Note array operators (`@>`, `&&`) evaluate fine + per row without any index; an index only enables *index scans*. +- **`study_datetime`.** §4.3 converts the date filters to half-open ranges, which + would make an index on this column *usable* — but the point of that change is + removing five million per-row timezone conversions, and it stands on its own + without an index. A date filter always co-occurs with a tsquery, so the planner + will usually prefer the scan anyway. Left out until a measurement justifies it. +- **`patient_id`, `report_created_at`.** No caller sets `patient_id`, + `created_after` or `created_before` — they exist on `SearchFilters` and are + handled in `_build_filter_query`, but nothing populates them. The columns are + still needed for the filter code; indexes are not. +- **`patient_sex`, `patient_age`, `language_code`, `study_description`.** Low + selectivity, or `icontains`, which a btree cannot serve anyway. + +`body`, `pacs_name` and `document_id` stay on `Report`. + +**Labels stay a subquery.** `LabelResult` churns constantly under relabeling +jobs, making `label_names` the highest-maintenance column for the least benefit. + +**Cost:** ~120 bytes per row, about 600 MB at 5M reports — a ~9.5% increase on +the 6.3 GB the scan reads, in exchange for eliminating the joins. + +### 4.2 Consistency + +Three **statement-level** triggers using transition tables: + +| trigger on | fires | maintains | +| --- | --- | --- | +| `reports_report` | AFTER UPDATE | `language_code` + the seven scalars | +| `reports_report_groups` | AFTER INSERT, DELETE | `group_ids` | +| `reports_report_modalities` | AFTER INSERT, DELETE | `modality_codes` | + +Statement-level, not row-level: the bulk-upsert API endpoint writes membership +rows with `group_through.objects.bulk_create(...)` and +`modality_through.objects.bulk_create(...)` (`reports/api/viewsets.py:215,229`), +so one statement can carry a whole batch. Row-level triggers would fire once per +row; with `REFERENCING NEW TABLE AS ...` each statement costs one set-based +`UPDATE ... FROM` instead. + +The `reports_report` trigger guards its write with `IS DISTINCT FROM` across the +mirrored columns so unrelated `Report.save()` calls do not rewrite projection +rows needlessly. + +The two paths that *create* `ReportSearchIndex` rows — the `post_save` receiver +in `pgsearch/signals.py` and `bulk_upsert_report_search_indexes` — populate the +projection at creation. Triggers only maintain it afterwards, so a trigger +firing before a row exists is a harmless zero-row `UPDATE`. + +**Why triggers rather than `m2m_changed`.** `group_ids` governs who may read a +patient's report. An application signal fires only for Django ORM operations, +missing `add_custom_report.py`, admin actions, management commands and raw SQL. +A trigger holds inside the same transaction as the write, for every writer. +Precedent exists in the repo: `Report.patient_age` is a +`GeneratedField(db_persist=True)` computed by a hand-written SQL function +installed via `RunSQL` in `reports/migrations/0011`. + +**Triggers must be AFTER, not BEFORE.** PostgreSQL does not expose stored +generated column values to BEFORE triggers, so a BEFORE trigger would mirror +`patient_age` as NULL. + +**Two edges the triggers deliberately do not cover.** Deleting a `Report` +cascades to both the membership rows and the projection row; the resulting +membership-delete trigger is a zero-row `UPDATE` against an already-deleted +projection row, which is harmless in either cascade order. And `Language.code` is +treated as immutable — no trigger watches `reports_language`, so renaming a code +would strand `language_code` values until the backfill is re-run. + +**Reconciliation.** A `check_search_projection` management command verifies the +projection against its sources with one set-based query per column and reports +drift. Cheap insurance on access-control data, and something operators can run +after a restore or bulk import. + +### 4.3 Query layer + +`_build_filter_query` becomes a single-table predicate builder: + +| today | after | +| --- | --- | +| `Q(report__groups=filters.group)` | `Q(group_ids__contains=[filters.group])` | +| `Q(report__language__code=...)` | `Q(language_code=...)` | +| `Q(report__modalities__code__in=...)` | `Q(modality_codes__overlap=...)` | +| `Q(report__patient_sex=...)`, age, description, patient_id | same, unprefixed | +| `Q(report__study_datetime__date__gte=)` | `Q(study_datetime__gte=)` | +| `Q(report__study_datetime__date__lte=)` | `Q(study_datetime__lt=)` | +| `Q(report__created_at__gte=...)` | `Q(report_created_at__gte=...)` | +| labels | unchanged — `report_id__in=`, no join | + +**The date filters become half-open ranges.** Django's `__date` lookup compiles +to `(study_datetime AT TIME ZONE 'Europe/Berlin')::date >= …` — a function over +the column, which no plain btree can serve, and which is evaluated once per row. +On the sequential scan that dominates our plans that is five million timezone +conversions and date casts per query, for a predicate that is equivalent to a +comparison against two timestamps. + +Computing the day boundaries in Python instead is exactly equivalent in meaning, +removes the per-row conversion, and makes the predicate sargable. A functional +index matching Django's expression was rejected: it would restore sargability but +leave the per-row cost untouched on the seq-scan path, and it hardcodes a +timezone that breaks silently if `TIME_ZONE` changes. Storing a precomputed local +`study_date` column was rejected for baking `TIME_ZONE` into data, which a +timezone change would then require a backfill to correct. + +**`.distinct()` is removed unconditionally.** It exists because the `modalities` +join duplicates a report that matches on more than one modality (the `groups` +join cannot, since `filters.group` is a single value against a `unique_together` +through table). With no joins there is nothing to +deduplicate. Worth ~30% on its own (1.44 s → 1.04 s at 1M). + +**Access-control edge case.** `filters.group=None` is documented as fail-closed: +`Q(report__groups=None)` compiles to `groups__isnull=True` and matches only +reports in no group. The array equivalent is `Q(group_ids=[])` — *not* any form +of `contains`. This is the easiest thing in the change to get subtly wrong and +gets its own test. + +`match_q`, `rank_expr`, `summary_expr` and `_exclude_negations` all currently +traverse `report__language__code` and become `language_code` on the row. The +vector half also sheds its `.distinct()` (6.5 ms → 1.0 ms). + +The FTS candidate query then compiles to the shape below, measured on the 5M rig +at **401 ms** with `LIMIT 10000` and `max_parallel_workers_per_gather = 4` — +confirming the production pool size costs nothing on this shape, not just the old +one: + +```sql +SELECT report_id, + CASE WHEN language_code IN ('en') + THEN ts_rank(search_vector, to_tsquery('english', ...)) END AS rank +FROM pgsearch_reportsearchindex +WHERE group_ids @> ARRAY[1] + AND language_code IN ('en') + AND search_vector @@ to_tsquery('english', ...) +ORDER BY rank DESC, report_id +LIMIT 10000; +``` + +**Consumers.** `search()`, `count()` and `retrieve()` inherit the fix with no +logic change; all three route through `_fuse_hybrid`. `filter()` (subscriptions) +keeps one join for `report__document_id`; it is bounded by `updated_after`, so +mirroring `document_id` would be scope for no measured gain. Hydration is +untouched — `page_rows` still `select_related("report")` for `ts_headline` over +`report__body`, which is 25 rows. + +### 4.4 Migration and backfill + +`pgsearch/migrations/0003_search_projection.py`, with `atomic = False`: + +1. **Add columns.** Arrays get `default=list` (`DEFAULT '{}'`, NOT NULL); scalars + go in nullable. All are constant-default or nullable, so every `AddField` is + metadata-only and instant even at 5M rows. Every row therefore carries an + empty `group_ids` until step 3 fills it, and that empty value is fail-closed + in one direction only: a group-scoped search (`group=`) compiles to + `group_ids @> ARRAY[id]` and matches nothing, but `group=None` compiles to + the exact match `group_ids = '{}'` and matches **every row in the archive**. + `group=None` is reachable — `extractions/views.py` passes it for a logged-in + user with no active group — so between this step and step 3 that path is + fail-*open*. It is contained only accidentally today (step 1 also leaves + `language_code` NULL corpus-wide, so the FTS half returns nothing), and the + vector half carries no language predicate, so on a deployment with embeddings + the leak is real. **The migration must therefore be run with the web tier + stopped**, which is what the admin guide's upgrade procedure prescribes. +2. **Create triggers** — *before* the backfill, so reports edited during a + multi-minute backfill still land correctly. A chunk that later reprocesses the + same row simply rewrites the same current values. +3. **Backfill**, chunked by `report_id` range (50,000 rows) with one transaction + per chunk (hence `atomic = False`). Each chunk locks its rows (`SELECT ... + FOR UPDATE`) and then applies a set-based `UPDATE ... FROM reports_report` + with two lateral `array_agg` subqueries for the arrays. The lock pass closes + an EvalPlanQual window under READ COMMITTED, where a stale aggregate could + silently restore a group a concurrent trigger had just removed. The chunk + size is a constant, not a setting: a migration is a historical record and + should do the same thing on every machine. +4. **Create indexes last, then `ANALYZE`.** Building GIN indexes on empty + columns is wasted work — but the stronger reason is that while the new columns + are unindexed the backfill can use HOT updates, whose old row versions are + reclaimed by page pruning without index cleanup. That is the main defence + against the bloat below, so this ordering must not be changed. The `ANALYZE` + is required, not hygiene: ten new columns and two new indexes carry no + statistics, and this design depends on the planner choosing a parallel + sequential scan with a top-N heapsort. No `VACUUM` alongside it: it cannot + run inside a transaction block, so it would cost this migration its + atomicity; it does not shrink the file (see below); and rewriting every row + puts the table far past the autovacuum threshold, so autovacuum does the + same work unattended minutes later. + +**Deployment consequences.** `docker-compose.prod.yml` runs `manage.py migrate` +in the `init` service and `web` waits on it, so the backfill blocks the deploy +rather than serving half-projected rows — the right failure mode for +access-control data. Deploy time therefore grows with corpus size: **measured at +9 min 20 s for 8M reports**, in 50,000-row chunks — a floor, since that run +predates the per-chunk lock pass in step 3. That extrapolates to roughly +18 minutes at 15M. Adding the ten columns is genuinely free (1.3 ms, confirming +the metadata-only claim); the nine minutes is the row rewrite. + +Every service that waits on `init` must therefore tolerate a wait that long: +they wait with `wait-for-it -t ${WAIT_INIT_TIMEOUT:-3600}` in both compose +files, because the previous fixed `-t 300` timed out mid-backfill and left the +containers exited after a technically successful migration. + +Roughly ten minutes of announced downtime for a one-time migration has been +accepted for this deployment, which is what keeps the blocking approach below. +A site that cannot take that window needs the online alternative instead. + +The ten-minute figure was measured on rigs built **without embeddings**. On an +embedded corpus every rewritten row is a non-HOT update that would also insert +into the HNSW index (~143 ms and ~2,160 buffers per embedded row measured on +staging: ~68 hours for 1.7M vectors). The backfill therefore drops the HNSW +index first and the indexes migration rebuilds it from the stored vectors in one bulk build +(7m36s at 1.7M with 16GB maintenance_work_mem and 7 parallel workers). The +build honors the server settings, tunable through the compose GUC knobs +`POSTGRES_MAINTENANCE_WORK_MEM` / `POSTGRES_MAX_PARALLEL_MAINTENANCE_WORKERS`, +with `/dev/shm` sized to match (`POSTGRES_SHM_SIZE_BYTES`). + +The backfill rewrites every row: the tsvectors average 1220 bytes and nothing is +TOASTed (`toast_heap` is 0 bytes), so the whole row is rewritten, not just the +new columns. Measured at 8M, the heap goes **10 GB → 21 GB** with 7,988,672 dead +tuples, and a plain `VACUUM` clears every dead tuple while leaving the file at +**21 GB** for ~11 GB of live data. + +Chunking and creating indexes last (steps 3 and 4) did **not** prevent this — the +rows grow by ~120 bytes, so most updated tuples cannot fit back into their +original page and HOT does not apply. The doubling should be treated as expected, +not as something the migration avoids. The consequence is worse than the linear +2x it looks like, because the inflated table stops fitting the page cache. +Measured on the 8M rig: at a 22 GB heap (27 GB total) `findings` took +**2,550 ms** and `pneumonia` **3,271 ms**; after a `VACUUM FULL` took the heap to +11 GB (13 GB total) the same queries took **870 ms** and **818 ms** — about 2.9x, +on a 31 GB host with ~20 GB of page cache. + +The 882 ms in §2 therefore describes the *compacted* state, not the bloated one. +An earlier draft of this section claimed the reverse and predicted 450–650 ms +after compaction; compaction was measured at 870 ms, so that prediction was +wrong in both directions. The free space is only consumed by net new inserts (an update frees its own old +version), so it amounts to one full copy of the table waiting on roughly as many +new reports as the archive already holds -- many years at hospital ingest rates. +Treat the penalty as persistent until the table is compacted deliberately, with +the caveats below, rather than as something that decays. + +**Do not run `VACUUM FULL` from the migration, and do not recommend it blindly.** +It rebuilds every index on the table, including `pgsearch_embedding_hnsw`; on a +deployment with embeddings populated that is an HNSW rebuild over millions of +vectors — plausibly hours, under an ACCESS EXCLUSIVE lock — and it needs peak +disk for old plus new (~19 GB at 5M). Sites that want the space returned should +run it deliberately in a maintenance window knowing that cost, or use `pg_repack` +if they have it. This design requires neither to be correct, but a deployment +with embeddings populated should expect to compact once after the migration, +since waiting for organic refill is a multi-year proposition. The accepted +approach for this deployment is a `VACUUM FULL` in its own maintenance window, +scheduled separately from the release. The admin guide carries the procedure and +the options for shortening the outage. + +An online alternative — resumable backfill command plus a feature flag on the +query path — was considered and rejected: it doubles the query-layer code for +the duration and the flag has to be removed later anyway. + +### 4.5 PostgreSQL configuration + +Delivered as `-c` GUCs on the `postgres` service's `command:` in +`docker-compose.base.yml`, substituted from `.env`: + +```yaml +postgres: + command: + - postgres + - -c + - max_parallel_workers_per_gather=${POSTGRES_MAX_PARALLEL_WORKERS_PER_GATHER:-4} + - -c + - max_parallel_workers=${POSTGRES_MAX_PARALLEL_WORKERS:-8} + - -c + - max_worker_processes=${POSTGRES_MAX_WORKER_PROCESSES:-8} + - -c + - shared_buffers=${POSTGRES_SHARED_BUFFERS:-128MB} +``` + +| variable | default | stock | rationale | +| --- | --- | --- | --- | +| `POSTGRES_MAX_PARALLEL_WORKERS_PER_GATHER` | `4` | 2 | measured 606 ms → 421 ms | +| `POSTGRES_MAX_PARALLEL_WORKERS` | `8` | 8 | unchanged default; exposed so larger hosts can raise all three coherently | +| `POSTGRES_MAX_WORKER_PROCESSES` | `8` | 8 | as above | +| `POSTGRES_SHARED_BUFFERS` | `128MB` | 128MB | unchanged default; exposed because the right value is a fraction of the host's RAM | + +Only the first changes behaviour out of the box; the other three keep PostgreSQL's +own defaults and exist so operators can raise them coherently. + +**None of them go in `example.env`.** The project already distinguishes the two +kinds of variable: things a deployer *chooses* are listed there (ports, +`EXAMPLE_REPORTS_LANGUAGE`, `REMOTE_DEBUGGING_PORT`), while tuning knobs with a +sensible default are compose-only overrides (`EMBEDDINGS_WORKER_CONCURRENCY`, +`WAIT_POSTGRES_TIMEOUT`, `RADIS_IMAGE`). These are the second kind. They are +documented in `docs/user-docs/admin-guide.md`, which is where operator-facing +guidance lives, and listed in the environment-variable section of `CLAUDE.md`. + +Measured at 5M on `findings`: + +| `max_parallel_workers_per_gather` | time | +| --- | --- | +| 2 (stock) | 606 ms | +| 4 | 421 ms | +| 6 | 379 ms | +| 8 | 343 ms | + +- **`max_parallel_workers_per_gather` = 4** captures most of the available gain + while leaving cores for concurrent searches. The cluster-wide caps + (`max_parallel_workers`, `max_worker_processes`, both 8 by default) are *not* a + throttle at this value — 8 covers two concurrent parallel searches, and a third + degrades gracefully to fewer workers rather than queueing. They are exposed so + that an operator raising `per_gather` on a larger host raises all three + together, which is the only case where they bind. +- **`shared_buffers` default is left at PostgreSQL's own 128 MB.** Raising it is + standard advice for a multi-GB database, but the 343 ms figure came from a warm + *OS* page cache, so no measured gain can be attributed to it — and a raised + default would break small installs, where 512 MB of shared memory on a 1 GB + container fails to start or thrashes. Exposed as an override with ~25%-of-RAM + guidance in the admin guide; not changed by default, for the same reason + `work_mem` is not. +- **`work_mem` deliberately not raised.** It mattered only because `.distinct()` + forced an external merge sort; without it the sort is a 26 kB top-N heapsort. + Raising it is per-connection-per-node memory risk for zero measured benefit. + +**GIN index:** `ALTER INDEX pgsearch_re_search__b0f715_gin SET (fastupdate = off)` +plus a one-time `gin_clean_pending_list()`. A storage-parameter change, so no +rebuild. Trade: index inserts get slower, read latency gets predictable. RADIS +writes in batches asynchronously through Procrastinate and reads interactively, +so that is the right side of the trade — but it is a trade. + +## 5. Testing + +**Filter equivalence via a reference oracle.** Keep the current joined +`_build_filter_query` in the test module as `_build_filter_query_legacy` and +assert old and new produce identical report-id sets across a matrix of filter +combinations. Highest-value test in the change: it catches +`modality_codes__overlap` vs `code__in` semantics, date boundaries and the +`group=None` case in one mechanism. Delete the oracle once the change settles. + +**Access control:** +- `group_ids = []` is the only thing `filters.group=None` matches. +- Adding a report to a group makes it findable by that group. +- Removing a report from a group makes it unfindable — the direction that is a + leak if the trigger is wrong. +- A raw `cursor.execute` INSERT into `reports_report_groups`, bypassing the ORM, + still updates the projection. This test is the justification for choosing + triggers over signals and should exist to defend that decision. + +**Bulk correctness:** a `bulk_create` of many rows into the `groups` through +model — the shape the bulk-upsert endpoint uses — updates the projection for +*every* affected report. Statement-level triggers have a classic failure mode +where only one row of the transition table is processed; this pins it. + +**No duplicates without `.distinct()`:** a report in several matching groups, or +with several matching modalities, appears exactly once. + +**Plan-shape assertion instead of timing.** Wall-clock assertions are flaky in +CI; structure is not. Run `EXPLAIN` on the generated FTS query and assert the +plan contains no join against `reports_report_groups`, and assert the compiled +SQL contains no `DISTINCT` keyword. This deterministically catches someone +reintroducing a `report__` traversal and silently restoring the slow path. + +Assert on the *candidate queryset the provider runs* — filter, tsquery match, +rank annotation, ordering and bound — and not on one rebuilt from +`_build_filter_query` alone. The `.distinct()` calls this change removes never +lived in the filter builder, so a test that rebuilds only that half cannot fail +however the production query changes. Keeping the candidate query in one named +function (`_fts_candidate_queryset`) is what makes the real thing reachable from +a test. + + +Do **not** assert on the absence of a `Unique` node: PostgreSQL implements +`SELECT DISTINCT` as either `Sort`+`Unique` or `HashAggregate` depending on +cardinality estimates, and measurement at 3, 1,000 and 100,000 rows — with the +same GIN array-containment pattern this design uses — produced `HashAggregate` +in every non-trivial case. An operator-name assertion therefore passes with the +regression present at exactly the scale it exists to protect. The `DISTINCT` +keyword is emitted unconditionally by Django whenever `.distinct()` is called, +so a SQL-text check is scale-independent. + +**`patient_age` mirrors `Report.patient_age` exactly.** Verified that this is +the trap it guards: a BEFORE trigger on a stored generated column reads `NULL` +where the committed value was 42, so converting the triggers to BEFORE would +silently null this column out. + +Existing search, extraction and subscription tests are the regression net for the +four consumers and should pass unchanged. + +## 6. Non-goals + +- **BM25 / pg_textsearch (#285).** Deferred and reclassified as a *relevance* + decision, not a performance one. Measured: as written it is **33x slower** than + the `ts_rank` path it replaces (440,954 ms vs 13,440 ms for `findings`), because + `.distinct()` and the joins prevent the index-ordered scan and `<@>` degenerates + to per-row scoring at ~92 µs/row. It needs this denormalisation *plus* a + two-phase small-k formulation to pay off (370 ms for `findings`, 242 ms for + `pneumonia`). Index build was 6m41s and 2014 MB at 5M, holding a lock on + `reports_report`. What BM25 still offers that PostgreSQL FTS cannot: IDF, term + frequency saturation and length normalisation. +- **#287 impact-ordered lexeme ranks.** Likely unnecessary once plain `ts_rank` + handles 8M matches in ~880 ms. Recommendation only; closing it is its author's + call. +- **`HYBRID_FTS_MAX_RESULTS`** stays at 10,000. Measured free under `ts_rank` on + both shapes: `LIMIT 100` and `LIMIT 10000` were indistinguishable on the old + joined query, and on the 5M rig `LIMIT 25` (421 ms) and `LIMIT 10000` (401 ms) + are indistinguishable on the new single-table one. It would only become + load-bearing with a pruning index. +- **`Report.groups` → `ArrayField`.** Would remove the redundancy altogether and + speed up `Report.objects.filter(groups=...)` elsewhere, at the cost of FK + integrity on an access-control field. A domain change, deliberately not bundled + into a search-performance change. +- **Result-page N+1s.** Measured at ~134 SQL queries per 25-result page + (`document.full_report` twice per row via `search/site.py`, `can_view_report`, + the collections count, the notes lookup). Once the scan is ~880 ms this is + proportionally significant, and it is the recommended next piece of work — but + it is a different subsystem. +- **#282 (`hnsw.ef_search`) and #284 (fused result cache).** Independent, + unaffected, still worth landing. Note `ef_search` defaults to 40 while + `HYBRID_VECTOR_TOP_K` is 100, so the vector half currently contributes at most + 40 candidates. + +## 7. Success criteria + +- At the 8M design target with `max_parallel_workers_per_gather` = 4, the FTS + candidate query drops from ~8,400 ms to under 1,000 ms immediately after the + backfill, bloat included (measured: 8,388 ms → 882 ms). +- The FTS candidate query plan contains no join to `reports_report_groups` and + no `Unique` node. +- `check_search_projection` reports zero drift after the backfill and after a + round of report and group mutations. +- Existing search, extraction and subscription tests pass unchanged. + +## 8. Measurement rig + +The BM25 figures in §6 additionally required a custom PostgreSQL image +(`pgvector/pgvector:pg17` plus `pg_textsearch` v1.4.0), and the vector-half +figures come from a third throwaway corpus of 200,000 rows with 1024-dimension +embeddings and an HNSW index. Everything else comes from two corpora built from +`samples/reports_en.json` (1000 real-shaped report bodies, cycled with a unique +md5 suffix per report): + +- **8M rig (the design target):** the 5M rig grown to 8,001,000 reports, 10 GB + `pgsearch_reportsearchindex` heap before the backfill and 21 GB after. All §2 + and §7 figures come from here. +- **5M rig:** the dev stack database, 5,001,000 reports, ~15 GB, 6.3 GB + `pgsearch_reportsearchindex` heap, average tsvector 1220 bytes. +- **1M rig:** a standalone container, 1,000,000 reports, 730,000 matching + `pneumonia`, average tsvector 1607 bytes. + +The benchmark host is a 31 GB Linux VM; the production target is a 128 GB M4 Max, +so an 11 GB live working set stays cached on both and these figures are a floor +rather than a ceiling. All rigs on stock PostgreSQL 17 settings (`shared_buffers` 128 MB, `work_mem` 4 MB, +`max_parallel_workers_per_gather` 2) on a 16-core host, unless a measurement +states otherwise. + +**Caveats that bound these numbers.** The corpora are synthetic: 1000 distinct +body templates means lexeme distribution and `ts_rank` score diversity are less +varied than real reports (a 200k-row sample produced only 7 distinct `ts_rank` +values). Measurements are single-user with a warm OS page cache — the 404 ms +figure read 6.2 GB of buffers in ~320 ms, which is page cache, not disk. On a +production host with less RAM relative to corpus size, this path becomes +I/O-bound and degrades. That, rather than CPU, is the real scaling ceiling. diff --git a/docs/user-docs/admin-guide.md b/docs/user-docs/admin-guide.md index 2fbe2636..6164fe67 100644 --- a/docs/user-docs/admin-guide.md +++ b/docs/user-docs/admin-guide.md @@ -118,6 +118,140 @@ When the embedding service is unreachable or rate limiting, searches silently fa See the [Contributing guide](../dev-docs/contributing.md) for how to run Ollama with a chat and an embedding model, and the troubleshooting sections there for connection problems. +Step 9 can take much longer than usual when a release ships a data migration. +The `init` service runs `manage.py migrate` and every other service waits for it +to finish, so the stack stays unavailable for the whole migration. The release +that added the search projection to the report search index is the current +example: it rewrites every row of that table, measured at about **ten minutes +for 8 million reports** without embeddings and proportionally longer on a +larger archive. On an archive **with embeddings** the migration first drops the +vector index and rebuilds it afterwards from the stored vectors (nothing is +re-embedded) -- without that, every rewritten row would also pay an insert into +the vector index (~143 ms per embedded row: days, not minutes). For a large +embedded archive raise `POSTGRES_MAINTENANCE_WORK_MEM` and +`POSTGRES_MAX_PARALLEL_MAINTENANCE_WORKERS` in `.env` for the migration window +so the rebuild runs in memory (a 1.7M-vector archive rebuilt in under 8 +minutes with `16GB` and 7 workers), and size `POSTGRES_SHM_SIZE_BYTES` at +least as large as that memory value -- the parallel build pre-allocates it in +`/dev/shm`, and other parallel queries can fail with "could not resize shared +memory segment" while it runs. Plan the +maintenance window around all of that, and do not shorten `WAIT_INIT_TIMEOUT` +(default one hour, see the compose files) below the expected migration time -- +the services that wait on `init` exit when that timeout expires. + +Keep the web tier stopped for the whole migration, which steps 5 and 9 above +already do -- do not start it early to "check on progress". While that +particular migration is between adding the projection columns and backfilling +them, every row carries an empty group list. A search that runs with no active +group (the extraction preview does this for a user without one) filters on +exactly that empty list, so during the window it would match the whole archive +instead of only the reports belonging to no group. + +## Database tuning + +Search scans the report index table in parallel, so the number of parallel +workers is the one PostgreSQL setting worth revisiting. These are optional +overrides -- set them in `.env` only if the defaults do not suit your hardware. + +| Variable | Default | Guidance | +| --- | --- | --- | +| `POSTGRES_MAX_PARALLEL_WORKERS_PER_GATHER` | `4` | Measured on 5M reports: 606 ms at 2, 421 ms at 4, 343 ms at 8. Four captures most of the gain while leaving cores for concurrent searches. | +| `POSTGRES_MAX_PARALLEL_WORKERS` | `8` | PostgreSQL's default. Raise together with the two others on a larger host. | +| `POSTGRES_MAX_WORKER_PROCESSES` | `8` | As above. | +| `POSTGRES_SHARED_BUFFERS` | `128MB` | PostgreSQL's default. Around 25% of host RAM is the usual recommendation; a value larger than the container's memory will prevent PostgreSQL from starting. | + +### Reclaiming space after the search-projection migration + +The release that added the search projection rewrites every row of the report +index table, which roughly doubles it on disk -- measured at 8 million reports, +the table grew from 10 GB to 21 GB. Autovacuum reclaims the dead rows within +minutes and no action is needed to keep the database healthy, but reclaiming +them does not shrink the file: the space is marked reusable and stays allocated. + +Search scans that table, so until the space is reused the scan reads about twice +the pages it needs to. How much that costs depends on whether the inflated table +still fits the host's page cache: + +- **It fits.** Expect roughly the page-count ratio, near 1.9x on the scan. At + 8 million reports the inflated table is about 27 GB including indexes, so any + host with 64 GB or more stays in this case. +- **It does not fit.** The scan starts reading from disk and the penalty grows + well beyond 2x. On a 31 GB test host with about 20 GB of page cache, a query + matching every report took 2,550 ms inflated against 870 ms compacted. + +**Do not expect this to resolve on its own within any useful timeframe.** The +backfill leaves free space equal to roughly one full copy of the table, and only +*net new* reports consume it -- an update frees its own old row version, so +report edits are space-neutral. Refilling an 8-million-row archive therefore +takes on the order of 8 million new reports, which at typical hospital volumes is +many years. Emptied pages stay part of the table and are still read by every +scan. + +Plan to compact the table once after this migration. The tool is +`VACUUM FULL pgsearch_reportsearchindex;`, and it needs a maintenance window. + +**Do not bolt this onto the upgrade window.** Nothing is broken in the meantime +-- the system is fully functional, search is just slower -- so deploy the release +first, bring the stack back up, and schedule the compaction separately. That way +a long compaction never blocks a release. + +**Budget the time realistically.** Copying the table is the quick part. The cost +is dominated by rebuilding the HNSW vector index, because `VACUUM FULL` relocates +every row and so invalidates and rebuilds every index on the table. Building an +HNSW graph over millions of vectors takes hours, and `VACUUM FULL` holds an +`ACCESS EXCLUSIVE` lock for the whole operation, so search is unavailable +throughout. With `EMBEDDINGS_MODEL` empty there is no HNSW index to rebuild and +the whole thing takes minutes instead (measured: 3 minutes for 8 million +reports). + +Nothing is recomputed and nothing is lost. Embeddings are stored data and are +copied across as they are -- the embedding service is never called, and the index +is rebuilt from the vectors already in the table. + +Before starting: + +- **Free disk.** Both copies of the table exist at once, so the filesystem needs + room for the compacted table on top of the inflated one. +- **Raise `maintenance_work_mem`** for the session. pgvector builds the HNSW + graph in memory when it fits and falls back to a much slower on-disk path when + it does not. At 8 million rows and 1024 dimensions the vectors alone are tens of + gigabytes, so the default leaves you on the slow path. Set it as high as the + host can spare, for example `SET maintenance_work_mem = '32GB';` in the same + session as the `VACUUM FULL`. +- **Enable maintenance mode** so users get an explanation rather than timeouts. + +If hours of unavailable search is not acceptable, there are two ways to shorten +it, both optional: + +- **Rebuild the vector index separately.** Drop the HNSW index first, run + `VACUUM FULL` (now minutes, since only small indexes are rebuilt), bring the + stack back up, then recreate the index without blocking: + + ```sql + DROP INDEX pgsearch_embedding_hnsw; + VACUUM FULL pgsearch_reportsearchindex; + -- bring the stack back up here, then: + CREATE INDEX CONCURRENTLY pgsearch_embedding_hnsw + ON pgsearch_reportsearchindex USING hnsw (embedding vector_cosine_ops) + WITH (m = 16, ef_construction = 64); + ``` + + This trades hours of full downtime for minutes of downtime plus hours of + degraded semantic search: without the index, vector similarity falls back to an + exact scan over every row, which is very slow on a large archive. Full-text + search is unaffected. Keep the index name and definition exactly as above, or + Django's migration state will no longer match the database. If + `CREATE INDEX CONCURRENTLY` fails it leaves an invalid index behind -- drop it + and retry. +- **Use [`pg_repack`](https://reorg.github.io/pg_repack/)**, which does the same + compaction against a shadow copy while the table stays online, taking a brief + exclusive lock only at the swap. It is not part of the PostgreSQL image RADIS + ships, so this means building or installing the extension and its client binary + first. + +A plain `VACUUM` is not an alternative for any of this: it reclaims dead rows, +which autovacuum has already done, and leaves the file at its inflated size. + ## User and Group Management Administrators can create users by navigating to the Django Admin section. Alternatively, users can self-register, after which an administrator must approve and activate their account. diff --git a/radis/pgsearch/admin.py b/radis/pgsearch/admin.py index a3614429..47a8f2d8 100644 --- a/radis/pgsearch/admin.py +++ b/radis/pgsearch/admin.py @@ -36,6 +36,31 @@ class ReportSearchIndexAdmin(admin.ModelAdmin): # load. raw_id_fields swaps that for a text input + lookup popup so # opening a single row doesn't enumerate the whole table. raw_id_fields = ("report",) + # Every field is machine-maintained (signals, the 0004/0007 triggers, the + # embedding tasks), so the change form is inspection-only. group_ids + # especially is access-control data: an editable form would grant or hide + # report visibility out of sync with the membership tables, and even + # saving an untouched form would clobber trigger-maintained values. + readonly_fields = ( + "report", + "search_vector", + "embedding", + "group_ids", + "modality_codes", + "language_code", + "patient_sex", + "patient_age", + "patient_id", + "study_datetime", + "study_description", + "report_created_at", + "report_updated_at", + ) + + def has_add_permission(self, request): + # Rows are created by the post_save signal on Report; a hand-made row + # would just be an orphan the next reindex fights with. + return False def has_delete_permission(self, request, obj=None): # RSI rows are managed by the post_save signal on Report — deleting diff --git a/radis/pgsearch/management/commands/check_search_projection.py b/radis/pgsearch/management/commands/check_search_projection.py new file mode 100644 index 00000000..4017da04 --- /dev/null +++ b/radis/pgsearch/management/commands/check_search_projection.py @@ -0,0 +1,82 @@ +"""Verify the ReportSearchIndex search projection against its sources.""" + +from django.core.management.base import BaseCommand, CommandError +from django.db import connection + +# Same aggregation as utils/projection.PROJECTION_UPDATE_SQL, the 0004 backfill +# and the trigger functions in migration 0003 -- four copies, keep them in +# sync. This one is the detector: if it ever drifts in the same direction as a +# writer, it silently agrees with the bug and reports a healthy projection. +DRIFT_SQL = """ +SELECT + count(*) FILTER (WHERE rsi.group_ids IS DISTINCT FROM COALESCE(g.ids, '{}')) + AS group_ids, + count(*) FILTER (WHERE rsi.modality_codes IS DISTINCT FROM COALESCE(m.codes, '{}')) + AS modality_codes, + count(*) FILTER (WHERE rsi.language_code IS DISTINCT FROM l.code) + AS language_code, + count(*) FILTER (WHERE rsi.patient_sex IS DISTINCT FROM r.patient_sex) + AS patient_sex, + count(*) FILTER (WHERE rsi.patient_age IS DISTINCT FROM r.patient_age) + AS patient_age, + count(*) FILTER (WHERE rsi.patient_id IS DISTINCT FROM r.patient_id) + AS patient_id, + count(*) FILTER (WHERE rsi.study_datetime IS DISTINCT FROM r.study_datetime) + AS study_datetime, + count(*) FILTER (WHERE rsi.study_description IS DISTINCT FROM r.study_description) + AS study_description, + count(*) FILTER (WHERE rsi.report_created_at IS DISTINCT FROM r.created_at) + AS report_created_at, + count(*) FILTER (WHERE rsi.report_updated_at IS DISTINCT FROM r.updated_at) + AS report_updated_at +FROM pgsearch_reportsearchindex rsi +JOIN reports_report r ON r.id = rsi.report_id +LEFT JOIN reports_language l ON l.id = r.language_id +LEFT JOIN LATERAL ( + SELECT array_agg(rg.group_id ORDER BY rg.group_id) AS ids + FROM reports_report_groups rg WHERE rg.report_id = r.id +) g ON true +LEFT JOIN LATERAL ( + SELECT array_agg(mo.code ORDER BY mo.code) AS codes + FROM reports_report_modalities rm + JOIN reports_modality mo ON mo.id = rm.modality_id + WHERE rm.report_id = r.id +) m ON true +""" + + +# Informational, not drift: an index row is created by a signal or by the bulk +# indexing path, so a report can legitimately be waiting for one. The number is +# only meaningful over time -- a few hundred that drain are normal, tens of +# thousands that never move mean indexing is stuck. +MISSING_INDEX_ROWS_SQL = """ +SELECT count(*) + FROM reports_report r + LEFT JOIN pgsearch_reportsearchindex rsi ON rsi.report_id = r.id + WHERE rsi.report_id IS NULL +""" + + +class Command(BaseCommand): + help = "Verify the ReportSearchIndex search projection against its sources." + + def handle(self, *args, **options): + with connection.cursor() as cursor: + cursor.execute(DRIFT_SQL) + columns = [column.name for column in cursor.description] + counts = dict(zip(columns, cursor.fetchone(), strict=True)) + + cursor.execute(MISSING_INDEX_ROWS_SQL) + missing_index_rows = cursor.fetchone()[0] + + self.stdout.write( + f"Reports without a search index row: {missing_index_rows} " + "(indexing is deferred, so a number that keeps falling is normal)" + ) + + drifted = {name: count for name, count in counts.items() if count} + if drifted: + details = ", ".join(f"{name}: {count}" for name, count in sorted(drifted.items())) + raise CommandError(f"Search projection drift detected -- {details}") + + self.stdout.write(self.style.SUCCESS("Search projection matches its sources.")) diff --git a/radis/pgsearch/migrations/0003_search_projection.py b/radis/pgsearch/migrations/0003_search_projection.py new file mode 100644 index 00000000..e0a4c476 --- /dev/null +++ b/radis/pgsearch/migrations/0003_search_projection.py @@ -0,0 +1,267 @@ +"""The search projection: mirror columns on ReportSearchIndex plus the +triggers that maintain them. + +The ten AddField operations are metadata-only (nullable scalars, arrays with a +constant default), so this migration is instant regardless of corpus size; the +row rewrite that fills the columns is the separate non-atomic backfill in +0004, and the indexes over them follow in 0005. + +Triggers rather than Django signals because group_ids is access-control data: +a signal fires only for ORM writes, missing management commands, the admin, +and raw SQL. A trigger holds inside the same transaction as the write, and +covers every INSERT and DELETE on the two membership tables, every UPDATE +of reports_report, and every rename of a language or modality code. + +Three writers still escape it, so a new code path that uses one of them has to +keep the projection correct itself: + +- An UPDATE of a membership row (``UPDATE reports_report_groups SET group_id = + ...``). No trigger watches UPDATE on those tables -- Django's ORM never emits + one, but hand-written SQL can, and it would move a report between groups + invisibly. +- TRUNCATE of a membership table. Transition tables cannot be declared on a + TRUNCATE trigger at all, and TRUNCATE fires no row-level events, so the + projection would keep memberships the table no longer holds. +- A re-add of an existing membership, which Django emits as ``INSERT ... ON + CONFLICT DO NOTHING`` (``groups.add()`` uses bulk_create with + ignore_conflicts). The conflicting row is never inserted, so the NEW + transition table is empty and the trigger updates nothing. The consequence + worth remembering: an already-drifted projection row cannot be repaired by + re-adding the membership -- that is what check_search_projection and a + backfill are for. + +Statement-level rather than row-level because the bulk-upsert endpoint writes +membership with bulk_create (reports/api/viewsets.py:215,229), so one statement +can carry a whole batch. The two code-rename triggers are the exception, and +row-level deliberately: PostgreSQL does not allow transition tables on +triggers with column lists ("transition tables cannot be specified for +triggers with column lists"), and a rename statement touches one dimension +row -- FOR EACH ROW with a WHEN guard fires exactly once per actual code +change and costs nothing on these tiny tables. Renaming a language rewrites +the projection row of every report in that language, and on an embedded corpus +each rewrite also inserts into the HNSW index (~143 ms/row measured), so a +corpus-wide rename belongs in a maintenance window. + +AFTER, never BEFORE: PostgreSQL does not expose stored generated column values +to BEFORE triggers, so patient_age would mirror as NULL. +""" + +import django.contrib.postgres.fields +from django.db import migrations, models + +# Same aggregation as utils/projection.PROJECTION_UPDATE_SQL, the 0004 backfill +# and check_search_projection.DRIFT_SQL -- four copies, keep them in sync. The +# drift query is the one that detects divergence, so a change made here and not +# there is a bug that reports itself as healthy. +SYNC_GROUP_IDS = """ +CREATE OR REPLACE FUNCTION pgsearch_sync_group_ids() RETURNS trigger +LANGUAGE plpgsql AS $$ +BEGIN + UPDATE pgsearch_reportsearchindex rsi + SET group_ids = COALESCE( + (SELECT array_agg(g.group_id ORDER BY g.group_id) + FROM reports_report_groups g WHERE g.report_id = rsi.report_id), + '{}') + WHERE rsi.report_id IN (SELECT DISTINCT report_id FROM changed); + RETURN NULL; +END $$; +""" + +# Same aggregation as utils/projection.PROJECTION_UPDATE_SQL, the 0004 backfill +# and check_search_projection.DRIFT_SQL -- four copies, keep them in sync. The +# drift query is the one that detects divergence, so a change made here and not +# there is a bug that reports itself as healthy. +SYNC_MODALITY_CODES = """ +CREATE OR REPLACE FUNCTION pgsearch_sync_modality_codes() RETURNS trigger +LANGUAGE plpgsql AS $$ +BEGIN + UPDATE pgsearch_reportsearchindex rsi + SET modality_codes = COALESCE( + (SELECT array_agg(m.code ORDER BY m.code) + FROM reports_report_modalities rm + JOIN reports_modality m ON m.id = rm.modality_id + WHERE rm.report_id = rsi.report_id), + '{}') + WHERE rsi.report_id IN (SELECT DISTINCT report_id FROM changed); + RETURN NULL; +END $$; +""" + +SYNC_REPORT_FIELDS = """ +CREATE OR REPLACE FUNCTION pgsearch_sync_report_fields() RETURNS trigger +LANGUAGE plpgsql AS $$ +BEGIN + UPDATE pgsearch_reportsearchindex rsi + SET language_code = l.code, + patient_sex = c.patient_sex, + patient_age = c.patient_age, + patient_id = c.patient_id, + study_datetime = c.study_datetime, + study_description = c.study_description, + report_created_at = c.created_at, + report_updated_at = c.updated_at + FROM changed c + LEFT JOIN reports_language l ON l.id = c.language_id + WHERE rsi.report_id = c.id + AND (rsi.language_code, rsi.patient_sex, rsi.patient_age, rsi.patient_id, + rsi.study_datetime, rsi.study_description, rsi.report_created_at, + rsi.report_updated_at) + IS DISTINCT FROM + (l.code, c.patient_sex, c.patient_age, c.patient_id, + c.study_datetime, c.study_description, c.created_at, c.updated_at); + RETURN NULL; +END $$; +""" + +SYNC_LANGUAGE_CODE = """ +CREATE OR REPLACE FUNCTION pgsearch_sync_language_code() RETURNS trigger +LANGUAGE plpgsql AS $$ +BEGIN + UPDATE pgsearch_reportsearchindex rsi + SET language_code = NEW.code + FROM reports_report r + WHERE r.language_id = NEW.id + AND rsi.report_id = r.id + AND rsi.language_code IS DISTINCT FROM NEW.code; + RETURN NULL; +END $$; +""" + +SYNC_MODALITY_CODE_RENAME = """ +CREATE OR REPLACE FUNCTION pgsearch_sync_modality_code_rename() RETURNS trigger +LANGUAGE plpgsql AS $$ +BEGIN + UPDATE pgsearch_reportsearchindex rsi + SET modality_codes = full_agg.codes + FROM ( + SELECT rm.report_id, + array_agg(m.code ORDER BY m.code) AS codes + FROM reports_report_modalities rm + JOIN reports_modality m ON m.id = rm.modality_id + WHERE rm.report_id IN ( + SELECT rm2.report_id FROM reports_report_modalities rm2 + WHERE rm2.modality_id = NEW.id) + GROUP BY rm.report_id + ) full_agg + WHERE rsi.report_id = full_agg.report_id + AND rsi.modality_codes IS DISTINCT FROM full_agg.codes; + RETURN NULL; +END $$; +""" + +CREATE_TRIGGERS = """ +CREATE TRIGGER pgsearch_group_ids_ins AFTER INSERT ON reports_report_groups +REFERENCING NEW TABLE AS changed +FOR EACH STATEMENT EXECUTE FUNCTION pgsearch_sync_group_ids(); + +CREATE TRIGGER pgsearch_group_ids_del AFTER DELETE ON reports_report_groups +REFERENCING OLD TABLE AS changed +FOR EACH STATEMENT EXECUTE FUNCTION pgsearch_sync_group_ids(); + +CREATE TRIGGER pgsearch_modality_codes_ins AFTER INSERT ON reports_report_modalities +REFERENCING NEW TABLE AS changed +FOR EACH STATEMENT EXECUTE FUNCTION pgsearch_sync_modality_codes(); + +CREATE TRIGGER pgsearch_modality_codes_del AFTER DELETE ON reports_report_modalities +REFERENCING OLD TABLE AS changed +FOR EACH STATEMENT EXECUTE FUNCTION pgsearch_sync_modality_codes(); + +CREATE TRIGGER pgsearch_report_fields_upd AFTER UPDATE ON reports_report +REFERENCING NEW TABLE AS changed +FOR EACH STATEMENT EXECUTE FUNCTION pgsearch_sync_report_fields(); + +CREATE TRIGGER pgsearch_language_code_upd +AFTER UPDATE OF code ON reports_language +FOR EACH ROW WHEN (OLD.code IS DISTINCT FROM NEW.code) +EXECUTE FUNCTION pgsearch_sync_language_code(); + +CREATE TRIGGER pgsearch_modality_code_upd +AFTER UPDATE OF code ON reports_modality +FOR EACH ROW WHEN (OLD.code IS DISTINCT FROM NEW.code) +EXECUTE FUNCTION pgsearch_sync_modality_code_rename(); +""" + +DROP_TRIGGERS = """ +DROP TRIGGER IF EXISTS pgsearch_group_ids_ins ON reports_report_groups; +DROP TRIGGER IF EXISTS pgsearch_group_ids_del ON reports_report_groups; +DROP TRIGGER IF EXISTS pgsearch_modality_codes_ins ON reports_report_modalities; +DROP TRIGGER IF EXISTS pgsearch_modality_codes_del ON reports_report_modalities; +DROP TRIGGER IF EXISTS pgsearch_report_fields_upd ON reports_report; +DROP TRIGGER IF EXISTS pgsearch_language_code_upd ON reports_language; +DROP TRIGGER IF EXISTS pgsearch_modality_code_upd ON reports_modality; +DROP FUNCTION IF EXISTS pgsearch_sync_group_ids(); +DROP FUNCTION IF EXISTS pgsearch_sync_modality_codes(); +DROP FUNCTION IF EXISTS pgsearch_sync_report_fields(); +DROP FUNCTION IF EXISTS pgsearch_sync_language_code(); +DROP FUNCTION IF EXISTS pgsearch_sync_modality_code_rename(); +""" + + +class Migration(migrations.Migration): + dependencies = [ + ("pgsearch", "0002_hybrid_search"), + ("reports", "0013_alter_report_options"), + ] + + operations = [ + migrations.AddField( + model_name="reportsearchindex", + name="group_ids", + field=django.contrib.postgres.fields.ArrayField( + base_field=models.IntegerField(), default=list + ), + ), + migrations.AddField( + model_name="reportsearchindex", + name="language_code", + field=models.CharField(max_length=10, null=True), + ), + migrations.AddField( + model_name="reportsearchindex", + name="modality_codes", + field=django.contrib.postgres.fields.ArrayField( + base_field=models.CharField(max_length=16), default=list + ), + ), + migrations.AddField( + model_name="reportsearchindex", + name="patient_age", + field=models.IntegerField(null=True), + ), + migrations.AddField( + model_name="reportsearchindex", + name="patient_id", + field=models.CharField(max_length=64, null=True), + ), + migrations.AddField( + model_name="reportsearchindex", + name="patient_sex", + field=models.CharField(max_length=1, null=True), + ), + migrations.AddField( + model_name="reportsearchindex", + name="report_created_at", + field=models.DateTimeField(null=True), + ), + migrations.AddField( + model_name="reportsearchindex", + name="report_updated_at", + field=models.DateTimeField(null=True), + ), + migrations.AddField( + model_name="reportsearchindex", + name="study_datetime", + field=models.DateTimeField(null=True), + ), + migrations.AddField( + model_name="reportsearchindex", + name="study_description", + field=models.CharField(blank=True, max_length=64, null=True), + ), + migrations.RunSQL(SYNC_GROUP_IDS, reverse_sql=migrations.RunSQL.noop), + migrations.RunSQL(SYNC_MODALITY_CODES, reverse_sql=migrations.RunSQL.noop), + migrations.RunSQL(SYNC_REPORT_FIELDS, reverse_sql=migrations.RunSQL.noop), + migrations.RunSQL(SYNC_LANGUAGE_CODE, reverse_sql=migrations.RunSQL.noop), + migrations.RunSQL(SYNC_MODALITY_CODE_RENAME, reverse_sql=migrations.RunSQL.noop), + migrations.RunSQL(CREATE_TRIGGERS, reverse_sql=DROP_TRIGGERS), + ] diff --git a/radis/pgsearch/migrations/0004_search_projection_backfill.py b/radis/pgsearch/migrations/0004_search_projection_backfill.py new file mode 100644 index 00000000..ec26bca9 --- /dev/null +++ b/radis/pgsearch/migrations/0004_search_projection_backfill.py @@ -0,0 +1,110 @@ +"""Backfill the ReportSearchIndex search projection for existing rows. + +atomic = False so each chunk commits on its own: at the 8M design target this +runs for roughly ten minutes once the HNSW index is out of the way (below), and +one transaction that long would pin an equally long-lived snapshot. + +The first operation drops the HNSW embedding index for the duration of the +backfill; 0005 rebuilds it from the stored vectors. Every backfilled row is a +full-row rewrite into pages that hold no free space -- a non-HOT update -- so +with the index in place each embedded row would also insert into the HNSW +graph: ~143 ms and ~2,160 buffer touches per row measured on an 8M corpus with +1.7M vectors, which turns the ten-minute window into days. Without embeddings +the drop and the rebuild are both effectively instant. The drop is +database-only (model state keeps declaring the index; 0005 makes it true +again) and IF EXISTS because atomic = False re-runs every operation after a +failure. + +Runs after the triggers (0003) on purpose. A report edited during the backfill +is corrected by its trigger, and a chunk that later reprocesses the same row +simply rewrites the current values. + +The statement below duplicates utils/projection.PROJECTION_UPDATE_SQL, the same +way bulk_upsert_report_search_indexes duplicates the tsvector logic. Migrations +must not import app code, which drifts under them. The two trigger functions in +0004 and check_search_projection.DRIFT_SQL repeat the same array_agg shape -- +keep all four in sync. +""" + +from django.db import migrations, transaction + +CHUNK_SIZE = 50_000 + +# Locked as its own statement before each chunk's UPDATE, so that UPDATE's +# snapshot is taken with the locks already held. See backfill() below and +# utils/projection.sync_projection for why that matters. +LOCK_SQL = """ +SELECT report_id FROM pgsearch_reportsearchindex +WHERE report_id > %s AND report_id <= %s +ORDER BY report_id +FOR UPDATE +""" + +BACKFILL_SQL = """ +UPDATE pgsearch_reportsearchindex rsi +SET group_ids = COALESCE(g.ids, '{}'), + modality_codes = COALESCE(m.codes, '{}'), + language_code = l.code, + patient_sex = r.patient_sex, + patient_age = r.patient_age, + patient_id = r.patient_id, + study_datetime = r.study_datetime, + study_description = r.study_description, + report_created_at = r.created_at, + report_updated_at = r.updated_at +FROM reports_report r +LEFT JOIN reports_language l ON l.id = r.language_id +LEFT JOIN LATERAL ( + SELECT array_agg(rg.group_id ORDER BY rg.group_id) AS ids + FROM reports_report_groups rg WHERE rg.report_id = r.id +) g ON true +LEFT JOIN LATERAL ( + SELECT array_agg(mo.code ORDER BY mo.code) AS codes + FROM reports_report_modalities rm + JOIN reports_modality mo ON mo.id = rm.modality_id + WHERE rm.report_id = r.id +) m ON true +WHERE rsi.report_id = r.id AND rsi.report_id > %s AND rsi.report_id <= %s +""" + + +def backfill(apps, schema_editor): + """Fill the projection in report_id ranges, one transaction per chunk. + + Each chunk locks its rows before updating them. Under READ COMMITTED a + single ``UPDATE ... FROM`` fed by a LATERAL aggregate can re-apply a stale + aggregate to a row a concurrent trigger committed mid-statement + (EvalPlanQual), which on ``group_ids`` would silently restore a group that + was just removed. The documented deploy runs this with the web tier stopped + and therefore has no concurrent writers at all, so this is belt-and-braces + -- it costs one extra pass over each chunk, which is cheap next to the row + rewrite the UPDATE performs anyway, and it removes the need to reason about + whether some other writer really is quiesced. + """ + connection = schema_editor.connection + with connection.cursor() as cursor: + cursor.execute("SELECT COALESCE(MAX(report_id), 0) FROM pgsearch_reportsearchindex") + max_id = cursor.fetchone()[0] + + low = 0 + while low < max_id: + high = low + CHUNK_SIZE + with transaction.atomic(using=connection.alias): + with connection.cursor() as cursor: + cursor.execute(LOCK_SQL, [low, high]) + cursor.execute(BACKFILL_SQL, [low, high]) + low = high + + +class Migration(migrations.Migration): + atomic = False + + dependencies = [("pgsearch", "0003_search_projection")] + + operations = [ + migrations.RunSQL( + "DROP INDEX IF EXISTS pgsearch_embedding_hnsw;", + reverse_sql=migrations.RunSQL.noop, + ), + migrations.RunPython(backfill, migrations.RunPython.noop), + ] diff --git a/radis/pgsearch/migrations/0005_search_projection_indexes.py b/radis/pgsearch/migrations/0005_search_projection_indexes.py new file mode 100644 index 00000000..88955f6c --- /dev/null +++ b/radis/pgsearch/migrations/0005_search_projection_indexes.py @@ -0,0 +1,55 @@ +# Generated by Django 6.0.7 on 2026-08-30 20:20 + +import django.contrib.postgres.indexes +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('pgsearch', '0004_search_projection_backfill'), + ('reports', '0013_alter_report_options'), + ] + + operations = [ + migrations.AddIndex( + model_name='reportsearchindex', + index=django.contrib.postgres.indexes.GinIndex(fields=['group_ids'], name='pgsearch_group_ids_gin'), + ), + migrations.AddIndex( + model_name='reportsearchindex', + index=models.Index(fields=['report_updated_at'], name='pgsearch_report_updated_at_idx'), + ), + migrations.RunSQL( + # fastupdate leaves an unsorted pending list that every query must + # scan; RADIS writes in async batches and reads interactively, so + # predictable read latency is the better side of the trade. + "ALTER INDEX pgsearch_re_search__b0f715_gin SET (fastupdate = off);" + "SELECT gin_clean_pending_list('pgsearch_re_search__b0f715_gin');", + reverse_sql="ALTER INDEX pgsearch_re_search__b0f715_gin SET (fastupdate = on);", + ), + # Recreate the embedding index 0004 dropped, from the stored vectors: + # one bulk build instead of the per-row inserts the backfill would + # otherwise have paid (7m36s versus ~68h measured at 1.7M vectors). + # The build honors the server's maintenance_work_mem and + # max_parallel_maintenance_workers -- raise them for a large embedded + # corpus (POSTGRES_* knobs in the compose files). IF NOT EXISTS keeps + # re-runs and installs that never dropped it safe; the definition must + # match the HnswIndex in ReportSearchIndex.Meta. Reverse is a no-op on + # purpose: unapplying 0005 then 0004 must leave the index present, + # which is the state before 0005 dropped it. + migrations.RunSQL( + "CREATE INDEX IF NOT EXISTS pgsearch_embedding_hnsw " + "ON pgsearch_reportsearchindex " + "USING hnsw (embedding vector_cosine_ops) WITH (m = 16, ef_construction = 64);", + reverse_sql=migrations.RunSQL.noop, + ), + migrations.RunSQL( + # Required, not hygiene: the projection columns and the indexes + # above carry no statistics, and the single-table query shape + # depends on the planner choosing a parallel sequential scan with + # a top-N heapsort. ANALYZE is transaction-legal, so this migration stays atomic. + "ANALYZE pgsearch_reportsearchindex;", + reverse_sql=migrations.RunSQL.noop, + ), + ] diff --git a/radis/pgsearch/models.py b/radis/pgsearch/models.py index b7c2bab0..2fbbe005 100644 --- a/radis/pgsearch/models.py +++ b/radis/pgsearch/models.py @@ -1,4 +1,5 @@ from django.conf import settings +from django.contrib.postgres.fields import ArrayField from django.contrib.postgres.indexes import GinIndex from django.contrib.postgres.search import SearchVector, SearchVectorField from django.db import models @@ -21,6 +22,24 @@ class ReportSearchIndex(models.Model): search_vector = SearchVectorField(null=True) embedding = VectorField(dimensions=settings.EMBEDDINGS_DIM, null=True) + # Search projection: mirrors of the Report fields the scan filters on, so + # the FTS candidate query stays single-table. Maintained by the triggers in + # migration 0003 and populated on creation by signals.py / indexing.py. + # The scalars are nullable and the arrays NOT NULL with a constant default, + # which is what keeps adding a column here metadata-only on a large table. + # Tightening them afterwards would cost a validating scan for no benefit: + # check_search_projection guards against drift. + group_ids = ArrayField(models.IntegerField(), default=list) + modality_codes = ArrayField(models.CharField(max_length=16), default=list) + language_code = models.CharField(max_length=10, null=True) # noqa: DJ001 + patient_sex = models.CharField(max_length=1, null=True) # noqa: DJ001 + patient_age = models.IntegerField(null=True) + patient_id = models.CharField(max_length=64, null=True) # noqa: DJ001 + study_datetime = models.DateTimeField(null=True) + study_description = models.CharField(max_length=64, blank=True, null=True) # noqa: DJ001 + report_created_at = models.DateTimeField(null=True) + report_updated_at = models.DateTimeField(null=True) + class Meta: verbose_name = "Report search index" verbose_name_plural = "Report search indexes" @@ -41,6 +60,8 @@ class Meta: condition=models.Q(embedding__isnull=True), name="pgsearch_pending_embedding_idx", ), + GinIndex(fields=["group_ids"], name="pgsearch_group_ids_gin"), + models.Index(fields=["report_updated_at"], name="pgsearch_report_updated_at_idx"), ] def __str__(self) -> str: diff --git a/radis/pgsearch/providers.py b/radis/pgsearch/providers.py index 22c39ff6..e15497ce 100644 --- a/radis/pgsearch/providers.py +++ b/radis/pgsearch/providers.py @@ -3,6 +3,7 @@ import logging import unicodedata from collections.abc import Iterator +from datetime import date, datetime, time, timedelta from typing import Literal, NamedTuple, cast import openai @@ -10,6 +11,7 @@ from django.contrib.postgres.search import SearchHeadline, SearchQuery, SearchRank from django.core.cache import cache from django.db.models import Case, F, FloatField, Q, TextField, Value, When +from django.utils import timezone from pgvector.django import CosineDistance from radis.core.utils.embedding_client import ( @@ -180,48 +182,58 @@ def _exclude_negations(queryset, node: QueryNode, configs: list[tuple[str, list[ return queryset for config, codes in configs: negative_tsquery = SearchQuery(negative_query_str, search_type="raw", config=config) - queryset = queryset.exclude( - Q(report__language__code__in=codes) & Q(search_vector=negative_tsquery) - ) + queryset = queryset.exclude(Q(language_code__in=codes) & Q(search_vector=negative_tsquery)) return queryset +def _local_day_start(day: date) -> datetime: + """Midnight of ``day`` in the active timezone, as an aware datetime.""" + return timezone.make_aware(datetime.combine(day, time.min)) + + def _build_filter_query(filters: SearchFilters) -> Q: - # Group-scoped access control: a report is only visible to a group when its - # ``groups`` M2M includes that group. This mirrors the report access model - # used everywhere else (e.g. ``Report.objects.filter(groups=active_group)`` - # in radis.reports.views.ReportListView/ReportDetailView and the - # ``groups=active_group`` lookup in radis.chats.views). ``SearchView`` - # supplies ``group=active_group.pk``; the extraction search preview may pass - # ``group=None`` when the user has no active group, which is fail-closed: - # ``Q(report__groups=None)`` becomes ``groups__isnull=True`` and matches only - # reports assigned to no group at all. The filter stays unconditional; without - # it the search would leak reports belonging only to other groups. - fq = Q(report__groups=filters.group) + # Every predicate here is a column on ReportSearchIndex itself -- the search + # projection (see migration 0003) mirrors the Report fields search filters + # on, so the candidate query stays single-table. Reintroducing a ``report__`` + # traversal restores the join and the multi-second query shape it caused; + # test_fts_candidate_query_is_single_table guards against that. + # + # Group-scoped access control. ``SearchView`` supplies + # ``group=active_group.pk``; the extraction preview may pass ``group=None`` + # when the user has no active group, which is fail-closed: an empty + # ``group_ids`` matches only reports assigned to no group at all. + if filters.group is None: + fq = Q(group_ids=[]) + else: + fq = Q(group_ids__contains=[filters.group]) # Apply hard filter criteria if filters.patient_sex: - fq &= Q(report__patient_sex=filters.patient_sex) + fq &= Q(patient_sex=filters.patient_sex) if filters.language: - fq &= Q(report__language__code=filters.language) + fq &= Q(language_code=filters.language) if filters.modalities: - fq &= Q(report__modalities__code__in=filters.modalities) + fq &= Q(modality_codes__overlap=filters.modalities) + # Half-open ranges rather than ``__date``: Django compiles that lookup to + # ``(study_datetime AT TIME ZONE ...)::date``, a function over the column + # evaluated once per row -- millions of timezone conversions per query on + # the sequential scan, for a predicate equal to two timestamp comparisons. if filters.study_date_from: - fq &= Q(report__study_datetime__date__gte=filters.study_date_from) + fq &= Q(study_datetime__gte=_local_day_start(filters.study_date_from)) if filters.study_date_till: - fq &= Q(report__study_datetime__date__lte=filters.study_date_till) + fq &= Q(study_datetime__lt=_local_day_start(filters.study_date_till + timedelta(days=1))) if filters.study_description: - fq &= Q(report__study_description__icontains=filters.study_description) + fq &= Q(study_description__icontains=filters.study_description) if filters.patient_age_from is not None: - fq &= Q(report__patient_age__gte=filters.patient_age_from) + fq &= Q(patient_age__gte=filters.patient_age_from) if filters.patient_age_till is not None: - fq &= Q(report__patient_age__lte=filters.patient_age_till) + fq &= Q(patient_age__lte=filters.patient_age_till) if filters.patient_id: - fq &= Q(report__patient_id=filters.patient_id) + fq &= Q(patient_id=filters.patient_id) if filters.created_after: - fq &= Q(report__created_at__gte=filters.created_after) + fq &= Q(report_created_at__gte=filters.created_after) if filters.created_before: - fq &= Q(report__created_at__lte=filters.created_before) + fq &= Q(report_created_at__lte=filters.created_before) if filters.labels: from radis.labels.models import LabelResult from radis.reports.models import Report @@ -230,9 +242,9 @@ def _build_filter_query(filters: SearchFilters) -> Q: label_results__label__name__in=filters.labels, label_results__value__in=LabelResult.SURFACING_VALUES, ).values("pk") - fq &= Q(report__in=surfacing_report_ids) + fq &= Q(report_id__in=surfacing_report_ids) if filters.updated_after: - fq &= Q(report__updated_at__gte=filters.updated_after) + fq &= Q(report_updated_at__gte=filters.updated_after) return fq @@ -332,26 +344,17 @@ def _embed_query_cached(query_text: str, caller: str) -> list[float] | None: return vec -class _FusedHybrid(NamedTuple): - ordered_ids: list[int] - rrf_score_by_id: dict[int, float] - vec_distance: dict[int, float] - total_relation: Literal["exact", "at_least", "approximately"] - configs: list[tuple[str, list[str]]] - query_str: str - - -def _fuse_hybrid(search: Search, caller: str) -> _FusedHybrid: - """Run both retrievers and fuse them with RRF. +def _fts_candidate_queryset(filter_query: Q, configs: list[tuple[str, list[str]]], query_str: str): + """The FTS candidate query: the single-table scan this whole design exists for. - Shared by search(), retrieve() and count() so all three describe the same - union. The vector half embeds only the positive branches (§7.8 strips - ``NOT``) and enforces the top-level negations on its candidates; the FTS - half consumes the full boolean tsquery. Returns the fused, ordered report - ids plus the per-id scores/distances and the query metadata callers reuse.""" - query_str = _build_query_string(search.query) - configs = _language_configs(search.filters) - filter_query = _build_filter_query(search.filters) + Kept as its own function rather than inlined in ``_fuse_hybrid`` so that + ``test_fts_candidate_query_is_single_table`` can assert on the SQL and the + ``EXPLAIN`` plan of exactly the queryset production runs -- filters, tsquery + match, rank annotation, ordering and bound included, not a re-creation of + it. Everything referenced here must be a column on ``ReportSearchIndex`` + itself: one lookup or annotation traversing the ``report`` relation restores + the joins and the multi-second query shape (see migration 0003). + """ tsqueries = { config: SearchQuery(query_str, search_type="raw", config=config) for config, _ in configs } @@ -359,7 +362,7 @@ def _fuse_hybrid(search: Search, caller: str) -> _FusedHybrid: # A document matches only under the configuration it was indexed with. match_q = Q() for config, codes in configs: - match_q |= Q(report__language__code__in=codes, search_vector=tsqueries[config]) + match_q |= Q(language_code__in=codes, search_vector=tsqueries[config]) # Rank strictly under the document's own configuration. Do not swap this for # Greatest over all configurations: a foreign config's ts_rank is not @@ -368,7 +371,7 @@ def _fuse_hybrid(search: Search, caller: str) -> _FusedHybrid: rank_expr = Case( *[ When( - report__language__code__in=codes, + language_code__in=codes, then=SearchRank(F("search_vector"), tsqueries[config]), ) for config, codes in configs @@ -377,6 +380,38 @@ def _fuse_hybrid(search: Search, caller: str) -> _FusedHybrid: output_field=FloatField(), ) + # No .distinct(): with no joins there is nothing to deduplicate, and it + # would block the top-N heapsort that keeps this query sub-second. + return ( + ReportSearchIndex.objects.filter(filter_query) + .filter(match_q) + .annotate(rank=rank_expr) + .order_by("-rank", "report_id") + .values("report_id", "rank")[: settings.HYBRID_FTS_MAX_RESULTS] + ) + + +class _FusedHybrid(NamedTuple): + ordered_ids: list[int] + rrf_score_by_id: dict[int, float] + vec_distance: dict[int, float] + total_relation: Literal["exact", "at_least", "approximately"] + configs: list[tuple[str, list[str]]] + query_str: str + + +def _fuse_hybrid(search: Search, caller: str) -> _FusedHybrid: + """Run both retrievers and fuse them with RRF. + + Shared by search(), retrieve() and count() so all three describe the same + union. The vector half embeds only the positive branches (§7.8 strips + ``NOT``) and enforces the top-level negations on its candidates; the FTS + half consumes the full boolean tsquery. Returns the fused, ordered report + ids plus the per-id scores/distances and the query metadata callers reuse.""" + query_str = _build_query_string(search.query) + configs = _language_configs(search.filters) + filter_query = _build_filter_query(search.filters) + # Vector side: skipped entirely when no embedding model is configured (FTS-only # deployment), and when stripping NOT branches leaves nothing to embed (see # docs/superpowers/specs/hybrid-search.md §7.8). @@ -391,8 +426,7 @@ def _fuse_hybrid(search: Search, caller: str) -> _FusedHybrid: vec_qs = ReportSearchIndex.objects.filter(filter_query) vec_qs = _exclude_negations(vec_qs, search.query, configs) vec_rows = list( - vec_qs.distinct() - .exclude(embedding__isnull=True) + vec_qs.exclude(embedding__isnull=True) .annotate(distance=CosineDistance("embedding", query_vec)) .order_by("distance", "report_id") .values_list("report_id", "distance")[: settings.HYBRID_VECTOR_TOP_K] @@ -405,16 +439,7 @@ def _fuse_hybrid(search: Search, caller: str) -> _FusedHybrid: # ``configs`` means an empty corpus (Report.language is a required FK), and # an empty match_q would match every row rather than none, so skip it. fts_rows = ( - [] - if not configs - else list( - ReportSearchIndex.objects.filter(filter_query) - .distinct() - .filter(match_q) - .annotate(rank=rank_expr) - .order_by("-rank", "report_id") - .values("report_id", "rank")[: settings.HYBRID_FTS_MAX_RESULTS] - ) + [] if not configs else list(_fts_candidate_queryset(filter_query, configs, query_str)) ) fts_rank = {row["report_id"]: i + 1 for i, row in enumerate(fts_rows)} @@ -476,17 +501,14 @@ def _headline(config: str) -> SearchHeadline: # highlights/scores nothing. A one-branch Case is legal (unlike Greatest), # so this covers the single-configuration case too with no special-casing. summary_expr = Case( - *[ - When(report__language__code__in=codes, then=_headline(config)) - for config, codes in configs - ], + *[When(language_code__in=codes, then=_headline(config)) for config, codes in configs], default=Value(""), output_field=TextField(), ) rank_expr = Case( *[ When( - report__language__code__in=codes, + language_code__in=codes, then=SearchRank(F("search_vector"), tsqueries[config]), ) for config, codes in configs diff --git a/radis/pgsearch/signals.py b/radis/pgsearch/signals.py index dc351fab..449929ce 100644 --- a/radis/pgsearch/signals.py +++ b/radis/pgsearch/signals.py @@ -4,11 +4,21 @@ from radis.reports.models import Report from .models import ReportSearchIndex +from .utils.projection import sync_projection @receiver(post_save, sender=Report) def create_or_update_report_search_index(sender, instance, created, **kwargs): if created: ReportSearchIndex.objects.create(report=instance) + # Groups and modalities are attached after the Report is created, so + # they stay empty here and the migration 0003 triggers fill them. + sync_projection([instance.pk]) return - instance.search_index.save() + + # update_fields is required here, not just an optimization: OneToOneField + # caches the index on the report instance, so instance.search_index below + # is often a stale copy from creation time. A bare save() would write all + # of its fields back, clobbering the projection columns the AFTER UPDATE + # trigger on reports_report set earlier in this transaction. + instance.search_index.save(update_fields=["search_vector"]) diff --git a/radis/pgsearch/tests/test_admin.py b/radis/pgsearch/tests/test_admin.py index 5d277307..8001aad3 100644 --- a/radis/pgsearch/tests/test_admin.py +++ b/radis/pgsearch/tests/test_admin.py @@ -490,3 +490,26 @@ def test_enqueue_pending_embeddings_warns_when_model_not_configured(settings): call = admin_instance.message_user.call_args assert "EMBEDDINGS_MODEL" in call.args[1] assert call.kwargs.get("level") == messages.WARNING + + +def test_change_form_is_read_only(admin_client): + report = ReportFactory.create() + rsi = ReportSearchIndex.objects.get(report=report) + + response = admin_client.get( + reverse("admin:pgsearch_reportsearchindex_change", args=[rsi.pk]) + ) + + assert response.status_code == 200 + content = response.content.decode() + # readonly_fields render as text, not form inputs; group_ids is the one + # that must never be editable (access-control data). + assert 'name="group_ids"' not in content + assert 'name="language_code"' not in content + assert 'name="search_vector"' not in content + + +def test_rows_cannot_be_added_in_the_admin(admin_client): + response = admin_client.get(reverse("admin:pgsearch_reportsearchindex_add")) + + assert response.status_code == 403 diff --git a/radis/pgsearch/tests/test_check_search_projection.py b/radis/pgsearch/tests/test_check_search_projection.py new file mode 100644 index 00000000..5831911b --- /dev/null +++ b/radis/pgsearch/tests/test_check_search_projection.py @@ -0,0 +1,65 @@ +"""The projection duplicates access-control data, so operators need a way to +prove it still matches its sources -- after a restore, a bulk import, or any +writer the triggers still miss (0004's docstring lists them).""" + +from io import StringIO + +import pytest +from adit_radis_shared.accounts.factories import GroupFactory +from django.core.management import call_command +from django.core.management.base import CommandError + +from radis.pgsearch.models import ReportSearchIndex +from radis.reports.factories import LanguageFactory, ModalityFactory, ReportFactory +from radis.reports.models import Language, Modality + +pytestmark = pytest.mark.django_db + + +def test_reports_no_drift_on_a_healthy_corpus(): + report = ReportFactory.create(language=LanguageFactory.create(code="en")) + report.groups.add(GroupFactory.create()) + + out = StringIO() + call_command("check_search_projection", stdout=out) + + assert "Reports without a search index row: 0" in out.getvalue() + + +def test_reports_how_many_reports_have_no_index_row_yet(): + """Informational, not an error: indexing is deferred, so a report can be + legitimately waiting for its index row. The number is what lets an operator + tell "200 pending" from "40,000 permanently stuck".""" + report = ReportFactory.create(language=LanguageFactory.create(code="en")) + report.groups.add(GroupFactory.create()) + ReportSearchIndex.objects.filter(report=report).delete() + + out = StringIO() + call_command("check_search_projection", stdout=out) + + assert "Reports without a search index row: 1" in out.getvalue() + assert "matches its sources" in out.getvalue() + + +def test_detects_drifted_group_ids(): + report = ReportFactory.create(language=LanguageFactory.create(code="en")) + report.groups.add(GroupFactory.create()) + + ReportSearchIndex.objects.filter(report=report).update(group_ids=[9999]) + + with pytest.raises(CommandError, match="group_ids"): + call_command("check_search_projection") + + +def test_reports_no_drift_after_code_renames(): + """The 0003 rename triggers keep the mirrors current through code renames, so the + checker must come back clean afterwards.""" + language = LanguageFactory.create(code="en") + report = ReportFactory.create(language=language, modalities=[]) + modality = ModalityFactory.create(code="CT") + report.modalities.add(modality) + + Language.objects.filter(pk=language.pk).update(code="en-gb") + Modality.objects.filter(pk=modality.pk).update(code="CTA") + + call_command("check_search_projection", stdout=StringIO()) diff --git a/radis/pgsearch/tests/test_filter_query_equivalence.py b/radis/pgsearch/tests/test_filter_query_equivalence.py new file mode 100644 index 00000000..7c1b7ab8 --- /dev/null +++ b/radis/pgsearch/tests/test_filter_query_equivalence.py @@ -0,0 +1,385 @@ +"""The new single-table filter must select exactly what the joined one did. + +_build_filter_query_legacy is the pre-change implementation kept as a reference +oracle. Delete both it and this module once the change has settled in production. +""" + +from datetime import date, datetime, time, timedelta + +import pytest +from adit_radis_shared.accounts.factories import GroupFactory +from django.db.models import Q +from django.test import override_settings +from django.utils import timezone + +from radis.labels.factories import LabelFactory, LabelResultFactory +from radis.labels.models import LabelResult +from radis.pgsearch.models import ReportSearchIndex +from radis.pgsearch.providers import ( + _build_filter_query, + _build_query_string, + _fts_candidate_queryset, + _language_configs, +) +from radis.reports.factories import LanguageFactory, ReportFactory +from radis.search.site import SearchFilters +from radis.search.utils.query_parser import QueryParser + +pytestmark = pytest.mark.django_db + +# A fixed calendar day used for the date-boundary tests below, well away from +# any of the corpus fixture's "N days ago" reports so it can't accidentally +# overlap with those. +BOUNDARY_DAY = date(2026, 3, 10) + + +def _build_filter_query_legacy(filters: SearchFilters) -> Q: + """The joined implementation this change replaces.""" + fq = Q(report__groups=filters.group) + if filters.patient_sex: + fq &= Q(report__patient_sex=filters.patient_sex) + if filters.language: + fq &= Q(report__language__code=filters.language) + if filters.modalities: + fq &= Q(report__modalities__code__in=filters.modalities) + if filters.study_date_from: + fq &= Q(report__study_datetime__date__gte=filters.study_date_from) + if filters.study_date_till: + fq &= Q(report__study_datetime__date__lte=filters.study_date_till) + if filters.study_description: + fq &= Q(report__study_description__icontains=filters.study_description) + if filters.patient_age_from is not None: + fq &= Q(report__patient_age__gte=filters.patient_age_from) + if filters.patient_age_till is not None: + fq &= Q(report__patient_age__lte=filters.patient_age_till) + if filters.patient_id: + fq &= Q(report__patient_id=filters.patient_id) + if filters.created_after: + fq &= Q(report__created_at__gte=filters.created_after) + if filters.created_before: + fq &= Q(report__created_at__lte=filters.created_before) + if filters.labels: + from radis.reports.models import Report + + surfacing_report_ids = Report.objects.filter( + label_results__label__name__in=filters.labels, + label_results__value__in=LabelResult.SURFACING_VALUES, + ).values("pk") + fq &= Q(report__in=surfacing_report_ids) + if filters.updated_after: + fq &= Q(report__updated_at__gte=filters.updated_after) + return fq + + +def _ids(fq: Q) -> set[int]: + return set(ReportSearchIndex.objects.filter(fq).distinct().values_list("report_id", flat=True)) + + +@pytest.fixture +def corpus(): + language_en = LanguageFactory.create(code="en") + language_de = LanguageFactory.create(code="de") + group_a = GroupFactory.create(name="A") + group_b = GroupFactory.create(name="B") + now = timezone.now() + + first = ReportFactory.create( + language=language_en, + patient_sex="M", + patient_id="P1", + study_description="CT Thorax", + study_datetime=now - timedelta(days=10), + modalities=["CT"], + ) + first.groups.add(group_a) + pneumonia = LabelFactory.create(name="pneumonia") + LabelResultFactory.create(report=first, label=pneumonia, value=LabelResult.Value.PRESENT) + + second = ReportFactory.create( + language=language_de, + patient_sex="F", + patient_id="P2", + study_description="MR Head", + study_datetime=now - timedelta(days=400), + modalities=["MR", "CT"], + ) + second.groups.add(group_a, group_b) + + orphan = ReportFactory.create(language=language_en, patient_sex="M", patient_id="P3") + + return {"group_a": group_a, "group_b": group_b, "orphan": orphan, "first": first} + + +def _make_boundary_corpus(): + """Reports pinned to explicit local times around BOUNDARY_DAY. + + "N days ago" filters (as used by the rest of this module) never land on or + near a report's exact date, so they cannot catch an off-by-one at the day + boundary or a UTC-vs-local mistake in ``_local_day_start``. These reports + are placed exactly where such mistakes would show: at local midnight + (the inclusive lower bound), at 23:30 local (deep inside the last day, but + the first casualty of an accidentally inclusive-upper-bound-on-exact-day + mistake), and at local midnight of the day *after* (which must be + excluded). + + A plain helper rather than a fixture: the non-UTC test below must create + these reports *and* run the query while the same overridden ``TIME_ZONE`` + is active, since "local midnight" is only meaningful relative to whatever + timezone is active at the moment ``timezone.make_aware`` runs. A fixture + is resolved before an ``@override_settings``-decorated test body starts, + which would pin the reports to the wrong timezone. + """ + language_en = LanguageFactory.create(code="en") + group = GroupFactory.create(name="Boundary") + + midnight = ReportFactory.create( + language=language_en, + study_datetime=timezone.make_aware(datetime.combine(BOUNDARY_DAY, time.min)), + ) + midnight.groups.add(group) + + late = ReportFactory.create( + language=language_en, + study_datetime=timezone.make_aware(datetime.combine(BOUNDARY_DAY, time(23, 30))), + ) + late.groups.add(group) + + next_midnight = ReportFactory.create( + language=language_en, + study_datetime=timezone.make_aware( + datetime.combine(BOUNDARY_DAY + timedelta(days=1), time.min) + ), + ) + next_midnight.groups.add(group) + + return {"group": group, "midnight": midnight, "late": late, "next_midnight": next_midnight} + + +@pytest.mark.parametrize( + "make_filters", + [ + pytest.param(lambda c: SearchFilters(group=c["group_a"].pk), id="group-only"), + pytest.param(lambda c: SearchFilters(group=c["group_b"].pk), id="other-group"), + pytest.param(lambda c: SearchFilters(group=c["group_a"].pk, language="en"), id="language"), + pytest.param( + lambda c: SearchFilters(group=c["group_a"].pk, modalities=["CT"]), id="modality" + ), + pytest.param( + lambda c: SearchFilters(group=c["group_a"].pk, modalities=["CT", "MR"]), + id="modalities-multi", + ), + pytest.param(lambda c: SearchFilters(group=c["group_a"].pk, patient_sex="M"), id="sex"), + pytest.param( + lambda c: SearchFilters(group=c["group_a"].pk, patient_id="P1"), id="patient-id" + ), + pytest.param( + lambda c: SearchFilters(group=c["group_a"].pk, study_description="thorax"), + id="description-icontains", + ), + pytest.param( + lambda c: SearchFilters( + group=c["group_a"].pk, study_date_from=date.today() - timedelta(days=30) + ), + id="date-from", + ), + pytest.param( + lambda c: SearchFilters( + group=c["group_a"].pk, study_date_till=date.today() - timedelta(days=5) + ), + id="date-till", + ), + pytest.param( + lambda c: SearchFilters( + group=c["group_a"].pk, + study_date_from=date.today() - timedelta(days=30), + study_date_till=date.today(), + ), + id="date-range", + ), + pytest.param( + lambda c: SearchFilters(group=c["group_a"].pk, patient_age_from=0), id="age-from" + ), + pytest.param( + lambda c: SearchFilters(group=c["group_a"].pk, patient_age_till=200), id="age-till" + ), + pytest.param( + lambda c: SearchFilters( + group=c["group_a"].pk, created_after=timezone.now() - timedelta(hours=1) + ), + id="created-after", + ), + pytest.param( + lambda c: SearchFilters( + group=c["group_a"].pk, created_before=timezone.now() + timedelta(hours=1) + ), + id="created-before", + ), + pytest.param( + lambda c: SearchFilters(group=c["group_a"].pk, labels=["pneumonia"]), id="labels" + ), + pytest.param( + lambda c: SearchFilters( + group=c["group_a"].pk, updated_after=timezone.now() - timedelta(hours=1) + ), + id="updated-after", + ), + ], +) +def test_new_filter_matches_the_legacy_one(corpus, make_filters): + filters = make_filters(corpus) + + assert _ids(_build_filter_query(filters)) == _ids(_build_filter_query_legacy(filters)) + + +def test_group_none_is_fail_closed(corpus): + """group=None must match only reports in no group at all -- never everything.""" + result = _ids(_build_filter_query(SearchFilters(group=None))) + + assert result == {corpus["orphan"].pk} + + +def test_no_duplicates_without_distinct(corpus): + """A report matching on two modalities must appear once, which is what the + removed .distinct() used to guarantee.""" + filters = SearchFilters(group=corpus["group_a"].pk, modalities=["CT", "MR"]) + + rows = list( + ReportSearchIndex.objects.filter(_build_filter_query(filters)).values_list( + "report_id", flat=True + ) + ) + + assert len(rows) == len(set(rows)) + + +def test_date_range_boundary_matches_legacy(): + """The half-open range must include local midnight and 23:30 local on the + selected day, and exclude local midnight of the following day. + + This is the exact hazard the brief calls out: an off-by-one + (``__lte=_local_day_start(till)`` instead of + ``__lt=_local_day_start(till + 1 day)``) would silently drop the 23:30 + report here, and an inclusive-instead-of-exclusive upper bound would wrongly + keep next_midnight -- neither is visible from "N days ago" filters alone. + """ + boundary = _make_boundary_corpus() + filters = SearchFilters( + group=boundary["group"].pk, + study_date_from=BOUNDARY_DAY, + study_date_till=BOUNDARY_DAY, + ) + + new_ids = _ids(_build_filter_query(filters)) + legacy_ids = _ids(_build_filter_query_legacy(filters)) + + assert new_ids == legacy_ids + assert new_ids == {boundary["midnight"].pk, boundary["late"].pk} + + +def test_no_report_traversal_remains_in_the_candidate_query(): + """Any report__ lookup re-adds a join and the multi-second query shape.""" + import inspect + + from radis.pgsearch import providers + + source = inspect.getsource(providers) + offending = [ + line.strip() + for line in source.splitlines() + if "report__" in line + and "report__body" not in line # hydration headline, deliberately joined + and "report__document_id" not in line # filter(), deliberately joined + and not line.strip().startswith("#") + ] + + assert offending == [], f"report__ traversals left in the hot path: {offending}" + + +def test_date_range_boundary_matches_legacy_in_non_utc_timezone(): + """Same boundary check under a timezone hours away from UTC. + + A mistake inside ``_local_day_start`` that resolved midnight in UTC rather + than the active timezone would only misplace the boundary here -- under + UTC (the default test timezone) local and UTC midnight coincide and the + bug is invisible. The corpus is built *inside* the override so "local + midnight" is pinned relative to the same timezone the query later uses. + """ + with override_settings(TIME_ZONE="America/New_York"): + boundary = _make_boundary_corpus() + filters = SearchFilters( + group=boundary["group"].pk, + study_date_from=BOUNDARY_DAY, + study_date_till=BOUNDARY_DAY, + ) + + new_ids = _ids(_build_filter_query(filters)) + legacy_ids = _ids(_build_filter_query_legacy(filters)) + + assert new_ids == legacy_ids + assert new_ids == {boundary["midnight"].pk, boundary["late"].pk} + + +def _candidate_queryset(filters: SearchFilters, query: str = "pneumonia"): + """The FTS candidate queryset ``_fuse_hybrid`` actually runs. + + Built by calling the same production helpers in the same order the provider + does, so a regression introduced anywhere along that path -- the filter, the + tsquery match, the rank annotation, the ordering or the bound -- shows up + here. A test that assembled its own queryset from ``_build_filter_query`` + alone could not fail, because the ``.distinct()`` calls this change removed + never lived there. + """ + node, _fixes = QueryParser().parse(query) + assert node is not None + return _fts_candidate_queryset( + _build_filter_query(filters), + _language_configs(filters), + _build_query_string(node), + ) + + +@pytest.mark.parametrize( + "language", + ["en", ""], + ids=["single-config", "every-config"], +) +def test_fts_candidate_query_is_single_table(corpus, language): + """The candidate query must touch no table but the search index, and must + not deduplicate. + + This is the guard against silently restoring the 8-second query shape, so + it asserts on the real candidate queryset rather than on a re-creation of + its filter half. + + The join check inspects the ``EXPLAIN`` plan, which is sound regardless of + corpus size: Django only emits ``reports_report``, ``reports_language`` or + ``reports_report_groups`` into the SQL when a lookup or an annotation + traverses ``report__``, and no planner choice can inject them otherwise. + Both parametrisations matter: without a language filter the match and rank + expressions carry one branch per text-search configuration in the corpus, + which is where a ``report__language__code`` traversal is most tempting. + + The dedup check inspects the compiled SQL text for the ``DISTINCT`` keyword + instead of looking for a physical operator name (e.g. ``Unique``) in the + plan -- the planner is free to implement ``DISTINCT`` as a ``Sort`` + + ``Unique`` or as a ``HashAggregate`` depending on cost estimates, and at + realistic corpus sizes it picks ``HashAggregate``, so a plan-based check + would silently miss a reintroduced ``.distinct()`` at scale even though it + would catch it on this test's tiny fixture. + """ + from django.db import connection + + filters = SearchFilters(group=corpus["group_a"].pk, language=language) + queryset = _candidate_queryset(filters) + sql, params = queryset.query.sql_with_params() + + assert "DISTINCT" not in sql, sql + + with connection.cursor() as cursor: + cursor.execute(f"EXPLAIN {sql}", params) + plan = "\n".join(row[0] for row in cursor.fetchall()) + + # "reports_report" also covers "reports_report_groups" and + # "reports_report_modalities". + for table in ("reports_report", "reports_language", "reports_modality"): + assert table not in plan, plan diff --git a/radis/pgsearch/tests/test_search_projection.py b/radis/pgsearch/tests/test_search_projection.py new file mode 100644 index 00000000..85058b77 --- /dev/null +++ b/radis/pgsearch/tests/test_search_projection.py @@ -0,0 +1,461 @@ +"""Tests for the ReportSearchIndex search projection. + +The projection mirrors the Report fields that search filters on, so the FTS +candidate query can stay single-table. group_ids is access-control data, so +its correctness has its own tests here. +""" + +import importlib +import threading + +import pytest +from adit_radis_shared.accounts.factories import GroupFactory +from django.db import OperationalError, connection, transaction +from django.test.utils import CaptureQueriesContext + +from radis.pgsearch.models import ReportSearchIndex +from radis.pgsearch.utils.indexing import bulk_upsert_report_search_indexes +from radis.pgsearch.utils.projection import sync_projection +from radis.reports.factories import LanguageFactory, ModalityFactory, ReportFactory +from radis.reports.models import Language, Modality, Report + +pytestmark = pytest.mark.django_db + +BACKFILL_MIGRATION = "radis.pgsearch.migrations.0004_search_projection_backfill" + + +class _MinimalSchemaEditor: + """What the migration's backfill() uses of a real schema editor.""" + + def __init__(self, connection): + self.connection = connection + + +def test_new_index_row_defaults_to_empty_arrays(): + # modalities=[] is explicit: ReportFactory attaches a random non-empty set + # by default, and the modality_codes trigger added in this module would + # otherwise correctly mirror it, defeating the point of this test. + report = ReportFactory.create(language=LanguageFactory.create(code="en"), modalities=[]) + index = ReportSearchIndex.objects.get(report=report) + + assert index.group_ids == [] + assert index.modality_codes == [] + + +def test_adding_a_group_updates_the_projection(): + report = ReportFactory.create(language=LanguageFactory.create(code="en")) + group = GroupFactory.create() + + report.groups.add(group) + + index = ReportSearchIndex.objects.get(report=report) + assert index.group_ids == [group.pk] + + +def test_removing_a_group_updates_the_projection(): + """The leak direction: a report removed from a group must stop being visible.""" + report = ReportFactory.create(language=LanguageFactory.create(code="en")) + group = GroupFactory.create() + report.groups.add(group) + + report.groups.remove(group) + + index = ReportSearchIndex.objects.get(report=report) + assert index.group_ids == [] + + +def test_adding_a_modality_updates_the_projection(): + report = ReportFactory.create(language=LanguageFactory.create(code="en"), modalities=[]) + modality = ModalityFactory.create(code="CT") + + report.modalities.add(modality) + + index = ReportSearchIndex.objects.get(report=report) + assert index.modality_codes == ["CT"] + + +def test_removing_a_modality_updates_the_projection(): + report = ReportFactory.create(language=LanguageFactory.create(code="en"), modalities=[]) + modality = ModalityFactory.create(code="CT") + report.modalities.add(modality) + + report.modalities.remove(modality) + + index = ReportSearchIndex.objects.get(report=report) + assert index.modality_codes == [] + + +def test_raw_sql_membership_write_updates_the_projection(): + """The whole reason for triggers over m2m_changed: writers that bypass the ORM.""" + report = ReportFactory.create(language=LanguageFactory.create(code="en")) + group = GroupFactory.create() + + with connection.cursor() as cursor: + cursor.execute( + "INSERT INTO reports_report_groups (report_id, group_id) VALUES (%s, %s)", + [report.pk, group.pk], + ) + + index = ReportSearchIndex.objects.get(report=report) + assert index.group_ids == [group.pk] + + +def test_bulk_membership_insert_updates_every_affected_row(): + """Statement-level triggers fail classically by processing only one transition row. + + This is the shape reports/api/viewsets.py:229 uses for bulk upsert. + """ + language = LanguageFactory.create(code="en") + reports = [ReportFactory.create(language=language) for _ in range(5)] + group = GroupFactory.create() + + through = Report.groups.through + through.objects.bulk_create( + [through(report_id=report.pk, group_id=group.pk) for report in reports] + ) + + for report in reports: + index = ReportSearchIndex.objects.get(report=report) + assert index.group_ids == [group.pk], f"report {report.pk} was not updated" + + +def test_bulk_membership_delete_updates_every_affected_row(): + """The removal direction, in the shape reports/api/viewsets.py:217 uses. + + That endpoint clears a whole batch's memberships with a single + ``filter(report_id__in=...).delete()``, so the DELETE trigger's OLD + transition table carries many rows at once -- and a statement-level trigger + that only processes one of them leaves every other report readable by a + group it was just removed from. Each report keeps a second group so the + assertion pins a recomputed aggregate rather than a blanket reset. + """ + language = LanguageFactory.create(code="en") + reports = [ReportFactory.create(language=language) for _ in range(5)] + removed_group = GroupFactory.create(name="removed") + kept_group = GroupFactory.create(name="kept") + + through = Report.groups.through + through.objects.bulk_create( + [ + through(report_id=report.pk, group_id=group.pk) + for report in reports + for group in (removed_group, kept_group) + ] + ) + for report in reports: + assert ReportSearchIndex.objects.get(report=report).group_ids == sorted( + [removed_group.pk, kept_group.pk] + ), "setup: both groups should be mirrored" + + through.objects.filter( + report_id__in=[report.pk for report in reports], group_id=removed_group.pk + ).delete() + + for report in reports: + index = ReportSearchIndex.objects.get(report=report) + assert index.group_ids == [kept_group.pk], f"report {report.pk} was not updated" + + +def test_deleting_a_report_does_not_error(): + """Deleting a Report cascades to both the membership rows and the index row, + firing the membership trigger against a row that may already be gone. The + spec calls that harmless in either cascade order; this pins it.""" + report = ReportFactory.create(language=LanguageFactory.create(code="en")) + report.groups.add(GroupFactory.create()) + report_pk = report.pk + + report.delete() + + assert not ReportSearchIndex.objects.filter(report_id=report_pk).exists() + + +def test_updating_a_report_updates_the_mirrored_scalars(): + report = ReportFactory.create(language=LanguageFactory.create(code="en")) + + report.patient_id = "CHANGED-123" + report.save() + + index = ReportSearchIndex.objects.get(report=report) + assert index.patient_id == "CHANGED-123" + + +def test_patient_age_is_mirrored(): + """patient_age is a stored generated column. A BEFORE trigger would read NULL + here; this test is what pins the triggers to AFTER.""" + report = ReportFactory.create(language=LanguageFactory.create(code="en")) + + report.patient_id = "TOUCH" + report.save() + + index = ReportSearchIndex.objects.get(report=report) + report.refresh_from_db() + assert index.patient_age == report.patient_age + assert index.patient_age is not None + + +def test_creation_populates_the_mirrored_scalars(): + """Build + explicit save() is a single INSERT with no follow-up UPDATE. + + ReportFactory.create() would not pin this: its post_generation hooks + (modalities, the metadata RelatedFactoryList) make factory_boy issue an + implicit extra save() afterwards, which fires the AFTER UPDATE trigger + from migration 0003 and would populate these scalars regardless of + whether the signal's own sync_projection() call exists. build() skips + those hooks, so the only thing that can fill the scalars here is the + signal's sync_projection() call on creation. + """ + report = ReportFactory.build(language=LanguageFactory.create(code="de")) + report.save() + + index = ReportSearchIndex.objects.get(report=report) + assert index.language_code == "de" + assert index.patient_id == report.patient_id + + +def test_bulk_upsert_populates_the_projection(): + language = LanguageFactory.create(code="en") + report = ReportFactory.create(language=language, modalities=["CT"]) + group = GroupFactory.create() + report.groups.add(group) + ReportSearchIndex.objects.filter(report=report).delete() + + bulk_upsert_report_search_indexes([report.pk]) + + index = ReportSearchIndex.objects.get(report=report) + assert index.language_code == "en" + assert index.group_ids == [group.pk] + assert index.modality_codes == ["CT"] + + +def test_backfill_fills_rows_that_predate_the_projection(): + """Simulates a row written before the projection existed.""" + language = LanguageFactory.create(code="en") + report = ReportFactory.create(language=language, modalities=["MR"]) + group = GroupFactory.create() + report.groups.add(group) + + ReportSearchIndex.objects.filter(report=report).update( + group_ids=[], modality_codes=[], language_code=None, patient_id=None + ) + + sync_projection([report.pk]) + + index = ReportSearchIndex.objects.get(report=report) + assert index.group_ids == [group.pk] + assert index.modality_codes == ["MR"] + assert index.language_code == "en" + assert index.patient_id == report.patient_id + + +def test_migration_backfill_executes_the_chunked_update(monkeypatch): + """Verify the migration's own SQL fills the projection, across several chunks. + + This exercises the literal SQL and the loop in + 0004_search_projection_backfill.py, not sync_projection(). CHUNK_SIZE is + 50,000 in production, so a handful of reports would run the loop body + exactly once and an off-by-one in the ``low``/``high`` arithmetic -- the + kind that silently skips whole id ranges -- would not show. Patching it to + 1 puts each report in its own chunk. + + The reports are given explicit ids 1..3 because the loop always starts at + id 0: with the sequence wherever the rest of the suite left it, a chunk + size of 1 would otherwise run one empty iteration per id below the first + report. + """ + assert not Report.objects.exists(), "this test needs the reports table to itself" + + language = LanguageFactory.create(code="en") + group = GroupFactory.create() + reports = [ + ReportFactory.create(id=report_id, language=language, modalities=["CT"]) + for report_id in (1, 2, 3) + ] + for report in reports: + report.groups.add(group) + + ReportSearchIndex.objects.update( + group_ids=[], + modality_codes=[], + language_code=None, + patient_sex=None, + patient_age=None, + patient_id=None, + study_datetime=None, + study_description=None, + report_created_at=None, + report_updated_at=None, + ) + + migration_module = importlib.import_module(BACKFILL_MIGRATION) + monkeypatch.setattr(migration_module, "CHUNK_SIZE", 1) + + with CaptureQueriesContext(connection) as captured: + migration_module.backfill(None, _MinimalSchemaEditor(connection)) + + statements = [query["sql"] for query in captured.captured_queries] + chunk_updates = [sql for sql in statements if sql.lstrip().startswith("UPDATE")] + assert len(chunk_updates) == 3, statements + + for report in reports: + index = ReportSearchIndex.objects.get(report=report) + assert index.group_ids == [group.pk], f"report {report.pk} was not backfilled" + assert index.modality_codes == ["CT"] + assert index.language_code == "en" + assert index.patient_id == report.patient_id + + +def test_migration_backfill_locks_each_chunk_inside_its_own_transaction(monkeypatch): + """Each chunk locks its rows in a separate statement before updating them. + + Same lost-update hazard as sync_projection() below: an ``UPDATE ... FROM`` + fed by a LATERAL aggregate can re-apply a stale aggregate to a row a + concurrent trigger committed mid-statement. Belt-and-braces here (the + documented deploy runs with the web tier stopped), but the structure is + cheap to pin, and the migration must keep one transaction *per chunk* + rather than one for the whole run. + """ + assert not Report.objects.exists(), "this test needs the reports table to itself" + + language = LanguageFactory.create(code="en") + for report_id in (1, 2): + ReportFactory.create(id=report_id, language=language, modalities=[]) + + migration_module = importlib.import_module(BACKFILL_MIGRATION) + monkeypatch.setattr(migration_module, "CHUNK_SIZE", 1) + + with CaptureQueriesContext(connection) as captured: + migration_module.backfill(None, _MinimalSchemaEditor(connection)) + + statements = [query["sql"].strip() for query in captured.captured_queries] + # The test itself runs inside a transaction, so each chunk's atomic block + # shows up as a savepoint pair rather than as BEGIN/COMMIT. + chunks = [ + statements[index : index + 4] + for index, sql in enumerate(statements) + if sql.startswith("SAVEPOINT") + ] + assert len(chunks) == 2, statements + for chunk in chunks: + assert "FOR UPDATE" in chunk[1], chunk + assert "pgsearch_reportsearchindex" in chunk[1], chunk + assert chunk[2].startswith("UPDATE pgsearch_reportsearchindex"), chunk + assert chunk[3].startswith("RELEASE SAVEPOINT"), chunk + + +def test_sync_projection_locks_the_rows_it_updates_in_one_transaction(): + """The structural half of the lost-update fix. + + Under READ COMMITTED a lone ``UPDATE ... FROM`` whose SET values come from + a LATERAL aggregate can write a stale aggregate onto a row a concurrent + trigger updated after the statement's snapshot was taken (EvalPlanQual + re-check). On group_ids that reverts a group removal, so the fix takes the + row locks in their own statement first and runs both statements in one + transaction. + + What this pins is exactly that structure: two statements, the lock first, + inside a single atomic block. It does not reproduce the race -- see + test_sync_projection_blocks_a_concurrent_trigger_write for the behavioural + half. The test runs inside pytest-django's transaction, so the atomic block + shows up as SAVEPOINT/RELEASE SAVEPOINT; in production (autocommit) the + same block is a real BEGIN/COMMIT. + """ + report = ReportFactory.create(language=LanguageFactory.create(code="en")) + + with CaptureQueriesContext(connection) as captured: + sync_projection([report.pk]) + + statements = [query["sql"].strip() for query in captured.captured_queries] + assert statements[0].startswith("SAVEPOINT"), statements + assert statements[-1].startswith("RELEASE SAVEPOINT"), statements + + body = statements[1:-1] + assert len(body) == 2, body + assert "FOR UPDATE" in body[0] and "pgsearch_reportsearchindex" in body[0], body + assert body[1].startswith("UPDATE pgsearch_reportsearchindex"), body + + +@pytest.mark.django_db(transaction=True) +def test_sync_projection_blocks_a_concurrent_trigger_write(): + """The behavioural half: the locks really do hold off the racing writer. + + A second connection removes the report's group -- the exact write whose + trigger the stale-aggregate race would overwrite -- while sync_projection's + transaction is still open. With a lock_timeout set it must fail rather than + slip through, and it must succeed once the lock is released. + + This proves the locks are taken and held for the whole transaction. It + still does not prove the absence of the race itself: that would need the + trigger to commit at a specific instant inside the UPDATE's execution, + which PostgreSQL gives no hook to arrange. + """ + report = ReportFactory.create(language=LanguageFactory.create(code="en"), modalities=[]) + group = GroupFactory.create() + report.groups.add(group) + + outcome: dict[str, str] = {} + + def remove_the_group(): + try: + with transaction.atomic(): + with connection.cursor() as cursor: + cursor.execute("SET LOCAL lock_timeout = '1s'") + report.groups.remove(group) + outcome["result"] = "committed" + except OperationalError: + outcome["result"] = "blocked" + except Exception as exc: # pragma: no cover - surfaces an unexpected failure + outcome["result"] = f"error: {exc!r}" + finally: + connection.close() + + with transaction.atomic(): + sync_projection([report.pk]) + worker = threading.Thread(target=remove_the_group) + worker.start() + worker.join(timeout=30) + + assert not worker.is_alive(), "the concurrent writer never finished" + assert outcome["result"] == "blocked", outcome + + report.groups.remove(group) + assert ReportSearchIndex.objects.get(report=report).group_ids == [] + + +def test_projection_indexes_exist(): + with connection.cursor() as cursor: + cursor.execute( + "SELECT indexname FROM pg_indexes WHERE tablename = 'pgsearch_reportsearchindex'" + ) + names = {row[0] for row in cursor.fetchall()} + + assert "pgsearch_group_ids_gin" in names + assert "pgsearch_report_updated_at_idx" in names + + +def test_language_code_rename_updates_the_projection(): + language = LanguageFactory.create(code="en") + report = ReportFactory.create(language=language) + + Language.objects.filter(pk=language.pk).update(code="en-gb") + + index = ReportSearchIndex.objects.get(report=report) + assert index.language_code == "en-gb" + + +def test_modality_code_rename_updates_the_projection(): + report = ReportFactory.create(language=LanguageFactory.create(code="en"), modalities=[]) + modality = ModalityFactory.create(code="CT") + report.modalities.add(modality) + + Modality.objects.filter(pk=modality.pk).update(code="CTA") + + index = ReportSearchIndex.objects.get(report=report) + assert index.modality_codes == ["CTA"] + + +def test_hnsw_index_survives_the_backfill_migrations(): + """0004 drops the embedding index and 0005 rebuilds it; the test database + runs both, so the index existing here proves the whole round trip.""" + with connection.cursor() as cursor: + cursor.execute("SELECT 1 FROM pg_indexes WHERE indexname = 'pgsearch_embedding_hnsw'") + assert cursor.fetchone() is not None diff --git a/radis/pgsearch/utils/indexing.py b/radis/pgsearch/utils/indexing.py index c69d4882..a58ea2d8 100644 --- a/radis/pgsearch/utils/indexing.py +++ b/radis/pgsearch/utils/indexing.py @@ -8,6 +8,7 @@ from ..models import ReportSearchIndex from .language_utils import code_to_language +from .projection import sync_projection logger = logging.getLogger(__name__) @@ -70,3 +71,5 @@ def bulk_upsert_report_search_indexes( """, [config, config_ids], ) + + sync_projection(chunk) diff --git a/radis/pgsearch/utils/projection.py b/radis/pgsearch/utils/projection.py new file mode 100644 index 00000000..3c94cf12 --- /dev/null +++ b/radis/pgsearch/utils/projection.py @@ -0,0 +1,79 @@ +"""The canonical SQL that fills the ReportSearchIndex search projection. + +Triggers (migration 0003) maintain the projection when its sources change. +This module fills it when an index row is first created, which happens before +the report's groups and modalities are attached. The backfill migration +duplicates this statement in SQL, the same way bulk_upsert_report_search_indexes +already duplicates the tsvector logic -- keep them in sync. The two trigger +functions in migration 0003 and check_search_projection.DRIFT_SQL repeat the +same array_agg shape and belong to that set as well. +""" + +from collections.abc import Iterable + +from django.db import connection, transaction + +# Taken as its own statement before the UPDATE below, so the UPDATE's snapshot +# is the one taken with these locks already held. See sync_projection(). +PROJECTION_LOCK_SQL = """ +SELECT report_id FROM pgsearch_reportsearchindex +WHERE report_id = ANY(%s) +ORDER BY report_id +FOR UPDATE +""" + +PROJECTION_UPDATE_SQL = """ +UPDATE pgsearch_reportsearchindex rsi +SET group_ids = COALESCE(g.ids, '{}'), + modality_codes = COALESCE(m.codes, '{}'), + language_code = l.code, + patient_sex = r.patient_sex, + patient_age = r.patient_age, + patient_id = r.patient_id, + study_datetime = r.study_datetime, + study_description = r.study_description, + report_created_at = r.created_at, + report_updated_at = r.updated_at +FROM reports_report r +LEFT JOIN reports_language l ON l.id = r.language_id +LEFT JOIN LATERAL ( + SELECT array_agg(rg.group_id ORDER BY rg.group_id) AS ids + FROM reports_report_groups rg WHERE rg.report_id = r.id +) g ON true +LEFT JOIN LATERAL ( + SELECT array_agg(mo.code ORDER BY mo.code) AS codes + FROM reports_report_modalities rm + JOIN reports_modality mo ON mo.id = rm.modality_id + WHERE rm.report_id = r.id +) m ON true +WHERE rsi.report_id = r.id AND rsi.report_id = ANY(%s) +""" + + +def sync_projection(report_ids: Iterable[int]) -> None: + """Fill the projection columns for the given reports from their sources. + + The row locks are taken first, in their own statement, to close a + lost-update race. Under READ COMMITTED a single ``UPDATE ... FROM`` whose + SET values come from a LATERAL aggregate can re-apply a *stale* aggregate: + if a trigger-driven update to a targeted row commits after this statement's + snapshot but before the statement reaches that row, PostgreSQL's + EvalPlanQual re-check re-evaluates the row against the new version and + writes the value computed from the old snapshot. On ``group_ids`` that + silently reverts a group removal -- a report readable by a group it was + deliberately removed from, which is the fail-open direction. + + Each statement in READ COMMITTED takes its own snapshot, so locking first + splits the race in two: a trigger write that would have committed later now + blocks on these locks until this transaction ends, and one that committed + earlier is already visible to the UPDATE's snapshot. ``ORDER BY report_id`` + makes the lock order deterministic so concurrent callers queue rather than + deadlock. + """ + ids = [int(report_id) for report_id in report_ids] + if not ids: + return + with transaction.atomic(): + with connection.cursor() as cursor: + cursor.execute(PROJECTION_LOCK_SQL, [ids]) + cursor.execute(PROJECTION_UPDATE_SQL, [ids])