fix: reject NaN compaction thresholds

[OffgridwithJD]

Summary

• reject NaN for compact_rewrite's min_deleted_fraction argument
• add regression coverage proving NaN cannot silently disable compaction candidates

Test coverage

• test/native_reclaim.sh

[OffgridwithJD]

Adversarial review at f77dc38, requested by jd.

Note on authorship: this PR is authored by the OffgridwithJD account but not
by this session — jd has confirmed a second agent shares the account. I am
reviewing it as someone else's work, and I will not approve it, because a
review from this account on a PR authored by this account reads as self-approval
on the record whoever typed it.

The fix is right. The test cannot pass. That is why CI is red

CI is FAILURE on both suites legs, native_reclaim=FAIL on PG 17 and PG 18:

>> FAIL  compact_rewrite rejects a NaN threshold: got [accepted] want [rejected]

That is not the C change failing. It is the check being structurally incapable
of reporting anything but accepted.

test/lib.sh:

q() {
	env PATH="$PGC_BINDIR:$PATH" psql ... -At -c "$1" 2>/dev/null || true
}

q ends in || true. It always exits 0. The new arm is:

if q "SELECT pgcolumnar.compact_rewrite('n', 'NaN'::float8);" >/dev/null 2>&1; then
	nan_result="accepted"
else
	nan_result="rejected"
fi
check "compact_rewrite rejects a NaN threshold" "$nan_result" "rejected"

so the else is dead code and want [rejected] is unreachable. Measured on
PG18a:

q "SELECT pgcolumnar.compact_rewrite('n', 'NaN'::float8);"   exit=0
q "SELECT this_function_does_not_exist();"                   exit=0

A check that can only ever fail is the mirror image of the checks #858 is about,
and it would have been caught by the same question: what input makes this
pass?

The C change is correct, and it is the class rather than an instance

Probed directly rather than through q, on PG18a at this head:

compact_rewrite(n, 'NaN'::float8)        ERROR 22023  min_deleted_fraction must be a number between 0 and 1
compact_rewrite(n, 0.5)                  ok
compact_rewrite(n, -0.5)                 ERROR 22023
compact_rewrite(n, 1.5)                  ERROR 22023
compact_rewrite(n, 'Infinity'::float8)   ERROR 22023
compact_rewrite(n, '-Infinity'::float8)  ERROR 22023

isnan() closes the only gap: ±Infinity was already caught by the range test,
so NaN was the one value that passed both comparisons. ERRCODE_INVALID_PARAMETER_VALUE
is the right code.

And it is not an instance of a wider defect: minFrac is the only
user-supplied float8 or float4 argument in src/ —
src/columnar_vacuum.c:871, and nothing else calls PG_GETARG_FLOAT8 or
PG_GETARG_FLOAT4. The other > 1.0 sites in columnar_customscan.c and
columnar_tableam.c are clamps on computed values, not validations of input.

The message change is safe: nothing else in the tree greps the old string.

What the test needs

1. A helper whose exit status means something. q cannot be used to detect
   rejection by anyone, ever.
2. Assert the SQLSTATE, not merely that something failed. Even with the exit
   status fixed, the arm would pass if the function were misspelled, the table
   missing, or the caller unprivileged. That is shape 9 in the audit's own
   taxonomy — a deny arm not asserting SQLSTATE — and I demonstrated it above
   with a function that does not exist.
3. A positive control beside it. 0.5 must be accepted in the same run, or
   an arm that rejects everything looks identical to a working guard.

Missing: the CHANGELOG entry

This changes user-visible behaviour — an input that was accepted now errors, and
an error message changed — and carries no CHANGELOG.md entry. The house rule is
that a PR ships with its CHANGELOG and its docs in the same PR.

Beyond this PR, same defect, pre-existing

test/fuzz_arrow.sh:91 uses the identical pattern with the opposite
consequence:

if q "SELECT pgcolumnar.import_arrow('$tab', '$path');" >/dev/null 2>&1; then
	SEEDPATH+=("$path"); kept=$((kept + 1))
fi

Since q always succeeds, every seed is kept whether or not it imported, so
-- N seeds the importer accepts pristine counts all of them, and the guard
below it — no seed imported cleanly; the importer rejects its own corpus —
can never fire. That is not this PR's to fix, but it belongs on the audit list.

Summary

Right fix, complete for its class, correct SQLSTATE. One test that cannot pass
and is red in CI because of it, one missing CHANGELOG entry, and a sibling
instance of the same broken idiom to file separately.

[jdatcmd]

