fix: retire old storage catalog rows on TRUNCATE

src/columnar_storage.c

@@ -33,9 +33,12 @@
 #include "storage/read_stream.h"
 #endif
 
-/* the metapage struct lives right after the page header on block 0 */
-#define COLUMNAR_METAPAGE_BLOCKNO 0
-#define COLUMNAR_EMPTY_BLOCKNO 1
+/*
+ * COLUMNAR_METAPAGE_BLOCKNO, COLUMNAR_EMPTY_BLOCKNO and the derived
+ * COLUMNAR_INITIALIZED_NBLOCKS are in columnar_storage.h: the metapage struct
+ * lives right after the page header on block 0, and columnar_tableam.c needs
+ * the block count to recognise an already-initialised fork.
+ */
 #define PgColumnarMetapagePointer(page) ((PgColumnarMetapage *) PageGetContents(page))
 
 /*

src/columnar_storage.h

@@ -19,6 +19,18 @@
 
 #include "columnar.h"
 
+/*
+ * Block layout of an initialised columnar main fork (spec 3). Block 0 is the
+ * metapage, block 1 is reserved and left empty; PgColumnarWriteNewMetapage
+ * writes both. COLUMNAR_INITIALIZED_NBLOCKS is what smgrnblocks() reports for a
+ * fork that has been through it, which is how a caller tells a rewrite of an
+ * existing columnar relation from a CREATE TABLE with no fork yet. Derived
+ * rather than written as 2, so that adding a block moves every reader at once.
+ */
+#define COLUMNAR_METAPAGE_BLOCKNO 0
+#define COLUMNAR_EMPTY_BLOCKNO 1
+#define COLUMNAR_INITIALIZED_NBLOCKS (COLUMNAR_EMPTY_BLOCKNO + 1)
+
 extern void PgColumnarWriteNewMetapage(const RelFileLocator *newrlocator,
 									 struct SMgrRelationData *srel,
 									 char persistence, uint64 storageId);

src/columnar_tableam.c

@@ -762,6 +762,31 @@ pgcolumnar_finish_bulk_insert(Relation rel, COLUMNAR_TABLE_OPTIONS options)
  * DDL callbacks
  * ------------------------------------------------------------------------- */
 
