Run the timing-sensitive suites alone instead of beside five others - #209
Conversation
The matrix runs PGC_JOBS suites at once, each with its own cluster, so every
wall-clock ratio in those suites is measured under six-way contention. The same
check on the same build measures 1.11 alone and 2.11 inside the matrix, against
a bound of 2.0. Three suites produced red gates that way in one session, each
costing a re-run to disprove, which is how a matrix teaches its readers to
discount a red result.
native_fetch_position, native_cancel and native_agg_deletes now run serially
after the parallel batch. Coverage is unchanged: they still run on every major
on every run. They are simply not measured against five competitors.
Fixture tuning was tried first and does not work, which is the part worth
recording. The group-doubling check shares one decode of the group across its
fetches, and that decode is O(group size), so the obvious lever is to raise the
fetch count until per-fetch work dominates it. Measured, the ratio moves the
other way:
6,000 fetches 1.08
20,000 fetches 1.23
40,000 fetches 1.29
Per-fetch cost is itself a function of group size, so more fetches amplify the
difference rather than averaging it away. The comment in
native_fetch_position.sh recommending that lever was mine, was reasoned from a
cost model rather than measured, and is corrected with the numbers that
disprove it.
Verified on PG18: ALL VERSIONS PASSED, with native_fetch_position, native_cancel
and native_agg_deletes all running and passing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012uKWWwBDt5TWWS5DR2tzDb
ChronicallyJD
left a comment
There was a problem hiding this comment.
This is right, and the reasoning that got it here is the valuable part — the correction on #204 (the matrix generates its own contention, so a red timing check is not evidence against the PR under it) generalises to exactly these three, and moving them out of the concurrent batch is the fix that follows from it rather than another threshold nudge.
I read the integration rather than trust it: the parallel loop continues past the timing suites, they run foreground after the wait, each writes its .rc, and the collection loop iterates the full SUITES list and reads every .rc — so the three are still folded into the pass/fail tally in suite order. Same env, same redirect, same rc capture as the parallel arm. Coverage is genuinely unchanged, not just asserted to be.
Ran the three alone on an 18.4 assert build: native_fetch_position 7/0, native_cancel 3/0, native_agg_deletes 8/0.
The one thing I pushed on: is the set of three complete?
The PR's own principle — a check whose margin does not survive six-way concurrency is measuring something the gate cannot — is a test every suite should pass, not just these three. So I swept all 98 suites for date +%s%N wall-clock ratios and found one structural look-alike that this PR leaves in the parallel batch: native_fetch_cache. It runs the identical index-driven UPDATE fetch and asserts a wall-clock ratio — one_big_group / ten_small_groups < 3 — measured with the same stopwatch. On shape alone it looks like a fourth member you missed.
It is not, and I measured it rather than guess. Alone vs six copies at once (your matrix's PGC_JOBS=6 contention, same box):
| ratio | |
|---|---|
| alone, 4 runs | 0.79, 0.84, 0.83, 0.67 |
| six-way, 6 copies | 0.95, 0.73, 0.83, 0.76, 1.15, 0.94 |
Absolute times roughly doubled under load (one ≈ 63→110 ms, ten ≈ 78→120 ms) but both sides moved together, so the ratio stayed near 1 against a bound of 3 — worst case 1.15, a 2.6x margin it never spent. That is the difference between the two checks stated precisely: native_fetch_cache is designed so the cache equalises per-fetch cost across both sides, so contention is common-mode and cancels in the ratio; native_fetch_position measures a quantity whose per-fetch cost is a function of group size, so contention is differential and does not. So the boundary you drew is the correct one — the three belong in is_timing_suite and the look-alike belongs out, on merit rather than by oversight. If it's cheap, a half-sentence in that comment naming why native_fetch_cache is exempt would save the next reader the same sweep I just did.
On the fetch-count table
The mechanism is sound and it matches what the contention numbers already show — per-fetch cost rising with group size is the same property that makes the check differential under load. I tried to reproduce the 1.08 → 1.23 → 1.29 rise directly and got a flat ~1.0 across the same K values, but with a two-int-column fixture rather than your suite's, and a lighter per-group decode is exactly the condition under which the effect vanishes — so that is a fixture mismatch on my end, not a challenge to your figures. Taking them as given; the corrected comment is a real correction, and the fifth stale comment this month being one that was wrong on arrival is worth the extra sentence you gave it.
Gate
Full bar on a clean box, PR branch @ 77cd885:
- preflight 15.18 / 16.14 / 17.6 / 18.4 / 19beta2, 0 warnings each
- matrix ALL VERSIONS PASSED, PG18 and PG19, 90/90 suites, no named failure
- the three ran in the serial phase and passed there:
native_fetch_position=PASS,native_cancel=PASS,native_agg_deletes=PASSon both majors
Approving.
…me from Two review points on #209. The set of three needs a membership test rather than a list, because native_fetch_cache asserts a wall-clock ratio on the same index-driven fetch with the same stopwatch and is deliberately not in it. The reviewer swept all 98 suites to find that, and measured it: six-way contention roughly doubles both sides of its ratio and the ratio stays near 1 against a bound of 3, because the cache equalises per-fetch cost across the two arms so contention is common-mode and cancels. The three that are in the list measure quantities whose per-unit cost is a function of the thing being varied, so contention is differential. That test is now in the comment, so the next reader does not repeat the sweep. And the fetch-count figures are attributed to the fixture that produced them. The reviewer measured flat ~1.0 across the same fetch counts on a two-integer fixture, which is consistent rather than contradictory -- a lighter per-group decode is the condition under which the effect vanishes -- but a correction that does not say which fixture it came from invites exactly that confusion twice.
|
Both taken, and the sweep is the part of this review I would keep. The exemption is now in the comment, with its measurementYou asked for a half-sentence; it got a short paragraph, because the useful thing is not that
with your numbers for why the look-alike passes it — both sides roughly doubling, ratio near 1 against a bound of 3, worst case 1.15. A list invites the next person to add to it by resemblance; a test lets them decide. Your sweep found the one case where resemblance and merit disagree, which is exactly the case that would have been added by mistake. The figures now say which fixture produced themYour flat ~1.0 on a two-integer fixture is the more useful result of the two, and I have written the reconciliation into the file rather than leaving two numbers in the world with no relation:
A correction that does not say which fixture it came from invites the confusion twice, and mine did not say. On reading the integration rather than trusting itChecking that the collection loop iterates the full Rebuilt at |
ChronicallyJD
left a comment
There was a problem hiding this comment.
The membership test is better than the half-sentence I asked for — "does the load move both sides of the ratio together" is the thing to check, and a list would have invited exactly the resemblance-based addition the sweep caught. Keeping the reconciliation-with-fixture note in the file is the right call too; a correction that doesn't name its fixture invites the confusion a second time.
No re-run needed. I checked 77cd885..3b4a219 rather than take "comments only" on faith — 19 changed lines across the two files, every one a comment or blank, zero code lines:
git diff 77cd885 3b4a219 | grep -E '^[+-]' | grep -Ev '^(\+\+\+|---)' \
| grep -vE '^[+-][[:space:]]*(#|$)' # -> empty
So the binary is identical to what I gated green (preflight 5/5, ALL VERSIONS PASSED 18+19, the three PASS in serial). Re-approving on 3b4a219 to attach the approval to the current head; the matrix result carries over unchanged.
Taking the timing-margin class, following @ChronicallyJD's finding on #204 that the matrix generates its own contention.
The root cause is the harness, not three fixtures
maxjobs="${PGC_JOBS:-6}"Six suites at once, each with its own cluster, so every wall-clock ratio in the matrix is measured against five others. Your measurement made it concrete: the same check, same build, 1.11 alone and 2.11 inside the matrix, against a 2.0 bound. "Passes on an idle box" is not a property the gate can observe, and three suites produced red gates this way in one session.
native_fetch_position,native_cancelandnative_agg_deletesnow run serially, after the parallel batch. Coverage is unchanged — every major, every run — they are simply not measured against five competitors.Fixture tuning was tried first and does not work
This is the part I would have got wrong without measuring, and the source of the error was my own comment.
The group-doubling check shares one decode of the group across its fetches, and that decode is O(group size). So the obvious lever is raising the fetch count until per-fetch work dominates it — which is what
native_fetch_position.shtold me to do, in a comment I wrote in the median-of-three PR. Measured:It moves the wrong way. Per-fetch cost is itself a function of group size, so more fetches amplify the difference rather than averaging it away. That comment is corrected in this PR with the numbers that disprove it — it was reasoned from a cost model and never checked, which makes it the fifth stale comment we have found this week and the first that was wrong on arrival rather than overtaken by events.
Verified
PG18, full matrix:
ALL VERSIONS PASSED, withnative_fetch_position=PASS,native_cancel=PASSandnative_agg_deletes=PASS— all three having run in the serial phase.One thing I did not do
native_cancelis thin for a different reason and this does not address it: it compares a 50 ms timeout's firing against a ~300 ms full load, sofull/2is a small absolute number and the interrupt's scheduling latency is a large fraction of it. Serializing removes the contention that made it fail, but the check would still be tight on a slow machine. Worth its own look if it recurs; I would rather see it recur than guess at a fixture now, having just been wrong about exactly that on the other suite.