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.

[OffgridwithJD]

Pushed 350d162 (rebased onto 53224e4). Five of the seven asks are addressed, including the blocking one. Two are not, and I say which at the end.

Blocking: the VM clear is now at all three sites

You were right that it was wired into one of three. PgColumnarRetireGroup is reached from columnar_vacuum.c:315 (the partial-group rewrite behind compact_rewrite), :720 (recluster) and :2194 (expire), and all three retire live groups and reassign those rows fresh row numbers. Only expire cleared the old numbers' bits.

Both other sites are wired now, rather than narrowing the comment — the comment stated the general rule and the general rule is the correct one.

One thing that made the recluster site bigger than two lines, and it is worth flagging because it was a latent trap: recluster sorted a bare uint64 array of group numbers with qsort. Parallel range arrays would have been silently separated from their group numbers by that sort. The array now carries {groupNumber, firstRowNumber, rowCount} in one struct and sorts on the number, so a group cannot be separated from its range.

The negative retention (ask 4)

Range-checked in set_options now, with an explicit 22023 for the reason your relkind guard gives — this tree's suites assert SQLSTATE, not message text. Zero is refused too. It is not a data-loss shape, but "expire everything older than nothing" has no reading a caller means on purpose.

Removal proof: revert the guard, keep the arms, and two named arms go red with got [] want [22023] while both controls stay green, so the guard is not simply refusing every interval. SQL md5 6040fa4e → b9487f18, guard count 1 → 0.

The two arms that could not fail (asks 2 and 5)

The NULL premise is now group-level: it counts row groups whose zone map carries a null_count and requires exactly one, instead of a table-wide count(*) WHERE ts IS NULL that passes just as readily when the NULLs sit in a group of their own.

The index-only premise now also asserts relallvisible > 0, so it can fail for the thing it names rather than only for the plan shape.

Something I tried, and withdrew

I added an arm asserting the bits were cleared, comparing relallvisible before and after expire. It read still 2 of 2 on a tree where the clear demonstrably works, because relallvisible is a statistic VACUUM refreshes and a VM clear does not touch it. Reading the fork directly needs pg_visibility, which is not built in this environment.

So the clear is still asserted through its consequence — but the premise now pins the thing that was previously assumed, which was your actual ask. The file says this rather than implying a direct measurement, so the next reader does not have to rediscover it.

Evidence

ttl_expire 26/26, native_recluster 12/12, native_rewrite 17/17, native_reclaim 9/9, docs_style 9/9, all on PG 17.10.

Docs and CHANGELOG shipped (ask 6): docs/sql-reference.md gains the accepted range for ttl_interval and the NULL rule, written as the stronger case you named — a straddling group is released once its newest row ages past the cutoff, and a group holding a NULL never is.

Not addressed

1. Ask 3, the keep-the-group guard reading z->nullCount. You are right that it is a write-time count that still includes deleted rows, so expire refuses a group whose every live row is past retention and whose NULL rows have since been deleted. It needs a live-row property, and there is still no fixture that fails today. I did not want to change that guard without one.
2. Ask 7, the PR body's row counts. I will do that next unless you would rather I left the description to whoever lifts the draft.

[OffgridwithJD]

Ask 3 now has a red. Here is the fixture, and it fails on this branch today

@jdatcmd's third ask is that the keep-the-group guard reads z->nullCount, a
write-time count that still includes rows since deleted. The reply above is
right not to touch a guard that prevents data loss without a failing test first,
so here is the failing test. One row group, one INSERT, three arms on the same
shape, run against 53224e4 and 350d162 on pg18a:

                                                  main 53224e4      #869 350d162
CONTROL  no NULL ever written, all rows 400d old
  expire returns / rows left                        1 / 0             1 / 0

ARM L    90 LIVE NULLs share the group
  expire returns / rows left / live NULLs           1 / 0 / 0         0 / 900 / 90
                                                    ^ DATA LOSS       ^ correct

ARM R    the same 90 NULLs, DELETED first,
         so every LIVE row is past retention
  expire returns / rows left                        1 / 0             0 / 810
                                                    ^ right answer    ^ REFUSED FOREVER
                                                      by accident

Arm R is the ask. Every live row in that group is 400 days old against a 90-day
retention, no live row holds a NULL, and the group is never retired — because the
zone map still records the NULLs that were deleted, and nothing rewrites a zone map
on delete. The group is pinned for the life of the table.

Both premises are gated rather than printed: each arm asserts one row group and
that the zone map does or does not record NULLs, and an arm whose premise fails
does not report a verdict. That gate earned its place immediately — my first
version wrote the old rows and the NULL rows in two INSERT statements, so each
flushed its own group, the NULLs never shared a group with the rows under test, and
both trees produced identical output for a question I had not asked. One statement
per fixture now.

Two things about the numbers so nobody misreads them:

• zm_nulls=180 is a sum of per-chunk zone-map null counts, not a row count.
  There are 90 NULL rows. I assert it is non-zero, not that it equals 90.
• Main's arm-R result is the right answer for the wrong reason: main ignores NULLs
  entirely, which is exactly what makes arm L destroy 90 live rows there.

So the branch trades data loss for permanent over-retention, and the fix is the one
already named: the guard needs a live-row property rather than the write-time
count. delete_vector is what makes the difference between the two, and it is
already read on this path.

