From 4aa2c1037f012a962200ef85262aa69a3daa7268 Mon Sep 17 00:00:00 2001 From: kirtimanmishrazipstack Date: Mon, 31 Aug 2026 19:56:45 +0530 Subject: [PATCH 1/5] UN-3972 [PERF] Index workflow_file_execution on (status, created_at) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dashboard metrics cron's documents_processed and failed_pages queries filter this table on status + a created_at window, but all four existing indexes lead with workflow_execution_id. With no entry point here the planner drives top-down from the org and sequentially scans all 1.28M rows of workflow_execution — 83% of the cron's DB time on production. Built CONCURRENTLY with atomic = False; a plain AddIndex would hold a SHARE lock over a 3.4GB table taking live inserts. Guarded against a leftover INVALID index from an interrupted build, which IF NOT EXISTS would otherwise keep silently. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc --- .../migrations/0007_wfe_status_created_idx.py | 105 ++++++++++++++++++ .../workflow_manager/file_execution/models.py | 8 ++ 2 files changed, 113 insertions(+) create mode 100644 backend/workflow_manager/file_execution/migrations/0007_wfe_status_created_idx.py diff --git a/backend/workflow_manager/file_execution/migrations/0007_wfe_status_created_idx.py b/backend/workflow_manager/file_execution/migrations/0007_wfe_status_created_idx.py new file mode 100644 index 0000000000..55d2a6919c --- /dev/null +++ b/backend/workflow_manager/file_execution/migrations/0007_wfe_status_created_idx.py @@ -0,0 +1,105 @@ +"""Add a (status, created_at) index to workflow_file_execution. + +The dashboard metrics cron asks "how many files reached status X in the last N days +for this org?". All four existing indexes on this table lead with +``workflow_execution_id``, so the planner has no way to drive from here and instead +goes top-down from the org: 600 workflows -> a sequential scan of all 1.28M rows of +``workflow_execution`` -> an index lookup per run. Measured on production 2026-08-31, +that shape costs 870ms (documents_processed) and 809ms (failed_pages) per call, 83% +of the cron's total DB time. Analysis in UN-3883. + +This index offers the other direction: start from this table, then join *up* to +workflow_execution and workflow. ``status = 'ERROR'`` is 0.4% of rows so failed_pages +becomes selective immediately; ``status = 'COMPLETED'`` is 97.6%, so +documents_processed only benefits once UN-3973 narrows its window. + +Design +------ +* CONCURRENTLY + ``atomic = False`` — a plain ``AddIndex`` holds a SHARE lock for the + whole build and would block every write to a 3.4GB table taking live inserts, + stalling file processing. +* INVALID-INDEX GUARD — ``IF NOT EXISTS`` silently no-ops over a leftover INVALID index + from an interrupted CONCURRENTLY build, and Django would then record this migration + as applied while the index is physically unusable (never read, write overhead only). + The second statement RAISEs in that case, so the failure is loud rather than + green-but-broken. + +Pattern follows ``workflow_v2/migrations/0026_workflowexecution_undispatched_idx.py``. + +Deployment +---------- +``CREATE INDEX CONCURRENTLY`` scans the table and can run for minutes at this size — +long enough to time out a deploy's ``migrate`` step. Prefer building it OUT OF BAND +*before* the deploy; the migration then no-ops via ``IF NOT EXISTS``:: + + CREATE INDEX CONCURRENTLY IF NOT EXISTS wfe_status_created_idx + ON workflow_file_execution (status, created_at); + +Then confirm it is valid and that the planner actually picks it up:: + + SELECT c.relname, i.indisvalid FROM pg_class c + JOIN pg_index i ON i.indexrelid = c.oid + WHERE c.relname = 'wfe_status_created_idx'; + -- indisvalid must be 't' + +Recovery +-------- +An interrupted build leaves an INVALID index that adds write overhead but is never +read. ``IF NOT EXISTS`` will NOT rebuild over it (and the guard below RAISEs on it), +so drop it first and re-run:: + + DROP INDEX CONCURRENTLY IF EXISTS wfe_status_created_idx; +""" + +from django.db import migrations, models + +INDEX_NAME = "wfe_status_created_idx" + +_ASSERT_INDEX_VALID = f""" +DO $$ +BEGIN + IF EXISTS ( + SELECT 1 FROM pg_class c + JOIN pg_index i ON i.indexrelid = c.oid + WHERE c.relname = '{INDEX_NAME}' AND NOT i.indisvalid + ) THEN + RAISE EXCEPTION 'Index {INDEX_NAME} exists but is INVALID (a prior CREATE INDEX CONCURRENTLY was interrupted). Drop it and re-run: DROP INDEX CONCURRENTLY IF EXISTS {INDEX_NAME};'; + END IF; +END +$$; +""" + + +class Migration(migrations.Migration): + # CREATE / DROP INDEX CONCURRENTLY cannot run inside a transaction block. + atomic = False + + dependencies = [ + ( + "file_execution", + "0006_workflowfileexecution_wf_file_hash_path_status_idx_and_more", + ), + ] + + operations = [ + migrations.SeparateDatabaseAndState( + database_operations=[ + migrations.RunSQL( + sql=( + f"CREATE INDEX CONCURRENTLY IF NOT EXISTS {INDEX_NAME} " + "ON workflow_file_execution (status, created_at);" + ), + reverse_sql=f"DROP INDEX CONCURRENTLY IF EXISTS {INDEX_NAME};", + ), + migrations.RunSQL( + sql=_ASSERT_INDEX_VALID, reverse_sql=migrations.RunSQL.noop + ), + ], + state_operations=[ + migrations.AddIndex( + model_name="workflowfileexecution", + index=models.Index(fields=["status", "created_at"], name=INDEX_NAME), + ), + ], + ), + ] diff --git a/backend/workflow_manager/file_execution/models.py b/backend/workflow_manager/file_execution/models.py index 105b3875a9..c0c7806eb0 100644 --- a/backend/workflow_manager/file_execution/models.py +++ b/backend/workflow_manager/file_execution/models.py @@ -198,6 +198,14 @@ class Meta: ], name="wf_provider_uuid_path_stat_idx", ), + # Every index above leads with workflow_execution, so the dashboard + # metrics cron's "files in status X within a date window" filter has no + # entry point here and the planner drives from workflow_execution + # instead — a full scan of it. See migration 0007. + models.Index( + fields=["status", "created_at"], + name="wfe_status_created_idx", + ), ] constraints = [ models.UniqueConstraint( From 6d21dc52b1cdf51f886ea637238de8fd75e718ed Mon Sep 17 00:00:00 2001 From: kirtimanmishrazipstack Date: Mon, 31 Aug 2026 20:30:01 +0530 Subject: [PATCH 2/5] UN-3972 [PERF] Trim the migration docstring to the project ceiling The docstring restated the prod plan, deployment runbook and recovery steps. That detail belongs in the PR, not in a file every future agent scans. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc --- .../migrations/0007_wfe_status_created_idx.py | 57 +++---------------- .../workflow_manager/file_execution/models.py | 6 +- 2 files changed, 11 insertions(+), 52 deletions(-) diff --git a/backend/workflow_manager/file_execution/migrations/0007_wfe_status_created_idx.py b/backend/workflow_manager/file_execution/migrations/0007_wfe_status_created_idx.py index 55d2a6919c..219cd0d194 100644 --- a/backend/workflow_manager/file_execution/migrations/0007_wfe_status_created_idx.py +++ b/backend/workflow_manager/file_execution/migrations/0007_wfe_status_created_idx.py @@ -1,60 +1,21 @@ """Add a (status, created_at) index to workflow_file_execution. -The dashboard metrics cron asks "how many files reached status X in the last N days -for this org?". All four existing indexes on this table lead with -``workflow_execution_id``, so the planner has no way to drive from here and instead -goes top-down from the org: 600 workflows -> a sequential scan of all 1.28M rows of -``workflow_execution`` -> an index lookup per run. Measured on production 2026-08-31, -that shape costs 870ms (documents_processed) and 809ms (failed_pages) per call, 83% -of the cron's total DB time. Analysis in UN-3883. +The dashboard metrics cron filters this table on status and a created_at window. +Every existing index leads with workflow_execution_id, so the planner cannot drive +from here and scans workflow_execution in full instead. Measurements in UN-3883. -This index offers the other direction: start from this table, then join *up* to -workflow_execution and workflow. ``status = 'ERROR'`` is 0.4% of rows so failed_pages -becomes selective immediately; ``status = 'COMPLETED'`` is 97.6%, so -documents_processed only benefits once UN-3973 narrows its window. - -Design ------- -* CONCURRENTLY + ``atomic = False`` — a plain ``AddIndex`` holds a SHARE lock for the - whole build and would block every write to a 3.4GB table taking live inserts, - stalling file processing. -* INVALID-INDEX GUARD — ``IF NOT EXISTS`` silently no-ops over a leftover INVALID index - from an interrupted CONCURRENTLY build, and Django would then record this migration - as applied while the index is physically unusable (never read, write overhead only). - The second statement RAISEs in that case, so the failure is loud rather than - green-but-broken. - -Pattern follows ``workflow_v2/migrations/0026_workflowexecution_undispatched_idx.py``. - -Deployment ----------- -``CREATE INDEX CONCURRENTLY`` scans the table and can run for minutes at this size — -long enough to time out a deploy's ``migrate`` step. Prefer building it OUT OF BAND -*before* the deploy; the migration then no-ops via ``IF NOT EXISTS``:: - - CREATE INDEX CONCURRENTLY IF NOT EXISTS wfe_status_created_idx - ON workflow_file_execution (status, created_at); - -Then confirm it is valid and that the planner actually picks it up:: - - SELECT c.relname, i.indisvalid FROM pg_class c - JOIN pg_index i ON i.indexrelid = c.oid - WHERE c.relname = 'wfe_status_created_idx'; - -- indisvalid must be 't' - -Recovery --------- -An interrupted build leaves an INVALID index that adds write overhead but is never -read. ``IF NOT EXISTS`` will NOT rebuild over it (and the guard below RAISEs on it), -so drop it first and re-run:: - - DROP INDEX CONCURRENTLY IF EXISTS wfe_status_created_idx; +Built CONCURRENTLY (atomic = False): a plain AddIndex holds a SHARE lock for the +whole build and would block writes to a large, write-heavy table. Prefer building +it out of band before the deploy; the migration then no-ops via IF NOT EXISTS. """ from django.db import migrations, models INDEX_NAME = "wfe_status_created_idx" +# An interrupted CONCURRENTLY build leaves an INVALID index that costs on every +# write and is never read. IF NOT EXISTS would keep it while Django recorded the +# migration as applied, so fail loudly instead. _ASSERT_INDEX_VALID = f""" DO $$ BEGIN diff --git a/backend/workflow_manager/file_execution/models.py b/backend/workflow_manager/file_execution/models.py index c0c7806eb0..1ba1b0a77a 100644 --- a/backend/workflow_manager/file_execution/models.py +++ b/backend/workflow_manager/file_execution/models.py @@ -198,10 +198,8 @@ class Meta: ], name="wf_provider_uuid_path_stat_idx", ), - # Every index above leads with workflow_execution, so the dashboard - # metrics cron's "files in status X within a date window" filter has no - # entry point here and the planner drives from workflow_execution - # instead — a full scan of it. See migration 0007. + # Serves the dashboard metrics cron's status + date-window filter; every + # index above leads with workflow_execution. See migration 0007. models.Index( fields=["status", "created_at"], name="wfe_status_created_idx", From 539ac204a947eb1386fe8fa4af88eb79a56415ec Mon Sep 17 00:00:00 2001 From: kirtimanmishrazipstack Date: Tue, 1 Sep 2026 11:26:12 +0530 Subject: [PATCH 3/5] UN-3972 [PERF] Guard the index migration's non-atomic CONCURRENTLY shape with tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The suite runs with --no-migrations, so 0007 is never executed in CI. Regenerating it with makemigrations, or dropping atomic = False / CONCURRENTLY while tidying, would land a plain AddIndex — a SHARE lock held for the whole build on a 3.4 GB table that takes live inserts — with every test still green. Five DB-free assertions on the migration module and the model's Meta.indexes: non-atomic, concurrent in both directions, the INVALID-index guard present, AddIndex confined to state_operations, and model/migration agreement. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc --- .../file_execution/tests/__init__.py | 0 .../tests/test_wfe_status_created_idx.py | 83 +++++++++++++++++++ 2 files changed, 83 insertions(+) create mode 100644 backend/workflow_manager/file_execution/tests/__init__.py create mode 100644 backend/workflow_manager/file_execution/tests/test_wfe_status_created_idx.py diff --git a/backend/workflow_manager/file_execution/tests/__init__.py b/backend/workflow_manager/file_execution/tests/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/backend/workflow_manager/file_execution/tests/test_wfe_status_created_idx.py b/backend/workflow_manager/file_execution/tests/test_wfe_status_created_idx.py new file mode 100644 index 0000000000..9a22ae3687 --- /dev/null +++ b/backend/workflow_manager/file_execution/tests/test_wfe_status_created_idx.py @@ -0,0 +1,83 @@ +"""Shape guard for the ``(status, created_at)`` index migration (UN-3972). + +``workflow_file_execution`` is ~3.4 GB in production and takes live inserts. A plain +``AddIndex`` — which is what ``makemigrations`` emits from ``Meta.indexes`` — holds a +``SHARE`` lock for the whole build and stalls file processing. ``0007`` is therefore +hand-written: non-atomic, ``CONCURRENTLY``, and split so the ``AddIndex`` updates model +state only. + +Nothing else guards that. The suite runs with ``--no-migrations``, so this migration is +never executed in CI; regenerating or "tidying" it would land the locking version with +every test still green. These assertions are what fails instead. + +DB-free by design: the migration module is imported and inspected directly. +""" + +from __future__ import annotations + +import importlib + +from django.db import migrations +from django.test import SimpleTestCase + +from workflow_manager.file_execution.models import WorkflowFileExecution + +_MIGRATION = "workflow_manager.file_execution.migrations.0007_wfe_status_created_idx" + +INDEX_NAME = "wfe_status_created_idx" +INDEX_FIELDS = ["status", "created_at"] +TABLE = "workflow_file_execution" + + +class MigrationShapeTests(SimpleTestCase): + """The properties that keep the build off the write path.""" + + @classmethod + def setUpClass(cls) -> None: + super().setUpClass() + cls.migration = importlib.import_module(_MIGRATION).Migration + cls.operation = cls.migration.operations[0] + + def test_migration_is_non_atomic(self) -> None: + """CREATE/DROP INDEX CONCURRENTLY is rejected inside a transaction block.""" + self.assertIs(self.migration.atomic, False) + + def test_index_is_built_and_dropped_concurrently(self) -> None: + """Both directions must stay off the write-blocking lock path.""" + create = self.operation.database_operations[0] + self.assertIn("CREATE INDEX CONCURRENTLY IF NOT EXISTS", create.sql) + self.assertIn(INDEX_NAME, create.sql) + self.assertIn(f"{TABLE} ({', '.join(INDEX_FIELDS)})", create.sql) + self.assertIn("DROP INDEX CONCURRENTLY IF EXISTS", create.reverse_sql) + + def test_invalid_index_guard_is_present(self) -> None: + """An interrupted concurrent build leaves an INVALID index. + + ``IF NOT EXISTS`` would keep it and let Django record the migration as applied — + green, but the index costs on every write and is never read. The guard turns that + into a loud failure. + """ + guard = self.operation.database_operations[1].sql + self.assertIn("indisvalid", guard) + self.assertIn("RAISE EXCEPTION", guard) + self.assertIn(f"DROP INDEX CONCURRENTLY IF EXISTS {INDEX_NAME}", guard) + + def test_add_index_updates_state_only(self) -> None: + """``AddIndex`` must not reach the database, or it builds a second time.""" + self.assertIsInstance(self.operation, migrations.SeparateDatabaseAndState) + self.assertTrue( + all( + isinstance(op, migrations.RunSQL) + for op in self.operation.database_operations + ) + ) + self.assertEqual(len(self.operation.state_operations), 1) + state_op = self.operation.state_operations[0] + self.assertIsInstance(state_op, migrations.AddIndex) + self.assertEqual(state_op.index.name, INDEX_NAME) + self.assertEqual(state_op.index.fields, INDEX_FIELDS) + + def test_model_meta_matches_the_migration(self) -> None: + """Model state and migration state drift silently otherwise.""" + declared = {idx.name: idx.fields for idx in WorkflowFileExecution._meta.indexes} + self.assertEqual(declared.get(INDEX_NAME), INDEX_FIELDS) From 1628a16814fd0f805c3224b12f2bedd571c458e0 Mon Sep 17 00:00:00 2001 From: kirtimanmishrazipstack Date: Wed, 2 Sep 2026 16:48:51 +0530 Subject: [PATCH 4/5] UN-3972 [FIX] Assert the index definition, not just its validity, and pin reversibility MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CREATE INDEX CONCURRENTLY IF NOT EXISTS matches on name alone, so a hand-built index with different columns was kept while Django recorded (status, created_at) into model state — a permanent, invisible divergence that makemigrations --check cannot see. The guard now compares pg_get_indexdef against the expected btree definition and qualifies the lookup by current_schema(), since app tables live in the unstract schema. Also pin that every database_operation is reversible: dropping the guard's reverse_sql=noop killed the whole rollback path with all five tests still green. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc --- .../migrations/0007_wfe_status_created_idx.py | 39 +++++++++++++------ .../tests/test_wfe_status_created_idx.py | 20 +++++++--- 2 files changed, 43 insertions(+), 16 deletions(-) diff --git a/backend/workflow_manager/file_execution/migrations/0007_wfe_status_created_idx.py b/backend/workflow_manager/file_execution/migrations/0007_wfe_status_created_idx.py index 219cd0d194..f292201527 100644 --- a/backend/workflow_manager/file_execution/migrations/0007_wfe_status_created_idx.py +++ b/backend/workflow_manager/file_execution/migrations/0007_wfe_status_created_idx.py @@ -6,26 +6,43 @@ Built CONCURRENTLY (atomic = False): a plain AddIndex holds a SHARE lock for the whole build and would block writes to a large, write-heavy table. Prefer building -it out of band before the deploy; the migration then no-ops via IF NOT EXISTS. +it out of band before the deploy; the migration then no-ops via IF NOT EXISTS and +asserts the existing index is valid and has the expected definition. """ from django.db import migrations, models INDEX_NAME = "wfe_status_created_idx" -# An interrupted CONCURRENTLY build leaves an INVALID index that costs on every -# write and is never read. IF NOT EXISTS would keep it while Django recorded the -# migration as applied, so fail loudly instead. -_ASSERT_INDEX_VALID = f""" +INDEX_DEF_SUFFIX = "USING btree (status, created_at)" + +# IF NOT EXISTS matches on name alone, so a hand-built index with different columns +# would be kept while Django recorded (status, created_at) into model state. An +# interrupted CONCURRENTLY build likewise leaves an INVALID index that costs on every +# write and is never read. Fail loudly on both rather than diverge silently. +_ASSERT_INDEX_MATCHES = f""" DO $$ +DECLARE + idx_def text; + idx_valid boolean; BEGIN - IF EXISTS ( - SELECT 1 FROM pg_class c - JOIN pg_index i ON i.indexrelid = c.oid - WHERE c.relname = '{INDEX_NAME}' AND NOT i.indisvalid - ) THEN + SELECT pg_get_indexdef(i.indexrelid), i.indisvalid INTO idx_def, idx_valid + FROM pg_class c + JOIN pg_namespace n ON n.oid = c.relnamespace + JOIN pg_index i ON i.indexrelid = c.oid + WHERE c.relname = '{INDEX_NAME}' AND n.nspname = current_schema(); + + IF idx_def IS NULL THEN + RAISE EXCEPTION 'Index {INDEX_NAME} is missing from schema % after CREATE INDEX.', current_schema(); + END IF; + + IF NOT idx_valid THEN RAISE EXCEPTION 'Index {INDEX_NAME} exists but is INVALID (a prior CREATE INDEX CONCURRENTLY was interrupted). Drop it and re-run: DROP INDEX CONCURRENTLY IF EXISTS {INDEX_NAME};'; END IF; + + IF idx_def NOT LIKE '%{INDEX_DEF_SUFFIX}' THEN + RAISE EXCEPTION 'Index {INDEX_NAME} exists with an unexpected definition (%), expected {INDEX_DEF_SUFFIX}. Drop it and re-run: DROP INDEX CONCURRENTLY IF EXISTS {INDEX_NAME};', idx_def; + END IF; END $$; """ @@ -53,7 +70,7 @@ class Migration(migrations.Migration): reverse_sql=f"DROP INDEX CONCURRENTLY IF EXISTS {INDEX_NAME};", ), migrations.RunSQL( - sql=_ASSERT_INDEX_VALID, reverse_sql=migrations.RunSQL.noop + sql=_ASSERT_INDEX_MATCHES, reverse_sql=migrations.RunSQL.noop ), ], state_operations=[ diff --git a/backend/workflow_manager/file_execution/tests/test_wfe_status_created_idx.py b/backend/workflow_manager/file_execution/tests/test_wfe_status_created_idx.py index 9a22ae3687..35896ddf1c 100644 --- a/backend/workflow_manager/file_execution/tests/test_wfe_status_created_idx.py +++ b/backend/workflow_manager/file_execution/tests/test_wfe_status_created_idx.py @@ -50,15 +50,25 @@ def test_index_is_built_and_dropped_concurrently(self) -> None: self.assertIn(f"{TABLE} ({', '.join(INDEX_FIELDS)})", create.sql) self.assertIn("DROP INDEX CONCURRENTLY IF EXISTS", create.reverse_sql) - def test_invalid_index_guard_is_present(self) -> None: - """An interrupted concurrent build leaves an INVALID index. + def test_every_database_operation_is_reversible(self) -> None: + """One irreversible operation kills the whole rollback, DROP INDEX included.""" + self.assertTrue( + all(op.reversible for op in self.operation.database_operations) + ) + + def test_pre_existing_index_guard_is_present(self) -> None: + """``IF NOT EXISTS`` matches on name alone, so the guard carries the rest. - ``IF NOT EXISTS`` would keep it and let Django record the migration as applied — - green, but the index costs on every write and is never read. The guard turns that - into a loud failure. + An interrupted concurrent build leaves an INVALID index, and a hand-built one + may have different columns; either would be kept while Django recorded the + migration as applied. The guard turns both into a loud failure, and looks the + index up in ``current_schema()`` because app tables do not live in ``public``. """ guard = self.operation.database_operations[1].sql self.assertIn("indisvalid", guard) + self.assertIn("pg_get_indexdef", guard) + self.assertIn(f"USING btree ({', '.join(INDEX_FIELDS)})", guard) + self.assertIn("n.nspname = current_schema()", guard) self.assertIn("RAISE EXCEPTION", guard) self.assertIn(f"DROP INDEX CONCURRENTLY IF EXISTS {INDEX_NAME}", guard) From b9f340601bb02887d8fd65410ca9e61c741991ab Mon Sep 17 00:00:00 2001 From: kirtimanmishrazipstack Date: Wed, 2 Sep 2026 17:26:41 +0530 Subject: [PATCH 5/5] UN-3972 [FIX] Address Athul's review: guard semantics, whole-migration assertions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three mutations that were green are now caught: appending a bare AddIndex after the SeparateDatabaseAndState (a real lock-taking build on a 3.4 GB table, invisible because every assertion read operations[0]); flipping the guard's NOT indisvalid polarity, which either raises on every healthy deploy or never fires at all; and a typo in reverse_sql, which makes rollback a silent no-op through IF EXISTS while Django unapplies the migration. The CREATE assertion matches the column order by regex instead of an exact byte sequence — removing one space used to fail it, a false-failure mode whose only outcome is someone loosening the assertion. Docstrings: the plan citation now points at UN-4045, which supersedes the earlier workflow_file_execution reading; "every existing index leads with workflow_execution_id" was false (the PK leads with id); the exact CREATE statement an operator should run out of band is spelled out, with a warning off the struck two-index variant; and the models.py comment no longer implies the index fixes both cron queries when it fixes one until UN-3973 narrows the window. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc --- .../migrations/0007_wfe_status_created_idx.py | 30 +++++++++++----- .../workflow_manager/file_execution/models.py | 6 ++-- .../tests/test_wfe_status_created_idx.py | 34 +++++++++++++++++-- 3 files changed, 57 insertions(+), 13 deletions(-) diff --git a/backend/workflow_manager/file_execution/migrations/0007_wfe_status_created_idx.py b/backend/workflow_manager/file_execution/migrations/0007_wfe_status_created_idx.py index f292201527..06746771fd 100644 --- a/backend/workflow_manager/file_execution/migrations/0007_wfe_status_created_idx.py +++ b/backend/workflow_manager/file_execution/migrations/0007_wfe_status_created_idx.py @@ -1,13 +1,27 @@ """Add a (status, created_at) index to workflow_file_execution. -The dashboard metrics cron filters this table on status and a created_at window. -Every existing index leads with workflow_execution_id, so the planner cannot drive -from here and scans workflow_execution in full instead. Measurements in UN-3883. - -Built CONCURRENTLY (atomic = False): a plain AddIndex holds a SHARE lock for the -whole build and would block writes to a large, write-heavy table. Prefer building -it out of band before the deploy; the migration then no-ops via IF NOT EXISTS and -asserts the existing index is valid and has the expected definition. +The dashboard metrics cron filters this table on status and a created_at window. No +existing index leads with status or created_at — every secondary index is prefixed by +the workflow_execution FK column — so the planner cannot drive from here and scans +workflow_execution in full instead. Execution plan in UN-4045 (2026-08-31, which +supersedes the earlier workflow_file_execution reading); cost measurements in UN-3883. + +Full rather than partial: get_failed_pages benefits at today's window (ERROR is 0.40% +of rows), and get_documents_processed only once UN-3973 narrows the window to 2 days — +at 52 days the COMPLETED slice is 20.4% of the table and the planner scans regardless. +A partial index on ERROR would serve the first and never the second. + +Built CONCURRENTLY (atomic = False): a plain AddIndex holds a SHARE lock for the whole +build and would block writes to a large, write-heavy table. Prefer building it out of +band before the deploy, exactly this statement and no other: + + CREATE INDEX CONCURRENTLY IF NOT EXISTS wfe_status_created_idx + ON workflow_file_execution (status, created_at); + +The migration then no-ops via IF NOT EXISTS and asserts the existing index is valid +and has the expected definition. Do not build the two-index variant from an older +revision of UN-3972's description: wfe_created_at_desc_idx was struck as worse than +nothing. """ from django.db import migrations, models diff --git a/backend/workflow_manager/file_execution/models.py b/backend/workflow_manager/file_execution/models.py index 1ba1b0a77a..9578f8b1be 100644 --- a/backend/workflow_manager/file_execution/models.py +++ b/backend/workflow_manager/file_execution/models.py @@ -198,8 +198,10 @@ class Meta: ], name="wf_provider_uuid_path_stat_idx", ), - # Serves the dashboard metrics cron's status + date-window filter; every - # index above leads with workflow_execution. See migration 0007. + # Every other index on this table is prefixed by the workflow_execution + # FK column, so none can serve a status + created_at filter. Effective for + # get_failed_pages today; the COMPLETED path stays inert until UN-3973 + # narrows the window. See migration 0007. models.Index( fields=["status", "created_at"], name="wfe_status_created_idx", diff --git a/backend/workflow_manager/file_execution/tests/test_wfe_status_created_idx.py b/backend/workflow_manager/file_execution/tests/test_wfe_status_created_idx.py index 35896ddf1c..47709bce1b 100644 --- a/backend/workflow_manager/file_execution/tests/test_wfe_status_created_idx.py +++ b/backend/workflow_manager/file_execution/tests/test_wfe_status_created_idx.py @@ -38,6 +38,18 @@ def setUpClass(cls) -> None: cls.migration = importlib.import_module(_MIGRATION).Migration cls.operation = cls.migration.operations[0] + def test_the_migration_has_exactly_one_operation(self) -> None: + """Every other assertion reads operations[0], so anything appended after the + SeparateDatabaseAndState is invisible — including a bare AddIndex, which is a + real lock-taking build on a 3.4 GB table. + """ + self.assertEqual(len(self.migration.operations), 1) + + def test_no_operation_builds_an_index_against_the_database(self) -> None: + """The same failure stated directly, so it survives the count changing.""" + for op in self.migration.operations: + self.assertNotIsInstance(op, migrations.AddIndex) + def test_migration_is_non_atomic(self) -> None: """CREATE/DROP INDEX CONCURRENTLY is rejected inside a transaction block.""" self.assertIs(self.migration.atomic, False) @@ -47,8 +59,17 @@ def test_index_is_built_and_dropped_concurrently(self) -> None: create = self.operation.database_operations[0] self.assertIn("CREATE INDEX CONCURRENTLY IF NOT EXISTS", create.sql) self.assertIn(INDEX_NAME, create.sql) - self.assertIn(f"{TABLE} ({', '.join(INDEX_FIELDS)})", create.sql) + # Column order is the point — (created_at, status) cannot serve an equality + # plus range filter. Whitespace and case are not, and a red test on + # semantically identical SQL only teaches people to loosen the assertion. + self.assertRegex( + create.sql, + rf"ON\s+{TABLE}\s*\(\s*{INDEX_FIELDS[0]}\s*,\s*{INDEX_FIELDS[1]}\s*\)", + ) self.assertIn("DROP INDEX CONCURRENTLY IF EXISTS", create.reverse_sql) + # A typo here makes rollback a silent no-op through IF EXISTS: Django unapplies + # the migration while the index stays on the table. + self.assertIn(INDEX_NAME, create.reverse_sql) def test_every_database_operation_is_reversible(self) -> None: """One irreversible operation kills the whole rollback, DROP INDEX included.""" @@ -65,10 +86,17 @@ def test_pre_existing_index_guard_is_present(self) -> None: index up in ``current_schema()`` because app tables do not live in ``public``. """ guard = self.operation.database_operations[1].sql - self.assertIn("indisvalid", guard) + # Polarity, not presence: `NOT idx_valid` raises on a broken index, `idx_valid` + # raises on every healthy deploy, and both contain "indisvalid". + self.assertIn("i.indisvalid", guard) + self.assertIn("IF NOT idx_valid THEN", guard) + # Scoped to this index, in this schema. + self.assertIn(f"c.relname = '{INDEX_NAME}'", guard) + self.assertIn("n.nspname = current_schema()", guard) + # Definition, not just validity. self.assertIn("pg_get_indexdef", guard) self.assertIn(f"USING btree ({', '.join(INDEX_FIELDS)})", guard) - self.assertIn("n.nspname = current_schema()", guard) + self.assertIn("NOT LIKE", guard) self.assertIn("RAISE EXCEPTION", guard) self.assertIn(f"DROP INDEX CONCURRENTLY IF EXISTS {INDEX_NAME}", guard)