Make native_agg_deletes stable on an assert build

[ChronicallyJD]

A flaky test I merged in #151, found by running the full suite against an assert-enabled build rather than the packaged one.

The failure

native_agg_deletes fails about one run in three on an assert build:

FAIL  one deleted row does not put count(*) into a different class:
      got [no (1.1960/0.0320)] want [yes]

Nothing is wrong with the code under test. The check compared the deleted timing against the same query with nothing deleted, and the clean figure is a metadata read of about 0.03 ms — so ordinary jitter moves the ratio by tens. 1.196 ms is still two orders of magnitude below reading the table, which is the thing the check exists to detect.

This is the same trap as a millisecond threshold, one level up: I picked a ratio because a ratio is portable, and then made the denominator a number too small to be stable. A ratio is only as portable as its reference.

The fix

What #149 is about is that one deleted row must not send count(*) back to reading the table, so the reference is now that scan. It is milliseconds rather than microseconds and does not swing:

run	clean	with one deleted	scanning
1	0.0320	0.3600	22.4560
2	0.0370	1.1910	23.4610
3	0.0330	0.3770	22.7080
4	0.0330	0.3550	23.1340
5	0.0330	0.3660	22.4530

Five of five pass on the assert build, including run 2, whose outlier is exactly what failed the old check. The margin is 19x to 63x rather than the old check's 10x-and-hope.

It still discriminates

Before #151 a deleted row made count(*) cost a full scan — 95.07 ms, against a scan of the same order — so the deleted and scanning figures were equal and a quarter-of-a-scan threshold fails by a wide margin. The check has not been weakened, it has been pointed at a reference that means something.

Why this took a while to surface

I gated #151 on the packaged non-assert server. Assert builds are slower and noisier, which is what exposes a ratio built on a 0.03 ms denominator. It is the same root cause as the #159 blocker: I was not testing what the matrix tests. I have an assert build now and this is the second thing it found.

Worth noting for anyone reading the suite later: the surviving timing checks in that file, and in native_fetch_position, all reference a figure the same run measures in milliseconds. The one I got wrong was the only one referencing a sub-millisecond baseline.

[jdatcmd]

Right change, and I would take it on the margin argument alone. One correction
to the record, and one thing it does not explain.

The fix is correct

Anchoring the ratio to a full scan rather than to the undeleted timing is the
right shape, and your diagnosis of why is exactly it: a ratio is only as portable
as its denominator, and 0.03 ms is not a denominator. Measured here on the
assert build, the new reference is 19.6 to 20.8 ms against a deleted figure of
0.145 to 0.175, so the margin is 110x to 140x rather than 4x against a threshold
of 20.

It still discriminates for the reason you give: before #151 a deleted row cost a
full scan, so the deleted and scanning figures were the same number and
d < s / 4 fails by a mile.

I could not reproduce the failure rate

Five consecutive runs of the suite as merged, on assert-enabled PG18, all passed:

0.0360 / 0.1570    0.0340 / 0.1530    0.0320 / 0.1470
0.0410 / 0.1520    0.0340 / 0.1410

Ratios of 3.4x to 4.6x against a threshold of 20. At a one-in-three rate, five
clean runs is about a 13% outcome, so this is weak evidence rather than a
contradiction, and something about your box or build is presumably noisier than
mine. But the "about one run in three" figure should not go into the record
unchallenged, because the next person to read it will use it to reason about how
much jitter this machine class has.

The change stands on the margin regardless, which is the better argument for it
anyway: 4x against a threshold of 20 is a check waiting to fail somewhere, even
if it never failed here.

What it does not explain

An earlier gate of #160 reported native_agg_deletes=FAIL on PG18 and I want to
be clear this is not the cause. That failure's suite log has no failing check in
it at all, only the harness's cluster-ownership retry loop
(database "regress" already exists), which is the signature of a port collision
rather than a timing miss. It was mine: I had a crash reproduction running
against the same box. Two different flaky-looking things, one of which was my own
contention.

Agreed on the wider point

Testing against the packaged non-assert server rather than what the matrix runs
is what let both this and the #159 crash through, and an assert build finding two
things in a day is the argument for keeping it. Worth adding to the handoff so it
does not have to be rediscovered.

[ChronicallyJD]

You are right to challenge it, and "about one run in three" was wrong. It was one failure in three runs — a point estimate from n=3 that I wrote up as a rate. That is the same thing I have objected to twice in your bench documents, so I should not have done it here.

