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

[OffgridwithJD]

Summary

• pgcolumnar.expire decided a row group was fully past retention from the zone-map maximum alone. That maximum ignores NULLs, so a group of expired timestamps plus NULL retention values was retired and the NULL rows disappeared.
• Retiring a live group also left the visibility-map bits VACUUM had set. An index-only scan then returned the expired keys from the index without fetching (the group is gone, so a fetch would have correctly failed).
• Keep any group whose retention column has a NULL. Clear the VM range covering a group before retiring it.

Test plan

• test/ttl_expire.sh on PostgreSQL 18 in cusor-2604 (NULL retention rows kept; index-only scan returns 0 after expire)
• Existing expire fixture still retires fully-expired groups and keeps straddling groups

Made with Cursor

[OffgridwithJD]

Premise verified — this is silent data loss, and worse than the summary says

Measured on pg18a. One row group holding 900 rows whose timestamps are all 400
days old, of which 90 have a NULL ts, retention 90 days. The fixture is gated:
it refuses to report a verdict unless there is exactly one group (the bug
needs the expired values and the NULLs in the same group), 900 rows, and 90
NULLs.

                        main        #869
premise: groups=1 rows=900 nulls=90 zone_map null_count=90   (both arms)
expire dropped          1 group     0 groups
rows after              0           900
NULL rows after         0           90

On main, pgcolumnar.expire deleted every row in the table, including all 90
whose retention was unknown. docs/sql-reference.md promises the opposite:

A group is kept whole or dropped whole. ... Retention is therefore approximate
at the group boundary, and it errs toward keeping data.

A NULL retention is not "expired"; it is "unknown". Deleting it errs the other
way, and nothing tells the user it happened. I would put the row counts in the
PR body — "the NULL rows disappeared" understates a table going to zero.

The fix is not vacuous — control run

A guard that returns early can pass a keep-the-rows test by never expiring
anything. Same fixture with no NULLs:

[#869 control] premise: groups=1 rows=900 nulls=0
[#869 control] expire dropped 1 group(s); rows after = 0

So a fully-expired group with no NULLs is still retired. That is your unchecked
box — "Existing expire fixture still retires fully-expired groups" — and it holds
on the NULL-free side at least.

z->nullCount is real, which is what the guard depends on

Worth recording because the guard is worthless if the field is not maintained:
columnar_write_state.c:1335 sets z->nullCount = group->rowCount - col->valueCount,
columnar_metadata.c:2394 persists it, :2847 reads it back. My run confirms it
end to end — zone_map.null_count was 90 for the group in question.

What I did not verify

The visibility-map half. I did not construct an index-only scan returning ghost
keys, so PgColumnarVMClearForRowRange is unmeasured by me. The block arithmetic
reads correctly (b0..b1 inclusive over
rowNumber / COLUMNAR_VALID_ITEMPOINTER_OFFSETS), and clearing before
PgColumnarRetireGroup rather than after is the right order, but that is reading,
not running.

One thing to change: the PR carries no docs

Four files, none of them docs/ or CHANGELOG.md. The rule here is that a PR
ships its documentation, and this one changes documented behaviour:
docs/sql-reference.md describes expire's approximation as a boundary effect,
where a straddling group is kept until every row in it has expired. The NULL rule
is a different and stronger case — a single NULL pins its whole group forever.
Those rows never expire, no matter how old the rest of the group gets, because the
condition never stops being true.

That is the safe direction and I would not change it, but a user reading the
current page cannot predict it. It needs a sentence, and the CHANGELOG needs the
data-loss note.

Reviewed as OffgridwithJD. Not approving — same account as the author.

[jdatcmd]

Reviewed adversarially. Requesting changes. The helper is right and it is wired into one of the three places that need it.

BLOCKING: two of the three sites that retire a live group are untouched

PgColumnarVMClearForRowRange is called from exactly one site. pgcolumnar.recluster and pgcolumnar.compact_rewrite both retire a live row group through the same PgColumnarRetireGroup, reassign those rows fresh row numbers, and leave the old row numbers' visibility-map bits set. An index-only scan then answers from the index for TIDs whose group is gone — which is verbatim the defect this PR describes as fixed.

The PR body states the general rule ("Retiring a live group also left the visibility-map bits VACUUM had set"), and the new comment repeats it as a rule, while the code applies it in one case out of three. Either the helper belongs inside PgColumnarRetireGroup where every caller gets it, or the two other call sites need it and the comment needs to stop claiming the general form.

MAJOR: the index-only-scan arm cannot fail while the VM bit is the thing under test

The arm at test/ttl_expire.sh:194 can only redden while VACUUM really wrote a VM bit, and nothing asserts that it did. The premise greps EXPLAIN for Index Only Scan, which is decided by pg_class.relallvisible and by enable_seqscan/enable_bitmapscan being off — not by the bit the fix clears. Make PgColumnarVMSetVisibleForRelation stop writing bits and the premise still passes, because the plan shape is unchanged.

Assert the bit: read relallvisible, or probe the VM directly, before relying on the plan shape as a proxy for it.

MAJOR: the new guard reads a write-time count, so it refuses groups it should retire

if (z->nullCount > 0) continue;

null_count comes from the whole-chunk zone map recorded at write time, and still counts rows that have since been deleted. So expire now refuses to retire a group in which every live row is past retention and no live row has unknown retention. The body and the comment both describe a live-row property; the code reads a historical one.

MAJOR: a negative ttl_interval is the same defect, uncovered

The title is "expire must not drop live rows", and the new guard covers the NULL case. A negative ttl_interval puts the cutoff in the future, so maximum < cutoff is true for groups entirely inside their retention and expire retires them — live rows dropped, which is the failure the title names. set_options range-checks every other option it accepts (encode_effort, compression, compression_level) and does not check this one.

MINOR: a group-level premise measured at table level

check "premise: the expired group also holds NULL retention rows" is named for a group-level fact but its got-expression is SELECT count(*) FROM ttl_null WHERE ts IS NULL — a table-level count that cannot see a row group. It passes just as readily on a fixture where the NULL rows occupy a group of their own, which is the arrangement the premise exists to exclude.

What is right

The defect is real, the helper is the right primitive, and clearing VM bits when row numbers are reassigned is the correct rule. It needs to be applied everywhere that rule holds.