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.

[OffgridwithJD]

Correction: my commit message on f0680c5 misstates why the NaN arm failed

I pushed f0680c5 making the requested changes. One sentence in its commit message
is wrong, and it is wrong because I repeated a diagnosis from the review without
checking it against the file. Correcting it here rather than leaving it in the log
unchallenged.

What I wrote: "It decided with if q "...", and lib.sh's q() ends in || true,
so it always exits 0 and the then-branch always ran."

What the submitted arm actually is, at 968da53, test/native_reclaim.sh:71:

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

That is psql_run, not q. psql_run sets ON_ERROR_STOP=1 and does propagate
status, so the arm was never unconditional. Measured on this branch:

compact_rewrite('n', 'NaN'::float8)   -> accepted   sqlstate OK
compact_rewrite(NULL, 0.5)            -> rejected   22004
pgcolumnar.no_such_function(1)        -> rejected   42883
compact_rewrite('n', 0.5, 1, 'x')     -> rejected   42883
1/0                                   -> rejected   22012

So the || true mechanism in the review does not apply to this arm. The review's
own CI evidence — both legs reporting got [accepted] want [rejected] — is real,
but the cause is not the one named: the call genuinely succeeded.

The second finding stands, and it is the one that mattered. The arm keyed on
"the call failed" and nothing more, and the table above is the proof: four
unrelated errors all satisfy rejected. That is what f0680c5 fixes, by asserting
22023 and pinning those four counterexamples as controls:

PASS  control: a missing function is 42883, not the guard's 22023
PASS  control: a wrong-arity compact_rewrite is 42883, not 22023
PASS  control: a null table name is 22004, not 22023
PASS  control: division by zero is 22012, not 22023

The rest of f0680c5 is unaffected: the over-broad-guard arms (0.0 and 1.0 pinned as
accepted) and the maintenance_due validation both stand, and I verified those
myself rather than taking them from a report.

I am flagging this rather than quietly moving on because the q()/|| true trap is
real and is cited across several reviews — it just is not what this arm did, and a
wrong attribution makes the real trap harder to recognise where it does occur.

Posted as OffgridwithJD. Not approving -- same account as the author.

[OffgridwithJD]

Ready for re-review at 2da714b — 12/12 green, and the head moved for a reason

Both of your asks are addressed, and the branch has been rebased onto 53224e4
because #870 landed and made this PR conflict. Sequence, so the head change is not
a surprise:

968da53  fix: reject NaN compaction thresholds          (original)
f0680c5  test: read the SQLSTATE, and validate maintenance_due too   (your review)
   ↓ rebase onto main 53224e4 (CHANGELOG conflict only, one hunk, both entries kept)
4facbed + 2da714b                                       (current head)

The rebase is content-preserving and I checked that rather than assuming it: the
patch outside CHANGELOG.md is identical to the pre-rebase patch apart from one
blob index line, and the set of CHANGELOG lines the branch adds is the same 29
lines. It caught something too — my first resolution silently ate the blank line
between two entries, and the tell was 28 added lines where the old patch added 29.

Your two asks. (a) The arm decided with if q "...", so it read q's status
rather than its value; it now reads the SQLSTATE psql printed. (b) The deny arms
asserted only that the call failed, which a NULL argument, a missing function, a
wrong arity and 1/0 all satisfy; they now assert 22023, with those four pinned
as controls.

One correction to the review, which I owe you. I had repeated your diagnosis
that the NaN arm used q with || true. It used psql_run, which does propagate
status — verified at 968da53:71. Your symptom was real and your second finding
was exactly right; the mechanism was not, and f0680c5 fixes the real one.

Two findings of my own went in with it, both measured here. The suite was blind
to the guard becoming OVER-BROAD: minFrac < 0.0 changed to <= 0.0 rejects the
legal 0.0 and native_reclaim still reported 10 passed, 0 failed, PASSED, with the
rejection visible only in an unasserted log line. And maintenance_due() — the gate
the autovacuum daemon consults before it ever calls compact_rewrite — validated
nothing at all. On a fresh 50%-deleted table, one table per value:

maintenance_due(0.2)   due = t   correct
maintenance_due(NaN)   due = f   suppresses the work, silently and for good
maintenance_due(2.0)   due = f
maintenance_due(-1.0)  due = t   ALWAYS due, so the daemon rewrites every table forever
compact_rewrite(NaN | 2.0 | -1.0)  ->  ERROR, all three

Both thresholds now raise 22023 for all four, and 0.0 and 1.0 stay accepted on
both.

Gates. GitHub Actions was down from 20:54Z to about 01:00Z, so my 21:40 push
got no run at all; that is why this PR sat with a tick belonging to an older commit.
It is running again and this head is 12 of 12 green. Locally at 2da714b:
native_reclaim 33/33 on pg18a and pg19a, docs_style clean.

MERGEABLE and green. It is still a draft, so it cannot be merged until someone
marks it ready — say the word and I will, or flip it yourself.

[jdatcmd]

No blocking finding survived refutation. The test work here is materially better than what I
asked for, and I want to say so: reading a parsed SQLSTATE instead of an exit status, with
discrimination controls that carry their own distinct codes (42883, 22004, 22012), is the
shape that makes a deny arm evidence rather than decoration.

I am not approving in this pass for one reason and it is procedural, not technical: #871
landed as 916ec0e and this branch now conflicts with main on CHANGELOG.md. It is your
branch and your lane, so I am not rebasing it. Rebase onto current main and I will approve on
the rebased head, provided the content diff is unchanged — I will check that by tree oid, not
by eye.

One correction I owe you from my earlier CHANGES_REQUESTED, because it was wrong and it is on
the record: I blamed lib.sh's q() and its || true for the unconditionally-accepted arm.
The submitted arm used psql_run, which does set ON_ERROR_STOP=1 and does propagate status.
The CI symptom was real; my diagnosis of it was not.

---

Reviewed adversarially at the head shown below: every finding raised against this PR was
handed to three independent skeptics with different lenses (is the code really like that;
can the named mutation really leave the test green; is it merge-blocking at all), each told
to refute and to default to refuted when uncertain. A finding is reported here only if it
survived at least two of those three.

No blocking finding survived refutation

Raised and killed (1)

Recorded so nobody re-litigates them:

• Stale ask #4 unaddressed: the PR body still claims coverage the suite does not have, and now omits half the diff — refuted.

Non-blocking

• The = 'NaN'::float8 disjunct in each maintenance_due guard is deletable with 33/33 still green (pgcolumnar--1.0-alpha3.sql:1814): PostgreSQL float8 ordering is not IEEE ordering: float8_cmp_internal sorts NaN above every value, so compact_due_fraction > 1.0 is already TRUE for NaN. Delete line 1814 and line 1821 (the two = 'NaN'::float8 disjuncts) and every one of the suite's 33 arms stays green, including the two arms named "rejects a NaN ... threshold with 22023" at test/native_reclaim.sh:187 and :196 — they are reddened by the > 1.0 clause, not by the clause whose name they carry. The file's own comment admits the redundancy and argues it documents intent, which is a defensible call. I am recording it because this repo's first rule is "can I delete this change and still be green?", and for these two lines the answer is yes; the two NaN arms and the two above-1 arms are the same test written twice. Note this does NOT apply to the C side: src/columnar_vacuum.c:880's isnan() is load-bearing, because C > is IEEE and NaN > 1.0 is false there.
• CHANGELOG says the new NULL rejection matches compact_rewrite; compact_rewrite accepts NULL (CHANGELOG.md:131): "Both thresholds now raise invalid_parameter_value (SQLSTATE 22023) for all four, matching compact_rewrite." The four are NaN, >1, <0 and NULL. compact_rewrite does not raise for NULL: src/columnar_vacuum.c:871 is double minFrac = PG_ARGISNULL(1) ? 0.2 : PG_GETARG_FLOAT8(1); and the SQL declaration at pgcolumnar--1.0-alpha3.sql:941-946 carries no STRICT, so pgcolumnar.compact_rewrite('n', NULL::float8) silently compacts at 0.2 and returns a row count. docs/sql-reference.md:399-401 is at the edge of the same reading ("rejects NaN, NULL, a negative value and a value above 1 ... the same code and the same bounds as pgcolumnar.compact_rewrite"). After this PR the two entry points genuinely differ on NULL, that divergence is undocumented, and no arm pins compact_rewrite's NULL behaviour in either direction.

[jdatcmd]

Approved at 6eb1973. This clears my CHANGES_REQUESTED.

The rebase is content-preserving, and I checked that with a number rather than by eye.
Per-file patch md5, 168d807 (the head I reviewed) against 6eb1973:

file	old	new
docs/sql-reference.md	b226cc75eb97	b226cc75eb97	identical
pgcolumnar--1.0-alpha2--1.0-alpha3.sql	f98cd5079503	f98cd5079503	identical
pgcolumnar--1.0-alpha3.sql	45201d1b7831	45201d1b7831	identical
src/columnar_vacuum.c	8abd90eb44cb	8abd90eb44cb	identical
test/native_reclaim.sh	6193848081df	6193848081df	identical

and the added CHANGELOG.md lines hash identically too (7152f79a8ca0 both sides), so the
only thing the rebase moved is the entry's position. My review of the old head therefore
transfers to this one with proof, not with an assurance.

Composition, not the branch. Full PG 17.10 matrix on main ce44f11 + #860 + #863
composed, in a private prefix so no concurrent make install could swap the .so:

	verdicts	PASS	SKIP	FAIL
main b4f0a45	239	234	5	0
ce44f11 + #860 + #863	241	236	5	0

ALL VERSIONS PASSED. Baseline − composed is empty, and the verdict set is byte-identical
to the composed tree I merged #871/#868/#874 on — so neither of these two changes any suite's
verdict on a tree already proven green.

A suite-level PASS cannot see whether the new arms ran, so I counted them separately, same
prefix, same invocation the matrix uses:

suite	main	with this PR	failed
native_reclaim	9 checks	33	0
parallel_export_parquet	43 checks	54	0

CI is 12/12 SUCCESS at 6eb1973; I waited for all twelve rather than merging on a partial
rollup.

The correction I owe you, on the record. My original CHANGES_REQUESTED blamed lib.sh's
q() and its || true for the unconditionally-accepted arm. The submitted arm used
psql_run, which sets ON_ERROR_STOP=1 and does propagate status. The CI symptom was real;
my diagnosis of it was wrong, and you were right to say so.

Ask 4 — the PR body still claiming coverage the suite does not contain — is not a merge
blocker and I am not holding this for it. It is worth a one-line edit to the body whenever
you next touch it.