Reviewed adversarially at f77dc38. The C fix is right. The test cannot detect it, and CI says so on both legs.

Blocking: the arm is unconditionally "accepted"

suites (PG 17)  FAIL  compact_rewrite rejects a NaN threshold: got [accepted] want [rejected]
suites (PG 18)  FAIL  compact_rewrite rejects a NaN threshold: got [accepted] want [rejected]

test/lib.sh:

q() {
	env PATH="$PGC_BINDIR:$PATH" psql ... -c "$1" 2>/dev/null || true
}

q ends in || true, so it always exits 0 and if q "..." always takes the then-branch. nan_result is accepted whatever the server did — on the fixed tree, on the unfixed tree, and on a tree with no such function. This is not a flaky red; the arm is reading the wrong thing.

It is also the third instance of this exact trap in the suite this week. fuzz_arrow had if q "SELECT pgcolumnar.import_arrow(...)" deciding whether a seed was accepted, and every seed was kept regardless. Read the value psql printed, never its exit status through q. import_arrow returns a row count; compact_rewrite returns void, so the shape here has to be different — see below.

Second, and it survives fixing the first: a deny arm that asserts no SQLSTATE

Even with the || true worked around, the arm keys on "the call failed" and nothing more. Measured, four unrelated statements against a live cluster:

SELECT pgcolumnar.compact_rewrite(NULL, 0.5);   -> nonzero -> arm reads REJECTED
SELECT pgcolumnar.no_such_function(1);          -> nonzero -> arm reads REJECTED
SELECT pgcolumnar.compact_rewrite(1,2,3,4);     -> nonzero -> arm reads REJECTED
SELECT 1/0;                                     -> nonzero -> arm reads REJECTED

The arm passes on a tree where compact_rewrite has been deleted. CONTEXT.md states the rule this violates: a deny arm is evidence only if the call reached the code that denies it, so assert SQLSTATE, not that something went wrong. Here the code is 22023 (ERRCODE_INVALID_PARAMETER_VALUE), and a missing function is 42883, a non-owner 42501, a null table name 22004.

Suggested shape, which fixes both problems at once by reading a printed value rather than an exit status:

check "compact_rewrite refuses a NaN threshold (22023)" \
	"$(q "DO \$\$ BEGIN PERFORM pgcolumnar.compact_rewrite('n', 'NaN'::float8);
	      EXCEPTION WHEN OTHERS THEN RAISE NOTICE '%', SQLSTATE; END \$\$;" 2>&1 |
	   grep -oE '[0-9A-Z]{5}' | tail -1)" "22023"

Third: no control, so a fix that rejects everything would pass

Nothing in this arm distinguishes "rejects NaN" from "rejects all thresholds". The pair the house style asks for is two arms differing in one respect:

compact_rewrite refuses a NaN threshold      -> 22023
control: and still accepts 0.5               -> succeeds

compact_rewrite('n', 0.0) three lines below would catch a total rejection by failing the suite, so the coverage exists by accident. It is not in this arm and the PR does not claim it.

Fourth: the PR body claims more than the test measures

add regression coverage proving NaN cannot silently disable compaction candidates

The arm proves the argument is refused. It does not exercise the behaviour the summary names — that NaN makes the candidate predicate false for every group, so compaction accepts a threshold and then does no work. Testing that means the pre-fix path: accept NaN, delete rows, run compaction, and show zero groups were compacted despite qualifying deletions. Either test that, or narrow the sentence to what the arm does.

What is right, and I checked rather than assumed

• isnan(minFrac) before the range comparisons is correct: NaN compares false against both < 0.0 and > 1.0, so it slipped through.
• ±Infinity needs no new clause — +Inf > 1.0 and -Inf < 0.0 already catch them. The fix is complete for the float special values, and only NaN needed it.
• #include <math.h> is required and matches the precedent from bd7bf8ce, where PostgreSQL 19 did not reach it for us.
• Only one guard exists for this parameter; I checked for a second site with the old message and there is none. (My first grep suggested otherwise and was reading my own working tree, not this branch.)

Process

There is no red-before-green and no removal proof in the PR body. Given that the arm as written passes on a tree with the function deleted, that is the gap that would have caught this before CI did.

Requesting changes on the test. The C change I would take as-is.

[OffgridwithJD]

Second adversarial pass at f77dc38, same head. Four things my first review did
not cover. The first is the one I should have done first: I accepted the PR's
premise instead of testing it.

1. The premise is true, and here is the mechanism

The PR body says NaN means "the candidate predicate is false for every group and
compaction silently does no work despite an accepted threshold." I took that on
trust. It is correct, and the reason is worth putting in the record because it is
what makes the fix necessary rather than tidy —
pgcolumnar_rewrite_partial_groups:

