Repository navigation
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## master #9550 +/- ##
===========================================
- Coverage 82.98% 69.32% -13.66%
===========================================
Files 670 666 -4
Lines 40875 40785 -90
Branches 3918 3933 +15
===========================================
- Hits 33920 28276 -5644
- Misses 6479 12033 +5554
Partials 476 476 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
junngo
left a comment
There was a problem hiding this comment.
At that time, we spent quite a bit of effort tuning the datum replicate query shape.
The Subquery and Exists conditions may look logically redundant, but they helped the DB optimizer choose the intended index path for PerformanceDatumReplicate.
I’m happy to hear any ideas for improving it further, and I’ll also try to double-check whether there’s a better query shape here.
| performance_datum__signature=signature, | ||
| performance_datum__repository=signature.repository, | ||
| performance_datum__push_timestamp__gte=alert_after_ts, | ||
| ).values_list("performance_datum_id", "value") |
There was a problem hiding this comment.
I’m concerned this may introduce a query plan regression.
This query [0] takes over 2 minutes per call in Redash. From the query plan [1], performance_datum uses the intended index quickly, but performance_datum_replicate still performs a full scan.
[0]
SELECT
pdr.performance_datum_id,
pdr.value
FROM
performance_datum_replicate pdr
INNER JOIN
performance_datum pd
ON pdr.performance_datum_id = pd.id
WHERE pd.push_timestamp >= '2026-05-11'
and pd.repository_id = 77
and pd.signature_id = 5763463
[1]
Execution Time: 138,480 ms ≈ 2m 18s
Gather
└─ Hash Join
Join: pdr.performance_datum_id = pd.id
├─ Outer: Parallel Seq Scan on performance_datum_replicate pdr
│ ├─ ....
│ ├─ Total scanned: ~1.36 billion rows
│
└─ Inner: Hash
└─ Index Scan on performance_datum pd
├─ Index: performance_reposit_c9d328_idx
├─ Cond:
│ repository_id = 77
│ signature_id = 5763463
│ push_timestamp >= 2026-05-11
├─ Actual rows: 55,900
└─ Actual time: ~49ms
There was a problem hiding this comment.
@junngo : Thank you so much for this information! I did a re-analysis and you're totally right. You did some deep, and truly excellent work to get this query optimized! Thanks for bearing with me on this!
| ) | ||
| replicates = PerformanceDatumReplicate.objects.filter( | ||
| performance_datum_id__in=Subquery(datum_with_replicates.values("id")) | ||
| ).values_list("performance_datum_id", "value") |
There was a problem hiding this comment.
The original query takes about 100 ms per call, which looks reasonable to me.
Execution Time: 107 ms
Nested Loop
├─ Outer: Aggregate / Gather
│ └─ Nested Loop Semi
│ ├─ Bitmap Heap Scan on performance_datum
│ │ ├─ Index: performance_reposit_c9d328_idx
│ │ ├─ Actual rows: ~55,494 total
│ │ └─ Uses repository/signature/push_timestamp condition
│ │
│ └─ Index Only Scan on performance_datum_replicate u0
│ ├─ Index: performance_datum_replicate_performance_datum_id_fe2ed518
│ ├─ Cond: performance_datum_id = v0.id
│ ├─ Loops: 55,494
│ └─ Heap Fetches: 0
│
└─ Inner: Index Scan on performance_datum_replicate
└─ Not executed because no matching datum ids with replicates
|
@junngo Thanks for bearing with me on this. Would you take a look at the changes I've made to the PR now? I did some exploring and there may still be room for tuning this query. Since it's the one called most / second, even a small gain will have a good impact. I had Claude make a description below: These are the exact SQL statements Django generates, before vs after, for the two changed queries. Drop in your own 1.
|
|
Sorry for the late review. Adding the |
bf18c08 to
c449e66
Compare
c449e66 to
8942e84
Compare
8942e84 to
2fedaee
Compare
✅ Deploy Preview for treeherder ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
2fedaee to
0843a55
Compare
junngo
left a comment
There was a problem hiding this comment.
Sorry for the delay in approving it. I'll monitor it too after this patch lands.
12aea3c to
eee0c6d
Compare
ae3eea9 to
f004c60
Compare
c99e975 to
ca258db
Compare
7083b97 to
72801d3
Compare
72801d3 to
2609f47
Compare
f659fd3 to
0dcead2
Compare
7a84804 to
e96084c
Compare
Bug 2041948
generate_new_alerts_in_series and generate_new_test_alerts_in_series run
on every signature during ingestion. This tightens their PerformanceDatum /
PerformanceDatumReplicate reads:
- Add repository=signature.repository to the series query so Postgres can
seek the (repository, signature, push_timestamp) composite index instead
of scanning a signature's entire history via the signature_id-only index.
A signature has exactly one repository, so results are unchanged. On the
staging replica this cut the series query from ~15 ms to ~4 ms (buffers
9,490 -> 1,755).
- Trim the series query to the four columns the consuming loop reads with
.only("id", "push_id", "push_timestamp", "value").
- Drop the no-op .select_related("summary__push__time") on the
latest-alert-timestamp lookup; Django ignores select_related when
.values_list() is chained, so it produced identical SQL.
- Extract the now-identical data-loading prologue into _load_revision_data()
(parameterized by the latest-alert-timestamp queryset and the RevisionDatum
class) and _collect_replicates_map(), shared by both alerting paths.
The replicate lookup keeps its two-step IN (SELECT ... WHERE EXISTS (...))
shape. It looks redundant but is load-bearing for the planner: it resolves
the small datum-id set via the composite index first, then probes
performance_datum_replicate by its FK index, instead of seq-scanning a
multi-hundred-million-row table (measured on staging: ~53 ms warm for this
shape vs ~22 s for a plain join).
Add a characterization test pinning _collect_replicates_map behavior.
e96084c to
d056385
Compare
Bug 2041948
generate_new_alerts_in_seriesandgenerate_new_test_alerts_in_seriesrun on every signature during ingestion. This PR addresses theirPerformanceDatum/PerformanceDatumReplicatereads:The
seriesquery filtered only onsignature+push_timestamp, so Postgres could not seek the composite (repository,signature,push_timestamp) index (its leading column,repository, was unconstrained). Addrepository=signature.repository; a signature has exactly one repository, so results are unchanged.Trimmed over-fetching on the
seriesquery with.only("id", "push_id", "push_timestamp", "value")-- the consuming loop only reads those 4 attributes, so this drops the SELECT from 10 columns to 4 (including three unused VARCHARs).Removed a dead
.select_related("summary__push__time")on the latest-alert query. Django silently ignoresselect_relatedwhen.values_list()is chained, so it produced identical SQL -- misleading dead code.De-duplicated the two near-identical functions into a shared
_load_revision_data()+_collect_replicates_map().seriesquery impact (the headline win)This is currently the #1 query by total load in prod Cloud SQL Query Insights -- not because any single call is slow (~30 ms avg), but because it is called ~8,858×/hour (once per signature on ingestion). Total load ≈ avg_exec_time × times_called, so cutting per-call cost cuts the dominant load proportionally.
Without
repository=, Postgres uses thesignature_id-only index and reads the signature's entire history, discarding everything outside the 2-week window. Addingrepository=switches it to the composite index, which range-seeks straight to the window. Measured on the staging replica (sig 5182079, repo 77: 9,462 rows all-time, 1,788 in the 2-week window):signature_idindex)performance_datum_signature_id_a9af3d6bperformance_reposit_c9d328_idx(repo, sig, push_timestamp).only())The gain scales with how much history a signature has beyond the alert window, so the worst offenders (which drive the prod average up) benefit most.
Note on the replicate lookup shape
An earlier revision of this PR replaced the replicate lookup's
IN (SELECT ... WHERE EXISTS (...))with a plain join, on the assumption theEXISTSwas redundant. It is logically redundant but load-bearing for the query planner, as @junngo flagged in review. Measured on the staging replica (autoland, sig 5182079, ~2.4k datums, ~118k replicate rows over the 2-week window):INNER JOINperformance_datumIN(Subquery(datum_ids)), noEXISTSIN (...)performance_datum_replicateIN(Subquery(... EXISTS ...))The original shape resolves the small datum-id set via the composite index first, then probes
performance_datum_replicateby its FK index, instead of letting the planner full-scan a giant table. It has been kept as-is (reverted to the original form); only the other optimizations above are new.Includes a characterization test pinning the replicate-map behavior.