Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
Split out of #661, which originally carried |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #664 +/- ##
==========================================
+ Coverage 93.75% 93.87% +0.12%
==========================================
Files 59 59
Lines 3152 3217 +65
==========================================
+ Hits 2955 3020 +65
Misses 197 197
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ffa6a2d to
1f0a9b7
Compare
…okups Closes #663. Three query shapes on the detection hot path had no index behind them, so each seq-scanned a growing table: - a pose's recently-seen sequences (camera_id, pose_id, last_seen_at), run on every POST /detections during spatial matching - the latest real bbox of a sequence (sequence_id, created_at), run once per candidate sequence per detection, and the same shape the player's sequence reads sort on - sibling rows sharing a frame object (bucket_key), on DELETE /detections/{id} Measured on the (sequence_id, created_at) shape with production-scale synthetic data, index scan vs forced seq scan on identical rows: 7.6ms -> 0.45ms at 200k detections, 34.8ms -> 0.42ms at 2M (roughly production today), 63.6ms -> 1.1ms at 5M. The seq-scan side grows linearly with the table while the index scan stays flat, so the gap widens as detections accumulate. Built with CREATE INDEX CONCURRENTLY inside an autocommit block: detections is the highest-write table and a plain build would hold ACCESS EXCLUSIVE against camera ingest for its whole duration. The migration also self-heals, dropping an index left INVALID by a cancelled build, which if_not_exists would otherwise skip while the upgrade reported success and the planner ignored it. The indexes are declared in models.py as well as the migration so create_all built test databases match production, with tests pinning both sides against one list since drift between them is otherwise invisible.
1f0a9b7 to
69692c3
Compare
|
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
left a comment
fe51
left a comment
There was a problem hiding this comment.
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 headon the real asyncpg path — all three come outindisvalid = true. Theautocommit_blockdoes 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_invalidfires,DROP INDEX CONCURRENTLYis emitted for each, all three come back valid. It works as described. alembic downgrade -1drops all three cleanly.- Revision chain is linear and single-headed,
down_revisionis 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 perPOST /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 > cutoffon 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
Review from @fe51 on #664. Three real holes, all independent of the CONCURRENTLY question still open on that thread. CREATE INDEX IF NOT EXISTS matches on the name alone: Postgres never compares the definition. So a same-named index over the wrong columns made the create a silent no-op, stamped the revision, and left the queries on sequential scans with nothing to say so. @fe51 reproduced it by pointing ix_detections_bucket_key at crop_bucket_key. The old guard only looked at indisvalid, which is no help because a wrong index is perfectly valid. The guard now compares the built column list against INDEXES and drops on any mismatch, so that repro ends with btree (bucket_key) instead of btree (crop_bucket_key). It also scopes the lookup to the current schema and checks the owning table, since the old query joined pg_class on relname alone and .first() would pick arbitrarily between same-named indexes. A non-index relation squatting the name now raises rather than being skipped: dropping someone's table to make room for an index is not a repair. Writing that guard needed two passes. relkind is Postgres "char" and arrives as bytes over this driver, so the first version compared b'i' to 'i' and raised on every legitimate index. Caught by running the repro, not by reading it. The index tests pinned names only. @fe51 swapped a column order in models.py and all four stayed green, which matters because a B-tree on (a, b) is useless for b alone, so a reorder serves none of these queries while looking like tidying, and the next --autogenerate propagates it into a real migration. Both sides now compare ordered column lists, the database side reading pg_index.indkey. The same swap now fails. Also corrects the lock claim: a plain CREATE INDEX takes SHARE, which blocks writes but not reads, not ACCESS EXCLUSIVE as the comment said.
2 participants
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 insequencestoday).camera_id,pose_id,last_seen_at >)POST /detectionssequence_id,created_at DESC, limit 1)bucket_keyDELETE /detections/{id}Attribution: rows 1 and 3 are the figures from #663 and I have not independently reproduced them. Row 2's query shape is the one I did measure, in more detail below. Happy to benchmark the other two if that is worth it before merge.
The
(sequence_id, created_at)index, measured by table sizeIndex scan vs forced seq scan (
enable_indexscan=off) on byte-identical data, so the two are directly comparable, for a 1000-frame sequence:detectionsrowsThe seq-scan side grows linearly with the whole table while the index scan stays flat, so this gain keeps widening as detections accumulate. Note this index serves two independent hot paths: the per-detection latest-bbox lookup described in #663, and the sequence reads the player pages through.
One measurement caveat worth recording, because it moves the number by nearly 3x: a first attempt showed 160x, because the test sequence's 1000 rows had been inserted consecutively and so occupied only 14 heap pages (~71 rows/page). Real detections arrive interleaved with the rest of the fleet's writes, so the sequence was rebuilt 1-in-24 across a 24k-row window (321 pages, ~3 rows/page). The table above uses the interleaved layout.
Implementation notes
CONCURRENTLY, in an autocommit block.detectionsis the highest-write table, and a plainCREATE INDEXholds ACCESS EXCLUSIVE against camera ingest for the whole build.CONCURRENTLYcannot run inside a transaction andenv.pywraps the migration run in one, henceop.get_context().autocommit_block().The migration self-heals. A cancelled
CONCURRENTLYbuild (deploy timeout, dropped connection) leaves an INVALID index behind.if_not_existswould then see the relation and skip creating it, the revision would stamp, and the upgrade would report success while the planner ignored the invalid index, silently leaving these queries on seq scans. So each index is checked inpg_indexand dropped first if invalid. The existence check joinspg_classby name rather than casting toregclass, since that cast raises when the relation is absent (the common case) instead of returning no rows.Declared in
models.pytoo. Per the issue,__table_args__declarations keepcreate_all-built test databases matching production.Verification
env.pydrives an async engine throughrun_sync, soautocommit_block()flips isolation on a greenlet-backed sync facade. All three indexes come outindisvalid = trueafteralembic upgrade head.indisvalid = false, re-ran the migration, and confirmed all three came back valid.pg_indexespasses whether or notmodels.pydeclares them, because the test database is migrated. So there are now two tests pinning each side against one canonical list. Removing an index frommodels.pyfails the declaration test; removing it from the migration fails the database test. Both were confirmed to fail.Coordination with #661
#661 (detection sampling for the player, issue #660) originally carried
ix_detections_sequence_id_created_aton its own, since the player's sequence reads need exactly that index. 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, which is the point of having split it out of #624.Worth noting the shared index is probably more valuable here than there: on this path it runs several times per
POST /detections, once per candidate sequence.Sequencing
#624 has now merged, so the dependency the issue mentions is satisfied. That also raises the value of this PR: the continuity pass doubles how often the
(camera_id, pose_id, last_seen_at)query runs, andget_latest_with_bbox(added by #624,src/app/crud/crud_detection.py) is exactly the(sequence_id, created_at DESC, limit 1)shape the second index serves, so it is now unindexed on every detection request.Rebased onto main after #624; no conflicts, and
down_revisionis still the current head (c4e9f1a2b3d5, since #624 added no migration).One deployment decision needs a human call: the container start command runs
alembic upgrade head, so on the first deploy the concurrent builds run at boot and delay the healthcheck in proportion to table size. Pre-creating the three indexes manually beforehand makes the migration a no-op (if_not_exists), which may be the calmer path for production.