Skip to content

Measure the cancel guard against the timeout floor, not half the load - #373

Merged
jdatcmd merged 1 commit into
mainfrom
fix/native-cancel-threshold
Aug 4, 2026
Merged

Measure the cancel guard against the timeout floor, not half the load#373
jdatcmd merged 1 commit into
mainfrom
fix/native-cancel-threshold

Conversation

@jdatcmd

@jdatcmd jdatcmd commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

What

The full five-major matrix at 488a2c0 (the first since 2026-07-30, and a
pre-alpha item on #367) came back:

PASS  PG15    PASS  PG16    FAIL  PG17    PASS  PG18    PASS  PG19

The single red is native_cancel, and it is not a product defect. The
cancellation worked in every run; the server log shows the statement cancelled by
statement_timeout each time. What failed is the threshold.

Why the threshold was wrong

The check was cancel < full / 2. cancel cannot go below the timeout itself, so
that is only a real threshold while full is comfortably more than twice the
timeout. At full=155 it leaves 27 ms of headroom above a 50 ms floor:

major cancel latency, repeated runs result
PG17 82, 80, 64 ms 2 of 3 fail
PG18 64, 63, 63 ms 3 of 3 pass

full is stable at 150-164 ms on both. PG17 is simply slower and more variable on
this measurement and sits on the threshold. That is a gate reporting the box rather
than the guard, which is the specific failure mode this project's own comments say
teaches readers to discount red.

The fix

Measure against the window the guard actually distinguishes. Cancel either fires
during the load (just after the timeout) or only after it (converging on
full), so require cancel in the lower half of the interval between those two:

limit = TIMEOUT_MS + (full - TIMEOUT_MS) / 2

Self-calibrating in both directions, and it cannot be squeezed by a fast box.
PG17 now passes 4 runs of 4, having passed 1 of 3.

Proven by removal, and the removal corrected the comment

The suite's header attributes the guard to the per-column-chunk
CHECK_FOR_INTERRUPTS in columnar_native_load_group(). That is wrong, and I
only found out by removing it:

guard removed cancel vs full suite
per-chunk CHECK_FOR_INTERRUPTS (reader) 64 vs 153 ms still PASSES
COLUMNAR_DECODE_INTERRUPT (encoding) 151 vs 150, 157 vs 154 FAILS, both runs

A two-column group reaches the per-chunk check twice, which is far too coarse to
matter. The guard that carries this suite is the per-value decode-loop check on a
65536 stride. When it is disabled, cancel converges on full exactly as the header
predicts, and the new threshold catches it.

That correction is written into the suite, so nobody deletes the cheap per-chunk
guard on the strength of a green run here.

Scope

Test-only. No source change, so the rest of the five-major matrix result stands as
the verification for 488a2c0: 112 suites on each of five majors, PG15/16/18/19
clean, PG17 clean apart from this threshold.

🤖 Generated with Claude Code

…e load

The full five-major matrix at 488a2c0 came back green on PG15, PG16, PG18 and
PG19 and red on PG17, on native_cancel's "cancel arrives well before the load
completes". The cancellation worked every time: the server log shows the
statement cancelled by statement_timeout in each run. What failed is the
threshold.

The check was cancel < full / 2. cancel cannot go below the timeout itself, so
that is only a real threshold while full is comfortably more than twice the
timeout. At full=155 it leaves 27 ms of headroom above a 50 ms floor, and PG17
measured 64-82 ms against PG18's stable 63-64, failing two runs in three. That
is a threshold reporting the box rather than the guard, which is the specific
thing this project does not want a gate to do.

Measure against the window the guard actually distinguishes instead: cancel
fires during the load (just after the timeout) or only after it (converging on
full), so require cancel in the lower half of the interval between those two.
Self-calibrating in both directions and it cannot be squeezed by a fast box.

PG17 now passes 4 runs of 4, having passed 1 of 3 before.

Proven by removal, and the removal found that the comment was wrong about which
guard this covers. Deleting the per-column-chunk CHECK_FOR_INTERRUPTS in
columnar_native_load_group() leaves the suite green: a two-column group reaches
it twice. The guard that matters is COLUMNAR_DECODE_INTERRUPT in
columnar_encoding.c, the per-value decode-loop check; disabling it makes cancel
converge on full (151 ms against 150, and 157 against 154) and fails this check
on both runs. Recorded in the suite so nobody removes the cheap guard on the
strength of a green run here.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@ChronicallyJD ChronicallyJD left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed, and the diagnosis is right: full / 2 is only a threshold while full is
comfortably more than twice the timeout, and at full=155 it was asking the guard to
land inside 27 ms of a 50 ms floor. Self-calibrating against the interval the guard
actually distinguishes is the correct shape.

I checked the arithmetic against your numbers. Old limit at full=155 is 77.5, so
PG17's 82 / 80 / 64 fails two of three — matches. New limit is
50 + (155-50)/2 = 102, so all three pass, and a build with the guard removed
converges on full (151) and fails. The check still discriminates the thing it names.

The paragraph establishing which guard this proves — by deletion, not by
description — is the most valuable part of the diff. COLUMNAR_DECODE_INTERRUPT on a
65536 stride rather than the per-chunk CHECK_FOR_INTERRUPTS that a two-column group
reaches twice. That is exactly the note that stops someone removing the cheap guard on
the strength of a green run here.

Two things, one small and one a judgement call.

1. The timeout is now written in two places and they must agree

TIMEOUT_MS=50 is a new constant, and the value it has to match is a literal in the
statement three lines up:

cancel="$(ms_for "SET statement_timeout = '50ms'; SELECT * FROM cancel_t LIMIT 1;")"
...
TIMEOUT_MS=50

Change one and the other goes quietly wrong — a larger timeout makes limit too low
and produces exactly the false red this PR removes. Worth defining it once at the top
and interpolating it into the SET:

TIMEOUT_MS=50
cancel="$(ms_for "SET statement_timeout = '${TIMEOUT_MS}ms'; ...")"

2. The fast-box end fails where it probably wants to skip

[ "$full" -gt $(( TIMEOUT_MS * 2 )) ] && [ "$cancel" -lt "$limit" ]

If full <= 100 the check reports no (...) and goes red. But that case is not the
guard failing — it is the measurement being unable to discriminate, because the load
finished too close to the timeout floor for the interval to mean anything. That is the
same failure mode as the one being fixed, at the other end of the hardware range: a
red that reports the box.

PGC_CANCEL_ROWS is tunable and hardware keeps getting faster, so it is reachable.
The project's own convention fits it — a skip is announced and is not counted as a
pass:

if [ "$full" -le $(( TIMEOUT_MS * 2 )) ]; then
    echo "SKIP  cancel arrives well before the load completes (full=${full}ms is too close to the ${TIMEOUT_MS}ms floor to discriminate)"
else
    check_timing ...
fi

Your call — it is a hypothetical on the hardware in front of us, and I would not hold
the PR for it. But the current form turns "cannot tell" into "failed", and the whole
argument of this PR is that those are different things.

Neither point affects the fix itself. Approving on the substance; happy to see it
merge with or without point 2.

@jdatcmd
jdatcmd merged commit 1e4dd0e into main Aug 4, 2026
11 checks passed
@jdatcmd
jdatcmd deleted the fix/native-cancel-threshold branch August 4, 2026 02:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants