UN-3972 [DEV] Cut dashboard cron DB time by indexing workflow_file_execution on (status, created_at) - #2264
Conversation
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc
|
| Filename | Overview |
|---|---|
| backend/workflow_manager/file_execution/migrations/0007_wfe_status_created_idx.py | Adds a non-atomic, concurrent composite-index migration with an invalid-index guard and state-only AddIndex. |
| backend/workflow_manager/file_execution/models.py | Declares the new (status, created_at) index in WorkflowFileExecution.Meta. |
| backend/workflow_manager/file_execution/tests/test_wfe_status_created_idx.py | Verifies concurrent migration SQL, non-atomic execution, invalid-index handling, and model-state consistency. |
Reviews (8): Last reviewed commit: "Merge branch 'UN-3883-Optimize-DB-cron-q..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc
Unstract test resultsPer-group results
Critical paths
|
|
@greptile-apps please re-review. The Confidence Score block above is stale — it still cites For a human reader: this PR is |
…into UN-3972-index-file-execution-status-created-at
…ape with tests 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KGZBF68CShem3pbUJM2tBc
…-causing-high-DB-load' into UN-3972-index-file-execution-status-created-at
|
@greptile-apps migration order is revised. Review again. |
…into UN-3972-index-file-execution-status-created-at
|
athul-rs
left a comment
athul-rs
left a comment
There was a problem hiding this comment.
@kirtimanmishrazipstack — standardized pre-merge review (16-lens rubric, unstract plugin v0.18.1), mode INITIAL, at 261cd664.
Verdict: REQUEST CHANGES
Critical: 0 · High: 2 · Medium: 8 · Low: 4 · Lenses run: 16/16
The shipped DDL is correct. Everything below is about the guard that protects it, the numbers in the prose, and one design question. Both High findings were mutation-tested against this branch.
Lens checklist
| # | Lens | Result |
|---|---|---|
| 1 | Spec & intent | See findings |
| 2 | Architectural fit | See findings |
| 3 | Correctness & edge cases | Clean |
| 4 | Security | Clean — f-string interpolation of INDEX_NAME verified a non-issue (module-level literal, no external input; sqlparse 0.5.5 confirmed to keep the dollar-quoted block as one statement) |
| 5 | Data integrity & migrations | See findings |
| 6 | Concurrency | N/A — no shared state or locking in the diff |
| 7 | API & contract compatibility | N/A — no wire format, serializer or payload touched |
| 8 | Reliability & resilience | Clean |
| 9 | Performance & cost | See findings |
| 10 | Observability | N/A — no new code path to instrument |
| 11 | Operational safety | See findings |
| 12 | LLM/agent | N/A — no prompts, model calls, tools or evals |
| 13 | Testing | See findings |
| 14 | Dependencies & build | N/A — no manifest, lockfile, Dockerfile or CI workflow |
| 15 | Code quality | Clean |
| 16 | Doc & comment accuracy | See findings |
Verified sound — recorded so it is not re-litigated
- Column order
(status, created_at)is right — equality-then-range againstservices.py:107-118and:423-437. - The guard does fire in the case it was written for: with a leftover INVALID index the
CREATE ... IF NOT EXISTSmatches the existing relation and no-ops, then the guard raises;atomic = Falsemeans the raise leaves the migration unrecorded. Operation order is correct. - Dependency on
0006_...is genuinely the leaf onmain, on the base branch and on every fetched ref; no competing branch claims0007(#2255 and #2265 add migrations to different apps). (created_at DESC)is fully dropped with no partial reintroduction. Becausestatusleads, the planner cannot use this index forget_recent_activity, so AC 5 remains unmet by design, exactly as comment 45015 intended — the PR claims nothing otherwise.Meta.indexesandstate_operationsagree; rolling deploy is safe and reversible in both directions.
Unanchored findings
- [Medium] [Lens 16] PR description, "Can this PR break any existing features" — "against 1,679 ms of query time saved per 6 h" is ~1,800x too small and argues against the change. 1,679 ms is
870 + 809, the two queries' per-call means, relabelled as a 6-hour total. The real figure is ≈2,980 s per 6 h (870 ms x 1,776 calls + 809 ms x 1,776 calls), consistent with the same PR body's "83% of 3,580 s" two paragraphs earlier. A reviewer weighing slower writes on a million-insert table against 1.7 seconds saved should reject the index. - [Medium] [Lens 16] PR description, Notes on Testing row 3 — AC-3 is stated against a seq scan on
workflow_file_executionwhile row 4 and the migration docstring sayworkflow_execution. The table contradicts itself between adjacent rows, and per UN-4045's planworkflow_file_executionis reached by index scan in both queries. Whoever runs the prod check will look for a scan that was never there. - [Medium] [Lens 13, 5] Nothing in any lane parses or executes this migration's SQL —
--no-migrationsis repo-wideaddopts(pyproject.toml:115), so a PL/pgSQL syntax error in theDO $$ ... $$block is undetectable here and surfaces first as a failed deploy-timemigrateon the 3.4 GB table. AC 1 (indisvalid = t) rests entirely on a manual throwaway-Postgres run.integration-backendalready provisions Postgres — one@pytest.mark.integrationtest would cover AC 1, the reversibility half of AC 2, and DO-block parseability. (AC 3/4 genuinely cannot be asserted in CI: on a tiny test table the planner seq-scans regardless.) - [Medium] [Lens 13, 2]
workflow_v2/migrations/0023is the identical construction —atomic = False,SeparateDatabaseAndState, a byte-identical validity guard — on a multi-million-row table, and has no shape guard today; its only test asserts onMeta.indexes. So this protects one of the two files that need it, and sets "hand-write an 83-line guard per concurrent-index migration" as the pattern. A parametrized guard walking the migration graph forCONCURRENTLYwould cover 0023 and every future one for free. - [Low] [Lens 5] The guard's
pg_classlookup is not schema-qualified whileDB_SCHEMAdefaults tounstract, notpublic(settings/base.py:155) — false-alarm direction only. - [Low] [Lens 13]
TABLE/INDEX_FIELDSis a tautology: Django field names supply both the expectation and the SQL column check, so it cannot detect a field-name != column-name divergence. The repo already contains that trap (workflow_v2/models/execution.py:156-163,db_column="workflow_id"). Derive from_meta.db_table/_meta.get_field(f).columninstead. - [Low] [Lens 13] The migration is located by hardcoded dotted path, so a renumber during the stacked merge errors all five tests as an import failure that reads as "stale test" rather than "the guard may have been regenerated".
- [Low] [Lens 13]
test_model_meta_matches_the_migrationnever reads the migration; it holds only transitively viatest_add_index_updates_state_only.
What the guard does earn
Recorded so none of the above reads as dismissal — three properties hold up under mutation: regenerating 0007 with makemigrations fails four of five tests; swapping AddIndex into database_operations fails the RunSQL check; and swapping create/guard order fails two tests, which is load-bearing and easy to lose in a refactor.
Merge order
#2264 first — it is the only one of the three with no code overlap. Then #2255, then #2265 rebased on it. AC-3 should be measured on prod only after all three land: at today's 52-day window COMPLETED is 20.4% of the table, so this index is inert for get_documents_processed until #2255's 2-day window ships.
Reviewed with the PR Review Toolkit agents (code-reviewer, pr-test-analyzer, comment-analyzer); lenses 5, 6, 8, 11, 12, 14 assessed directly. Verdict is advisory — posted as a comment, not a merge gate.
| def setUpClass(cls) -> None: | ||
| super().setUpClass() | ||
| cls.migration = importlib.import_module(_MIGRATION).Migration | ||
| cls.operation = cls.migration.operations[0] |
There was a problem hiding this comment.
[High] [Lens 13] — every assertion reads operations[0], so a second write-blocking operation is invisible
setUpClass binds cls.operation = cls.migration.operations[0] and no test asserts len(migration.operations). Anything appended after the SeparateDatabaseAndState is never inspected. A plain migrations.AddIndex in that position is executed against the database, takes the SHARE lock for the whole build on the 3.4 GB table, and stalls file processing — exactly what this file exists to prevent.
Mutation-tested against this branch: appending
migrations.AddIndex(
model_name="workflowfileexecution",
index=models.Index(fields=["status", "created_at"], name="wfe_dup_idx"),
),to the real migration gives 5 passed, 2 warnings in 0.02s.
Suggested fix: self.assertEqual(len(self.migration.operations), 1), or iterate all operations and assert none is a bare AddIndex.
| into a loud failure. | ||
| """ | ||
| guard = self.operation.database_operations[1].sql | ||
| self.assertIn("indisvalid", guard) |
There was a problem hiding this comment.
[High] [Lens 13, 16] — the INVALID-index guard is checked for three substrings; its semantics are entirely unasserted
assertIn("indisvalid", guard), assertIn("RAISE EXCEPTION", guard) and the DROP INDEX ... check are satisfied by any text containing those characters — including a SQL comment. Neither the polarity (AND NOT i.indisvalid, which is the whole point) nor the c.relname = '<name>' predicate that scopes the guard to this index is checked.
Mutation-tested, both green:
- replacing the entire guard with
f"-- indisvalid RAISE EXCEPTION DROP INDEX CONCURRENTLY IF EXISTS {INDEX_NAME}\nSELECT 1;"→5 passed - flipping
AND NOT i.indisvalid→AND i.indisvalid→ all four shape assertions still pass
Flip it one way and the migration raises on every healthy deploy; flip it the other and it never fires, so Django records the migration applied while the index is physically unusable — the exact silent failure the docstring above claims to turn into a loud one.
Note the third assertIn only matches the text inside the RAISE EXCEPTION message string, not the WHERE clause, so it carries no information about the predicate.
Suggested fix: assertIn("NOT i.indisvalid", guard) and assertIn(f"c.relname = '{INDEX_NAME}'", guard).
| 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) |
There was a problem hiding this comment.
[Medium] [Lens 13] — this assertion couples to incidental SQL formatting
f"{TABLE} ({', '.join(INDEX_FIELDS)})" demands the exact byte sequence workflow_file_execution (status, created_at).
The column-order coupling is genuinely the point — (created_at, status) would not serve the cron's equality-plus-range filter, and this correctly catches that, and an appended DESC. The whitespace and casing coupling is not: mutation-tested, ON workflow_file_execution(status, created_at); (one space removed) fails, as does a wrapped line and uppercased identifiers. That is a false-failure mode with no matching true-failure mode, and the natural response to a red test on semantically identical SQL is to loosen the assertion.
Also [Medium]: reverse_sql is never checked for the index name. Setting it to DROP INDEX CONCURRENTLY IF EXISTS wfe_typo_idx; gives 5 passed — a typo there makes rollback a silent no-op through IF EXISTS, so migrate file_execution 0006 unapplies the migration while the index stays on the table, and Django's recorded state diverges from the physical schema.
Suggested fix: re.search(rf"ON\s+{TABLE}\s*\(\s*status\s*,\s*created_at\s*\)", create.sql, re.I), and add assertIn(INDEX_NAME, create.reverse_sql).
| 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 |
There was a problem hiding this comment.
[Medium] [Lens 5, 11] — "prefer building it out of band" without saying what to build, and the nearest fallback runbook is harmful
Commit 6d21dc52b removed the CREATE INDEX CONCURRENTLY ... block this docstring refers to, so an operator following this instruction reaches for the nearest runbook — and UN-3972's description step 3 still carries the two-index version, including wfe_created_at_desc_idx.
That index was struck in comment 45015 as worse than nothing: with it present the planner gains an ordered-index-scan path costed against a near-uniform ~80 K estimate, so it looks nearly free and will likely be chosen, then walks up to 3.3 M entries with a random heap fetch each for a small tenant — slower than today's sequential scan plus top-N sort.
Suggested fix: restore the exact single-index CREATE statement to the PR body's Database Migrations section, and correct UN-3972's description so the stale snippet is not the nearest source.
| database_operations=[ | ||
| migrations.RunSQL( | ||
| sql=( | ||
| f"CREATE INDEX CONCURRENTLY IF NOT EXISTS {INDEX_NAME} " |
There was a problem hiding this comment.
[Medium] [Lens 5] — neither IF NOT EXISTS nor the guard checks the index's shape
Both match on name only. If an index called wfe_status_created_idx already exists with a different column list or order, the CREATE no-ops, the guard checks only indisvalid and passes, and Django records 0007 as applied — while Meta.indexes asserts (status, created_at) and the database holds something else. The shape test cannot catch this; it inspects the Python module, never the database.
Suggested fix: extend the same DO block with a second IF comparing pg_get_indexdef(c.oid) against the expected definition (~4 lines, no new pattern).
[Medium] [Lens 9, 2] — full index where the measurement and the repo's own precedent argue for partial
By comment 45015's own numbers, the only query this serves today is get_failed_pages (ERROR = 0.40% of rows); COMPLETED is 97.6%, so the 52-day (COMPLETED, created_at) range is 20.4% of the table and get_documents_processed keeps its scan.
More importantly status is this index's leading key and is mutated on every row: models.py:140 (self.status = status.value, from update_status) rewrites it at each PENDING → EXECUTING → COMPLETED transition, relocating the entry each time. The real cost is roughly three index tuples plus two dead ones per row lifecycle, not the "one more index entry per inserted row" the ticket budgeted — on a table taking millions of inserts, with sustained autovacuum pressure in the in-flight regions.
The precedent this PR follows, workflow_v2/migrations/0023, chose partial for exactly this reason and documented the trade (~2 MB vs ~20 MB).
Counter-argument that may be decisive: the full shape is correct once #2255 narrows the window, and the merge table requires #2255 first. If that's the reasoning, one docstring line should say so.
| """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 |
There was a problem hiding this comment.
[Medium] [Lens 16] — this docstring cites UN-3883 for a claim UN-3883 contradicts, and its absolute is false
Two issues in one sentence.
-
"scans
workflow_executionin full instead. Measurements in UN-3883." — the prose is correct, but UN-3883's 2026-08-10 analysis is the source of theworkflow_file_executionseq-scan claim (quoted verbatim in UN-3972's description). Theworkflow_executionfinding is UN-4045's, dated 2026-08-31, which explicitly overturns it. A maintainer chasing the citation lands on contradicting text with no way to tell which is current. Suggest: "(execution plan in UN-4045; cost measurements in UN-3883)". -
"Every existing index leads with
workflow_execution_id" — false. The table carries 8 indexes and the primary-key index leads withid(models.py:69). The conclusion holds; the absolute doesn't, and it is the kind of claim a reader trusts rather than re-verifies. Suggest: "No existing index leads withstatusorcreated_at— every secondary index is prefixed byworkflow_execution_id."
| ], | ||
| name="wf_provider_uuid_path_stat_idx", | ||
| ), | ||
| # Serves the dashboard metrics cron's status + date-window filter; every |
There was a problem hiding this comment.
[Medium] [Lens 16, 1] — this comment states a benefit the ticket's own measurement says is not realised for the larger query
"Serves the dashboard metrics cron's status + date-window filter" reads as: this index fixes both cron queries. Per comment 45015 it fixes one. The next engineer reads this, concludes UN-3883's hot path is handled, and either doesn't ship the window narrowing the index depends on, or debugs a Query Insights window that has barely moved with no comment explaining why.
Two smaller points on the same line: "every index above" is scoped by position in a list, so appending or reordering an index anywhere above silently falsifies it with nothing to catch the drift — test_model_meta_matches_the_migration asserts nothing about the other entries. And workflow_execution here means the FK column, while the identical token in migration 0007's docstring means the table.
Suggested fix: "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."
2 participants
What
One database index on processed files, covering status and date together.
The second index the ticket also asks for is not here — dropped in comment 45015.
Why
The dashboard cron costs ~3,580 s of database time per 6 h on prod. Two queries are 83% of
that, and both look up processed files by date and status.
Nothing organises the data that way, so the database reads all 1,278,885 execution rows on every
call. This index lets it start from the files in the date range instead.
How
SeparateDatabaseAndState:RunSQLbuilds the indexCONCURRENTLY IF NOT EXISTS, a state-onlyAddIndexkeeps Django's model state in step, andatomic = FalsebecauseCONCURRENTLYcannotrun in a transaction. A
RAISE EXCEPTIONguard fails loudly on a leftover INVALID index, whichIF NOT EXISTSwould otherwise keep while Django recorded the migration as applied.Can this PR break any existing features. If yes, please list possible items. If no, please explain why.
No. Nothing but an index is added — no behaviour changes — and it is built without blocking
writes. The cost is slightly slower writes on that table, against 1,679 ms of query time saved
per 6 h.
Database Migrations
file_execution/0007_wfe_status_created_idx.py— builds the index without locking writes, andreverses cleanly. Optionally build it by hand first; the deploy then skips it:
Migration Order
The three UN-3883 PRs stack on the same integration branch. Merge in this order.
dashboard_metrics/0005_add_reconciliation_task0004_pg_periodic_tasks(UN-3445, already onmain)file_execution/0007_wfe_status_created_idxfile_execution/0006_…dashboard_metrics/0006_split_aggregation_schedule0005_add_reconciliation_task(#2255)#2264's migration is in a different app and has no interaction with the other two. The one
hard dependency is #2265 on #2255 — merged out of order, #2265 fails at graph build:
Its tests are unaffected — the backend suite runs with
--no-migrations.Verified on a throwaway Postgres:
0004→0005→0006applies from an empty database,reverses, and re-applies, with
makemigrations --checkclean at each step.Env Config
None.
Relevant Docs
UN-3883 analysis §6.4.
Related Issues or PRs
Parent UN-3883. #2255 (UN-3973) — AC-3 depends on it. #2265 (UN-3974). See Migration Order above.
Dependencies Versions
None.
Notes on Testing
Nine scenarios and five unit tests. Detail in Jira comment 45381.
indisvalid = tCONCURRENTLY, no write-blocking lockget_documents_processedfree of a seq scan onworkflow_file_executionget_failed_pagesfree of a seq scan onworkflow_executionget_recent_activityunder 1 sAfter deploy: confirm the index is valid, then re-pull the 6-hour Query Insights window against
the 870 / 809 ms baseline.
Screenshots
n/a — schema-only change.
Checklist
I have read and understood the Contribution Guidelines.