Skip to content

native_cancel: the interrupt-correctness half is lost in CI, and it is the half that matters #254

Description

@jdatcmd

Follow-up from the #236 review, raised by @ChronicallyJD and verified.

The problem

PGC_SKIP_TIMING=1 drops three suites in CI, which is right for the wall-clock
ratio suites. But native_cancel.sh is not purely a ratio suite. It contains two
checks of different kinds:

native_cancel.sh:62   check "the short timeout is what fired"
                      asserts the error is "canceling statement due to statement timeout"
                      -> a CORRECTNESS property: the interrupt was honoured

native_cancel.sh:68   check "cancel arrives well before the load completes"
                      asserts cancel < full / 2
                      -> a RATIO, which is why the suite is skipped

Skipping the whole suite means CI would not catch a regression that removes
CHECK_FOR_INTERRUPTS from the decode path. After such a regression
statement_timeout, pg_cancel_backend() and standby recovery-conflict
resolution would all hang, and every remaining CI check would stay green.

That is not hypothetical for this codebase: three interrupt floors have been
added in the last two days (#220 columnar_fetch_row, #231 the buffered walk,
and the guard in ColumnarThriftSkip), each because a loop had none. The class
recurs.

Why it cannot simply be un-skipped

The correctness check at :62 is still timing-shaped: the 50 ms
statement_timeout only fires if the load outlasts it. On a fast runner the load
can finish first, no timeout fires, and the check fails for reasons unrelated to
interrupt handling. Un-skipping as-is trades a coverage gap for a flaky gate,
which is the trade this project keeps refusing.

What to build

A CI-safe interrupt-correctness check with no ratio in it:

  • start a long columnar scan,
  • cancel it from another session with pg_cancel_backend() (or a generous
    statement_timeout, seconds not milliseconds),
  • assert it ERRORs with canceling statement due to ... within a generous bound,
  • assert the backend is still alive and the cluster is usable afterwards.

That asserts the interrupt was honoured without asserting how long anything took,
so it is safe on shared hardware and can run in CI unconditionally. The existing
ratio check stays where it is, local-only.

Acceptance: the new check runs in CI (not under PGC_SKIP_TIMING), and removing
a CHECK_FOR_INTERRUPTS from the decode path turns it red. Prove it by removal,
or it is not known to work.

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions