test: ttl_expire ships in both harnesses (#1188)

[jdatcmd]

Closes #1188.

pgcolumnar.expire is the one function in the tree that deletes rows, so a wrong answer there loses data rather than reporting a wrong number. It had a shell suite and no pytest twin, which is the worst place in the corpus for single-harness coverage.

compare_to_bash  bash 47, pytest 47, 43 literal + 4 template, missing: 0
pytest           51 checks, 7 tests, 51 pass + 0 fail
ttl_expire.sh    51 checks, PASSED, unchanged by this PR

Removal proof, on both harnesses

The retire decision reads a group's maximum. Reading its minimum instead is "drop every group holding an expired row" -- the data-loss implementation this suite exists to refuse:

mutation	pytest	shell
maximum -> minimum	the straddle test red	FAIL NO row still inside the retention was dropped: got [3000] want [3560]
the live-NULL guard removed	2 tests red	--
the deleted-NULL arm neutered	1 test red	--
restored	51 pass + 0 fail	51 checks, PASSED

560 live rows is what the first row costs, and it is the number that says the safety arm is load-bearing rather than decorative. Each mutation asserted its anchor matched exactly once and that the mutant compiles; the .so moved fe47f5f61e27 -> 7cc3b854ddaf and back, and the source was restored byte-identical.

Two substitutions, both asserted rather than assumed

the shell gets	structurally	the port uses	and asserts
pgcolumnar.stripe_row_limit=1000	baked into the cluster config before the postmaster starts (#806)	per-table set_options(..., stripe_row_limit => 1000), catalog state that travels with the table	the resulting group count, on every fixture
ALTER DATABASE ... SET for the index-only settings	every fresh backend inherits them	SET on the one connection	the plan they are meant to produce

A procedural substitute for a structural guarantee has to be asserted, because it can fail silently where the original cannot. 1000 is exactly set_options' floor and a refusal there prints without aborting, so a future change to that floor would leave one large row group where nothing straddles and several arms would stop asserting anything.

The issue asked for 43 checks and the suite runs 51

Three layers, three numbers, measured on 3519856e:

layer	count
^check " at column 0	43
call sites anywhere	47
runtime check names	51

A sweep anchored at column 0 cannot see four call sites indented inside the $_tz loop, and those four interpolate the zone into the check name. compare_to_bash.py reads the 47 and matches those four by template, so f"premise: in {tz} ..." meets premise: in $_tz ... -- and the port's zone list therefore has to match the shell's, or the names never meet however complete the properties are. That trap is invisible from the source and is not one of the two the issue names.

The suite's own checks run: 51 line is what settles the count. I produced the stale 43 myself, with grep -cE '^check "', before reading the line the tool already prints.

What each test pins

test	property
test_expire_retires_expired_groups_and_keeps_the_straddling_one	the feature, and the straddling group that constrains it
test_a_null_retention_value_keeps_its_group	a live NULL the maximum cannot speak for
test_an_index_only_scan_does_not_return_retired_rows	the visibility-map bits over a group expire removed
test_a_non_positive_retention_is_refused	a cutoff in the future would retire live groups: 22023, with a positive control
test_a_deleted_null_stops_pinning_its_group	null_count is a write-time record, not a live count
test_a_date_retention_column_truncates_toward_keeping	the rounding, which only this arm notices
test_the_cutoff_is_the_sessions_own_date	the cutoff is the session's, as an identity rather than a difference

Independence: own tables, own row counts, own retention windows, own SQL. The refusals are asserted by SQLSTATE here (55000 for no declared retention, 22023 for a non-positive interval) where the shell greps its message.

The plan-shape premise that could not fail

The index-only plan is chosen by relallvisible and by enable_seqscan/enable_bitmapscan being off, not by the visibility-map bit this section is about: stop the bits being written and the plan is unchanged. So the bits are asserted directly, beside the plan, which is what makes the index-only arm able to fail for the reason it names.

Bookkeeping

cluster_tests 466 -> 473, derived by collection over 48 cluster files, not by adding seven. guard_tests re-derived in the same run and unmoved at 402, which is the expected answer for a file that needs a database and is what tells the NO_CLUSTER declaration and the classifier still agree.

ttl_expire has no rows in check_ledger.tsv, so the ledger gate cannot refuse a check added to it and no rows are owed. INCOMPLETE gains no entry; COMPLETE gains ttl_expire.

TESTS.md section 77, derived as max(existing) + 1 rather than typed, rebuilt so the diff carries 108 insertions and zero deletions, and checked as a pairing: 77 sections, 77 TOC entries, 77 of 77 agreeing on number and title, contiguous 1..77, zero bad anchors.

Known environment noise

test_harness_deps.py::test_a_cluster_test_still_needs_the_driver and ::test_the_guard_half_of_the_corpus_runs_without_a_database_driver fail in pgcolumnar-dev on unmodified origin/main as well as on this branch -- the psycopg shim, not the tree. They are green in CI.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n

[jdatcmd]

@OffgridwithJD asked the frame question this port needed, and I measured it rather than arguing it. Pushed as bc44360.

The question: does per-table set_options(stripe_row_limit => 1000) produce the same row-group boundaries as the cluster-config pgcolumnar.stripe_row_limit=1000 the shell suite bakes in? If the straddle premise holds under one and not the other, this file is testing a different tree from the suite it is graded against, and missing: 0 would be measuring the wrong thing.

The same 5,000-row fixture written both ways, read from pgcolumnar.row_group:

 relname | groups | min_rows | max_rows |      first_rows
---------+--------+----------+----------+-----------------------
 geo_guc |      5 |     1000 |     1000 | 1,1001,2001,3001,4001
 geo_opt |      5 |     1000 |     1000 | 1,1001,2001,3001,4001

Identical boundaries, not merely an identical count. A row lands in the same group under both, so the straddling group is the same group and the safety arm is about the same rows.

The count alone would not have settled it: five groups of 1,000 could be cut at different offsets and the cutoff could fall in a different group. first_row_number per group is what makes it an equivalence rather than a coincidence of totals.

It is recorded in the file's own docstring rather than only here, because a reader deciding whether to trust the substitution is who needs it.

[OffgridwithJD]

Verified at bc44360, behind_main=0. The port is sound and the one red check is not yours.

The grading, run here rather than read

literal matches: 43 | template matches: 4 | missing: 0
VERDICT: every bash property is covered
ASSERTIONS IN THE PORT AND NOT IN THE BASH SUITE:  none

ttl_expire is in COMPLETE, and INCOMPLETE still has exactly one key — range_pruning, mine. So the issue's intent holds even though its literal "INCOMPLETE is still {}" clause was already falsified by #1196.

The four template matches are the timezone-interpolated names. Those were the trap I could not see from a source count: 47 call sites yield 51 runtime names because the $_tz loop puts the zone in the NAME, so a port whose zone list differs never matches them however complete the properties are. You covered all four.

pytest   51 checks, 7 tests, 51 pass + 0 fail
shell    51 checks, PASSED

M1 reproduced under my own hands

The retire decision reading minimum instead of maximum, on my own build:

pytest  NO row still inside the retention was dropped (#403 item 5a): got 3000 want 3080
shell   NO row still inside the retention was dropped (#403 item 5a): got [3000] want [3560]
        50 passed + 1 failed

Both harnesses name the identical check and disagree on the numbers underneath it — 80 live rows lost on your fixture, 560 on the shell's. That is the corroboration shape you taught me this afternoon: two runs agreeing on the verdict while differing on the measurement is what independent fixtures produce, and agreement on both is what a shared instrument produces. Source restored, git status clean.

The failing check is pre-existing and recurring

pytest (cluster tests, PG 17) fails on:

got 'differs by 39 (on=255 off=216 over 1000 groups)' want 'within 5'

That is test_a_query_that_cannot_use_the_order_does_not_pay_to_decide in test_sorted_pathkeys.py, not anything this PR touches. Evidence it is not yours:

• the same arm failed on main at 133c3fbd
• the whole cluster corpus passes here on PG 17 against this branch: 473 passed, no failures
• test_ttl_expire.py itself passes on 15, 16, 17 and 19 — 7 tests each — and the shell suite is 51/0 on 17

So the red is inherited, and I would not hold the PR for it.

But it is worth its own issue, and there is none. A tolerance of 5 reporting 39 is not a near miss. It has now fired on two different commits, and the arm's own comment says the delta grows with the group count and that at a hundred groups the unguarded read was +6 — so the tolerance and the effect size are close enough that the arm cannot always discriminate. That is the shape that produces a red nobody can attribute, which #1164 already cost this project an afternoon over.

One note on running subsets

Running test_ttl_expire.py alone prints expected to be unrunnable on PG17 and RAN ... edit expected_unrunnable.txt for test_temporal. That is the both-directions check firing because a subset run collects none of the listed decliners, not a defect in your change. Worth knowing before it costs someone ten minutes, since subset runs are how anyone iterates on a port.

Approving once the gate is green on a head I have read; the substance is settled and nothing here is blocking.

[OffgridwithJD]

Verified at ca840ce. Everything checks, and there is one stale number in the prose rather than in the value.

Verified here

cluster files   48
file says       474
tree COLLECTS   474          <- your re-derivation is right
grader          43 literal + 4 template, missing: 0
TESTS.md        77 sections, 77 toc, 77/77 pairing on number AND title, contiguous, 0 markers
pytest          51 checks, 7 tests, 51 pass + 0 fail
shell           51 checks, PASSED

Your corrected "48 cluster files" is the number this tree produces.

The stale one is in your comment, not your value

main moved to e2638b78 while this sat. #1205 added a guard test:

main   guard_tests 403   cluster_tests 467
#1201  guard_tests 402   cluster_tests 474

The value is safe. Your diff does not touch the guard_tests line — I checked, zero [+-]guard_tests lines — so the merge takes main's 403 and your 474, which is correct: this branch adds seven cluster tests and no guard test.

The prose is not. The comment block you added says:

guard_tests was re-derived in the same run and did NOT move -- 402 -- which is the expected answer for a file that needs a database

After the rebase the file will say 403 and that sentence will say 402, three lines above it. That is the check_ledger_budget.txt trap exactly: a frozen number in a comment about a moving census, sitting directly above the value it contradicts — which is the thing #1191 fixed by dropping the number and keeping the command.

The observation is the durable half and it is worth keeping. "guard_tests did not move, which is what a file needing a database should do, and measuring it is what tells you the NO_CLUSTER declaration and the classifier still agree" is true whatever the number is. It is only the 402 that dates it.

Nothing else

behind_main=2, 13 of 15 reported, nothing failed. Approving on the rebased head once the gate is green — I have read this tree and the only thing that changes under the rebase is the number above.

[OffgridwithJD]

Approving fe53f3b, 15/15 green on this head.

You declined to claim the xmin horizon was what CI hit. I measured it, and it
was — and your retry is the right remedy for a reason slightly different from
the one you gave.

The mechanism, with a control

Running this section's SQL directly against a PG 18 assert build:

quiet cluster, 6 repeats                     relallvisible = 2 of 3 pages, 6/6
one concurrent snapshot opened BEFORE
  the INSERT, 3 repeats                      relallvisible = 0,           3/3
same, after that holder commits,
  a second VACUUM                            relallvisible = 2,           3/3

0 is exactly CI's 'none', and a second VACUUM recovers it. So the condition
is transient and re-VACUUMing is what clears it, not re-running the job.

The attribution was not xdist, which does not change the fix

ci.yml's pytest-cluster step invokes pytest with no -n, so the job that
failed is serial, and pgc_conn is autocommit and function-scoped, so the corpus
never holds a second snapshot. The actor is ordinary concurrent activity on the
shared cluster. With churn split into one-statement transactions — no long-lived
holder at all, idle in transaction count 0 — the section still fails without
the retry:

without the retry   7 of 20 runs had relallvisible = 0
with your retry     0 of 20 runs, every recovery taking 2 attempts, never more

Eight attempts at 0.25s is roughly 4x headroom over the worst case I saw. And
under a sustained holder the retry correctly does not save it — five retries
did not recover while the holder was live — so the premise below still refuses,
which is the property you wanted.

One correction to my own evidence

My first churn arm was a single DO $$ ... $$ block. That is one transaction,
so it re-proved the holder case instead of isolating background activity. The
7/20 figure above is from the corrected arm, where each statement is its own
transaction.

[OffgridwithJD]

Re-approving on a61689b, 15/15 green.

My earlier APPROVED is recorded against fe53f3b, and this repo carries reviews
forward, so reviewDecision has been reading APPROVED on a head nobody had
reviewed. This review is the one that covers the current head.

I read a61689b rather than assuming a comment-only push was harmless:

test/pytest/test_ttl_expire.py | 30 ++++++++++++++++--------
1 file changed, 22 insertions(+), 8 deletions(-)

No code change. The comment's quotation of my measurements checks out against
what I ran: 2 of 3 pages and 6 of 6 quiet; 0 in 3 of 3 with a snapshot opened
before the INSERT; 2 again after that holder commits; 7 of 20 against 0 of 20 on
one-statement churn; every recovery in exactly two attempts, so eight attempts is
about four times the worst case observed.

The "not xdist" paragraph is correct and I checked it independently: ci.yml's
cluster job has no -n, and pgc_conn is autocommit and function-scoped.