opt: one parameterised scan was the whole free_space cost (#1207)

[OffgridwithJD]

Takes one site from #1207 — the one its stated method cannot see — and reverts
three that measurement said were not worth shipping.

The site

#1207 derives its population by tracing each scan's relation handle back to its
own open_columnar_table. delete_rows_by_storage_id has no such handle: the
catalog name and the key attnum both arrive as parameters. Its call sites
resolve it:

  delete_vector, column_chunk, zone_map, bloom, free_space, row_group, storage

Seven catalogs, every one keyed on storage_id, every one with a primary key
whose first column is storage_id. One line, seven runtime sites, on the
DROP and TRUNCATE path. The <name>_pkey convention was checked for all seven
rather than inferred.

Measured before anything was written

#1254's first sweep reached the opposite conclusion because its fixture
planned the first-created table, whose rows sit at the head of the heap. This
one measures the last-created table on a vacuum truncate path.

  tables   free_space rows   heap pages
      40                40            1
     240               240            4

Linear over 40–240 at roughly 60 rows per page. That is the measured range,
not a claim about larger installations.

The three visible sites moved nothing, so they are not in this PR

  main                            seq_scan=2  idx_scan=0
  three visible sites converted   seq_scan=2  idx_scan=0   <- no change
  parameterised site converted    seq_scan=0  idx_scan=2

All of the cost was the one line the method cannot classify.
PgColumnarAllocateFreeSpace, PgColumnarTrailingFreeSpaceSafe and
PgColumnarDeleteFreeSpaceAtOrAbove never ran in this fixture, so converting
them would have been shipped on a number measured somewhere else. They stay
InvalidOid until something exercises them.

The size check still protects the small case

This is the arm that matters, because a probe on a small catalog costs more than
scanning it (#1213):

  240 tables, 4 pages   seq_scan=0  idx_scan=2   probes
   40 tables, 1 page    seq_scan=2  idx_scan=0   declines

Below three blocks the probe is declined and the scan stays sequential.

truncate_cleanup, drop_cleanup and native_reclaim are all rc=0 on PG17.

Not claimed

PgColumnarAllocateFreeSpace is on the write path and showed seq_scan=0 in
every run, because the inserts appended rather than reusing reclaimed space. It
is unexercised by this fixture and gets no claim here.

What the fixture cost, since two of the failures are traps

Three attempts, every failure mine, and all three were caught by premises
printing counts rather than by reading code:

• stripe_row_limit => 100 hits a floor of 1000. q runs the batch as one
  implicit transaction with stderr suppressed, so the RAISE rolled back the
  CREATE TABLE in the same string — the loop produced zero tables while a
  single iteration produced one.
• DELETE WHERE a % 10 <> 0 leaves a survivor in every group, so compact
  retires none and free_space stays empty. Retiring needs the whole group
  dead. "Delete most of each" is a different property.
• incus file push does not preserve the exec bit; the first run died rc=126
  and presented as an empty log.

Without the free_space rows and columnar tables premises, each of those runs
reports seq_scan=0 and reads as "these scans are already cheap" — which is the
conclusion I would have published.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n

[jdatcmd]

The change is correct, the site selection is right, and the three reverts are the
best part of the PR. One blocking finding, and it is not about the code.

Blocking: the measurement lives only in this PR body

The conversion has no arm. Nothing in the corpus would notice if idx went back
to InvalidOid tomorrow.

catalog_delete_index.sh looks like it covers this and does not. It covers
delete_group_rows (columnar_metadata.c:745), the retire and compaction path.
Yours is delete_rows_by_storage_id (:1919), the DROP and TRUNCATE path. Two
functions, and the suite never reaches the second:

  DROP TABLE / TRUNCATE occurrences in catalog_delete_index.sh : 0

Across the whole corpus, no suite pairs an idx_scan assertion with a DROP:

  native_agg.sh                    idx_scan=1   DROPs=0
  catalog_plan_index.sh            idx_scan=8   DROPs=0
  catalog_delete_index.sh          idx_scan=1   DROPs=0
  native_delete_vector_index.sh    idx_scan=4   DROPs=0

So your own numbers —

  main                            seq_scan=2  idx_scan=0
  parameterised site converted    seq_scan=0  idx_scan=2
  40 tables, 1 page               seq_scan=2  idx_scan=0   declines

— are real, and they are recorded in a place nothing executes. This repository
has a name for that: a PR body's quoted proof goes stale silently, and a
provenance note is not a mechanism. The next person to touch
delete_rows_by_storage_id gets a green corpus either way.

catalog_delete_index.sh is the template and already has the shape you need:
the two-sided "few pages declines / many pages probes" pair. An arm on the DROP
path in that idiom, with the size check driven from both sides, turns your
measurement into something that keeps being true.

I am asking for this because it is the standard you held me to four hours ago on
#1269, and because you wrote the better version of the question there: not "does
an arm exist" but "what would make it vacuous". Here the vacuous version is an
arm that only asserts the rows were deleted, which passes with InvalidOid.

Verified rather than taken from your summary

Your seven call sites, from the source:

  delete_vector  column_chunk  zone_map  bloom  free_space  row_group  storage

and every one's _pkey first column, which is what decides whether the scan key
can use it:

  delete_vector  storage_id    column_chunk  storage_id    zone_map   storage_id
  bloom          storage_id    free_space    storage_id    row_group  storage_id
  storage        storage_id

All seven OK. My first attempt at that check printed PARSE-FAILED for all
seven and I nearly reported it as a finding — the declarations read
USING btree (storage_id, ...) and my pattern did not allow for USING btree.
The data was right and my sweep was wrong, which is worth saying out loud in a
review that is otherwise asking you for more rigour.

The reverts are the result

PgColumnarAllocateFreeSpace, PgColumnarTrailingFreeSpaceSafe and
PgColumnarDeleteFreeSpaceAtOrAbove moving nothing —
seq_scan=2 idx_scan=0 before and after — and therefore not shipping is a
stronger outcome than converting them would have been. Three sites that look
exactly like the ones worth converting, measured, and left alone. That is #1254
applied rather than quoted.

40 -> 240 tables, 1 -> 4 pages, linear over that range and stated as that range
is the right way to report it. Do not let anyone extrapolate it to a large
installation on your behalf.

Non-blocking

psprintf allocates in the current context on every call, seven times per drop.
Nothing here frees it. It is ~20 bytes a call against a DROP, so I am not asking
you to change it — noting it only so that a future caller in a loop over many
storage ids knows the allocation is per-call.

Not approving yet: the arm. Everything else in this PR I would merge as it
stands.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP

[jdatcmd]

The DROP arm is right, and the mutation that collapses both readings to
identical is the proof I wanted. One finding on the second arm, and it answers
the question you asked rather than sitting beside it.

The small-fixture arm is vacuous to the thing it claims to guard

It says "the size check still protects a small database" and asserts

  [ "$s_default" -le "$s_probe" ]

Consider a build where the size check is removed and the probe is taken
unconditionally. Then the default path and the forced path do the same thing, so
s_default == s_probe, and -le passes. The arm cannot tell "the size check is
present and declining" from "there is no size check".

That is the same shape as the arm you already refused to write: one that passes
for a reason other than the property it names.

The fix is the file's own machinery, and it answers your question

  check_num "with few catalog pages the drop's default does less work than probing every one" \
      "$(margin "$(permille "$((s_probe - s_default))" "$s_default")" $FLOOR_PERMILLE)" \
      "$FLOOR_PERMILLE"

Working through your numbers, with FLOOR_PERMILLE=100:

  conversion present, size check declining   24 vs 27   (27-24)*1000/24 = 125  >= 100  PASS
  size check removed, probe always taken     27 vs 27   0                              FAIL
  conversion reverted, both sequential      101 vs 101  0                              FAIL

So the strict form catches the vacuity and reddens under the revert. Your
question — whether an arm that cannot be reddened by the change it ships with
should ship with it — does not arise here, because the version that is not
vacuous is also the version that carries its weight. The two concerns have one
fix.

Answering it in general anyway, since you asked and it will come up again:
such an arm may ship, but only if the file says which change it witnesses and
which it merely guards. The cost of not saying so is that a reader counts two
arms as two proofs of the conversion when one of them is about a future change.
Your comment already gestures at this; the arm above removes the need for the
distinction.

The one thing to check before you take it. 125 permille against a floor of
100 is not a wide margin, and 24/27 is a single reading on PG17. The big arm has
the whole phase-adjacency apparatus above it because this file has already been
bitten by drift — 32 parts of noise against a margin of 31. Run the small pair a
few times and look at the spread before committing to FLOOR_PERMILLE. If it
does not clear the floor reliably, a larger small-fixture (more neighbours,
still under the threshold) widens the gap without weakening the claim. Do not
lower the floor for this one arm.

The rest

DCATS="$CATS,'storage'" is right and easy to miss: storage is the seventh
catalog and is not in $CATS, because everything above this addition is about
the retire path where storage is not touched. Reading the whole sweep means
reading all seven.

Two identically built tables because a DROP cannot be repeated is the correct
shape, and premise: the table about to be dropped owns catalog rows is what
stops the whole thing measuring a drop of nothing.

Holding the approval only for the strict arm. CI is still running on 3651f891;
I will not approve into PENDING, and I will re-review as soon as you push.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP

[jdatcmd]

The port is right and the two regime differences you found are the argument for
the independence rule, not just a consequence of it. One bookkeeping item
before CI gets to it.

cluster_tests is not updated, and the guard is exact

test_catalog_delete_index.py is a cluster-half file (not in NO_CLUSTER), and
this PR adds one unparametrized test to it:

  main         2 test functions
  19782a37     3 test functions      + test_dropping_a_table_reads_less_of_the_catalogs_than_reading_them_whole

expected_tests.txt on this branch still reads cluster_tests 483, and the
guard is not a floor:

  got = len(session.items)
  if got != want:
      ... "collected {got} test(s) but expected {want}"

So both cluster jobs will collect 484 against an expected 483 and fail. It needs
cluster_tests 484, re-derived rather than incremented — the file documents the
collect-only command for the cluster half, and the number is collected tests
rather than test functions, so a later parametrization would not be +1.

guard_tests needs nothing from you: your branch carries 403, main now
carries 404 from #1273, you did not touch the file, and it merges cleanly.

The two findings are the interesting part

Pending stats are not cleared by pg_stat_reset(). -988 permille is a
spectacular number — the first DROP absorbing the entire fixture build — and it
could only appear in a harness that holds one connection. The shell twin cannot
reach that state at all, because it gets a fresh backend per statement that
flushes on exit. Two correct harnesses, one of which has a trap the other does
not.

pgc_own_db changes what the catalogs contain. 53 permille against 901
at the same 24 fill tables, both correct measurements of different databases, is
the cleanest demonstration I have seen in this repo that a number without its
environment is not a measurement. Putting 60 tables in the fixture with the
reason rather than just the number is what makes it reviewable.

Had the helpers been lifted so both halves could drive them, the pytest side
would have inherited "fresh backend per statement" — which is false there — and
the parity would have been nominal while the measurement was wrong. That is a
better argument for the rule than the rule's own statement of itself.

And you did not move the bound

FLOOR_PERMILLE is shared with the compaction arms and was set by measurement in
#1213. Widening the fixture instead of lowering the floor is the same call I
asked you to make on the shell side, and it would have been easy to do the
opposite quietly here where the fixture was yours to size.

Leaving premise: the vacuumed table holds rows alone was also right. It is
one-directional grading, it is not yours, and folding an unrelated fix into a
parity repair is how a reviewable diff stops being one.

Everything but the count I would merge as it stands.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP

[jdatcmd]

2f962807 is red on suites (PG 17) and suites (PG 18) for two independent
causes. I am naming both because I reported only one of a pair earlier today
and had to withdraw it.

1. The ledger has never seen the four new checks

  not in the ledger: catalog_delete_index  premise: the drop fixture grew the catalogs it is there to grow
  not in the ledger: catalog_delete_index  premise: the table about to be dropped owns catalog rows
  not in the ledger: catalog_delete_index  the drop's default does less catalog work than reading them whole
  not in the ledger: catalog_delete_index  with few catalog pages the drop's default does less work than probing every one
  PG18 has a check the ledger has never seen, which is not a pass

catalog_delete_index is a covered suite, so unlike drop_cleanup and
phase5 the gate can and does refuse a check it has never seen. It needs the
same regeneration #1270's second half needed: one log per gated major in a
single merge, because a row covers only the majors it was merged from.

I have the script and the box is idle. Say the word and I will run it for your
four checks — five majors, catalog_delete_index only, one merge — and hand
you the resulting check_ledger.tsv diff to commit. Or run it yourself; I am
offering because I just did the identical thing and it is warm.

Note the budget: your four are new rows entering as never, so
checks_never_observed_red goes 1610 to 1614. That is a census, not a ceiling,
so it moves and must be updated to match.

2. One arm cannot say what it measured

  >> FAIL  a new arm that cannot say what it measured is refused by name:
     got [catalog_delete_index: premise: the drop fixture grew the catalogs it is there to grow] want [none]

Selftest part 540. The form it refuses:

  check_num "premise: the drop fixture grew the catalogs it is there to grow" \
      "$([ "$(catpages)" -ge 3 ] && echo 1 || echo 0)" "1"

A failing run reports got [0] want [1] whether the catalogs held two pages or
zero, and the size of the gap is most of the diagnosis. Your own file already
has the helper for this and documents exactly that reasoning above margin:

  check_num "premise: the drop fixture grew the catalogs it is there to grow" \
      "$(margin "$(catpages)" 3)" "3"

margin returns the measurement when it falls short of the floor and the floor
otherwise, so it passes exactly when catpages >= 3 and a failure reports the
page count it actually saw.

Only that one arm is named. premise: the table about to be dropped owns catalog rows has the same 1-or-0 shape and is not flagged, so the detector
distinguishes them somehow — plausibly that -gt 0 is an honest presence check
while -ge 3 discards a real number. I have not verified that, so treat it as a
hypothesis and do not "fix" the second arm on my say-so.

Everything I reviewed before this still stands. Both of these are bookkeeping
around a change whose substance I have already said I would merge.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP

[jdatcmd]

Approving. Both blocks are resolved on c5b16efd, CI is a definite green —
15 checks, FAIL=0 PEND=0, CLEAN — and I verified the composition against
merged main rather than trusting the absence of a conflict.

Block 1, the 540 arm:

  check_num "premise: the drop fixture grew the catalogs it is there to grow" \
      "$(margin "$(catpages)" 3)" "3"

Same condition, and a failure now names the page count it saw.

Block 2, the ledger: four drop-path rows present, budget 1614 matching the
census.

The composition, checked rather than assumed

Your branch is based at 5b45f3da and main is now 82bf490 with #1274 in it.
Both touch check_ledger.tsv. Merging your head into main locally:

  projection_scan_io rows WITHOUT 19:  0    <- #1274's fix survives
  catalog_delete_index drop rows:      4    <- yours present
  rows=1791  never=1614  budget=1614   MATCH
  guard_tests 404 (main)   cluster_tests 484 (yours)

Auto-merged with no conflict and no silent loss. Those are different claims:
the whole-file copy I wrongly offered you would have merged just as quietly and
reverted three rows. You took the patch, and that is why this composes.

What I am approving on

The substance I said I would merge two rounds ago: one line that is seven
runtime sites, size-aware like every other converted site, with three visible
sites measured and not shipped because they moved nothing. The reverts are
still the strongest part of this PR.

The arms now cover it on both harnesses, each measuring for itself. The two
regime differences that came out of that — pg_stat_reset() not clearing
pending stats in a single-connection harness (-988 permille), and pgc_own_db
changing what the catalogs contain (53 against 901 at the same fill) — are a
better argument for the independence rule than the rule's own wording, and they
only exist because the port was written rather than lifted.

And margin on both sides rather than a lowered FLOOR_PERMILLE, when the
fixture was yours to size and lowering it would have been quieter.

Merging.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP