fix: reject NaN compaction thresholds

CHANGELOG.md

@@ -150,6 +150,35 @@ true until the next version shipped.
   own range-table entry has `inh` false: the resulting plan is an `Append` of
   two `PgColumnarScan` nodes with pushdown intact, not a fallback to `Seq Scan`.
 
+- Both compaction thresholds are validated, in both entry points (#860).
+  `pgcolumnar.compact_rewrite` accepted `NaN` for `min_deleted_fraction`, because
+  `NaN < 0.0` and `NaN > 1.0` are each false. The call then matched no row group
+  and reclaimed nothing while reporting success.
+
+  `pgcolumnar.maintenance_due` is the gate the `pgcolumnar.autovacuum` daemon
+  consults before it ever calls `compact_rewrite`, and it validated nothing at
+  all. Measured on a table with 10000 of 20000 rows deleted: `NaN` and `2.0` both
+  reported `compact_rewrite_due = f`, suppressing the work for good, and `-1.0`
+  reported `t`, which makes the daemon believe compaction is always due and
+  rewrite every columnar table on every sweep. `NULL` behaved like `NaN`, because
+  the daemon reads a NULL verdict as "not due". Both thresholds now raise
+  `invalid_parameter_value` (SQLSTATE `22023`) for all four, matching
+  `compact_rewrite`.
+
+  0 and 1 remain legal on every threshold, and `test/native_reclaim.sh` now pins
+  both endpoints as accepted. That is the direction a bounds fix usually breaks,
+  and nothing could see it: the suite's only call at 0 was a bare `psql_run`
+  whose exit status nothing read, so a rejected 0 reached nothing but the server
+  log. Measured with `minFrac < 0.0` changed to `minFrac <= 0.0`: exactly one arm
+  fails, the new one, and the other 32 pass. The same mutation applied to
+  `compact_due_fraction` reddens the matching `maintenance_due` arm and nothing
+  else.
+
+  Every deny arm asserts the SQLSTATE, not "it failed". Four calls that satisfy
+  "it failed" without touching the guard are pinned as controls: a missing
+  function and a wrong-arity call (`42883`), a null table name (`22004`) and
+  `1/0` (`22012`).
+
 - A shebang and the execute bit go together, and every directory that documents
   a command is swept (#856). Two things were left over from #852.
 

docs/sql-reference.md

@@ -200,6 +200,12 @@ Rewrites partially-deleted row groups, those whose deleted fraction is at least
 `ShareUpdateExclusiveLock`. `max_groups` caps how many groups a single call
 rewrites; 0 means no cap. Returns the number of groups rewritten.
 
+`min_deleted_fraction` must be a number from 0 to 1, both ends included. The
+function rejects `NaN`, a negative value and a value above 1. Each raises SQLSTATE
+`22023`, `invalid_parameter_value`. `NaN` needs a test of its own because it
+compares false against every bound. An accepted `NaN` would match no row group, so
+the call would reclaim nothing and still report success.
+
 ```sql
 SELECT pgcolumnar.compact_rewrite('events', 0.3);
 ```
@@ -389,9 +395,17 @@ monitoring query. Returns one row:
 | `recluster_due` | boolean | True when a sorted run exists and `appended_fraction` reaches `recluster_due_fraction`. |
 | `recommendation` | text | The verbs to run, comma-separated, or NULL when nothing is due. |
 
-The two thresholds default to the values the daemon uses. The function is
-`SECURITY DEFINER` and checks that the caller may `SELECT` the table. A monitoring
-role that owns the table can therefore call it without superuser rights.
+The two thresholds default to the values the daemon uses. Each must be a number
+from 0 to 1, both ends included. The function rejects `NaN`, `NULL`, a negative
+value and a value above 1. Each raises SQLSTATE `22023`, the same code and the
+same bounds as `pgcolumnar.compact_rewrite`. An unchecked threshold fails silently
+rather than loudly. `NaN`, `NULL` or a value above 1 reports nothing as due, which
+suppresses maintenance for good. A negative value reports every table as due on
+every sweep.
+
+The function is `SECURITY DEFINER` and checks that the caller may `SELECT` the
+table. A monitoring role that owns the table can therefore call it without
+superuser rights.
 
 ```sql
 SELECT recommendation FROM pgcolumnar.maintenance_due('events');

pgcolumnar--1.0-alpha2--1.0-alpha3.sql

@@ -296,3 +296,118 @@ COMMENT ON FUNCTION pgcolumnar.sort_status(regclass)
 
 COMMENT ON FUNCTION pgcolumnar.vacuum_sorted(regclass, name[])
 	IS 'compact a columnar table, storing rows sorted ascending (NULLS LAST) on the given columns. With no columns, applies the table''s declared sort_by key from set_options (#288), like a bare CLUSTER re-applying a remembered index; errors if none is declared. Supports any btree-orderable column including text and numeric, unlike Z-order cluster(), which takes integer, date/time, boolean and floating-point columns only. One-shot: not auto-maintained.';
+
+-- pgcolumnar.maintenance_due(): validate both threshold parameters (#860).
+-- Unvalidated, a NaN or above-1 threshold silently suppressed all
+-- maintenance and a negative one made every table permanently "due", in the
+-- gate the autovacuum daemon consults before calling compact_rewrite.
+-- The body below is verbatim from pgcolumnar--1.0-alpha3.sql.
+CREATE OR REPLACE FUNCTION pgcolumnar.maintenance_due(
+	rel regclass,
+	compact_due_fraction float8 DEFAULT 0.2,
+	recluster_due_fraction float8 DEFAULT 0.05,
+	OUT total_rows bigint,
+	OUT deleted_rows bigint,
+	OUT deleted_fraction float8,
+	OUT sort_key name[],
+	OUT appended_groups bigint,
+	OUT appended_rows bigint,
+	OUT appended_fraction float8,
+	OUT compact_rewrite_due boolean,
+	OUT recluster_due boolean,
+	OUT recommendation text)
+	RETURNS record
+	-- SECURITY DEFINER, mirroring stats(): the report reads pgcolumnar's internal
+	-- catalogs through sort_status(), which ordinary roles cannot SELECT, so an
+	-- invoker-rights function false-denied every non-superuser caller -- the
+	-- cron/monitoring role this report is for. require_caller_select (inside
+	-- stats()) still gates the REAL caller via GetOuterUserId(), so definer rights
+	-- do not widen who may read a table's statistics. search_path is pinned as a
+	-- definer function must.
+	LANGUAGE plpgsql STABLE SECURITY DEFINER
+	SET search_path = pg_catalog, pg_temp
+	AS $maintenance_due$
+DECLARE
+	st_rows bigint;
+	st_del  bigint;
+	ss      record;
+BEGIN
+	-- Validate both thresholds before reading anything (#860). Neither one was
+	-- checked, and this is the gate the autovacuum daemon consults BEFORE it ever
+	-- calls compact_rewrite, which does check its own. Four ways an unchecked
+	-- threshold goes wrong, none of which raises anything:
+	--   NaN   -- `fraction >= NaN` is false in IEEE, so nothing is ever due and
+	--            the work is suppressed silently and permanently.
+	--   > 1   -- the same outcome for any fraction: never due.
+	--   < 0   -- `fraction >= -1` is true for EVERY table, so the daemon believes
+	--            compaction is always due and rewrites every columnar table on
+	--            every pass. This is the dangerous direction: not a suppressed
+	--            report but a permanent, self-renewing rewrite.
+	--   NULL  -- the verdict is NULL, and the daemon reads a NULL verdict as
+	--            "not due" (SPI_getbinval isnull), so it is the NaN case again.
+	-- 0.0 and 1.0 are LEGAL and stay legal: 0.0 means "any decay at all is worth
+	-- acting on", 1.0 means "only a fully dead table". The bounds are inclusive,
+	-- matching pgcolumnar.compact_rewrite's own guard, and test/native_reclaim.sh
+	-- pins both endpoints as ACCEPTED so this guard cannot quietly become
+	-- over-broad, which is how a bounds check usually breaks.
+	--
+	-- The explicit NaN test is redundant with `> 1.0` today, because PostgreSQL
+	-- float8 ordering is not IEEE ordering: it sorts NaN above every other value.
+	-- It is written out anyway so the intent survives an edit to the bounds.
+	IF compact_due_fraction IS NULL
+	   OR compact_due_fraction = 'NaN'::float8
+	   OR compact_due_fraction < 0.0
+	   OR compact_due_fraction > 1.0 THEN
+		RAISE EXCEPTION 'compact_due_fraction must be a number between 0 and 1'
+			USING ERRCODE = 'invalid_parameter_value';
+	END IF;
+	IF recluster_due_fraction IS NULL
+	   OR recluster_due_fraction = 'NaN'::float8
+	   OR recluster_due_fraction < 0.0
+	   OR recluster_due_fraction > 1.0 THEN
+		RAISE EXCEPTION 'recluster_due_fraction must be a number between 0 and 1'
+			USING ERRCODE = 'invalid_parameter_value';
+	END IF;
+
+	-- stats() enforces require_caller_select(rel) before it returns a row, so a
+	-- caller without SELECT on rel is refused here rather than reported to.
+	SELECT COALESCE(sum(s.rowcount), 0), COALESCE(sum(s.deletedrows), 0)
+	  INTO st_rows, st_del
+	  FROM pgcolumnar.stats(rel) s;
+
+	SELECT * INTO ss FROM pgcolumnar.sort_status(rel);
+
+	total_rows   := st_rows;
+	deleted_rows := st_del;
+	deleted_fraction := CASE WHEN st_rows > 0
+							 THEN st_del::float8 / st_rows ELSE 0 END;
+
+	sort_key        := ss.sort_key;
+	appended_groups := ss.appended_groups;
+	appended_rows   := ss.appended_rows;
+	appended_fraction := CASE WHEN (ss.sorted_rows + ss.appended_rows) > 0
+							  THEN ss.appended_rows::float8
+								   / (ss.sorted_rows + ss.appended_rows)
+							  ELSE 0 END;
+
+	compact_rewrite_due := (deleted_fraction >= compact_due_fraction);
+	-- A sorted RUN must exist for recluster to mean anything. sort_status()
+	-- reports a never-ordered table as entirely appended (no run), and
+	-- vacuum_sorted() establishes a run without setting options.sort_by, so the
+	-- run -- sorted_groups > 0 -- is the signal, not the sort_by label (sort_key
+	-- is reported for information and may be NULL on an ordered table).
+	recluster_due := (ss.sorted_groups > 0
+					  AND ss.appended_groups > 0
+					  AND appended_fraction >= recluster_due_fraction);
+
+	recommendation := NULLIF(
+		concat_ws(', ',
+			CASE WHEN compact_rewrite_due THEN 'compact_rewrite' END,
+			CASE WHEN recluster_due THEN 'recluster' END),
+		'');
+	RETURN;
+END;
+$maintenance_due$;
+
+COMMENT ON FUNCTION pgcolumnar.maintenance_due(regclass, float8, float8)
+	IS 'report whether an online maintenance verb (compact_rewrite, recluster) is worth running, from table statistics alone; thresholds are parameters with defaults measured on #415, each required to be a number between 0 and 1 inclusive (#860); pure report, takes no lock and rewrites nothing (#415)';

pgcolumnar--1.0-alpha3.sql

@@ -1788,6 +1788,43 @@ DECLARE
 	st_del  bigint;
 	ss      record;
 BEGIN
+	-- Validate both thresholds before reading anything (#860). Neither one was
+	-- checked, and this is the gate the autovacuum daemon consults BEFORE it ever
+	-- calls compact_rewrite, which does check its own. Four ways an unchecked
+	-- threshold goes wrong, none of which raises anything:
+	--   NaN   -- `fraction >= NaN` is false in IEEE, so nothing is ever due and
+	--            the work is suppressed silently and permanently.
+	--   > 1   -- the same outcome for any fraction: never due.
+	--   < 0   -- `fraction >= -1` is true for EVERY table, so the daemon believes
+	--            compaction is always due and rewrites every columnar table on
+	--            every pass. This is the dangerous direction: not a suppressed
+	--            report but a permanent, self-renewing rewrite.
+	--   NULL  -- the verdict is NULL, and the daemon reads a NULL verdict as
+	--            "not due" (SPI_getbinval isnull), so it is the NaN case again.
+	-- 0.0 and 1.0 are LEGAL and stay legal: 0.0 means "any decay at all is worth
+	-- acting on", 1.0 means "only a fully dead table". The bounds are inclusive,
+	-- matching pgcolumnar.compact_rewrite's own guard, and test/native_reclaim.sh
+	-- pins both endpoints as ACCEPTED so this guard cannot quietly become
+	-- over-broad, which is how a bounds check usually breaks.
+	--
+	-- The explicit NaN test is redundant with `> 1.0` today, because PostgreSQL
+	-- float8 ordering is not IEEE ordering: it sorts NaN above every other value.
+	-- It is written out anyway so the intent survives an edit to the bounds.
+	IF compact_due_fraction IS NULL
+	   OR compact_due_fraction = 'NaN'::float8
+	   OR compact_due_fraction < 0.0
+	   OR compact_due_fraction > 1.0 THEN
+		RAISE EXCEPTION 'compact_due_fraction must be a number between 0 and 1'
+			USING ERRCODE = 'invalid_parameter_value';
+	END IF;
+	IF recluster_due_fraction IS NULL
+	   OR recluster_due_fraction = 'NaN'::float8
+	   OR recluster_due_fraction < 0.0
+	   OR recluster_due_fraction > 1.0 THEN
+		RAISE EXCEPTION 'recluster_due_fraction must be a number between 0 and 1'
+			USING ERRCODE = 'invalid_parameter_value';
+	END IF;
+
 	-- stats() enforces require_caller_select(rel) before it returns a row, so a
 	-- caller without SELECT on rel is refused here rather than reported to.
 	SELECT COALESCE(sum(s.rowcount), 0), COALESCE(sum(s.deletedrows), 0)
@@ -1829,4 +1866,4 @@ END;
 $maintenance_due$;
 
 COMMENT ON FUNCTION pgcolumnar.maintenance_due(regclass, float8, float8)
-	IS 'report whether an online maintenance verb (compact_rewrite, recluster) is worth running, from table statistics alone; thresholds are parameters with defaults measured on #415; pure report, takes no lock and rewrites nothing (#415)';
+	IS 'report whether an online maintenance verb (compact_rewrite, recluster) is worth running, from table statistics alone; thresholds are parameters with defaults measured on #415, each required to be a number between 0 and 1 inclusive (#860); pure report, takes no lock and rewrites nothing (#415)';

src/columnar_vacuum.c

@@ -25,6 +25,8 @@
 #include "columnar_write_state.h"
 #include "columnar_compat.h"
 
+#include <math.h>
+
 #include "fmgr.h"
 #include "access/genam.h"
 #include "access/xact.h"
@@ -875,10 +877,10 @@ pgcolumnar_compact_rewrite(PG_FUNCTION_ARGS)
 		ereport(ERROR,
 				(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
 				 errmsg("table name cannot be null")));
-	if (minFrac < 0.0 || minFrac > 1.0)
+	if (isnan(minFrac) || minFrac < 0.0 || minFrac > 1.0)
 		ereport(ERROR,
 				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("min_deleted_fraction must be between 0 and 1")));
+				 errmsg("min_deleted_fraction must be a number between 0 and 1")));
 
 	PgColumnarRequireTableOwnerByOid(relid);