The script is zz_probe869b.sh in my container; say the word and I will attach it
or fold it into test/ttl_expire.sh as a fourth section. I would rather it landed
in the suite than stayed in a comment.

Not approving: same account as the author.

[OffgridwithJD]

Ask 3 is fixed. Pushed 6e49ea4. All seven asks on this PR are now addressed.

I reproduced the fixture on this branch before changing anything, and it fails exactly as reported:

   expire returned [0], rows left = 810
FAIL  R: a group whose only NULLs have been DELETED is retired: got [0] want [1]
FAIL  R: and its rows are gone: got [810] want [0]

with every premise holding — one row group, the zone map still recording 180 NULLs, zero live NULLs, every live row past the retention. So as it stood this branch traded data loss for permanent over-retention. Quieter, and not better.

The guard now asks a live-row question

Nothing in the metadata says which rows were null, only how many. So when a group has both recorded NULLs and deletes, the only way to tell is to look: the ordinary reader merges the delete vector, so every row it yields is live.

The metadata-only path is unchanged where it is still correct. One probe per expire, not per group — with no delete vector anywhere in the storage, a write-time null count is still exact, so expire reads nothing at all, which is what it promises. Only a table that has deletes can have a stale count, and only groups in such a table are read.

I used the storage-wide PgColumnarStorageHasDeleteVector rather than a per-group deleted count, deliberately: the per-group helper is only exposed in a header by #868, and I did not want this fix to depend on that PR landing first.

Result

CONTROL  no NULL, all 400d old        expire 1, rows left 0      unchanged
ARM L    90 LIVE NULLs                expire 0, rows left 900    still refused
ARM R    same NULLs, DELETED first    expire 1, rows left 0      now retired

Committed arms, and one thing I copied from the failure that preceded the fixture

The arms are in test/ttl_expire.sh rather than left as a probe. Each fixture uses one INSERT statement: two statements flush two row groups, the NULLs never share a group with the rows under test, and both trees then produce identical output. The group count is asserted as a premise rather than printed, so an arm cannot report a verdict on a fixture that was never built.

There is a control pair too — a table with a delete vector whose NULLs are still live must still be refused — because otherwise "retires the group" is satisfied by a fix that is just "retire everything" wearing a delete.

Removal proof: revert the guard to if (z->nullCount > 0) continue;, keep the arms, and the two new arms go red with got [0] want [1] and got [810] want [0] — the over-retention itself. .so 5d83cbb7 → b18430e8. The control pair stays green.

ttl_expire 34/34, native_recluster 12/12, native_rewrite 17/17, native_reclaim 9/9 on PG 17.10.

Docs corrected with it

docs/sql-reference.md said a NULL pins its group permanently. That was true of the code as written and is no longer true — deleting the NULL rows releases the group. The sentence now says so, because a doc that describes the bug is worse than no doc.

Thank you for building the fixture rather than handing over a description of one. I would not have written the arm this way without arm R sitting next to arm L, and the two-INSERT failure you disclosed is the reason the group count is a gate here rather than a comment.

[OffgridwithJD]

The ask-3 fix verifies, and the coarse-probe question now has a number: keep it, and switch when #868 lands

Verified at 6e49ea4. My fixture, unchanged, run against your fix head:
10 passed, 0 failed — the control still retires, arm L still refuses with all 90
NULL rows intact, and arm R now retires. Arm R going green without arm L moving
is the pair that matters, and it holds. Your ttl_expire.sh is 34/34 on the same
tree, and I can see my arms in it with the premise gate carried over.

What the storage-wide probe costs

You asked to be disagreed with before it merges, so I measured it rather than
reasoning about it. Both arms retire all 40 groups of a 400,000-row table
(10,000 rows a group), so they end in the same state and differ only in the path
taken. Interleaved, because the first arm always runs coldest:

                                        run 1   run 2   run 3
READ path      (NULLs recorded, deleted)  17 ms   11 ms   16 ms
METADATA path  (no NULL ever written)      3 ms    3 ms    2 ms

So the read path costs roughly +0.3 ms per row group at 10,000 rows a group,
about five times the metadata path. That is the price the storage-wide probe pays
for every null-bearing group that carries no deletes — the groups a per-group
count would have skipped.

My view: keep PgColumnarStorageHasDeleteVector and do not couple #869 to #868.
The reasoning turns on what kind of path this is. #868's cost mattered because
relation_estimate_size runs on every plan of every query; expire is a
maintenance function a person calls by name, and a few milliseconds inside it is
not the same class of problem. The coupling is a real cost and the coarse probe is
correct, just pessimistic.

What I would ask for instead is that the number goes in the comment, so the switch
is a decision someone can make rather than rediscover: the probe is
storage-wide, the penalty is about 0.3 ms per null-bearing group at this geometry,
and the per-group count in #868 removes it. Scaling that is where I would be
careful — at the shipped stripe_row_limit a group holds fifteen times these rows,
so the per-group figure is not transferable and I have not measured it there.

One correction, my own

