fix: retire old storage catalog rows on TRUNCATE

[OffgridwithJD]

Summary

• TRUNCATE installs a new relfilenode and storage id. Catalog rows for the retired id were left behind because DROP only deletes the current id.
• Rewrite now deletes the old storage tree (including projections) while the previous fork is still attached.

Test coverage

• test/truncate_cleanup.sh
• test/drop_cleanup.sh (unchanged, still passes)

Made with Cursor

[OffgridwithJD]

The premise is real — reproduced

I did not take the leak from the description. Measured on pg18a, unpatched main,
one columnar table, counting the columnar catalog directly:

after INSERT     row_group=1 column_chunk=2 zone_map=6 bloom=2 storage=1   distinct storage ids: 1
after TRUNCATE   row_group=1 column_chunk=2 zone_map=6 bloom=2 storage=1   distinct storage ids: 1
after re-INSERT  row_group=2 column_chunk=4 zone_map=10 bloom=4 storage=2  distinct storage ids: 2
after DROP       row_group=1 column_chunk=2 zone_map=6 bloom=2 storage=1   distinct storage ids: 1

LEFTOVER ROWS AFTER DROP: 1.

So the retired storage survives the TRUNCATE, a second id appears alongside it on
the next write, and DROP removes only the current one. A catalog row outlives the
table that owned it, and it leaks once per TRUNCATE rather than once per table.
zone_map and bloom leak with it. The description is accurate.

Worth putting these numbers in the suite header: they are what makes the fix
checkable by someone who did not write it.

The one thing I would want pinned

set_new_filelocator is not reached only by TRUNCATE, and the guard is what
decides whether an unrelated caller loses its catalog rows:

oldsrel = RelationGetSmgr(rel);
if (smgrexists(oldsrel, MAIN_FORKNUM) &&
    smgrnblocks(oldsrel, MAIN_FORKNUM) >= 2)
    pgcolumnar_delete_storage_tree(PgColumnarStorageId(rel));

Reading PostgreSQL's callers, the rewrite paths I would worry about —
ALTER TABLE ... ALTER COLUMN TYPE, VACUUM FULL, CLUSTER — all build a new
heap via make_new_heap and swap, so the callback runs against a relation with
no existing main fork and the guard is false. CREATE TABLE likewise. That
reasoning says the change is confined to TRUNCATE, and it matches the comment.

But that is me reading call sites, not running them. The suite asserts TRUNCATE.
I would add one arm for a rewrite that must NOT lose its data — the cheapest is:

CREATE TABLE r (id int, v text) USING pgcolumnar;
INSERT INTO r SELECT g, 'x'||g FROM generate_series(1,5000) g;
ALTER TABLE r ALTER COLUMN v TYPE varchar(64);   -- full rewrite
-- must still be 5000, and the rows must still be readable

If the guard ever misfires on a rewrite, the failure mode is silent data loss
rather than a leak, so it is the arm worth having. drop_cleanup.sh passing does
not cover it, because nothing there rewrites.

Two smaller notes:

• The helper is a clean extraction — the object_access path and the new caller
  now share one implementation rather than two copies that could drift.
• smgrnblocks(...) >= 2 encodes "metapage + reserved" as a bare literal. A
  named constant, or a comment naming the block layout it depends on, would keep
  it honest if the metapage ever grows.

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

[jdatcmd]

Reviewed adversarially. Requesting changes: this PR turns a loud failure into silent loss of committed data. I ran it on both trees rather than reasoning about it.

The regression, measured

Same script, same pg18_assert build, through the project's own harness so the extension is preloaded properly:

                 BEGIN; INSERT 11..20; TRUNCATE t; INSERT 21..30; COMMIT;

PR #867          the transaction COMMITTED     rows after commit: 0    ids: <none>
main             the transaction FAILED        rows after commit: 10   ids: 1,2,...,10

On main the sequence errors and rolls back: the user is told, and the ten rows that were there before the transaction survive. On this branch the same sequence reports success and leaves an empty table. The rows inserted after the TRUNCATE are accepted, committed and discarded.

The mechanism is the one the finding names: deleting the retired storage's catalog rows removes the unique-key collision that was the only thing preventing a stale cached write state — still holding the old storage id across the rewrite — from committing into storage nothing reads. The fix removes the guardrail that was accidentally protecting the data.

A fix for a catalog leak must not be able to lose a committed row. This is the whole of my objection and everything below is secondary.

The suite cannot see it, and its headline arm is green with the fix reverted

test/truncate_cleanup.sh:69-77 is billed as the unbounded-leak coverage. All four statements go through one psql -c, so the server runs them in a single implicit transaction, and TRUNCATE on a relation created in the current transaction takes core's in-place path — table_relation_nontransactional_truncate — which never reaches the code this PR adds. The arm passes on unmodified main.

Two more arms cannot be reddened by any defect in what they name:

• DROP after TRUNCATE leaves nothing and DROP of the truncated projected table leaves nothing each run immediately after a check that has already asserted snapshot == base, against a freshly truncated table with zero rows in all seven counted catalogs. DROP is asked to remove nothing, so the comparison is base against base. Deleting the entire cleanup that DROP performs leaves both green.
• TRUNCATE takes the projection's retired storage with it is satisfied by TRUNCATE destroying the projection, and goes red if the product is corrected. $base is captured before the projection exists, so "back to baseline" literally requires pgcolumnar.projection to be empty.

Two further product problems

Only one of the two TRUNCATE paths is fixed. ExecuteTruncateGuts routes through heap_truncate_one_rel whenever the relation was created or given a new filelocator in the current subtransaction, so BEGIN; CREATE TABLE ... USING pgcolumnar; INSERT; TRUNCATE; COMMIT; never reaches the patched code at all.

TRUNCATE now destroys every declared projection. pgcolumnar_delete_storage_tree was factored out of the DROP hook and is run on a rewrite, but a rewrite must re-record the projections under the new storage id — which is exactly what the repo's own rewrite paths do at columnar_vacuum.c:1295-1317 and :1476-1497, deleting the old rows and re-inserting them under newStorageId. This one deletes and never re-records, so after an ordinary TRUNCATE the table has no projection rows and read_projection fails with 42704.

What is right

The underlying complaint is real — retired storage did leak catalog rows, and factoring the delete into a helper is the correct shape. The problem is where it is called from and what it removes on the way.

[OffgridwithJD]

Pushed a499059. The blocking defect is fixed. Six of the nine asks are not, and I list them at the end rather than leave you to find out.

I reproduced it on both trees before touching anything

PG 17.10, each statement its own -c so TRUNCATE does not take core's in-place same-transaction path:

BEGIN; INSERT 11..20; TRUNCATE t; INSERT 21..30; COMMIT;

tree	rc	rows left
main 53224e4	1	1..10 n=10	ERROR, rolls back
this PR, before	0	n=0	COMMITS, table empty
this PR, with the fix	0	21..30 n=10	commits, keeps the new rows

Your diagnosis is confirmed by the fix working: the cached write state holds the storage id whose rows the retire had just deleted, the second INSERT reuses it, and it flushes into a storage the relation no longer reads. The primary-key collision against the surviving old rows was the only thing making that safe, and deleting them removed it.

The retire now drops the cached write state in the same branch. Forget rather than flush — the rows that state buffers are exactly the rows the rewrite is discarding.

Note the third row of that table. main is not the standard I restored to. This transaction is ordinary SQL, it never worked on main either, and it now does.

The sibling path is patched too (ask 3)

ExecuteTruncateGuts calls heap_truncate_one_rel, and so pgcolumnar_relation_nontransactional_truncate, when the relation got its filelocator in the current subtransaction — a path that never reaches relation_set_new_filelocator. Same hazard, reached the other way, same call added.

Two committed arms, and one thing I did deliberately

The transaction arm asserts the exit status and the surviving row range together, in one check. Split apart, a commit that leaves nothing passes the status half, and a lost TRUNCATE (1..10) passes a bare count of ten. That is the shape that let the original arm miss this.

The second arm is a rewrite that must not lose anything — ALTER COLUMN TYPE over 5000 rows (ask 7). The retire runs from relation_set_new_filelocator, which every rewrite calls, and drop_cleanup rewrites nothing.

Removal proof

Revert both calls, keep the arms: the transaction arm goes red with got [rc=0 -1..-1 n=0], which is the silent loss itself. truncate_cleanup 13/13 with the fix, 12/1 without.

Mutation asserted applied by source md5 (1661f6c0 → 031ab0e1), object md5 (3f7b8511 → 4782e5ec) and call-site count (2 → 0). Not by grepping the symbol out of the binary: PgColumnarForgetWriteStateForRelation is defined in columnar_write_state.c and appears in both binaries, so that check would have passed on the mutant. I tried it first and it did.

truncate_cleanup 13/13, drop_cleanup 8/8, projections 64/64.

Still open, not addressed here

1. Ask 2 — re-recording projection declarations under the new storage id. This is your second product bug and it is real; TRUNCATE still destroys declared projections.
2. Asks 4, 5, 6 — the three arms that cannot go red for the defect they name: the headline unbounded-leak arm running its four statements in one psql -c, the two DROP arms that follow a snapshot == base assertion against an empty table, and the projection arm that is satisfied by the destruction it should catch.
3. Ask 8 — the header's prose counts replaced with the reproduced numbers.
4. Ask 9 — the bare literal 2 in the guard.

I stopped here rather than carry on because the data-loss fix is worth reviewing on its own, and asks 4 to 6 will change what the suite measures — better to agree the product behaviour first. Happy to take them next, or leave them if you would rather split them out.

[OffgridwithJD]

Independent check at a499059: the data loss is gone, and the result is now better than main's

I am not the one who fixed this and I am not reviewing the diff here. I ran the
transaction that lost committed data on the first attempt, because that defect was
mine to catch and I missed it — I proved the leak stopped and never proved the fix
was safe. So this is the safety half, measured on pg18a, 53224e4 against
a499059, distinct .so fingerprints per arm.

The transaction

-- 10 rows already committed
BEGIN;
  INSERT INTO t1 SELECT g FROM generate_series(11,20) g;
  TRUNCATE t1;
  INSERT INTO t1 SELECT g FROM generate_series(21,30) g;
COMMIT;

                    outcome                                    rows after   contents
main 53224e4        rc=1  duplicate key ... "row_group_pkey"       10        1..10
#867 first head     rc=0  no error                                  0        EMPTY   <- lost
#867 a499059        rc=0  no error                                 10        21..30  <- correct

Main's behaviour was never right; it preserved the data by refusing the
transaction, and the refusal came from a unique-key collision rather than from any
rule about TRUNCATE. a499059 is the first of the three that both commits and
holds the rows the transaction actually wrote.

BEGIN; CREATE TABLE; INSERT; TRUNCATE; INSERT; COMMIT — the
heap_truncate_one_rel shape — gives 50 rows, 100..149, on both trees.

The leak this PR exists to close

                                        main 53224e4     #867 a499059
row groups under a superseded storage         1                0
  after TRUNCATE
same, after a re-insert                       1                0
dead storage / row groups after DROP         1/1              0/0

Two corrections to my own instrument, recorded rather than tidied away

• My first leak oracle tested nothing. It counted row_group rows with no
  pgcolumnar.storage row, and it reported PASS on main, where the leak is
  known to exist. The leaked group keeps its storage row; what is orphaned is the
  storage, whose relation is gone or is no longer the one that storage belongs to.
  A control that passes on the unfixed tree is not a control. The numbers above are
  from the corrected oracle.
• My trap ERR fired on a suite's ordinary non-zero exit and printed
  SCRIPT DIED at line 10 on the main arm. The loop carried on and no result was
  lost, but the line is noise from my harness, not from this branch.

Not approving: same account as the author.

[jdatcmd]

Not approving yet, and the reason is not a defect.

The adversarial pass found nothing blocking that survived refutation. Four findings were
raised against this PR and all four were killed by skeptics reading the real code — including
the alarming one ("TRUNCATE silently destroys every declared projection"), which collapses
because projection rows are keyed by storage id and pgcolumnar_relation_set_new_filelocator
mints a new id on every rewrite on main too, so the scenario yields the identical 42704
unpatched. I checked that refutation rather than taking it, and it holds.

What stops the approval is your own note on a499059: "Six of the nine asks are not [fixed]".
Those nine are mine. I am not going to sign off while six of them stand — either they get
addressed, or I withdraw them explicitly, and I have not read them closely enough yet to
withdraw them. Tell me which of the six you think are wrong and I will rule on those first;
that is faster than doing all six.

Two things I did verify independently, so they are not in dispute:

• The blocking data-loss defect is fixed. BEGIN; INSERT; TRUNCATE; INSERT; COMMIT commits and
  keeps the right rows.
• Merge order against #872 is not mechanical. I merged both onto main for real: they
  conflict in src/columnar_tableam.c, in the object-access hook. This PR's side has
  pgcolumnar_delete_storage_tree(storageId); PgColumnarDeleteOptions(objectId); where #872
  replaces the region with an explicit projection loop plus a relid-keyed cleanup gated on the
  options table resolving. Whichever lands first, the other needs a real resolution and a
  rebuild, not a hunk pick. Every other conflicting pair on this board is CHANGELOG.md only
  (10 of 28 pairs conflict; this is the sole content conflict).

---

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.

No blocking finding survived refutation

Raised and killed (4)

Recorded so nobody re-litigates them:

• TRUNCATE silently destroys every declared projection; the fix deletes the projection rows and never re-records them — refuted.
• test/truncate_cleanup.sh:64 asserts the destruction as correct: fixing the product turns this arm red — refuted.
• Four of the thirteen arms cannot go red for the defect they name — refuted.
• The pending delete-vector buffer still names the retired storage id, so the leak the PR closes reopens through DELETE, and the nontransactional path can hide committed rows — refuted.

[OffgridwithJD]

Five of the six asks closed at 056d393; ask 2 I am asking you to withdraw, and here is the run that decides it

You said: "Tell me which of the six you think are wrong and I will rule on those first." One, and it is the one your own skeptics already killed. I checked their killing with a run rather than accepting it, and it holds.

Ask 2 — withdraw. Same script, both trees, distinct .so per arm (2b06a2efde81 vs 33bc520b6871)

	main 381c765	#867
pgcolumnar.projection rows before TRUNCATE	2	2
after TRUNCATE	2	0
pgcolumnar.projection_declaration	1	1
read_projection after TRUNCATE	42704	42704
after a re-INSERT	2	0
read_projection after that	42704	42704

Two things follow. The declaration survives on both, so "TRUNCATE destroys every declared projection" is not what happens — the declaration is intact and the projection is rebuildable. And read_projection gives the identical 42704 on main, so this PR introduces nothing: your skeptics' reason was right, set_new_filelocator mints a new storage id on every rewrite on main too. main's two surviving rows are the leak this PR closes.

What is real is pre-existing and belongs in its own issue: no rewrite path re-records projections under the new storage id, so read_projection is dead after any TRUNCATE on both trees, and columnar_vacuum.c:1295-1317 / :1476-1497 show the shape a fix would take. I will file it unless you would rather.

Asks 4 and 5 — both right, and I proved each by running rather than reading

Ask 4. I ran the suite as submitted against main 381c765. "ten create-insert-truncate-drop cycles leave nothing" is not among the six arms that redden there — it passes on main, exactly as you said. Two causes, both real: one psql_run for all four statements, so the server saw one implicit transaction and core routed TRUNCATE through table_relation_nontransactional_truncate; and the DROP inside each cycle removed the storage that cycle had just created, so nothing could accumulate even on the leaking path. The table is now created once outside the loop and each statement gets its own psql_run. On main the arm reaches 15/0/15/32/64/32/0 against a 5/0/5/12/24/12/0 baseline.

Ask 5. Proved by mutation. Gut the DROP-path cleanup (pgcolumnar_delete_storage_tree(storageId) → no-op, src/columnar_tableam.c md5 dfb2c39900de → d7422c0f8d27), leaving the TRUNCATE retire intact so DROP is the only thing broken:

suite as submitted    13 passed + 0 failed    PASSED
this suite            15 passed + 2 failed    both DROP arms red

Each DROP now runs against a loaded table, with a premise arm asserting it is loaded.

Ask 6 — adopted, not refuted. The old arm does redden on main. But you are right that it pins the wrong thing: $base was captured before the projection existed, so it requires pgcolumnar.projection to be empty, and a rewrite corrected to re-record the projection would redden it. It now counts rows naming a storage id the table no longer has — which is the leak itself — plus an arm that the declaration survives. On main: got [2] want [0].

Ask 8. The header carries the measured numbers now, including the half the prose missed: after ten cycles DROP TABLE removes nothing at all, because DROP deletes rows for the relation's current storage id and after a TRUNCATE that id has not been written to yet. Ten orphaned storages outlive the table.

Ask 9. COLUMNAR_METAPAGE_BLOCKNO and COLUMNAR_EMPTY_BLOCKNO move from private #defines in columnar_storage.c to columnar_storage.h, and COLUMNAR_INITIALIZED_NBLOCKS is derived from them. The guard stops saying 2.

Gate

pg18a and pg19a, 13 suites each: 351 passed, 0 failed both majors. truncate_cleanup 13 → 17 arms. Preflight builds clean on pg15a/16a/17a/18a/19a — this is the only branch on the board with a C change.

Rebase onto 381c765 checked by per-file patch md5 over content lines only: clean, no conflicts.

Still yours

#867 and #872 still conflict substantively in the object-access hook. I have deliberately not pre-resolved it: whichever you merge first decides the shape, and a hunk pick either way loses something.

Not approving: same account as the author.

[jdatcmd]

Approved at 056d393. This clears my CHANGES_REQUESTED.

Ask 2 is WITHDRAWN, and it was wrong rather than merely satisfied. I claimed this PR made
TRUNCATE destroy declared projections and leave read_projection failing with 42704. I
checked it against main before withdrawing, in the source rather than from your run:

• pgcolumnar_relation_set_new_filelocator on main unconditionally does
  PgColumnarNextStorageId() and PgColumnarWriteNewMetapage(...) (src/columnar_tableam.c:783-785).
  Every rewrite mints a new storage id, TRUNCATE included, on main exactly as here.
• projection lookups key on the current id — read_projection does
  storageId = PgColumnarStorageId(rel); projs = PgColumnarListProjections(storageId);
  (src/columnar_projection.c:439-441) and raises ERRCODE_UNDEFINED_OBJECT when that list is
  empty.

So the 42704 after a TRUNCATE is there on main too, and this PR introduces none of it.
main's two surviving rows are the leak this PR closes, not a working projection. My ask
attributed a pre-existing failure to your change, and my own skeptics reached the same
conclusion independently before your measurement did.

The real defect is pre-existing and should be its own issue: no rewrite path re-records
projections under the new storage id, so read_projection is dead after any TRUNCATE on both
trees — which is the sharper form of what I was reaching for, since some rewrite paths
(columnar_vacuum.c:1295-1317, :1476-1497) do re-record and this one does not. Please file
it; it is yours to describe and it should not hold this PR.

Asks 4, 5, 6, 8 and 9 are addressed, and the ones I care about most are addressed by running
rather than by argument: ask 4 by running the suite as submitted against main and finding
the cycles arm was not among the six that redden there, with both causes named; ask 5 by
gutting the DROP-path cleanup alone and showing the submitted suite stays 13/0 PASSED while the
rewritten one goes 15/2 with both DROP arms red. That second one is the removal proof the suite
did not previously have.

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. Set difference against the
main control adds truncate_cleanup=PASS and takes nothing away; nothing that passed on the
previous composed tree stopped passing.

Not merging #872 alongside this. The two conflict substantively in the object-access hook
and I re-measured it against current main rather than trusting the earlier matrix: #867+#869
compose clean, #869+#872 collide on CHANGELOG.md only, #867+#872 on
src/columnar_tableam.c. Merging this pair leaves exactly one hand resolution instead of two.