fix: expire must not drop live rows or leave them visible to index-only scans

CHANGELOG.md

@@ -35,6 +35,42 @@ true until the next version shipped.
 
 ### Fixed
 
+- `pgcolumnar.expire()` no longer drops live rows, and every path that renumbers
+  live rows now clears the visibility map (#403).
+
+  **Three separate ways to lose data or read a row that is gone.**
+
+  A group holding a row with a `NULL` retention value could be retired, taking
+  live rows with it. `expire` now keeps any group whose retention column has a
+  LIVE `NULL`, because a `NULL` has no age and the group cannot be known to be
+  wholly expired.
+
+  Live is the operative word. The zone map's null count is recorded when the
+  group is written and never revised, so it still counts rows a later `DELETE`
+  marked. Reading it as the live count keeps a group whose every live row is
+  past retention and whose `NULL` rows have all been deleted, and keeps it
+  permanently, because nothing rewrites a zone map on delete. That trades data
+  loss for silent over-retention. `expire` now checks the live rows, and only
+  for a group that has both recorded `NULL`s and deletes: with no delete vector
+  the recorded count is still exact, so the metadata-only path is unchanged and
+  `expire` still reads nothing.
+
+  A negative `ttl_interval` put the cutoff in the future, so `expire` retired
+  groups that were entirely inside their retention. `set_options` range-checked
+  every other option it accepts and not this one. Zero and negative intervals
+  now raise `22023`.
+
+  Retiring a group leaves its old row numbers in the visibility map, so an
+  index-only scan answers from the index for a row group that is gone. `expire`
+  cleared them; `pgcolumnar.recluster()` and the partial-group rewrite behind
+  `pgcolumnar.compact_rewrite()` did not, and both renumber live rows through
+  the same retire. All three clear now. The rule is that visibility-map bits go
+  wherever row numbers are reassigned, not only where rows expire.
+
+  `docs/sql-reference.md` gains the accepted range for `ttl_interval` and the
+  `NULL` rule, which is stronger than the straddling behaviour the page already
+  described: a straddling group is released once its newest row ages past the
+  cutoff, and a group holding a `NULL` never is.
 - Arrow import reads the temporal unit and carrier width the file declares,
   rather than assuming the ones our own exporter writes (#864, #865).
 

docs/sql-reference.md

@@ -38,6 +38,10 @@ only by [`pgcolumnar.expire`](#pgcolumnarexpiretablename-regclass-returns-bigint
 which you run yourself. Declaring a retention does not delete anything on its
 own. Both are needed: either one alone means no retention.
 
+`ttl_interval` must be a positive interval. Zero and negative intervals raise
+`22023`. A negative interval would put the cutoff in the future, so `expire`
+would drop rows that are still inside their retention.
+
 ```sql
 SELECT pgcolumnar.set_options('events', sort_by => ARRAY['customer_id','ts']);
 SELECT pgcolumnar.reset_options('events', sort_by => true);   -- clear it
@@ -187,6 +191,17 @@ The retention column must be `timestamp` or `timestamptz`. The table must have
 both `ttl_column` and `ttl_interval` declared, or the function raises an error
 rather than reporting that it did nothing.
 
+`expire` works on whole row groups. It never reads or rewrites them, so it drops
+a group only when every row in it is past the retention. A group that straddles
+the cutoff stays whole, and rows older than the retention survive in it.
+
+One live `NULL` in the retention column pins its entire row group. A `NULL` has
+no age, so the group cannot be known to be wholly expired. Deleting the `NULL`
+rows releases the group, and the next `expire` can drop it. This is stronger than the straddling rule
+above. A straddling group is released once its newest row ages past the cutoff.
+A group holding a `NULL` never is. Keep the retention column `NOT NULL` if you
+want `expire` to reclaim the space.
+
 ```sql
 SELECT pgcolumnar.set_options('events', ttl_column => 'ts',
                               ttl_interval => '90 days');

pgcolumnar--1.0-alpha3.sql

@@ -497,6 +497,30 @@ BEGIN
 		RAISE EXCEPTION 'compression_level must be between 1 and 22';
 	END IF;
 
+	/*
+	 * A negative retention puts the cutoff in the FUTURE, so expire finds
+	 * `maximum < cutoff` true for groups that are entirely inside their
+	 * retention and retires them. That drops live rows, which is the failure
+	 * this option exists to prevent. Every other option here is range-checked
+	 * and this one was not.
+	 *
+	 * Zero is refused too. It is not a data-loss shape -- the cutoff is now, so
+	 * only groups already wholly in the past go -- but "expire everything older
+	 * than nothing" has no reading a caller means on purpose, and accepting it
+	 * silently makes a typo indistinguishable from an instruction.
+	 *
+	 * ERRCODE is explicit for the reason the relkind guard above gives: this
+	 * tree's suites assert SQLSTATE rather than message text, and plpgsql would
+	 * otherwise default to P0001.
+	 */
+	IF ttl_interval IS NOT NULL AND ttl_interval <= interval '0' THEN
+		RAISE EXCEPTION 'ttl_interval must be a positive interval, not %', ttl_interval
+			USING ERRCODE = 'invalid_parameter_value',
+				HINT = 'A negative retention puts the cutoff in the future, '
+				'so pgcolumnar.expire() would retire groups whose rows are '
+				'still within their retention.';
+	END IF;
+
 	/*
 	 * sort_by declares the physical sort key applied by vacuum_sorted() with no
 	 * explicit columns (#288). This is a cheap early check only: each named

src/columnar.h

@@ -385,6 +385,8 @@ extern uint64 PgColumnarItemPointerToRowNumber(ItemPointer tid);
  * visibility map for index-only scans (pgcolumnar_visibilitymap.c, gap 28)
  * ------------------------------------------------------------------------- */
 extern void PgColumnarVMClearForRow(Relation rel, uint64 rowNumber);
+extern void PgColumnarVMClearForRowRange(Relation rel, uint64 firstRowNumber,
+										 uint64 rowCount);
 extern uint64 PgColumnarVMSetVisibleForRelation(Relation rel);
 
 /* index maintenance for callers that insert rows without an executor (#153) */