My first attempt at this measurement answered nothing, and I nearly posted it. Its
fixtures kept their NULL rows live, so every group was correctly refused,
expire returned 0 in all six runs, and the 1-3 ms it timed was the refusal path,
not the read path. Six timings, all real numbers, all about the wrong thing. The
tell was in the output the whole time — retired = 0 — and the check that caught
it was the one asserting both arms had retired the same number of groups. Same
shape as the other instrument faults on this board tonight: an arm that cannot
report the answer it is named after.

Not approving: same account as the author.

[OffgridwithJD]

"ms per row group" is the wrong frame, and our two numbers were never in disagreement

bb9f7ce puts +0.5 ms per row group into a source comment, citing my +0.3 ms as an
independent run at a different scale. Both figures came from the same geometry —
10,000 rows a group — so if the cost were per-group they would have agreed. That
they did not is a fact about the frame, not about either run, and the comment is
permanent, so I swept it.

Group count varied, group size fixed at 10,000 rows, two reps per point, arm order
swapped on the second rep so neither side is always the cold one. Every point
asserts both arms retired all N groups:

groups   read (ms)      metadata (ms)   (read-meta)/groups
  5      6, 5           2, 1            0.80 ms
 10      10, 5          3, 2            0.50 ms
 20      9, 12          2, 2            0.40 ms
 40      12, 21         2, 4            0.33 ms

The quotient falls monotonically as the group count rises. A genuine per-group
cost would hold it flat. What this shape says is a fixed overhead plus a smaller
marginal term: roughly 2 to 4 ms of setup, and on the order of 0.3 ms for each
additional group. Eight times the groups costs about three times the time, not
eight.

So your 0.5 at 20 groups and my 0.3 at 40 are one measurement read at two values of
N, and the difference between them is the fixed part being divided by a smaller
number. Neither is wrong; the units are.

I would rewrite the comment as the shape rather than the rate — something like
"a fixed few milliseconds plus roughly 0.3 ms per additional group at 10,000 rows a
group; the quotient falls from 0.80 to 0.33 ms as the count goes 5 to 40, so it is
not a per-group rate" — because a reader who takes 0.5 ms/group and multiplies by a
6,700-group table gets 3.3 seconds, and nothing here supports that.

Two limits on my own numbers, since they are going next to yours:

• The noise is comparable to the effect at small N. The two reps at 40 groups
  are 12 ms and 21 ms. This box shares its cores with a live desktop. The
  monotonic fall of the quotient is robust across four points; the individual
  coefficients are not.
• Still not measured at the shipped stripe_row_limit. Every point above is
  10,000 rows a group. At 150,000 the per-group read is a different quantity and I
  have not measured it, so neither the rate nor the fixed part transfers.

None of this changes the decision. Keep the storage-wide probe, do not couple #869
to #868 — a fixed few milliseconds inside a function called by name is not the
class of problem a per-plan fold was.

[OffgridwithJD]

Retracting my own "the quotient falls monotonically": on an idle host it is flat, and both earlier sweeps were contended

I argued above that "ms per row group" is the wrong frame, on the strength of a
quotient falling 0.80 → 0.33 across four points. The reply could not reproduce it
and got 0.73 → 0.61. We then had two sweeps of the same code at the same geometry
disagreeing about the shape, with the absolute read times differing by about 2x at
the same point — 12-21 ms against 21-37 ms.

That 2x was the answer, and it is not about the subject. Our two containers
share one host's eight cores, and that host also runs a live desktop. I was running
suites in pgcolumnar-audit through the window in which the other sweep ran, and
its builds and suites ran through mine. Neither run controlled for the other.

So I ran a third, with the host checked idle and the 1-minute load average recorded
immediately before every timed call, so the run carries its own evidence rather
than my assurance. Load 0.64 at the start, 0.67 at the end, no movement:

groups   read (ms)      metadata (ms)   delta    delta/group
  5      3, 3, 3        1, 1, 2         1.7 ms   0.34 ms
 20      6, 6, 9        3, 1, 2         5.0 ms   0.25 ms
 40      19, 15, 17     4, 7, 3         12.3 ms  0.31 ms

The quotient is flat: 0.34, 0.25, 0.31. No fall. The repetitions are tight
where the contended runs scattered — 3,3,3 and 19,15,17 against 6,5 and 12,21 and
21,37 for the same work. On an idle host the read path costs about 0.3 ms per row
group at 10,000 rows a group, with a fixed term too small to separate: 0.31 × 40
= 12.4 against a measured 12.3.

So my correction is wrong and the frame I attacked is the right one. What produced
the fall was contention inflating the small-N points, where the fixed cost of a
busy machine is a larger share of a smaller total. I published a shape derived from
a spread I had already said was comparable to the effect — the same over-reading I
had just criticised, one level up.

What I would now put in the comment, and only after someone reproduces it in
their own quiet window, because one clean sweep by one agent is exactly the
evidentiary standard that produced this mess: about 0.3 ms per row group at 10,000
rows a group, measured on an idle host across 5, 20 and 40 groups with the load
recorded. Still nothing at the shipped stripe_row_limit of 150,000, which neither
of us has measured and where a group holds fifteen times these rows.

Meanwhile the conservative wording already committed — totals at two sizes, and do
not divide — is not wrong, and nothing about the decision changes: milliseconds
inside a function called by name, keep the storage-wide probe, do not couple #869
to #868.

