perf(db): add hot-path indexes for sequence matching, latest-bbox and shared frame lookups

[Acruve15]

Closes #663.

Three query shapes on the detection hot path had no index behind them, so each one seq-scanned a table that keeps growing (~2.6M rows in detections, ~56k in sequences today).

CREATE INDEX ix_sequences_camera_pose_last_seen ON sequences (camera_id, pose_id, last_seen_at);
CREATE INDEX ix_detections_sequence_id_created_at ON detections (sequence_id, created_at);
CREATE INDEX ix_detections_bucket_key ON detections (bucket_key);

Query	Call site	Without index	With index
Recently-seen sequences of a pose (camera_id, pose_id, last_seen_at >)	spatial matching and continuity, on every POST /detections	8 ms	0.07 ms
Latest real bbox of a sequence (sequence_id, created_at DESC, limit 1)	spatial matching, once per candidate sequence per detection	128 ms	0.08 ms
Sibling rows by bucket_key	shared-frame check in DELETE /detections/{id}	48 ms	0.10 ms

Rows 1 and 3 are the figures from #663, not independently reproduced. Row 2 is the shape I measured, below.

(sequence_id, created_at) has five consumers

Full list from @fe51's review, and the strongest argument in this PR:

• get_latest_with_bbox, once per candidate sequence per POST /detections
• the player's sequence reads, endpoints/sequences.py
• the validation worker's frame fetch, services/validation.py
• the frame-count subquery in crud_sequence.py, on every completed validation job
• the new-sequence path in endpoints/detections.py: sequence_id IS NULL plus a created_at cutoff. B-trees index NULLs, so that is an exact prefix-plus-range match, and an unindexed hot query this PR fixes for free.

ix_detections_bucket_key is the opposite case: its only consumer is the admin DELETE /detections/{id}. It buys a rare operation at the price of index maintenance on the highest-write table. Keys are short and roughly append-ordered per camera so the write cost is acceptable, but it is a real cost.

Measured by table size

Index scan vs forced seq scan (enable_indexscan=off) on byte-identical data, for a 1000-frame sequence:

detections rows	On disk	Seq scan	Index scan	Speedup
200k	29 MB	7.6 ms	0.45 ms	17x
1M	140 MB	18.0 ms	0.53 ms	34x
2M (roughly production today)	266 MB	34.8 ms	0.42 ms	83x
5M	672 MB	63.6 ms	1.14 ms	56x

The seq-scan side grows with the whole table while the index scan stays flat, so the gain keeps widening.

Caveat worth recording, because it moved the number by nearly 3x: a first attempt showed 160x, because the test sequence's 1000 rows were inserted consecutively and occupied only 14 heap pages. Real detections arrive interleaved with the rest of the fleet's writes, so it was rebuilt 1-in-24 across a 24k-row window (321 pages). The table above uses the interleaved layout.

Implementation notes

Plain CREATE INDEX, not CONCURRENTLY. .github/workflows/push.yml runs docker compose stop backend && docker compose up -d --wait, and deploy-dev is the only deploy job in the repo. The backend is stopped before the migration runs, so nothing writes to detections while these build, and CONCURRENTLY was paying two table passes for availability no writer could use. Removing it takes the autocommit block, if_not_exists and the invalid-index self-heal with it: a plain CREATE INDEX is transactional, so it cannot leave an invalid index behind. It also closes a hole, since if_not_exists matched on the index name alone and would have skipped silently over an index defined on the wrong columns. Correcting the earlier version of this description: that lock is SHARE, not ACCESS EXCLUSIVE. It blocks writes, not reads.

ix_sequences_validation_due_at is now declared on the model too, alongside the three new ones. It existed only in migration c5e2f7a8b1d0, so create_all-built databases never had it: the same drift this PR exists to close. Its partial predicate needs pinning separately from its columns, because autogenerate compares index columns but not postgresql_where. Verified by mutation: with the predicate removed, alembic check still reports no new operations, so nothing but the test would catch a partial index quietly becoming a full one over all 56k sequences.

Verification