+/*
+ * pgcolumnar_delete_storage_tree
+ *		Drop catalog rows for one storage id and any projections hanging off it.
+ *		Options and projection declarations are keyed by relation OID and are
+ *		left to the caller: they still apply after a rewrite, and they must go
+ *		on DROP.
+ */
+static void
+pgcolumnar_delete_storage_tree(uint64 storageId)
+{
+	List	   *projs = PgColumnarListProjections(storageId);
+	ListCell   *lc;
+
+	foreach(lc, projs)
+	{
+		PgColumnarProjection *p = (PgColumnarProjection *) lfirst(lc);
+
+		if (p->projStorageId != storageId)
+			PgColumnarDeleteMetadata(p->projStorageId);
+		PgColumnarDeleteProjectionRow(storageId, p->projectionId);
+	}
+
+	PgColumnarDeleteMetadata(storageId);
+}
+
 static void
 pgcolumnar_relation_set_new_filelocator(Relation rel,
 									  const RelFileLocator *newrlocator,
@@ -770,6 +795,7 @@ pgcolumnar_relation_set_new_filelocator(Relation rel,
 									  MultiXactId *minmulti)
 {
 	SMgrRelation srel;
+	SMgrRelation oldsrel;
 	uint64		storageId;
 
 	*freezeXid = InvalidTransactionId;
@@ -780,6 +806,40 @@ pgcolumnar_relation_set_new_filelocator(Relation rel,
 				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
 				 errmsg("unlogged columnar tables are not supported")));
 
+	/*
+	 * CREATE TABLE calls this with no existing main fork. TRUNCATE and other
+	 * rewrites call it while the old fork is still attached, so the metapage
+	 * still names the storage id whose catalog rows would otherwise remain
+	 * after the new file is installed. DROP only deletes the current id, so
+	 * a TRUNCATE-then-DROP left every previous storage behind.
+	 */
+	oldsrel = RelationGetSmgr(rel);
+	if (smgrexists(oldsrel, MAIN_FORKNUM) &&
+		smgrnblocks(oldsrel, MAIN_FORKNUM) >= COLUMNAR_INITIALIZED_NBLOCKS)
+	{
+		pgcolumnar_delete_storage_tree(PgColumnarStorageId(rel));
+
+		/*
+		 * And drop the cached write state, which still names the storage id
+		 * whose rows were just deleted. Without this, a transaction that
+		 * writes, truncates and writes again COMMITS into storage nothing
+		 * reads: the second insert reuses the stale state, flushes into the
+		 * retired storage id, and the relation then reads the new one and
+		 * finds it empty.
+		 *
+		 * Before the delete above existed, the retired storage's catalog rows
+		 * survived and the stale flush collided with them on the primary key,
+		 * so the transaction ERRORed and rolled back. That collision was the
+		 * only thing making this safe, and deleting the rows removed it. A
+		 * loud failure became silent loss of committed data, which is why this
+		 * call belongs in the same branch rather than anywhere else.
+		 *
+		 * Forget rather than flush: the rows this state buffers are exactly the
+		 * rows the rewrite is discarding.
+		 */
+		PgColumnarForgetWriteStateForRelation(RelationGetRelid(rel));
+	}
+
 	srel = PgColumnarRelationCreateStorage(*newrlocator, persistence);
 	storageId = PgColumnarNextStorageId();
 	PgColumnarWriteNewMetapage(newrlocator, srel, persistence, storageId);
@@ -791,6 +851,16 @@ pgcolumnar_relation_nontransactional_truncate(Relation rel)
 	uint64		storageId = PgColumnarStorageId(rel);
 
 	PgColumnarDeleteMetadata(storageId);
+
+	/*
+	 * The same stale-write-state hazard as the rewrite path above, reached the
+	 * other way: ExecuteTruncateGuts calls heap_truncate_one_rel, and so this
+	 * callback, when the relation got its filelocator in the current
+	 * subtransaction. The metapage keeps its storage id here, but the buffered
+	 * rows are still the ones being truncated away.
+	 */
+	PgColumnarForgetWriteStateForRelation(RelationGetRelid(rel));
+
 	RelationTruncate(rel, 2);
 	PgColumnarResetMetapage(rel);
 }