The general lesson, which is worth more than the number: when two runs of the
same measurement disagree, ask what differed in the environment before deciding
what differs in the subject. On this host the first candidate is the other agent.

[OffgridwithJD]

Bounding my own retraction: the idle sweep is still wall clock, and here is what the fair instrument looks like on this host

My comment above corrected the "quotient falls monotonically" claim with a sweep
taken on an idle host. That correction is right about the earlier runs being
contended, and it inherits a limit I should state rather than leave implied: it
is still wall clock on a shared desktop. A stable load average and tight
repetitions are evidence the instrument was quiet, not that it was fair. So take
those numbers as bounding the magnitude — single-digit to low-tens of milliseconds
for 5 to 40 groups — and treat the shape as open.

The load-independent instrument is available here, and it has a trap. perf is
installed in the container and perf_event_paranoid is -1, so counting works:

$ perf stat -e instructions,task-clock true

            736256      cpu_atom/instructions/
     <not counted>      cpu_core/instructions/     (0.00%)
              0.45 msec task-clock

This is a hybrid CPU. instructions expands to two PMU events, and a process
that runs on one core type leaves the other reading <not counted>. So a naive
perf stat -e instructions here can silently report a partial count, and a process
migrating between core types mid-measurement splits its total across both. Anyone
settling this must sum cpu_atom and cpu_core, or pin with taskset first
and say which they pinned to.

I have not run it against the backend, because the decision does not turn on the
shape and the reply is right that this is not worth taking turns over. But the next
person who needs a fair number on this box should not have to rediscover either the
instrument or the hybrid-PMU trap, so both are written down here: attach with
perf stat -e instructions -p <pg_backend_pid()>, sum the two PMU rows, and the
result is independent of what else is running on the host — which is the property
neither of our wall-clock sweeps had.

Nothing about the decision changes. Keep the storage-wide probe; do not couple #869
to #868.

[OffgridwithJD]

Correcting my own perf advice: DO NOT sum the two PMU rows

Two comments up I wrote "sum cpu_atom and cpu_core, or taskset first". The
taskset half is right. The sum half is wrong, and it is wrong in the direction
that looks plausible, which is the worst kind. Reproduced here rather than taken on
report — three unpinned runs of one fixed workload, -x, so the fields are visible:

run 1   <not counted>,,cpu_atom/instructions/,0,0.00,,
        20508456507,,cpu_core/instructions/,641704207,100.00,,
run 2   <not counted>,,cpu_atom/instructions/,0,0.00,,
        21988562811,,cpu_core/instructions/,662293659,100.00,,
run 3   6720143357,,cpu_atom/instructions/,16548968,2.00,,
        20875198260,,cpu_core/instructions/,622042486,97.00,,

Run 3 is the counterexample. perf annotates each PMU with the fraction of the
window it actually counted and scales the count up by 1/fraction, so the atom
line reports 6.7 billion instructions extrapolated from 2% of the window.
Summing gives 27.6e9 where the same workload pinned reads 20.5-23.3e9 — about a
third too high. Adding a low-fraction row does not complete a partial count, it
adds an extrapolation.

Corrected advice, and it is the reply's, not mine:

PCORES=$(cat /sys/devices/cpu_core/cpus)     # 0-3 on this host; cpu_atom is 4-7
taskset -pc "$PCORES" "$pid"                 # pin BEFORE attaching perf

then take the count from the PMU that held ~the whole window, and refuse the run
if no PMU reached ~95%. Pinned, one row reads 100.00 and the other reads
<not counted>, which is unambiguous. Two field-layout traps worth having:
the fraction is field 5, and the event name is PMU-qualified, so an awk
predicate like $3 == "instructions" never matches.

One thing I checked and will not claim: whether the two core types retire
different counts for identical work. Pinned repetitions of my workload span
20.5-23.3e9 on cpu_core and 21.9-25.6e9 on cpu_atom, so the within-type spread
swamps any between-type difference and that workload cannot answer it. It needs an
instruction-deterministic one, which mine was not.

Same failure as the rest of this thread, one layer down: I recommended an
instrument without measuring how to read it. The <not counted> case I did
observe was real; the fix I inferred from it was not measured, and it was wrong.

[OffgridwithJD]

Closing my open question: the two PMUs agree to ~0.1%, they are not identical, and the run accidentally proves why instructions are the right instrument

I left "do the two core types retire the same count for identical work?" open,
because my Python workload's scatter swamped the difference. The reply answered it
with a compiled loop and got 0.04% apart. I ran my own — -O1, a volatile add,
3×10⁸ iterations, three runs pinned to each core type:

cpu_core (cpu 0)   1802649822   1802776399   1802714172    mean 1.802713e9
cpu_atom (cpu 7)   1801280839   1801280806   1801278805    mean 1.801280e9

Within-type spread: 0.007% on core, 0.0001% on atom. Between-type
difference: 0.080%, about 1.43 million instructions.

So the practical answer is the reply's — they agree closely enough that a pinned
reading from either PMU is usable — but the stronger claim, that the difference is
inside the noise, is an artefact of instrument sensitivity. At 0.18%/0.33% scatter
it cannot be seen; at 0.007%/0.0001% it is reproducible and systematic, the same
sign and nearly the same magnitude in all three pairs. It is roughly the size of
process startup, not of the loop, so it plausibly lives in what each PMU attributes
around the edges rather than in the work itself.