• 632 tests pass, 1 skipped, ruff lint and format clean. alembic downgrade -1 && alembic upgrade head round-trips on the real asyncpg path, now in a single transaction.
• The index tests pin column order and partiality, not just names, on @fe51's review. A B-tree on (a, b) is useless for b alone, so a reordered index serves none of these queries while looking like harmless tidying, and the next --autogenerate would propagate the reorder into a real migration.
• Every assertion was mutation-checked. My first attempt at these tests was tautological and is worth flagging as a trap: asserting the indexes exist in pg_indexes passes whether or not models.py declares them, because the test database is migrated. Two tests now pin each side against one canonical list, both confirmed to fail in the right direction.
• @fe51 independently verified the previous revision on a scratch Postgres 15, concurrent build and self-heal included. That code is gone, but the review is what raised the deploy-shape question that removed it.

Sequencing

#624 has merged, so the dependency the issue mentions is satisfied, and it raises the value here: the continuity pass doubles how often the (camera_id, pose_id, last_seen_at) query runs, and get_latest_with_bbox (added by #624) is exactly the shape the second index serves. Rebased onto main, no conflicts, down_revision is still the current head (c4e9f1a2b3d5).

#661 originally carried ix_detections_sequence_id_created_at on its own. It has been rebased to drop that migration and now depends on this PR, so the index has a single owner and stays independently revertable.

Deploying

No manual pre-creation. An earlier version of this description recommended creating the three indexes by hand first; that is the one path where a human types the column lists at a keyboard, which is exactly what made the old if_not_exists dangerous. At 266 MB it saves under a minute. Instead, both from @fe51:

1. Measure the real build times once on a restored backup (\timing on), then let the migration run during a quiet evening.
2. The morning after, EXPLAIN ANALYZE the three shapes in production. Postgres will happily ignore an index it does not think is worth it, and that is the only real proof this worked.

[Acruve15]

Split out of #661, which originally carried ix_detections_sequence_id_created_at on its own for the player's sequence reads. Same index, same name, so it now has one owner here and stays independently revertable.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.87%. Comparing base (c203470) to head (af6a1a2).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #664      +/-   ##
==========================================
+ Coverage   91.30%   93.87%   +2.57%     
==========================================
  Files           3       59      +56     
  Lines         138     3217    +3079     
==========================================
+ Hits          126     3020    +2894     
- Misses         12      197     +185     

Flag	Coverage Δ
backend	93.99% <100.00%> (?)
client	91.30% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Acruve15]

hey @fe51 - this is an easy one to review and should also help speed up things 😇 if you have time to (approve) review! thx

[fe51]

Nice work — the self-heal reasoning is sound, and the measurement caveat about heap-page clustering (1-in-24 interleaving, 321 pages instead of 14) is the kind of thing most benchmarks quietly get wrong. Flagging that it moved the number by 3x is worth more than the number itself.

I verified the mechanics rather than just reading them, on a scratch Postgres 15:

• alembic upgrade head on the real asyncpg path — all three come out indisvalid = true. The autocommit_block does survive the greenlet-backed sync facade.
• The self-heal, end to end: forced all three to indisvalid = false, rewound the stamp, re-ran. _drop_if_invalid fires, DROP INDEX CONCURRENTLY is emitted for each, all three come back valid. It works as described.
• alembic downgrade -1 drops all three cleanly.
• Revision chain is linear and single-headed, down_revision is correct.
• ruff clean, the 4 new tests pass.

Two things did not survive, and one of them is the reason for the main comment below.

---

The main thing: I'd drop CONCURRENTLY

CONCURRENTLY buys availability, not speed — it's slower, two passes over the table plus a wait for in-flight transactions between them. What the extra time purchases is that writes keep flowing during the build.

But the start command is alembic upgrade head && python app/db.py && uvicorn ..., so uvicorn doesn't come up until the builds finish. The API refuses connections for the whole duration either way. There are no writes to protect, so we're paying the cost and getting nothing back.

The volumes make it moot regardless. 2.6M detections / 56k sequences is ~46 detections per sequence — a burst of frames at one per 30s. At a few sequences a day across ~50 cameras that's on the order of 2k detections/day, so 1-2 inserts per minute on average, maybe 10/min with several bursts overlapping. Not per second. A plain CREATE INDEX on a 266MB table takes seconds and holds a SHARE lock; at that rate it would block one or two inserts, which then proceed normally. That's the full extent of what CONCURRENTLY is defending here.