@@ -2531,8 +2601,6 @@ pgcolumnar_object_access(ObjectAccessType access, Oid classId, Oid objectId,
 		if (rel->rd_tableam == &pgcolumnar_am_methods)
 		{
 			uint64		storageId = PgColumnarStorageId(rel);
-			List	   *projs = PgColumnarListProjections(storageId);
-			ListCell   *lc;
 
 			/*
 			 * A projection keeps its own storage, so dropping the table has to
@@ -2543,16 +2611,7 @@ pgcolumnar_object_access(ObjectAccessType access, Oid classId, Oid objectId,
 			 * drop. This is the same loop pgcolumnar_vacuum.c runs when it
 			 * rewrites into fresh storage.
 			 */
-			foreach(lc, projs)
-			{
-				PgColumnarProjection *p = (PgColumnarProjection *) lfirst(lc);
-
-				if (p->projStorageId != storageId)
-					PgColumnarDeleteMetadata(p->projStorageId);
-				PgColumnarDeleteProjectionRow(storageId, p->projectionId);
-			}
-
-			PgColumnarDeleteMetadata(storageId);
+			pgcolumnar_delete_storage_tree(storageId);
 			PgColumnarDeleteOptions(objectId);
 			/*
 			 * And the projection declarations, for the same reason and in the

test/run_all_versions.sh

@@ -265,6 +265,7 @@ SUITES=(
 	stats_privilege
 	tablesample
 	temporal
+	truncate_cleanup
 	ttl_expire
 	ungrouped_vector_agg
 	unique_conc

test/truncate_cleanup.sh

@@ -0,0 +1,200 @@
+#!/usr/bin/env bash
+#
+# SQL TRUNCATE must not leave the old storage's catalog rows behind.
+#
+# DROP deletes metadata for the relation's current storage id. TRUNCATE
+# installs a new relfilenode and a new storage id, and used to leave the old
+# catalog rows keyed by the retired id. DROP after that only cleaned the new
+# id, so each truncate-and-reload cycle leaked one storage's worth of
+# row_group, column_chunk, zone_map and bloom rows.
+#
+# Measured on unpatched main (381c765, pg18a) with this suite's own first
+# fixture -- tcl_plain, 5000 rows -- counting
+# storage/projection/row_group/column_chunk/zone_map/bloom/delete_vector:
+#
+#   baseline, no table              0/0/0/0/0/0/0
+#   after CREATE TABLE              0/0/0/0/0/0/0   storage is written on the
+#                                                   first write, not at CREATE
+#   after INSERT 5000               1/0/1/2/4/2/0
+#   after TRUNCATE                  1/0/1/2/4/2/0   unchanged: the retired
+#                                                   storage stays
+#   after a second INSERT           2/0/2/4/8/4/0   doubled
+#   after ten insert/TRUNCATE      10/0/10/20/40/20/0
+#   after DROP TABLE               10/0/10/20/40/20/0
+#
+# Ten distinct storage ids for one table. The last line is the sharper half:
+# DROP removes rows for the relation's CURRENT storage id, and after a TRUNCATE
+# that id has not been written to yet, so DROP takes nothing at all and all ten
+# orphans outlive the table. Counting rather than sampling is deliberate, as in
+# drop_cleanup.sh: a leak of one storage per truncate is exactly the size of
+# this defect.
+#
+# Usage:  test/truncate_cleanup.sh [PG_CONFIG]
+# Written fresh for pgColumnar.
+
+set -uo pipefail
+. "$(dirname "${BASH_SOURCE[0]}")/lib.sh"
+pgc_setup "${1:-/usr/local/pg17/bin/pg_config}"
+
+snapshot() {
+	q "SELECT (SELECT count(*) FROM pgcolumnar.storage) || '/' ||
+		(SELECT count(*) FROM pgcolumnar.projection) || '/' ||
+		(SELECT count(*) FROM pgcolumnar.row_group) || '/' ||
+		(SELECT count(*) FROM pgcolumnar.column_chunk) || '/' ||
+		(SELECT count(*) FROM pgcolumnar.zone_map) || '/' ||
+		(SELECT count(*) FROM pgcolumnar.bloom) || '/' ||
+		(SELECT count(*) FROM pgcolumnar.delete_vector);" | tail -1
+}
+
+base="$(snapshot)"
+
+psql_run "CREATE TABLE tcl_plain (id int, v text) USING pgcolumnar;
+	INSERT INTO tcl_plain SELECT g, 'x' || g FROM generate_series(1, 5000) g;" >/dev/null
+grew="$(snapshot)"
+check "insert did add metadata" \
+	"$(awk -v a="$base" -v b="$grew" 'BEGIN { print (a == b) ? "no" : "yes" }')" "yes"
+
+psql_run "TRUNCATE tcl_plain;" >/dev/null
+check "TRUNCATE returns the catalog to the empty baseline" "$(snapshot)" "$base"
+check_num "TRUNCATE leaves no rows" "$(q 'SELECT count(*) FROM tcl_plain')" "0"
+
+psql_run "INSERT INTO tcl_plain SELECT g, 'x' || g FROM generate_series(1, 5000) g;" >/dev/null
+psql_run "TRUNCATE tcl_plain;" >/dev/null
+psql_run "INSERT INTO tcl_plain SELECT g, 'x' || g FROM generate_series(1, 5000) g;" >/dev/null
+psql_run "TRUNCATE tcl_plain;" >/dev/null
+check "three truncate-and-reload cycles still sit at the baseline" "$(snapshot)" "$base"
+
+# The DROP arm used to run straight after a check that had already asserted
+# snapshot == base, against a table TRUNCATE had just emptied. DROP was asked
+# to remove nothing and the comparison was base against base: deleting the
+# whole cleanup DROP performs left it green. Load the table first, and assert
+# that it is loaded, so the DROP has work to do.
+psql_run "INSERT INTO tcl_plain SELECT g, 'x' || g FROM generate_series(1, 5000) g;" >/dev/null
+check "premise: the table carries metadata again, so the DROP has work to do" \
+	"$(awk -v a="$base" -v b="$(snapshot)" 'BEGIN { print (a == b) ? "no" : "yes" }')" "yes"
+psql_run "DROP TABLE tcl_plain;" >/dev/null
+check "DROP of a loaded table after TRUNCATE leaves nothing" "$(snapshot)" "$base"
+
+# a projection has its own storage id; TRUNCATE used to leak that too
+base="$(snapshot)"
+psql_run "CREATE TABLE tcl_proj (id int, a int, b text) USING pgcolumnar;" >/dev/null
+psql_run "SELECT pgcolumnar.add_projection('tcl_proj','tcl_p',ARRAY['a','b'],ARRAY['a']);" >/dev/null
+psql_run "INSERT INTO tcl_proj SELECT g, g % 50, 'b' || g FROM generate_series(1, 5000) g;" >/dev/null
+grew="$(snapshot)"
+check "the projection did add metadata" \
+	"$(awk -v a="$base" -v b="$grew" 'BEGIN { print (a == b) ? "no" : "yes" }')" "yes"
+
+# What must hold is that no projection row names a storage id the table no
+# longer has. The arm here used to compare the whole snapshot against a $base
+# captured BEFORE the projection existed, so it required pgcolumnar.projection
+# to be EMPTY -- which a rewrite that correctly re-recorded the projection
+# under the new storage id would fail. Count the retired rows instead: that is
+# the leak, it reddens on main, and it stays green if re-recording is ever
+# added.
+#
+# Measured, pg18a, distinct .so per arm, same script both sides:
+#
+#                              main 381c765   this PR
+#   projection rows before          2            2
+#   after TRUNCATE                  2            0
+#   projection_declaration          1            1
+#
+psql_run "TRUNCATE tcl_proj;" >/dev/null
+check "TRUNCATE leaves no projection row under a retired storage id" \
+	"$(q "SELECT count(*) FROM pgcolumnar.projection p
+	      WHERE p.storage_id <> pgcolumnar.get_storage_id('tcl_proj'::regclass)" | tail -1)" "0"
+check "and the declaration survives the TRUNCATE, so the projection is rebuildable" \
+	"$(q "SELECT count(*) FROM pgcolumnar.projection_declaration
+	      WHERE rel = 'tcl_proj'::regclass" | tail -1)" "1"
+
+psql_run "INSERT INTO tcl_proj SELECT g, g % 50, 'b' || g FROM generate_series(1, 5000) g;" >/dev/null
+check "premise: the projected table carries metadata again before the DROP" \
+	"$(awk -v a="$base" -v b="$(snapshot)" 'BEGIN { print (a == b) ? "no" : "yes" }')" "yes"
+psql_run "DROP TABLE tcl_proj;" >/dev/null
+check "DROP of a loaded projected table leaves nothing" "$(snapshot)" "$base"
+
+# Repetition, which is where an unbounded leak shows.
+#
+# This arm used to run all four statements through ONE psql_run, so the server
+# saw one implicit transaction, and core routes TRUNCATE of a relation created
+# in the current (sub)transaction through table_relation_nontransactional_
+# truncate -- an in-place path that never reaches the code this PR adds. The
+# arm passed on unmodified main. The DROP inside the loop hid it a second way:
+# it removed the storage each cycle had just created, so nothing could
+# accumulate even on the path that leaks.
+#
+# One statement per psql_run now, and the table is created once outside the
+# loop, so every TRUNCATE is the transactional path and any leak accumulates.
+# On main this reaches 10/0/10/20/40/20/0 against a 0/0/0/0/0/0/0 baseline.
+base="$(snapshot)"
+psql_run "CREATE TABLE tcl_rep (id int, v text) USING pgcolumnar;" >/dev/null
+for i in $(seq 1 10); do
+	psql_run "INSERT INTO tcl_rep SELECT g, 'x' || g FROM generate_series(1, 2000) g;" >/dev/null
+	psql_run "TRUNCATE tcl_rep;" >/dev/null
+done
+check "ten insert-and-truncate cycles on one table leave nothing" "$(snapshot)" "$base"
+psql_run "DROP TABLE tcl_rep;" >/dev/null
+check "and dropping it afterwards still leaves nothing" "$(snapshot)" "$base"
+
+# ---- a transaction that writes, truncates and writes again --------------------
+#
+# Retiring the old storage's catalog rows removed the only thing that made a
+# stale cached write state safe. The state holds the storage id the rows were
+# just deleted from; the second INSERT reuses it and flushes into a storage the
+# relation no longer reads. Before the rows were deleted that flush collided
+# with them on the primary key and the transaction ERRORed, so the hazard was
+# loud. Deleting them made it silent: the transaction COMMITTED and left the
+# table EMPTY.
+#
+# Measured on three trees, PG 17.10, each statement on its own -c so TRUNCATE
+# does not take core's in-place same-transaction path:
+#
+#   main 53224e4              rc=1  1..10  n=10   loud failure, rolls back
+#   this PR before the fix    rc=0         n=0    COMMITTED, table empty
+#   this PR with the fix      rc=0  21..30 n=10   commits, keeps the new rows
+#
+# main is not the standard to restore here. This transaction is ordinary SQL and
+# it never worked; the arm asserts what it should do, which is commit and hold
+# exactly the rows written after the TRUNCATE.
+
+psql_run "CREATE TABLE tc_txn (id int) USING pgcolumnar;"
+psql_run "INSERT INTO tc_txn SELECT g FROM generate_series(1,10) g;"
+check "premise: the table holds its first ten rows" \
+	"$(q 'SELECT count(*) FROM tc_txn')" "10"
+
+tc_txn_rc=0
+env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres -d "$PGC_DB" \
+	-v ON_ERROR_STOP=1 -At \
+	-c "BEGIN;" \
+	-c "INSERT INTO tc_txn SELECT g FROM generate_series(11,20) g;" \
+	-c "TRUNCATE tc_txn;" \
+	-c "INSERT INTO tc_txn SELECT g FROM generate_series(21,30) g;" \
+	-c "COMMIT;" >/dev/null 2>&1 || tc_txn_rc=$?
+
+# One arm over the exit status AND the surviving rows together. Split apart, a
+# commit that leaves nothing passes the status half, and 1..10 (the TRUNCATE
+# lost) passes a bare count of 10.
+check "INSERT, TRUNCATE, INSERT in one transaction commits and keeps the new rows" \
+	"rc=$tc_txn_rc $(q "SELECT coalesce(min(id),-1) || '..' || coalesce(max(id),-1) ||
+	                    ' n=' || count(*) FROM tc_txn")" \
+	"rc=0 21..30 n=10"
+
+psql_run "DROP TABLE tc_txn;"
+
+# ---- a rewrite that is not a truncate must not lose anything -----------------
+#
+# The retire runs from relation_set_new_filelocator, which every rewrite calls,
+# not only TRUNCATE. If the guard ever misfires on a plain rewrite it takes the
+# rows with it, and drop_cleanup rewrites nothing, so nothing else would notice.
+
+psql_run "CREATE TABLE tc_rw (id int, v text) USING pgcolumnar;"
+psql_run "INSERT INTO tc_rw SELECT g, 'v' || g FROM generate_series(1,5000) g;"
+check "premise: five thousand rows before the rewrite" \
+	"$(q 'SELECT count(*) FROM tc_rw')" "5000"
+psql_run "ALTER TABLE tc_rw ALTER COLUMN v TYPE varchar(64);"
+check "a full rewrite keeps every row, and they are still readable" \
+	"$(q "SELECT count(*) || '/' || count(v) || '/' || max(id) FROM tc_rw")" \
+	"5000/5000/5000"
+psql_run "DROP TABLE tc_rw;"
+
+pgc_summary