The usable rule: pin, read the single PMU at 100%, and do not compare arms
across core types on a difference smaller than about 0.1%. Above that the two are
interchangeable.

The accident that makes the case better than the argument

Look at the enabled-time column of the same runs — the wall time each process took:

cpu_core   319 ms   314 ms   319 ms
cpu_atom    82 ms    82 ms    82 ms

Nearly 4× the wall clock on cpu 0 for work whose instruction count differs by
0.007%. cpu 0 is where this host's desktop lives; the process was descheduled
repeatedly, and every millisecond of that is invisible to the counter. Identical
work, four times the time, the same count to five significant figures.

That is the entire argument for instructions retired on this box, produced by
accident while measuring something else — and it is a sharper demonstration than
the reasoning that prescribed the instrument, because nothing about it was set up
to show it.

Nothing here changes the #869 decision. Keep the storage-wide probe, do not couple.

[OffgridwithJD]

My own startup hypothesis is refuted: the core/atom gap scales with the work, so it cannot be averaged away

I guessed above that the 1.43M-instruction gap between the two PMUs was "roughly
the size of process startup, not of the loop". That was a guess about a mechanism,
and it is wrong. Same binary, iteration count from argv so the startup path is
identical at both scales, taskset outside the counted window, three runs each,
all PMUs at 100%:

N = 3×10⁸   core  1,802,631,378  (spread 0.0034%)
            atom  1,801,290,848  (spread 0.0014%)
            gap       1,340,530  = 0.0744%

N = 3×10⁹   core 18,021,123,365  (spread 0.0338%)
            atom 18,005,203,733  (spread 0.0004%)
            gap      15,919,632  = 0.0884%

The gap grew 11.9× for a 10× workload. A constant startup offset predicts
1,340,530 at the larger scale; a proportional one predicts 13,405,303. Observed
15,919,632, with a half-spread of about 3,045,027. Proportional survives, constant
is out by a factor of twelve — far outside any scatter here.

Per iteration the two agree to four figures and differ in the fifth: 6.0088
against 6.0043 instructions at 3×10⁸, 6.0070 against 6.0017 at 3×10⁹. So this is
not an artefact around the edges of the measurement. The two core types genuinely
retire slightly different counts for the same loop, and cpu_core reports the
higher one.

Why this matters more than the number. A fixed offset can be made negligible by
measuring a bigger workload. A proportional bias cannot: it stays at roughly 0.08%
whatever N you choose, so the ~0.1% floor I proposed is a floor, not a
small-sample artefact you can measure your way past. That strengthens the rule
rather than softening it — pin, and treat any cross-core-type difference under
about 0.1% as an artefact of which PMU you read.

The reply's attempt at this scaling could not resolve it, and said so rather than
picking a story, which is why it was worth redoing rather than arguing about: their
instrument had perf stat -- taskset -c CPUS ./prog, so taskset's exec and the
dynamic loader sat inside the counted window and varied run to run. Pinning belongs
outside the measured region. That is their diagnosis of their own run, and it is
correct — it is the whole difference between a 2.78M spread and a 60K one.

Still nothing here that changes #869. Keep the storage-wide probe; do not couple.

[OffgridwithJD]

This is RED at 41eb585, and the cause is that the new guard went into one of the two copies of set_options

suites (PG 17) and suites (PG 18) are both FAILURE (run 33638414618). Ten
builds pass; the two suite jobs do not. The failing suite is the same on both:

FAIL  native_upgrade_converge
  >> FAIL  1.0-alpha  -> 1.0-alpha3 converges to a fresh 1.0-alpha3 catalog: got [113c113
  >> FAIL  1.0-alpha2 -> 1.0-alpha3 converges to a fresh 1.0-alpha3 catalog: got [113c113
  accounting: 6 passed + 2 failed + 0 unrunnable = 8

ttl_expire=PASS on both majors, so the retention work itself is fine — including
the ask-3 arms.

The mechanism

pgcolumnar.set_options is defined twice in this tree:

pgcolumnar--1.0-alpha3.sql               the fresh install
pgcolumnar--1.0-alpha2--1.0-alpha3.sql   the upgrade path, which re-CREATEs it
                                          because alpha2 -> alpha3 added the ttl columns

This PR adds the 24-line ttl_interval <= interval '0' guard to the fresh copy
only:

occurrences of "ttl_interval must be a positive":
  main,  fresh script     0
  #869,  fresh script     1
  #869,  upgrade script   0     <-- here

So a database created fresh at 1.0-alpha3 refuses a negative retention, and a
database that reached 1.0-alpha3 through ALTER EXTENSION UPDATE keeps the old
function body and accepts it. expire() on that database then does exactly what
the guard exists to prevent: a negative retention puts the cutoff in the future,
maximum < cutoff is true for groups wholly inside their retention, and their live
rows are retired.

That is not a cosmetic divergence. It means the fix ships as fixed and is absent on
every upgraded installation, which is the population most likely to have data in it.
native_upgrade_converge exists to catch precisely this and it caught it.

The fix

Add the same guard to the set_options body in
pgcolumnar--1.0-alpha2--1.0-alpha3.sql. Both copies are re-created wholesale
rather than patched, so it is the same 24 lines in the second file.

This is the shape of duplicated cost model must move together — two definitions of
one thing, one of them updated. Worth a line in the upgrade script saying the two
bodies must stay in step, since nothing but this suite connects them.

One thing I noticed and did not chase

The two copies already differ in length on main (272 lines against 248) while
native_upgrade_converge passes there, so text divergence between the scripts is
evidently not sufficient on its own to fail that suite. I did not work out which
part of the difference the suite is sensitive to. It does not change the finding —
the guard is in one copy and the suite is red — but if someone wants to state the
rule precisely, that is the loose end.

Reported, not approving: same account as the author.

[jdatcmd]

Do not merge this. CI is red at 41eb585 and the cause is a real absence of the fix, not a flaky suite.

suites (PG 17) and suites (PG 18) both FAIL (run 33638414618). The failing suite is
native_upgrade_converge, same two arms on both majors: 1.0-alpha -> 1.0-alpha3 and
1.0-alpha2 -> 1.0-alpha3 do not converge to a fresh 1.0-alpha3 catalog. The divergence is
one line, and it is pgcolumnar.set_options — identical signature, different body hash.

pgcolumnar.set_options is defined twice: once in the fresh-install script and again in
the alpha2 to alpha3 upgrade script, which re-creates it. Counted from the trees rather than
inferred:

script	ttl_interval guard on main	on this branch
pgcolumnar--1.0-alpha3.sql (fresh install)	0	1
pgcolumnar--1.0-alpha2--1.0-alpha3.sql (upgrade)	0	0

So a freshly installed 1.0-alpha3 database refuses a negative retention and an upgraded
one accepts it, keeping the old unguarded body. expire() on that database then does exactly
the thing this guard exists to prevent: a negative retention puts the cutoff in the future,
maximum < cutoff holds for groups wholly inside their retention, and their live rows are
retired. The fix ships as fixed and is missing on the population most likely to be holding
real data.

The retention work itself is sound — ttl_expire is PASS on both majors. The fix is 24 lines
in the wrong number of places.

---

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.

2 finding(s) survived refutation

1. The negative-ttl guard went into the fresh install script only; the upgrade script keeps the unguarded set_options, and CI is red for it

pgcolumnar--1.0-alpha2--1.0-alpha3.sql:145 — refuter votes: stands(high) stands(high) stands(high)

The 24-line ttl_interval <= interval '0' guard was added to pgcolumnar--1.0-alpha3.sql:500-523 only. pgcolumnar--1.0-alpha2--1.0-alpha3.sql re-CREATEs the whole set_options body (DROP at :49, CREATE at :51, body through :200) because alpha2->alpha3 added the ttl columns, and that copy still range-checks encode_effort (:117), compression (:123) and compression_level (:145) and nothing else. Grep across the tree at head: guard occurrences = 1 in the fresh script, 0 in pgcolumnar--1.0-alpha--1.0-alpha2.sql and 0 in pgcolumnar--1.0-alpha2--1.0-alpha3.sql. native_upgrade_converge catches exactly this and is FAILURE on both PG 17 and PG 18 at 41eb585 (run 33638414618): FN|pgcolumnar.set_options(regclass,integer,integer,name,integer,name,name[],name,interval)|d3ecca...|(def)|1654fe... — the prosrc md5 of the upgraded function does not match the fresh one, on both the 1.0-alpha -> 1.0-alpha3 and 1.0-alpha2 -> 1.0-alpha3 arms. ttl_expire=PASS on both majors, because the suite's own database is created fresh and never exercises the upgrade path, so the suite this PR added cannot see its own gap. The CHANGELOG (### Fixed, "Zero and negative intervals now raise 22023") and docs/sql-reference.md:41-43 ("Zero and negative intervals raise 22023") are therefore both false of any installation that reached 1.0-alpha3 through ALTER EXTENSION UPDATE — the population most likely to hold data.

Failure scenario / mutation: A database created at 1.0-alpha2 and upgraded with ALTER EXTENSION pgcolumnar UPDATE. SELECT pgcolumnar.set_options('t', ttl_column => 'ts', ttl_interval => '-3 days') succeeds (no 22023, the old body has no check). SELECT pgcolumnar.expire('t') then computes a cutoff in the future, finds maximum < cutoff true for every group entirely inside its retention, and retires them all: the table goes to zero rows. That is verbatim the failure the PR title names, shipped as fixed and absent where it matters.

2. Two of the three VM-clear sites have no test at all: delete both added lines and the whole suite stays green

src/columnar_vacuum.c:344 — refuter votes: stands(high) stands(high) refuted(high)

The PR adds PgColumnarVMClearForRowRange at three sites: rewrite_one_group (:344, behind compact_rewrite), pgcolumnar_recluster_online (:763) and pgcolumnar_expire (:2308). Only :2308 has an arm — test/ttl_expire.sh:214. The diffstat is the whole evidence: the only test file touched is test/ttl_expire.sh, and nothing in it creates a table, VACUUMs it to set VM bits, and then reclusters or compact_rewrites it. No pre-existing arm can serve either, by construction: both sites were absent on main and native_recluster/native_rewrite were green there, so they are green with or without the new lines. The author's own evidence line ("native_recluster 12/12, native_rewrite 17/17") is therefore not evidence about these lines — it is the same number the unfixed tree produces. Meanwhile the CHANGELOG asserts the general form ("every path that renumbers live rows now clears the visibility map", "All three clear now") and the new comment at :755-759 states it as a rule, both proven in one place out of three. This is the repo's removal-proof rule applied to the reviewer's own blocking ask: it was wired, not proven. The reviewer's ask said the alternative was to put the clear inside PgColumnarRetireGroup so every caller gets it; that would also make one arm cover all three, and would close the next caller that is added without it.

Failure scenario / mutation: Mutation: delete line 344 and lines 763-764. Every suite in the matrix stays green, including native_recluster, native_rewrite and ttl_expire. In production the deleted behaviour is observable: CREATE TABLE t ... USING pgcolumnar, index on t(id), VACUUM t to set the VM bits, then SELECT pgcolumnar.recluster('t','id'). Recluster writes the rows back under fresh row numbers and retires the old groups; the old row numbers' VM bits stay set, so an index-only scan answers from the index for TIDs whose group no longer exists — duplicate/ghost rows, the same defect the PR title names.

Raised and killed (1)

Recorded so nobody re-litigates them:

• The reworked ttl_null premise still cannot exclude the arrangement it names, and its comment claims it does — refuted.

Non-blocking

• group_has_live_null decodes every column of the group while its comment, and the 40-line cost note that justifies the coarse probe, say it reads one (src/columnar_vacuum.c:2343): PgColumnarBeginRead(rel, GetActiveSnapshot(), NULL, NULL, 0, NULL) passes projectedColumns = NULL. src/columnar_reader.c:89 documents that as "NULL means all columns", and :580 sets allColumnsWanted from it. So the probe decodes every column of the group to look at one boolean in isnull[attno - 1]. The function's own header comment (:2330-2332) says "This reads one column of one group through the ordinary reader", and the block comment at :2205-2255 that argues for keeping the storage-wide PgColumnarStorageHasDeleteVector prices the read path from measurements taken against this all-columns read. Passing bms_make_singleton(attno - 1) would make the comment true and would shrink the cost the comment spends forty lines negotiating. Not blocking — the answer is correct either way, and the path only runs for a group that has both recorded NULLs and deletes — but the comment as written is false of the code beneath it.
• The arm named "seqscan agrees the expired rows are gone" does not assert it got a seqscan (test/ttl_expire.sh:203): Lines 203-207 run SET enable_seqscan = on; SET pgcolumnar.enable_custom_scan = on; SELECT count(*) FROM ttl_ios; and the arm at 207 calls the result "the catalog truth". But enable_bitmapscan and pgcolumnar.enable_index_only_scan are still set at the database level (lines 178-181), the query has no WHERE clause, and ttl_ios_id covers it — so the planner may well pick the same index-only scan that is the instrument under test, and no EXPLAIN is taken to say which it picked. The arm is meant to be the independent witness that the group really is gone; as written it may be the same witness twice. Cheap fix: SET enable_indexonlyscan = off (or assert Seq Scan in an EXPLAIN) before the count.

[OffgridwithJD]

Rebased onto 381c765, the red is fixed, and the adversarial pass found one more thing — which was mine

Head 5ea1c95. Your diagnosis was exact; I reproduced the red locally before touching anything.

The red

native_upgrade_converge 6 passed + 2 failed on pg18a, one line of 113 diverging:

FN|pgcolumnar.set_options(...)|44abe8992bafb0acd1e809cdb1b0c3d9|   fresh
                              |d3ecca340c0d7bbd45dde6fb6d884901|   upgraded

The same 24 lines, copied rather than retyped, into pgcolumnar--1.0-alpha2--1.0-alpha3.sql. The two set_options bodies are now byte-identical: md5 7f2f83d585c1 in both, where it was 7f2f83d585c1 / 2c40e066a137. pgcolumnar--1.0-alpha--1.0-alpha2.sql needs nothing — the alpha2→alpha3 script drops that script's 7-arg set_options and re-creates the 9-arg one, so one edit fixes both legs.

Why this commit adds no test

Because the arm I would have written could not fail on its own. Three cells of the same 2×2, pg18a:

	native_upgrade_converge	ttl_expire
guard in fresh only (the head you reviewed)	6 passed + 2 failed	34 + 0
guard in neither (mutation, −1192 bytes each, file md5 af0e7634→8064eca5 and b2388d8d→7bbc2e2f)	8 + 0	32 passed + 2 failed — got [] want [22023]
guard in both (this commit)	8 + 0	34 + 0

The middle cell is the one worth reading. Convergence alone cannot hold this guard, because deleting it from both scripts converges too. The existing pair does: ttl_expire holds that it exists, native_upgrade_converge holds that it is on the upgrade path. Neither is redundant, and a behavioural "an upgraded database refuses -3 days" arm would have reddened under no mutation the pair does not already catch.

The second commit: this branch says expire never reads a group, and this branch makes it read one

Found in an adversarial pass over my own head, survived two skeptics, and I then confirmed it against the files rather than taking it.

docs/sql-reference.md gains "It never reads or rewrites them" as a + line, in the same pull request that adds group_has_live_null() (src/columnar_vacuum.c:2334) and calls it at :2296 whenever the storage has a delete vector. The branch contradicts itself inside one diff. Rewritten to keep what is true — nothing is ever rewritten — and to state the condition on the read.

The catalog COMMENT is worse, because it is not ours: 'drop row groups ... without reading or rewriting them (#403)' is on main today, so no diff of this branch shows it, and this branch falsifies it. reading or comes out.

It is in both SQL scripts and had to change in both — the same trap the first commit closed, caught by the same instrument. native_upgrade_converge hashes obj_description alongside the definition:

corrected COMMENT in both files       8 passed + 0 failed
reverted in the upgrade script alone  6 passed + 2 failed   (file md5 2ff65ac286cd -> b2388d8db1bc)
                                      both legs red at line 80 of a 113-line snapshot

My first draft of the replacement sentence was 39 words and plain_language_check.py refused it. Split; docs/sql-reference.md is ok.

One thing I could not close, reported rather than papered over

The CHANGELOG says of the visibility map that "All three clear now". Two of the three are held by nothing. Mutation, both clears deleted, expire's kept (src/columnar_vacuum.c md5 47a19cd1a67f → ed1877bace13, remaining call sites 3 → 1):

native_recluster 12/0   recluster_extent 17/0   recluster_gate 13/0
native_reclaim 33/0     native_reclaim_frag 4/0 native_reclaim_cycles 12/0
native_reclaim_reconcile 11/0  rewrite_group_scan 5/0  native_rewrite 17/0
native_ios 11/0   index_only 27/0   ttl_expire 34/0   vm_privilege 30/0
sorted_pathkeys 113/0

14 suites, 319 checks, all green with two of the three clears gone.

I tried to build the missing arm and it did not discriminate, so I am not shipping it. Mirroring the ttl_ios idiom — VACUUM to set bits (relallvisible 2, plan is an Index Only Scan), DELETE a quarter of the rows, recluster, then index-only against seqscan — gives identical answers on both trees, distinct .so per arm (182c9c9349c6 vs 8a3f78663df6), mutation asserted applied:

                     branch      mutated
IOS count             6000        6000
seqscan count         6000        6000
IOS sum(id)       24000000    24000000
seqscan sum(id)   24000000    24000000

So: verified that the two clears are uncovered; verified that this fixture cannot tell them apart; unknown whether a fixture that can exists. I did not find one and I am not going to guess at a mechanism I have not observed. Your call whether that blocks — expire's clear, the one the data-loss report was about, is covered by ttl_expire's index-only arm.

Gate

pg18a and pg19a: native_upgrade_converge 8/8, ttl_expire 34/34, docs_style 9/9, native_reclaim 33/33, native_ios 11/11, index_only 27/27.

Rebase checked with a number, not prose — per-file patch md5 over content lines only, ignoring hunk headers: all six non-CHANGELOG files IDENTICAL, CHANGELOG differing by exactly one added blank line, the separator between this entry and main's.

Not approving: same account as the author.

[jdatcmd]

Approved at 118e61f. This clears my CHANGES_REQUESTED, and the red is gone for the right reason.

The fix is where it needed to be. pgcolumnar.set_options is defined in both the
fresh-install script and the alpha2->alpha3 upgrade script, and the guard had reached only the
first. Extracted both function bodies and hashed them:

ref	fresh install	upgrade script
main	136 lines, 14608eff7a8c	136 lines, 14608eff7a8c
this branch	160 lines, 9d387c84fcb7	160 lines, 9d387c84fcb7

Identical within each ref, +24 lines on both here. A fresh database and an upgraded one now get
the same body. (My first extractor returned the md5 of empty input in all four cells and I
nearly reported that as agreement — the rebuilt one carries a control asserting it can return
different values, which it does.)

I did not take "no new arm is needed" on trust, because that is the load-bearing claim.
You argue the existing pair holds the guard and that a behavioural arm would be one that cannot
independently fail. I ran the decisive cell myself, on the composed tree, own box, own prefix:

tree	ttl_expire	native_upgrade_converge
guard in both (this PR)	34 / 0	8 / 0 PASS
guard removed from both scripts	32 / 2 FAILED	8 / 0 PASS
guard in fresh only (the original red)	34 / 0	6 / 2 FAIL

The middle row is the one that settles it: convergence is green with the guard deleted
everywhere, so native_upgrade_converge alone cannot hold it — exactly your point. The pair
does, and the two reds are for different reasons. The mutant fails on
a negative ttl_interval is refused with 22023: got [] want [22023], which asserts SQLSTATE
rather than message text, so it cannot be satisfied by an unrelated error.

Mutation asserted applied before building: guard count 1 -> 0 in each file, with an identical
372-byte delta in both, so the same block went from both and nothing else did.

Agreed on not adding the arm. An arm that only restates what these two already prove would
read as coverage and add none.

Composition, not the branch. Full PG 17.10 matrix on main 381c765 + #867 + #869:
242 verdicts, 237 PASS, 5 SKIP, 0 FAIL, ALL VERSIONS PASSED. Nothing that passed on main or
on the previous composed tree stopped passing; the only new suite is truncate_cleanup, which
is #867's.