So:

def upgrade() -> None:
    for index_name, table_name, columns in INDEXES:
        op.create_index(index_name, table_name, columns, unique=False)


def downgrade() -> None:
    for index_name, table_name, _ in reversed(INDEXES):
        op.drop_index(index_name, table_name=table_name)

That removes the autocommit_block (and the asyncpg question with it), if_not_exists, and _drop_if_invalid — a plain CREATE INDEX is transactional, so it cannot leave an invalid index behind. There's nothing to self-heal.

The condition is that nothing else writes to detections during the build, which holds with a stop-then-start deploy. If there's a rolling strategy I'm not seeing, where the old container keeps serving while the new one migrates, then CONCURRENTLY earns its keep and we just fix the next point instead. You'd know better than me — what's the actual deploy shape?

My general feeling is that CONCURRENTLY is worth writing the day it's needed, properly, once. Adding it pre-emptively costs real complexity, and the proof is that there's a hole in it:

---

if_not_exists can make the migration lie

CREATE INDEX ... IF NOT EXISTS matches on the name alone — Postgres never compares the definition. Reproduced:

CREATE INDEX ix_detections_bucket_key ON detections (crop_bucket_key);   -- wrong column

alembic upgrade head
  → "Running upgrade c4e9f1a2b3d5 -> e8f3a6c9d1b7"
  → no error
  → alembic_version = e8f3a6c9d1b7
  → ix_detections_bucket_key ON detections USING btree (crop_bucket_key)

Clean success, revision stamped, wrong index left in place, bucket_key lookups on a sequential scan permanently, and nothing will ever say so. Same outcome if any non-index relation squats the name — I parked a plain CREATE TABLE ix_detections_bucket_key (x int) there and the migration skipped it with a notice and stamped.

_drop_if_invalid doesn't fire because the index is perfectly valid. It's just the wrong index.

This is exactly the failure the docstring argues against — "the upgrade would report success while the planner ignored the invalid index" — reached through a different door. And it's live specifically on the manual pre-creation path the description recommends: the one scenario where these indexes exist before the migration runs, and the one where a human types the column list by hand.

If CONCURRENTLY stays, the guard needs widening from "invalid" to "invalid or not the index we want". The expected columns are already in INDEXES, so it's a matter of comparing them to pg_index.indkey, plus restricting the lookup to the current schema and the target table — right now it joins pg_class on relname alone, so a same-named index elsewhere can be matched and .first() picks arbitrarily. Happy to push that as a commit if you want to keep CONCURRENTLY.

---

The tests pin names, not columns

Worth doing either way, and it's about two lines.

I swapped the column order in models.py to Index("ix_detections_sequence_id_created_at", "created_at", "sequence_id") — an index that serves none of these queries, since a B-tree on (a, b) sorts by a first and is useless for b alone — and all 4 tests stayed green.

Column order is what decides whether an index gets used at all, so it's the drift most worth catching, and it's also the one that looks most like harmless tidying to someone who hasn't thought about B-trees. The propagation path is the concerning part: editing models.py alone doesn't change production, but the next alembic revision --autogenerate sees the difference and generates a migration that drops and recreates the index in the wrong order — inside a file nobody reads closely because it's auto-generated.

EXPECTED_INDEXES = {
    "detections": {
        "ix_detections_sequence_id_created_at": ["sequence_id", "created_at"],
        "ix_detections_bucket_key": ["bucket_key"],
    },
    "sequences": {
        "ix_sequences_camera_pose_last_seen": ["camera_id", "pose_id", "last_seen_at"],
    },
}

Model side becomes {idx.name: [c.name for c in idx.columns] ...}; database side reads the columns out of pg_index.indkey instead of just indexname.

One caveat worth a comment in the file: the database-side test is only non-tautological because the container runs alembic upgrade head before pytest. async_session calls create_all, so against a fresh unmigrated database that test would pass off the model declarations alone. It works — it just isn't self-evident from reading it.

---

Smaller notes