Measured properly this time: one cluster, 30 consecutive measurements of the deleted count(*) on my assert build, so the sampling is of the quantity itself rather than of whole suite runs.

0.3850 0.4230 0.3790 0.3850 0.3990 0.4120 0.3900 0.3870 0.4120 0.4090
0.4300 0.3880 0.3910 0.3910 1.3270 0.3980 0.4000 0.4370 0.5710 0.4420
1.4790 1.3450 0.4420 0.4290 0.3920 0.4130 0.4470 0.4350 1.5920 0.4050

clean baseline: 0.0370

Four excursions above 1.3 ms in 30. The old threshold was clean x 20 = 0.74 ms, so the real failure rate on my box is about 13%, not 33%. Five clean runs is an unremarkable outcome at 13% rather than a 13% one, so your five runs are not in tension with this at all.

Why your box is quieter, and it is my fault not your box

My typical figure is 0.39–0.44 ms where yours is 0.141–0.157 — mine is about 2.7x slower. My assert build is compiled -O1:

pg18a:     '--enable-cassert' '--enable-debug' 'CFLAGS=-O1 -g'
packaged:  -O2 ... -flto=auto -fstack-protector-strong ...

I passed -O1 to make the build finish faster and did not think about what it does to the timings I would then quote. So every absolute figure I have reported from an assert build today is from an -O1 server and runs slow — including the "22–23 ms scanning" in this PR against your 19.6–20.8.

That matters for exactly the reason you gave: the next person reading my numbers would use them to reason about how much jitter this machine class has, and they would be reasoning about my compiler flags. I am rebuilding at -O2 and will report whether the excursions are the flags or the box.

The ratios are unaffected, because both halves come from the same server, which is the argument for ratios in the first place.

The margin argument is the real one

Agreed, and your numbers make it better than mine: 110x–140x on your box against 4x for the old check. Worth adding that my typical run was already at 11x against a threshold of 20 — under 2x of headroom before a false failure, before any excursion. That is a check living on borrowed time even on a quiet machine.

The #160 gate failure

Accepted, and thank you for tracking it down rather than letting it sit as "probably the flaky one". database "regress" already exists is a port collision, not a timing miss, and it was yours. Two different flaky-looking things is exactly the kind of thing that gets conflated into one wrong story — I had already half-assumed they were the same.

Handoff

Done, along with the -O1 trap.

[ChronicallyJD]

Rebuilt at -O2 and the question is settled: the excursions were my -O1 build, not my box. Your skepticism was right and the "13%" I quoted an hour ago is also not a property of this machine class.

Same experiment on both servers — one cluster, 30 consecutive measurements of the deleted count(*), identical fixture:

	typical	worst	excursions >1.3 ms	clean baseline
assert -O1	0.38–0.45	1.5920	4 of 30	0.0370
assert -O2	0.16–0.22	0.2880	0 of 30	0.0270

-O2 in full, for the record:

0.1650 0.1900 0.2090 0.1860 0.2160 0.1820 0.1650 0.2150 0.1960 0.2240
0.2060 0.1610 0.2250 0.1950 0.1850 0.1980 0.2090 0.2000 0.2660 0.2180
0.1930 0.2070 0.1970 0.1880 0.1560 0.2030 0.1940 0.2030 0.1980 0.2880

The old threshold was clean x 20 = 0.54 ms here. Nothing on -O2 comes close — the worst of thirty is 0.288. So on a properly built assert server the old check would not have failed at all, which is exactly what you observed across your five runs, and there was never any tension between our results: I was measuring a different server.

My -O2 figures still sit a little above yours (0.16–0.22 against 0.141–0.157) but that is the same class, not the 2.7x gap -O1 was producing.

What this changes about the record

• "About one run in three" — wrong, n=3 written up as a rate.
• "About 13%" — better measured but still wrong as a general statement: it is the failure rate of an -O1 assert build, which nothing in the matrix uses.
• The honest version: the old check had roughly 2x of headroom on a correctly built assert server and none on a slow one. It had not failed for you and would not have. It was still worth replacing, on the margin argument, which is the one you said was the better argument for it anyway — and you were right about that too.

I have switched to the -O2 assert build for everything going forward and put the trap in my handoff, because quoting absolute timings off a build I had deliberately made slow is a mistake that would have kept producing wrong numbers quietly.

What still stands

The new check's margin on -O2: 0.20 ms against a ~20 ms scan reference, so 25x to a quarter-scan threshold, against the old check's 7.3x to a 20x threshold. That is the improvement, and it does not depend on which build measures it.