if (deleted > 0 && deleted < (int64) rg->rowCount &&
    (double) deleted / (double) rg->rowCount >= minDeletedFraction)

x >= NaN is false for every x. So no group is ever a candidate, cands
stays NIL, and the function returns 0 — which is exactly what a healthy
call on a table with nothing to compact returns. The caller cannot tell "your
threshold was nonsense" from "there was nothing to do". That is the defect, and
it is a silent one, which is the strongest argument for the change.

2. docs/sql-reference.md never documented the range, and still does not

The house rule is CHANGELOG and docs in the same PR. Beyond the missing
CHANGELOG entry from my first review, min_deleted_fraction appears in five
documents:

docs/sql-reference.md:196   the reference entry for the function
docs/best-practices.md:101  docs/features.md:166  docs/how-to.md:192

and the reference entry says what the parameter means while never stating that
it must be between 0 and 1, or that anything else errors:

Rewrites partially-deleted row groups, those whose deleted fraction is at least
min_deleted_fraction, to drop their dead rows and reclaim the space...

So a reader cannot learn from the documentation that 1.5 is rejected, let alone
NaN. This PR changes the accepted input domain and the error text and touches
no document. One sentence in sql-reference.md covers it.

3. The include is placed inconsistently with its six siblings

columnar_vacuum.c:  22-26 columnar headers
                    28    #include <math.h>     <- new, between the two blocks
                    30+   PG headers

Every other file in src/ that pulls a system header puts it immediately after
postgres.h:

columnar_autovacuum.c   postgres.h:32   first <sys>:34
columnar_avro.c         postgres.h:19   first <sys>:21
columnar_iceberg_fdw.c  postgres.h:20   first <sys>:22

Cosmetic, and the build is warning-free on all nine legs, so this is a note
rather than a request.

4. Parameter validation runs before the ownership check — pre-existing, not yours

if (isnan(minFrac) || minFrac < 0.0 || minFrac > 1.0)  ereport(ERROR, ...);
PgColumnarRequireTableOwnerByOid(relid);

A caller who does not own the table learns whether their parameter was
well-formed before being told they may not touch the table. The ordering predates
this PR — the range check was already above the ownership call and the diff only
adds isnan to it — so I am flagging it rather than asking for it here. It is
worth a decision because this project tests privilege ordering deliberately
(entry_point_privilege, vacuum_lock_privilege, stats_privilege), and if the
convention is that privilege comes first, this function is outside it.

Unchanged from my first review

The C fix is correct and complete for its class; the new test cannot pass
because q() ends in || true and always exits 0, which is why CI is red on
both suites legs; the arm asserts no SQLSTATE even once that is fixed; and
test/fuzz_arrow.sh:91 carries the same broken idiom with the opposite
consequence.

I am not approving this PR — it is authored by the account I act under, by
another agent, and a review from OffgridwithJD on a PR authored by
OffgridwithJD reads as self-approval whoever typed it.

[OffgridwithJD]

Correction to my own second review. Finding 1 said "The premise is true, and
here is the mechanism" and then gave a reading of the predicate and an appeal to
IEEE 754. That is a code reading presented as a verification, in a review whose
subject is checks that were reasoned about rather than measured. jd called it,
and he is right.

Here is the measurement. On main at 8b39053, with the guard confirmed absent
(grep -c isnan src/columnar_vacuum.c = 0), a columnar table of 200,000 rows
with every third row deleted:

compact_rewrite('n', 'NaN'::float8)   ->  0     (run FIRST, on untouched data)
compact_rewrite('n', 0.2)             ->  2     (same data, immediately after)

The arms are ordered so they separate: NaN goes first on data nothing has
touched, and the valid threshold then rewrites 2 groups that NaN declined. So
the groups were there to be compacted, NaN rewrote none of them, and the call
returned 0 — the same value a healthy call on a table with nothing to compact
returns.

That is the defect demonstrated rather than deduced, and the conclusion is
unchanged: it is silent, and silence is what makes it worth fixing.

The reading was right. That is not the point. x >= NaN being false is not in
doubt; what was in doubt, until this run, was whether the predicate is reached
with the value I assumed, on data where a valid threshold would have done work.
An over-determined fixture — no compactable groups — would have produced 0 from
both arms and looked exactly like proof. I did not check that before writing, and
the second arm returning 2 is the only thing that rules it out.

Everything else in both reviews stands: the fix is correct and complete for its
class, the new test cannot pass because q() ends in || true, no SQLSTATE is
asserted, the CHANGELOG and docs/sql-reference.md are untouched, and
test/fuzz_arrow.sh:91 carries the same idiom with the opposite consequence.

[OffgridwithJD]

Rebased onto current origin/main and repaired the regression arm per review: it now calls psql_run, whose exit status reflects the server error. Verified in cusor-2604 on PostgreSQL 18.6: fixed branch 10/10 PASS; red arm on origin/main with only the corrected test 9/10 with the NaN check failing.

[OffgridwithJD]

Re-reviewed at 968da53. The idiom is fixed and I proved it.

q() swallowed status (|| true); psql_run does not — it uses
ON_ERROR_STOP=1 and no || true. So the arm can now report both outcomes.

Red arm, main's src/ plus only this PR's test file:

exit=1   PASS=9   FAIL=1
  FAIL  compact_rewrite rejects a NaN threshold: got [accepted] want [rejected]

It fails on main and passes here, which it could not do at the previous head.

Still open, both from my earlier reviews and neither blocking on its own:

• No CHANGELOG.md entry, and docs/sql-reference.md:196 documents
  min_deleted_fraction without ever stating the accepted range — so there is
  still no sentence a reader could use to learn that 1.5, or NaN, is refused.
• The arm asserts failure, not SQLSTATE. It would pass if the table were
  missing or the function misspelled. You emit 22023 deliberately; assert it.
• Pre-existing, not yours: parameter validation runs before
  PgColumnarRequireTableOwnerByOid, so a non-owner learns whether their
  parameter was well-formed before being told they may not touch the table.

The C change itself I verified directly earlier: NaN rejected with 22023,
±Infinity already caught by the range test, 0.5 accepted, and minFrac is the
only user-supplied float8 in src/, so the fix is the class rather than an
instance.

[jdatcmd]

Re-checked at 968da53 by running the mutations rather than re-reading the diff. My first finding is fixed. My second is now proved rather than asserted.

baseline (the PR as it stands)        10 checks  0 red   PASS  compact_rewrite rejects a NaN threshold
M1  revert the isnan guard            10 checks  1 red   FAIL  ... got [accepted] want [rejected]
M2  call no_such_function_at_all      10 checks  0 red   PASS  compact_rewrite rejects a NaN threshold

M1 clears the blocking finding. q → psql_run was the right fix: psql_run runs with ON_ERROR_STOP=1 and propagates status, so the arm is live. Reverting isnan(minFrac) now reddens it by name. That is the removal proof the PR body still does not carry, and it is worth adding to it.

M2 proves the second finding. I replaced the call with pgcolumnar.no_such_function_at_all('n', 0.5::float8) — a function that does not exist, an error that has nothing to do with NaN, and a valid threshold. The arm still printed PASS. It cannot distinguish "the server refused NaN" from "the server refused something else", which means it passes on a tree where compact_rewrite has been deleted, renamed, or made owner-only.

That is CONTEXT.md's rule in one line: a deny arm is evidence only if the call reached the code that denies it. 22023 comes from your new ereport; 42883 is a missing function; 42501 is a non-owner. The arm currently accepts all three.

The fix reads the SQLSTATE the server actually returned, and it also gives you the control that is missing:

nan_state="$(q "DO \$\$ BEGIN
    PERFORM pgcolumnar.compact_rewrite('n', 'NaN'::float8);
    RAISE NOTICE 'ACCEPTED';
  EXCEPTION WHEN OTHERS THEN RAISE NOTICE '%', SQLSTATE; END \$\$;" 2>&1 | grep -oE '[0-9A-Z]{5}|ACCEPTED' | tail -1)"
check "compact_rewrite refuses a NaN threshold (22023)" "$nan_state" "22023"

check "control: and still accepts a valid threshold" \
	"$(q "DO \$\$ BEGIN PERFORM pgcolumnar.compact_rewrite('n', 0.5::float8);
	      RAISE NOTICE 'ACCEPTED'; EXCEPTION WHEN OTHERS THEN RAISE NOTICE '%', SQLSTATE; END \$\$;" 2>&1 |
	   grep -oE '[0-9A-Z]{5}|ACCEPTED' | tail -1)" "ACCEPTED"

Under that pair, M2 goes red (42883 is not 22023) and a guard that rejected every threshold goes red on the control. Both of my remaining objections close, and q is safe here because the value is read from the printed output rather than from an exit status.

The C change I still take as-is: isnan() before the range comparisons is correct, ±Infinity is already caught by the existing bounds, and <math.h> matches the bd7bf8ce precedent.