From 7ec3868d1a10f70ec329f846a8febcee2daaf2b3 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Tue, 1 Sep 2026 18:50:26 +0000 Subject: [PATCH 1/3] fix: retire old storage catalog rows on TRUNCATE Co-authored-by: Cursor --- src/columnar_tableam.c | 51 ++++++++++++++++++++------ test/run_all_versions.sh | 1 + test/truncate_cleanup.sh | 79 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 119 insertions(+), 12 deletions(-) create mode 100755 test/truncate_cleanup.sh diff --git a/src/columnar_tableam.c b/src/columnar_tableam.c index 78a43575..6c8f20de 100644 --- a/src/columnar_tableam.c +++ b/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,18 @@ 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) >= 2) /* metapage + reserved */ + pgcolumnar_delete_storage_tree(PgColumnarStorageId(rel)); + srel = PgColumnarRelationCreateStorage(*newrlocator, persistence); storageId = PgColumnarNextStorageId(); PgColumnarWriteNewMetapage(newrlocator, srel, persistence, storageId); @@ -2531,8 +2569,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 +2579,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 diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index 86743d3d..53286c89 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -265,6 +265,7 @@ SUITES=( stats_privilege tablesample temporal + truncate_cleanup ttl_expire ungrouped_vector_agg unique_conc diff --git a/test/truncate_cleanup.sh b/test/truncate_cleanup.sh new file mode 100755 index 00000000..b3d8a285 --- /dev/null +++ b/test/truncate_cleanup.sh @@ -0,0 +1,79 @@ +#!/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 before the fix, one insert-then-TRUNCATE left the catalog at the +# post-insert counts instead of the empty baseline; a second insert doubled +# them. Counting rather than sampling is deliberate, as in drop_cleanup.sh: +# a leak of one row 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" + +psql_run "DROP TABLE tcl_plain;" >/dev/null +check "DROP 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" + +psql_run "TRUNCATE tcl_proj;" >/dev/null +check "TRUNCATE takes the projection's retired storage with it" "$(snapshot)" "$base" + +psql_run "DROP TABLE tcl_proj;" >/dev/null +check "DROP of the truncated projected table leaves nothing" "$(snapshot)" "$base" + +# repetition, which is where an unbounded leak shows +base="$(snapshot)" +for i in $(seq 1 10); do + psql_run "CREATE TABLE tcl_rep (id int, v text) USING pgcolumnar; + INSERT INTO tcl_rep SELECT g, 'x' || g FROM generate_series(1, 2000) g; + TRUNCATE tcl_rep; + DROP TABLE tcl_rep;" >/dev/null +done +check "ten create-insert-truncate-drop cycles leave nothing" "$(snapshot)" "$base" + +pgc_summary From 7e429b76bc9dd75ef3c50094b7122627cc6433c5 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Tue, 1 Sep 2026 20:44:10 -0600 Subject: [PATCH 2/3] fix: retiring the old storage must drop the cached write state with it Requesting-changes was right: as it stood this PR turned a loud failure into silent loss of committed data. Reproduced on both trees before changing 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; main 53224e4 rc=1 1..10 n=10 ERROR, rolls back this PR before rc=0 n=0 COMMITS, table EMPTY The cached write state holds the storage id whose catalog rows the retire had just deleted. The second INSERT reuses that state and flushes into a storage the relation no longer reads; the relation reads the new one and finds it empty. Before the retire existed, the old rows survived and the stale flush collided with them on the primary key, so the transaction ERRORed. That collision was the only thing making this safe, and deleting the rows removed it. So 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. this PR with the fix rc=0 21..30 n=10 commits, keeps the new rows Note what that third line says. main is not the standard being restored here. This transaction is ordinary SQL and it never worked on main either; it now does. The second TRUNCATE path is patched too. ExecuteTruncateGuts calls heap_truncate_one_rel, and so pgcolumnar_relation_nontransactional_truncate, when the relation got its filelocator in the current subtransaction, and that path never reaches relation_set_new_filelocator. Same hazard, reached the other way. Two arms added to test/truncate_cleanup.sh. The transaction arm asserts the exit status and the surviving row RANGE together, because split apart a commit that leaves nothing passes the status half and a lost TRUNCATE (1..10) passes a bare count of ten. The second arm is a rewrite that must not lose anything -- ALTER COLUMN TYPE over 5000 rows -- because 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, and the transaction arm goes red with `got [rc=0 -1..-1 n=0]`, which is the silent loss itself. 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: it is defined in columnar_write_state.c and present in both, so that check would have passed on the mutant. truncate_cleanup 13/13, drop_cleanup 8/8, projections 64/64 on PG 17.10. Six of the review's asks are NOT addressed here and are still open: re-recording projection declarations after the rewrite deletes them, the three arms that cannot go red for the defect they name, the header's prose counts, and the bare literal 2 in the guard. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WDbfRym2V1sYFmMZ5gnsQL --- src/columnar_tableam.c | 32 +++++++++++++++++++++ test/truncate_cleanup.sh | 61 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+) diff --git a/src/columnar_tableam.c b/src/columnar_tableam.c index 6c8f20de..467a8312 100644 --- a/src/columnar_tableam.c +++ b/src/columnar_tableam.c @@ -816,8 +816,30 @@ pgcolumnar_relation_set_new_filelocator(Relation rel, oldsrel = RelationGetSmgr(rel); if (smgrexists(oldsrel, MAIN_FORKNUM) && smgrnblocks(oldsrel, MAIN_FORKNUM) >= 2) /* metapage + reserved */ + { 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); @@ -829,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); } diff --git a/test/truncate_cleanup.sh b/test/truncate_cleanup.sh index b3d8a285..9348b472 100755 --- a/test/truncate_cleanup.sh +++ b/test/truncate_cleanup.sh @@ -76,4 +76,65 @@ for i in $(seq 1 10); do done check "ten create-insert-truncate-drop cycles leave 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 From 056d393007f61b222f3a8208065aa259ced46ec2 Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Wed, 2 Sep 2026 16:40:54 +0000 Subject: [PATCH 3/3] test: close the five instrument asks, and name the block layout Five of jdatcmd's six open asks. The sixth (ask 2) is answered in the PR thread with a measurement rather than a commit, because it is not a defect this branch introduces. ASK 4 -- the headline unbounded-leak arm passed on unmodified main. Confirmed by running the suite as submitted against main 381c765: "ten create-insert-truncate-drop cycles leave nothing" is not among the six arms that redden there. Two reasons, both real. All four statements went through one psql_run, so the server saw one implicit transaction, and core routes TRUNCATE of a relation created in the current subtransaction through table_relation_nontransactional_truncate -- an in-place path this PR's code never sees. 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 -- the two DROP arms could not redden for anything DROP does. Each ran straight after a check that had already asserted snapshot == base, against a table TRUNCATE had just emptied, so DROP was asked to remove nothing and the comparison was base against base. Proved by mutation rather than by reading: gut the DROP-path cleanup at src/columnar_tableam.c (pgcolumnar_delete_storage_ tree(storageId) -> a no-op, src md5 dfb2c39900de -> d7422c0f8d27) and leave the TRUNCATE-path 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 The table is loaded before each DROP now, with a premise arm asserting it is loaded, so the DROP has work to do. ASK 6 -- "TRUNCATE takes the projection's retired storage with it" compared the whole snapshot against a $base captured before the projection existed, so it required pgcolumnar.projection to be EMPTY. It does redden on main today, but it would also redden if a rewrite were corrected to re-record the projection under the new storage id, which is the ask's point. It now counts the rows that name a storage id the table no longer has -- which is the leak itself -- and a second arm holds that the declaration survives, so the projection stays rebuildable. On main: got [2] want [0]. ASK 8 -- the header's prose counts are replaced by the numbers this suite's own fixture produces on unpatched main, including the sharper 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 -- smgrnblocks(...) >= 2 encoded the block layout as a bare literal. COLUMNAR_METAPAGE_BLOCKNO and COLUMNAR_EMPTY_BLOCKNO move from private #defines in columnar_storage.c to columnar_storage.h, which already declares that file's interface, and COLUMNAR_INITIALIZED_NBLOCKS is derived from them rather than written as 2. Adding a block now moves the guard with the writer. Green on pg18a at this head: truncate_cleanup 17/17, drop_cleanup 8/8, projections 64/64. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EtQbQUiMSpGWembJV1jxob --- src/columnar_storage.c | 9 +++-- src/columnar_storage.h | 12 ++++++ src/columnar_tableam.c | 2 +- test/truncate_cleanup.sh | 86 ++++++++++++++++++++++++++++++++++------ 4 files changed, 92 insertions(+), 17 deletions(-) diff --git a/src/columnar_storage.c b/src/columnar_storage.c index e930ea3c..6f140656 100644 --- a/src/columnar_storage.c +++ b/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)) /* diff --git a/src/columnar_storage.h b/src/columnar_storage.h index ed4cff4b..5c1a08bb 100644 --- a/src/columnar_storage.h +++ b/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); diff --git a/src/columnar_tableam.c b/src/columnar_tableam.c index 467a8312..2224270e 100644 --- a/src/columnar_tableam.c +++ b/src/columnar_tableam.c @@ -815,7 +815,7 @@ pgcolumnar_relation_set_new_filelocator(Relation rel, */ oldsrel = RelationGetSmgr(rel); if (smgrexists(oldsrel, MAIN_FORKNUM) && - smgrnblocks(oldsrel, MAIN_FORKNUM) >= 2) /* metapage + reserved */ + smgrnblocks(oldsrel, MAIN_FORKNUM) >= COLUMNAR_INITIALIZED_NBLOCKS) { pgcolumnar_delete_storage_tree(PgColumnarStorageId(rel)); diff --git a/test/truncate_cleanup.sh b/test/truncate_cleanup.sh index 9348b472..e670d447 100755 --- a/test/truncate_cleanup.sh +++ b/test/truncate_cleanup.sh @@ -8,10 +8,26 @@ # id, so each truncate-and-reload cycle leaked one storage's worth of # row_group, column_chunk, zone_map and bloom rows. # -# Measured before the fix, one insert-then-TRUNCATE left the catalog at the -# post-insert counts instead of the empty baseline; a second insert doubled -# them. Counting rather than sampling is deliberate, as in drop_cleanup.sh: -# a leak of one row per truncate is exactly the size of this defect. +# 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. @@ -48,8 +64,16 @@ psql_run "INSERT INTO tcl_plain SELECT g, 'x' || g FROM generate_series(1, 5000) 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 after TRUNCATE leaves nothing" "$(snapshot)" "$base" +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)" @@ -60,21 +84,57 @@ 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 takes the projection's retired storage with it" "$(snapshot)" "$base" +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 the truncated projected table leaves nothing" "$(snapshot)" "$base" +check "DROP of a loaded projected table leaves nothing" "$(snapshot)" "$base" -# repetition, which is where an unbounded leak shows +# 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 "CREATE TABLE tcl_rep (id int, v text) USING pgcolumnar; - INSERT INTO tcl_rep SELECT g, 'x' || g FROM generate_series(1, 2000) g; - TRUNCATE tcl_rep; - DROP TABLE tcl_rep;" >/dev/null + 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 create-insert-truncate-drop cycles leave nothing" "$(snapshot)" "$base" +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 -------------------- #