ix_detections_sequence_id_created_at has five consumers, not two — worth putting in the description, it strengthens the case:

• get_latest_with_bbox, once per candidate sequence per POST /detections
• the player's sequence reads, sequences.py:96
• the validation worker, validation.py:116
• the frame-count subquery in crud_sequence.py:165, on every validation job completion
• and incidentally detections.py:653 — sequence_id IS NULL AND created_at > cutoff on the new-sequence path. B-trees index NULLs, so that's an exact prefix-plus-range match. An unindexed hot query this PR fixes for free.

ix_sequences_validation_due_at (from c5e2f7a8b1d0) is in neither models.py nor EXPECTED_INDEXES. It's partial, so create_all-built databases don't have it — the same drift this PR is closing, still open. Out of scope here, but it means the list isn't quite canonical yet.

ix_detections_bucket_key's only consumer is the admin DELETE /detections/{id}. Keys are short and roughly append-ordered per camera so the write cost is fine, but worth saying out loud that this one buys a rare admin operation at the price of index maintenance on the highest-write table.

Small correction: a plain CREATE INDEX takes a SHARE lock, not ACCESS EXCLUSIVE — it blocks writes but not reads. Doesn't change the argument, just so we don't propagate it.

Cosmetic: the migration filename dates don't follow the graph order (c4e9f1a2b3d5 is dated 05-27 but comes after c5e2f7a8b1d0 dated 06-10). Pre-existing, but it makes the chain unreadable from ls.

---

On the deployment question

I'd argue against pre-creating the indexes by hand: it's what opens the if_not_exists hole above, it requires typing column lists at a keyboard, and at 266MB it saves under a minute. Better to measure the real build times once on a restored backup (\timing on, run the three statements) and then just let the migration do it during a quiet evening.

Worth doing the morning after either way: EXPLAIN ANALYZE on the three query shapes in production, to confirm the planner is actually using them. Postgres will happily ignore an index it doesn't think is worth it, and that's the only real proof it worked.

---

Over to you

None of the above is a blocker in my mind, and I'd rather ask than assume: what do you think about dropping CONCURRENTLY?

I might missed context here. There may be a deploy shape I'm not seeing, a plan to scale detections well past the current volume, an incident that motivated the caution in the first place, or simply a reason you'd rather have the machinery in place before it's needed than write it under pressure later. Any of those would change my answer, and you're closer to this than I am — so push back if I've got it wrong.

The one point I'd still want addressed whichever way we go is the if_not_exists silent no-op, since the description recommends the manual pre-creation path that walks straight into it. The rest is cheap enough to fold into the same pass.

Co-reviewed with Claude Code

[Acruve15]

Hey @fe51 👋

Feedback taken into account:

• CONCURRENTLY and the guard are gone (deploy is stop-then-start, push.yml)
• the tests now pin column order
• the five consumers plus the SHARE correction are in the description

One extra from doing it: alembic check ignores postgresql_where, so ix_sequences_validation_due_at is declared on the model with its predicate pinned in the test rather than left to autogenerate. Hoping it's good to erge!

[MateoLostanlen]

Hi @Acruve15 , sorry, I'm closing #664. We took too long to review it and coordinated badly with @frgfm , who opened #705 building on your indexes. Both PRs create the same indexes, so we could only merge one. Thanks a lot for your work on this, your analysis and benchmarks really laid the groundwork 🙏

[Acruve15]

@MateoLostanlen it's indeed a shame and it could have been better handled. My main frustration is the time it took to review this PR vs the one opened by @frgfm. The overlap was flagged on #705 the day it was opened, so it could have been built on top of this PR, adding only the extra partial index.

One thing that would help next time: when a new PR overlaps an open one, ping the original author before merging, so they can merge, rebase or close their own PR

Anyway, glad the indexes landed 🙂

[MateoLostanlen]

@Acruve15 I completely understand, and you're right. Your PR waited way too long for review on our side, and since the overlap was flagged early, pyronear/pyro-api#705 should have been built on top of yours with just the extra partial index. Pinging the original author before merging an overlapping PR is a good rule, we'll follow it from now on. @frgfm please keep this in mind next time. Thanks again for the work and for taking it so well 🙏