From 9db60a54cae3e1cb5b5804bf95ddc86d4d9683b3 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Tue, 1 Sep 2026 19:14:45 +0000 Subject: [PATCH 1/6] fix: do not let expire drop live rows or leave them visible to index-only scans Co-authored-by: Cursor --- src/columnar.h | 2 ++ src/columnar_vacuum.c | 20 +++++++++++ src/columnar_visibilitymap.c | 25 ++++++++++++++ test/ttl_expire.sh | 66 ++++++++++++++++++++++++++++++++++++ 4 files changed, 113 insertions(+) diff --git a/src/columnar.h b/src/columnar.h index 9ee40969..21d22f41 100644 --- a/src/columnar.h +++ b/src/columnar.h @@ -385,6 +385,8 @@ extern uint64 PgColumnarItemPointerToRowNumber(ItemPointer tid); * visibility map for index-only scans (pgcolumnar_visibilitymap.c, gap 28) * ------------------------------------------------------------------------- */ extern void PgColumnarVMClearForRow(Relation rel, uint64 rowNumber); +extern void PgColumnarVMClearForRowRange(Relation rel, uint64 firstRowNumber, + uint64 rowCount); extern uint64 PgColumnarVMSetVisibleForRelation(Relation rel); /* index maintenance for callers that insert rows without an executor (#153) */ diff --git a/src/columnar_vacuum.c b/src/columnar_vacuum.c index b041973b..3914f0f9 100644 --- a/src/columnar_vacuum.c +++ b/src/columnar_vacuum.c @@ -2039,6 +2039,17 @@ pgcolumnar_cluster(PG_FUNCTION_ARGS) * by group rather than by row, and it is the safe direction: the * alternative drops rows that are still inside the retention. * + * A NULL in the retention column is the same kind of straddle. The zone + * map's maximum covers only the non-NULL timestamps, so a group whose + * timestamps are all past the cutoff can still hold rows whose retention + * is unknown. Dropping it would delete those rows. + * + * Retiring a live group also clears the visibility-map bits covering its + * row numbers. VACUUM may already have marked the group all-visible, and + * an index-only scan would then return the expired keys from the index + * without fetching. A fetch would correctly fail once the catalog rows + * are gone; skipping the fetch is what made the ghosts visible. + * * This is called by name and never runs on its own. It deletes rows, and an * operation a user runs for maintenance must not do that silently, so it is * not wired into vacuum, compact or autovacuum. @@ -2163,6 +2174,14 @@ pgcolumnar_expire(PG_FUNCTION_ARGS) if (z == NULL || !z->hasMinMax) continue; + /* + * Keep any group that stored a NULL in the retention column. The + * maximum cannot speak for those rows, and treating "every timestamp + * we can see is expired" as "the group is expired" drops them. + */ + if (z->nullCount > 0) + continue; + cur = (char *) z->maximum; maxv = PgColumnarDecodeValue(att, &cur, z->maximum + z->maximumLen, CurrentMemoryContext); @@ -2171,6 +2190,7 @@ pgcolumnar_expire(PG_FUNCTION_ARGS) att->attcollation, maxv, cutoff)); if (c < 0) { + PgColumnarVMClearForRowRange(rel, rg->firstRowNumber, rg->rowCount); PgColumnarRetireGroup(storageId, rg->groupNumber); retired++; } diff --git a/src/columnar_visibilitymap.c b/src/columnar_visibilitymap.c index 2046a535..e2aa4153 100644 --- a/src/columnar_visibilitymap.c +++ b/src/columnar_visibilitymap.c @@ -169,6 +169,31 @@ PgColumnarVMClearForRow(Relation rel, uint64 rowNumber) PgColumnarVMClearVisible(rel, blk); } +/* + * PgColumnarVMClearForRowRange + * Clear the all-visible bit for every synthetic block covering + * [firstRowNumber, firstRowNumber + rowCount). Used when a whole live + * row group is retired (pgcolumnar.expire) so an index-only scan cannot + * keep answering from the VM after the group's catalog rows are gone. + */ +void +PgColumnarVMClearForRowRange(Relation rel, uint64 firstRowNumber, uint64 rowCount) +{ + uint64 last; + BlockNumber b0; + BlockNumber b1; + BlockNumber blk; + + if (rowCount == 0) + return; + + last = firstRowNumber + rowCount - 1; + b0 = (BlockNumber) (firstRowNumber / COLUMNAR_VALID_ITEMPOINTER_OFFSETS); + b1 = (BlockNumber) (last / COLUMNAR_VALID_ITEMPOINTER_OFFSETS); + for (blk = b0; blk <= b1; blk++) + PgColumnarVMClearVisible(rel, blk); +} + /* * PgColumnarVMIsVisible * True if `blk` is marked all-visible in the VM fork. Thin wrapper over the diff --git a/test/ttl_expire.sh b/test/ttl_expire.sh index 271f81f8..c8db9a6b 100755 --- a/test/ttl_expire.sh +++ b/test/ttl_expire.sh @@ -25,6 +25,15 @@ # data disappears would pass just as well on an implementation that dropped # everything. # +# Two more ways a "the maximum is expired" reading is not "every row is +# expired": +# * a NULL in the retention column. The zone map's max covers only non-NULL +# timestamps, so a group of old timestamps plus NULLs looks fully expired +# and would drop the NULLs. +# * an index-only scan after VACUUM. expire retires live groups without +# going through the delete vector, so the VM bits VACUUM set stay on and +# the scan answers from the index without fetching the (now missing) group. +# # Usage: test/ttl_expire.sh [PG_CONFIG] # Written fresh for pgColumnar. @@ -127,4 +136,61 @@ ERR="$(q "SELECT pgcolumnar.expire('ttl_none')")" check "a table with no declared retention is an error, not a silent success" \ "$(grep -qiE 'ERROR|no retention|ttl' <<<"$ERR" && echo "refused" || echo "ACCEPTED ($ERR)")" "refused" +# ---- NULL in the retention column is a straddle, not an expiry ------------ +psql_run "CREATE TABLE ttl_null (id int, ts timestamptz, v text) USING pgcolumnar;" +psql_run "INSERT INTO ttl_null + SELECT g, + CASE WHEN g <= 10 THEN NULL + ELSE now() - interval '10 days' END, + 'v'||g + FROM generate_series(1,1000) g;" +psql_run "SELECT pgcolumnar.set_options('ttl_null', ttl_column => 'ts', + ttl_interval => '3 days');" +NULL_BEFORE="$(q "SELECT count(*) FROM ttl_null WHERE ts IS NULL")" +check "premise: the expired group also holds NULL retention rows" "$NULL_BEFORE" "10" +NULL_RETIRED="$(q "SELECT pgcolumnar.expire('ttl_null')")" +NULL_AFTER="$(q "SELECT count(*) FROM ttl_null WHERE ts IS NULL")" +NULL_ROWS="$(q "SELECT count(*) FROM ttl_null")" +check "expire does not retire a group that still holds NULL retention rows" \ + "$NULL_RETIRED" "0" +check "and those NULL rows are still there" "$NULL_AFTER" "10" +check "and the expired timestamps sharing the group were kept with them" \ + "$NULL_ROWS" "1000" + +# ---- index-only scan must not return rows expire already retired ---------- +psql_run "CREATE TABLE ttl_ios (id int, ts timestamptz) USING pgcolumnar;" +psql_run "SELECT pgcolumnar.set_options('ttl_ios', stripe_row_limit => 16384);" +psql_run "INSERT INTO ttl_ios SELECT g, now() - interval '10 days' + FROM generate_series(1,8000) g;" +psql_run "CREATE INDEX ttl_ios_id ON ttl_ios (id);" +psql_run "ALTER DATABASE $PGC_DB SET pgcolumnar.enable_index_only_scan = on;" +psql_run "ALTER DATABASE $PGC_DB SET pgcolumnar.enable_custom_scan = off;" +psql_run "ALTER DATABASE $PGC_DB SET enable_seqscan = off;" +psql_run "ALTER DATABASE $PGC_DB SET enable_bitmapscan = off;" +psql_run "VACUUM ttl_ios;" +psql_run "SELECT pgcolumnar.set_options('ttl_ios', ttl_column => 'ts', + ttl_interval => '3 days');" +ios_plan_before="$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" \ + -U postgres -d "$PGC_DB" -Atq -c \ + "EXPLAIN (COSTS OFF) SELECT id FROM ttl_ios WHERE id BETWEEN 1 AND 8000;")" +check "premise: an index-only scan is chosen on the all-visible table" \ + "$(printf '%s' "$ios_plan_before" | grep -c 'Index Only Scan')" "1" +IOS_RETIRED="$(q "SELECT pgcolumnar.expire('ttl_ios')")" +check "expire retires the all-visible expired group" \ + "$([ "${IOS_RETIRED:-0}" -gt 0 ] && echo retired || echo "RETIRED NOTHING ($IOS_RETIRED)")" \ + "retired" +# Seqscan is the catalog truth: the group is gone. +SEQ_AFTER="$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" \ + -U postgres -d "$PGC_DB" -Atq -c \ + "SET enable_seqscan = on; SET pgcolumnar.enable_custom_scan = on; + SELECT count(*) FROM ttl_ios;")" +check "seqscan agrees the expired rows are gone" "$(printf '%s' "$SEQ_AFTER" | tail -1)" "0" +ios_plan_after="$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" \ + -U postgres -d "$PGC_DB" -Atq -c \ + "EXPLAIN (COSTS OFF) SELECT id FROM ttl_ios WHERE id BETWEEN 1 AND 8000;")" +check "the index-only scan is still the plan after expire" \ + "$(printf '%s' "$ios_plan_after" | grep -c 'Index Only Scan')" "1" +IOS_AFTER="$(q "SELECT count(*) FROM ttl_ios WHERE id BETWEEN 1 AND 8000")" +check "index-only scan does not return rows expire already retired" "$IOS_AFTER" "0" + pgc_summary From a53d50d7992649b701b10e15385a41d93bbefd67 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Tue, 1 Sep 2026 20:53:36 -0600 Subject: [PATCH 2/6] fix: clear the visibility map wherever live rows are renumbered, and refuse a negative retention Three of the review's asks, and the blocking one first. THE VM CLEAR WAS WIRED INTO ONE OF THREE SITES. PgColumnarRetireGroup is reached by expire (columnar_vacuum.c:2194), by the partial-group rewrite behind compact_rewrite (:315) and by recluster (:720). All three retire LIVE groups and reassign those rows fresh row numbers; only expire cleared the old numbers' visibility-map bits. The other two left an index-only scan answering from the index for a group that is gone, which is verbatim the defect this PR describes as fixed. The rule is that the bits go wherever row numbers are reassigned, not only where rows expire, and the comment said so while the code did it once. Both sites are now wired. rewrite_one_group already receives firstRow and rowCount, so that one is direct. recluster sorted a bare array of group numbers, which would have separated each group from its row range, so the array now carries the range in the same struct and sorts on the number. That is why the diff there is larger than two lines. A NEGATIVE ttl_interval PUT THE CUTOFF IN THE FUTURE. `maximum < cutoff` was then true for groups entirely inside their retention, and expire retired them: live rows dropped, which is the failure this PR is named for. set_options range-checks encode_effort, compression, chunk_group_row_limit, stripe_row_limit and compression_level, and did not check this one. Zero is refused too -- it is not a data-loss shape, but "expire everything older than nothing" has no reading a caller means on purpose. ERRCODE is explicit (22023) for the reason the relkind guard gives: this tree's suites assert SQLSTATE, not message text. TWO ARMS COULD NOT FAIL FOR WHAT THEY NAMED. The NULL premise was named for a group-level fact and measured a table-level count, so it passed just as readily on a fixture where the NULL rows sat in a group of their own -- the arrangement it exists to exclude. It now counts row groups whose zone map carries a null_count, and requires exactly one. The index-only premise asserted a PLAN SHAPE, which is decided by relallvisible and by enable_seqscan/enable_bitmapscan being off, not by the bit the fix clears. It now also asserts relallvisible > 0, so the arm can fail for the thing it names. WHAT I TRIED AND WITHDREW, because it is worth recording rather than quietly dropping: I added an arm asserting the bits were CLEARED, comparing relallvisible before and after. It read "still 2 of 2" on a tree where the clear demonstrably works, because relallvisible is a statistic VACUUM refreshes and a VM clear does not touch it. Reading the fork itself needs pg_visibility, which is not built in this environment. The clear is therefore asserted through its consequence, and the file now says so instead of implying a direct measurement. Removal proof for the range check: revert it, keep the arms, and two named arms go red (`got [] want [22023]`) while both controls stay green, so the guard is not refusing every interval. SQL md5 6040fa4e -> b9487f18, guard count 1 -> 0. ttl_expire 26/26, native_recluster 12/12, native_rewrite 17/17, native_reclaim 9/9 on PG 17.10. STILL OPEN, not addressed here: the keep-the-group guard reads z->nullCount, which is a write-time count that still includes deleted rows, so expire refuses a group whose every live row is past retention and whose NULL rows have since been deleted. That needs a live-row property and nobody has a failing fixture for it yet. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WDbfRym2V1sYFmMZ5gnsQL --- CHANGELOG.md | 26 ++++++++++++++++ docs/sql-reference.md | 15 +++++++++ pgcolumnar--1.0-alpha3.sql | 24 ++++++++++++++ src/columnar_vacuum.c | 64 ++++++++++++++++++++++++++++++++------ test/ttl_expire.sh | 62 +++++++++++++++++++++++++++++++++++- 5 files changed, 180 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 707a7e5b..415ca26a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,32 @@ true until the next version shipped. ### Fixed +- `pgcolumnar.expire()` no longer drops live rows, and every path that renumbers + live rows now clears the visibility map (#403). + + **Three separate ways to lose data or read a row that is gone.** + + A group holding a row with a `NULL` retention value could be retired, taking + live rows with it. `expire` now keeps any group whose retention column has a + `NULL`, because a `NULL` has no age and the group can never be known to be + wholly expired. + + A negative `ttl_interval` put the cutoff in the future, so `expire` retired + groups that were entirely inside their retention. `set_options` range-checked + every other option it accepts and not this one. Zero and negative intervals + now raise `22023`. + + Retiring a group leaves its old row numbers in the visibility map, so an + index-only scan answers from the index for a row group that is gone. `expire` + cleared them; `pgcolumnar.recluster()` and the partial-group rewrite behind + `pgcolumnar.compact_rewrite()` did not, and both renumber live rows through + the same retire. All three clear now. The rule is that visibility-map bits go + wherever row numbers are reassigned, not only where rows expire. + + `docs/sql-reference.md` gains the accepted range for `ttl_interval` and the + `NULL` rule, which is stronger than the straddling behaviour the page already + described: a straddling group is released once its newest row ages past the + cutoff, and a group holding a `NULL` never is. - `UPDATE` now fans the new row version out to every covering projection, so a projection scan stops answering as if the updated rows were gone. diff --git a/docs/sql-reference.md b/docs/sql-reference.md index 5b0d26d9..4dc16de7 100644 --- a/docs/sql-reference.md +++ b/docs/sql-reference.md @@ -38,6 +38,10 @@ only by [`pgcolumnar.expire`](#pgcolumnarexpiretablename-regclass-returns-bigint which you run yourself. Declaring a retention does not delete anything on its own. Both are needed: either one alone means no retention. +`ttl_interval` must be a positive interval. Zero and negative intervals raise +`22023`. A negative interval would put the cutoff in the future, so `expire` +would drop rows that are still inside their retention. + ```sql SELECT pgcolumnar.set_options('events', sort_by => ARRAY['customer_id','ts']); SELECT pgcolumnar.reset_options('events', sort_by => true); -- clear it @@ -187,6 +191,17 @@ The retention column must be `timestamp` or `timestamptz`. The table must have both `ttl_column` and `ttl_interval` declared, or the function raises an error rather than reporting that it did nothing. +`expire` works on whole row groups. It never reads or rewrites them, so it drops +a group only when every row in it is past the retention. A group that straddles +the cutoff stays whole, and rows older than the retention survive in it. + +One `NULL` in the retention column pins its entire row group, permanently. A +`NULL` has no age, so the group can never be known to be wholly expired, and no +later `expire` will drop it. This is stronger than the straddling rule +above. A straddling group is released once its newest row ages past the cutoff. +A group holding a `NULL` never is. Keep the retention column `NOT NULL` if you +want `expire` to reclaim the space. + ```sql SELECT pgcolumnar.set_options('events', ttl_column => 'ts', ttl_interval => '90 days'); diff --git a/pgcolumnar--1.0-alpha3.sql b/pgcolumnar--1.0-alpha3.sql index 56596d89..f72a2ea8 100644 --- a/pgcolumnar--1.0-alpha3.sql +++ b/pgcolumnar--1.0-alpha3.sql @@ -497,6 +497,30 @@ BEGIN RAISE EXCEPTION 'compression_level must be between 1 and 22'; END IF; + /* + * A negative retention puts the cutoff in the FUTURE, so expire finds + * `maximum < cutoff` true for groups that are entirely inside their + * retention and retires them. That drops live rows, which is the failure + * this option exists to prevent. Every other option here is range-checked + * and this one was not. + * + * Zero is refused too. It is not a data-loss shape -- the cutoff is now, so + * only groups already wholly in the past go -- but "expire everything older + * than nothing" has no reading a caller means on purpose, and accepting it + * silently makes a typo indistinguishable from an instruction. + * + * ERRCODE is explicit for the reason the relkind guard above gives: this + * tree's suites assert SQLSTATE rather than message text, and plpgsql would + * otherwise default to P0001. + */ + IF ttl_interval IS NOT NULL AND ttl_interval <= interval '0' THEN + RAISE EXCEPTION 'ttl_interval must be a positive interval, not %', ttl_interval + USING ERRCODE = 'invalid_parameter_value', + HINT = 'A negative retention puts the cutoff in the future, ' + 'so pgcolumnar.expire() would retire groups whose rows are ' + 'still within their retention.'; + END IF; + /* * sort_by declares the physical sort key applied by vacuum_sorted() with no * explicit columns (#288). This is a cheap early check only: each named diff --git a/src/columnar_vacuum.c b/src/columnar_vacuum.c index 3914f0f9..a1c2432b 100644 --- a/src/columnar_vacuum.c +++ b/src/columnar_vacuum.c @@ -221,6 +221,29 @@ uint64_cmp(const void *a, const void *b) return (x < y) ? -1 : (x > y) ? 1 : 0; } +/* + * A retired group and the row numbers it held. Recluster needs both: the number + * to lock and retire in ascending order, and the range to clear from the + * visibility map, because those row numbers are being reassigned. Carried in + * one struct rather than parallel arrays so the sort below cannot separate a + * group from its range. + */ +typedef struct RetiredGroup +{ + uint64 groupNumber; + uint64 firstRowNumber; + uint64 rowCount; +} RetiredGroup; + +static int +retired_group_cmp(const void *a, const void *b) +{ + uint64 x = ((const RetiredGroup *) a)->groupNumber; + uint64 y = ((const RetiredGroup *) b)->groupNumber; + + return (x < y) ? -1 : (x > y) ? 1 : 0; +} + /* ------------------------------------------------------------------------- * Online rewrite of partially-deleted row groups (Phase F3b) * @@ -311,7 +334,12 @@ rewrite_one_group(Relation rel, PgColumnarIndexInsertState *ris, uint64 storageI PgColumnarFlushWriteStateForRelation(relid); /* atomically (same transaction) the new group is now in the catalog; drop the - * old one. Heap MVCC keeps the old group readable to older snapshots. */ + * old one. Heap MVCC keeps the old group readable to older snapshots. + * + * The rows just moved carry NEW row numbers, so the old numbers' visibility + * map bits must go with the group. Without this an index-only scan answers + * from the index for a TID whose group no longer exists. */ + PgColumnarVMClearForRowRange(rel, firstRow, rowCount); PgColumnarRetireGroup(storageId, groupNumber); PopActiveSnapshot(); @@ -539,7 +567,7 @@ pgcolumnar_recluster_online(Relation rel, int ncols, AttrNumber *atts) Snapshot listSnap; List *rgList; ListCell *lc; - uint64 *oldGroups; + RetiredGroup *oldGroups; int nGroups = 0; int i; Snapshot snap; @@ -569,12 +597,16 @@ pgcolumnar_recluster_online(Relation rel, int ncols, AttrNumber *atts) /* capture the current groups (retired at the end, after the new ones exist) */ listSnap = RegisterSnapshot(GetLatestSnapshot()); rgList = PgColumnarReadRowGroupList(storageId, listSnap); - oldGroups = palloc(sizeof(uint64) * (list_length(rgList) > 0 ? list_length(rgList) : 1)); + oldGroups = palloc(sizeof(RetiredGroup) * + (list_length(rgList) > 0 ? list_length(rgList) : 1)); foreach(lc, rgList) { NativeRowGroupMetadata *rg = (NativeRowGroupMetadata *) lfirst(lc); - oldGroups[nGroups++] = rg->groupNumber; + oldGroups[nGroups].groupNumber = rg->groupNumber; + oldGroups[nGroups].firstRowNumber = rg->firstRowNumber; + oldGroups[nGroups].rowCount = rg->rowCount; + nGroups++; } UnregisterSnapshot(listSnap); @@ -588,7 +620,7 @@ pgcolumnar_recluster_online(Relation rel, int ncols, AttrNumber *atts) errhint("Use pgcolumnar.cluster() for a one-shot reorg of a very large table."))); /* lock every group in ascending order (deadlock-safe), held to commit */ - qsort(oldGroups, nGroups, sizeof(uint64), uint64_cmp); + qsort(oldGroups, nGroups, sizeof(RetiredGroup), retired_group_cmp); /* * Self-gate (#415): if the whole live relation is already the Z-order run @@ -627,8 +659,8 @@ pgcolumnar_recluster_online(Relation rel, int ncols, AttrNumber *atts) } } if (sameKey && sfrom >= 0 && sthrough >= 0 && - (int64) oldGroups[0] >= sfrom && - (int64) oldGroups[nGroups - 1] <= sthrough) + (int64) oldGroups[0].groupNumber >= sfrom && + (int64) oldGroups[nGroups - 1].groupNumber <= sthrough) { pfree(oldGroups); /* no active snapshot pushed yet at this point -- see below */ @@ -637,7 +669,7 @@ pgcolumnar_recluster_online(Relation rel, int ncols, AttrNumber *atts) } for (i = 0; i < nGroups; i++) - PgColumnarLockChunkGroup(storageId, oldGroups[i]); + PgColumnarLockChunkGroup(storageId, oldGroups[i].groupNumber); /* read all live rows into a Morton-keyed tuplesort (as in eager cluster). * Register the snapshot (not just push active) so the catalog snapshot copies @@ -715,9 +747,21 @@ pgcolumnar_recluster_online(Relation rel, int ncols, AttrNumber *atts) tuplesort_end(tsort); ExecDropSingleTupleTableSlot(augSlot); - /* retire the old groups; heap MVCC keeps them readable to older snapshots */ + /* + * Retire the old groups; heap MVCC keeps them readable to older snapshots. + * + * Clear the visibility map over the row numbers each retired group held. + * Recluster reassigns those rows fresh numbers, so an index-only scan that + * answers from the index for an old TID would answer for a group that is + * gone. That is the same rule expire follows, and it holds wherever LIVE + * rows are renumbered rather than only where they expire. + */ for (i = 0; i < nGroups; i++) - PgColumnarRetireGroup(storageId, oldGroups[i]); + { + PgColumnarVMClearForRowRange(rel, oldGroups[i].firstRowNumber, + oldGroups[i].rowCount); + PgColumnarRetireGroup(storageId, oldGroups[i].groupNumber); + } /* record how far the reordered run reaches (#311) and BY WHAT (#415) */ record_online_sorted_extent(rel, storageId, writeState, stripeMark, diff --git a/test/ttl_expire.sh b/test/ttl_expire.sh index c8db9a6b..888feff4 100755 --- a/test/ttl_expire.sh +++ b/test/ttl_expire.sh @@ -147,7 +147,19 @@ psql_run "INSERT INTO ttl_null psql_run "SELECT pgcolumnar.set_options('ttl_null', ttl_column => 'ts', ttl_interval => '3 days');" NULL_BEFORE="$(q "SELECT count(*) FROM ttl_null WHERE ts IS NULL")" -check "premise: the expired group also holds NULL retention rows" "$NULL_BEFORE" "10" +check "premise: the table holds NULL retention rows at all" "$NULL_BEFORE" "10" +# The arm below is about a GROUP that mixes expired rows with NULL ones. A +# table-level count cannot see a row group, and passes just as readily on a +# fixture where the NULL rows sit in a group of their own -- which is the +# arrangement the premise exists to exclude. Count the groups that hold both. +check "premise: and the SAME row group holds expired rows and NULL ones" \ + "$(q "SELECT count(*) FROM ( + SELECT z.group_number + FROM pgcolumnar.zone_map z + JOIN pgcolumnar.storage s ON s.storage_id = z.storage_id + WHERE s.relation_oid = 'ttl_null'::regclass + AND z.null_count > 0 + GROUP BY z.group_number) g")" "1" NULL_RETIRED="$(q "SELECT pgcolumnar.expire('ttl_null')")" NULL_AFTER="$(q "SELECT count(*) FROM ttl_null WHERE ts IS NULL")" NULL_ROWS="$(q "SELECT count(*) FROM ttl_null")" @@ -175,6 +187,14 @@ ios_plan_before="$(env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" "EXPLAIN (COSTS OFF) SELECT id FROM ttl_ios WHERE id BETWEEN 1 AND 8000;")" check "premise: an index-only scan is chosen on the all-visible table" \ "$(printf '%s' "$ios_plan_before" | grep -c 'Index Only Scan')" "1" +# The plan shape is decided by pg_class.relallvisible and by enable_seqscan and +# enable_bitmapscan being off -- NOT by the visibility-map bit the fix clears. +# Stop PgColumnarVMSetVisibleForRelation writing bits and the plan is unchanged, +# so the arm above cannot fail for the thing under test. Assert the bit itself. +check "premise: and VACUUM really did set visibility-map bits to clear" \ + "$(q "SELECT CASE WHEN relallvisible > 0 THEN 'set' ELSE 'none' END + FROM pg_class WHERE oid = 'ttl_ios'::regclass")" "set" +IOS_VM_BEFORE="$(q "SELECT relallvisible FROM pg_class WHERE oid = 'ttl_ios'::regclass")" IOS_RETIRED="$(q "SELECT pgcolumnar.expire('ttl_ios')")" check "expire retires the all-visible expired group" \ "$([ "${IOS_RETIRED:-0}" -gt 0 ] && echo retired || echo "RETIRED NOTHING ($IOS_RETIRED)")" \ @@ -193,4 +213,44 @@ check "the index-only scan is still the plan after expire" \ IOS_AFTER="$(q "SELECT count(*) FROM ttl_ios WHERE id BETWEEN 1 AND 8000")" check "index-only scan does not return rows expire already retired" "$IOS_AFTER" "0" +# NOT asserted here: that the visibility-map bits were CLEARED, as opposed to +# the consequence above. I tried and the arm was wrong -- pg_class.relallvisible +# is a statistic that VACUUM refreshes, and clearing a VM bit does not touch it, +# so the arm read "still 2 of 2" on a tree where the clear demonstrably works. +# Reading the fork itself needs pg_visibility, which is not built in this +# environment (no pg_visibility.control under the prefix's extension directory). +# +# So the clear is asserted through its consequence, with the premise above +# pinning the thing that was previously assumed: that VACUUM really did set bits +# for this table. That was the reviewer's ask, and it is what makes the +# index-only arm able to fail for the reason it names. + +# ---- a negative retention must be refused, not applied ---------------------- +# +# A negative ttl_interval puts the cutoff in the FUTURE, so expire finds +# `maximum < cutoff` true for groups entirely inside their retention and retires +# them: live rows dropped, which is the failure this suite is named for. +# SQLSTATE, not message text -- 22023 comes from the range check, while a +# missing function would be 42883 and a non-owner 42501. +ttl_state() { + env PATH="$PGC_BINDIR:$PATH" psql -h 127.0.0.1 -p "$PGC_PORT" -U postgres \ + -d "$PGC_DB" -v ON_ERROR_STOP=1 -Atq -v VERBOSITY=sqlstate -c "$1" 2>&1 \ + | sed -n 's/^ERROR: \([0-9A-Z]\{5\}\).*/\1/p' | head -1 +} +psql_run "CREATE TABLE ttl_neg (id int, ts timestamptz) USING pgcolumnar;" +psql_run "INSERT INTO ttl_neg SELECT g, now() FROM generate_series(1,100) g;" +check "a negative ttl_interval is refused with 22023" \ + "$(ttl_state "SELECT pgcolumnar.set_options('ttl_neg', ttl_column => 'ts', + ttl_interval => '-3 days');")" "22023" +check "and a zero ttl_interval is refused too" \ + "$(ttl_state "SELECT pgcolumnar.set_options('ttl_neg', ttl_column => 'ts', + ttl_interval => '0 seconds');")" "22023" +# Control: without this pair, a guard that refused EVERY interval would look +# identical to one that refuses only the dangerous ones. +check "control: and a positive one is still accepted" \ + "$(ttl_state "SELECT pgcolumnar.set_options('ttl_neg', ttl_column => 'ts', + ttl_interval => '3 days');")" "" +check "control: and the rows are all still there" \ + "$(q 'SELECT count(*) FROM ttl_neg')" "100" + pgc_summary From fc6641496f56964e0653486175c3dd7878e6317a Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Tue, 1 Sep 2026 21:11:40 -0600 Subject: [PATCH 3/6] fix: the NULL guard must read live rows, or it over-retains forever Ask 3, with the failing fixture that was missing when I deferred it. The reviewer built it and it goes red on this branch, showing both directions of the trade at once: main 53224e4 this branch, before CONTROL no NULL, all 400d old 1 / 0 1 / 0 ARM L 90 LIVE NULLs in the group 1 / 0 / 0 0 / 900 / 90 ^ DATA LOSS ^ correct ARM R same NULLs, DELETED first 1 / 0 0 / 810 ^ by accident ^ REFUSED FOREVER Arm R is the defect. Every live row 400 days old against a 90-day retention, no live row holding a NULL, and the group is never retired -- because z->nullCount is recorded at WRITE time, still counts the deleted NULLs, and nothing rewrites a zone map on delete. So the branch as it stood traded data loss for permanent over-retention. Quieter, and not better. The guard now asks a live-row question. Nothing in the metadata says WHICH rows were null, only how many, so when a group has both recorded NULLs and deletes the only way to tell is to look: the ordinary reader merges the delete vector, so every row it yields is live. The metadata-only path is unchanged where it is still correct. One probe per expire, not per group: with no delete vector anywhere in the storage a write-time null count is still exact, so expire reads nothing at all, which is what it promises. Only a table that has deletes can have a stale count, and only groups in such a table are read. Committed arms, not just the reviewer's probe. ONE INSERT statement per fixture: his first attempt used two, each flushed its own row group, the NULLs never shared a group with the rows under test, and both trees produced identical output. The group count is asserted rather than printed for that reason. Removal proof: revert the guard to `if (z->nullCount > 0) continue;`, keep the arms, and the two new arms go red with `got [0] want [1]` and `got [810] want [0]` -- the over-retention itself. .so 5d83cbb7 -> b18430e8. The control pair stays green, so the fix is not "retire everything wearing a delete". ttl_expire 34/34, native_recluster 12/12, native_rewrite 17/17, native_reclaim 9/9 on PG 17.10. The docs sentence is corrected with it. It said a NULL pins its group permanently, which was true of the code as written and is no longer true: deleting the NULL rows releases the group. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WDbfRym2V1sYFmMZ5gnsQL --- CHANGELOG.md | 12 ++++++- docs/sql-reference.md | 6 ++-- src/columnar_vacuum.c | 75 ++++++++++++++++++++++++++++++++++++++++--- test/ttl_expire.sh | 73 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 158 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 415ca26a..ab89219c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,9 +42,19 @@ true until the next version shipped. A group holding a row with a `NULL` retention value could be retired, taking live rows with it. `expire` now keeps any group whose retention column has a - `NULL`, because a `NULL` has no age and the group can never be known to be + LIVE `NULL`, because a `NULL` has no age and the group cannot be known to be wholly expired. + Live is the operative word. The zone map's null count is recorded when the + group is written and never revised, so it still counts rows a later `DELETE` + marked. Reading it as the live count keeps a group whose every live row is + past retention and whose `NULL` rows have all been deleted, and keeps it + permanently, because nothing rewrites a zone map on delete. That trades data + loss for silent over-retention. `expire` now checks the live rows, and only + for a group that has both recorded `NULL`s and deletes: with no delete vector + the recorded count is still exact, so the metadata-only path is unchanged and + `expire` still reads nothing. + A negative `ttl_interval` put the cutoff in the future, so `expire` retired groups that were entirely inside their retention. `set_options` range-checked every other option it accepts and not this one. Zero and negative intervals diff --git a/docs/sql-reference.md b/docs/sql-reference.md index 4dc16de7..1ba0d00f 100644 --- a/docs/sql-reference.md +++ b/docs/sql-reference.md @@ -195,9 +195,9 @@ rather than reporting that it did nothing. a group only when every row in it is past the retention. A group that straddles the cutoff stays whole, and rows older than the retention survive in it. -One `NULL` in the retention column pins its entire row group, permanently. A -`NULL` has no age, so the group can never be known to be wholly expired, and no -later `expire` will drop it. This is stronger than the straddling rule +One live `NULL` in the retention column pins its entire row group. A `NULL` has +no age, so the group cannot be known to be wholly expired. Deleting the `NULL` +rows releases the group, and the next `expire` can drop it. This is stronger than the straddling rule above. A straddling group is released once its newest row ages past the cutoff. A group holding a `NULL` never is. Keep the retention column `NOT NULL` if you want `expire` to reclaim the space. diff --git a/src/columnar_vacuum.c b/src/columnar_vacuum.c index a1c2432b..cebbc6d5 100644 --- a/src/columnar_vacuum.c +++ b/src/columnar_vacuum.c @@ -202,6 +202,8 @@ PgColumnarRequireTableOwnerByOid(Oid relid) /* Z-order helpers (defined later, used by the online recluster below) */ static bool cluster_type_supported(Oid typid); +static bool group_has_live_null(Relation rel, NativeRowGroupMetadata *rg, + AttrNumber attno); static bytea *cluster_zorder_key(Datum *values, bool *isnull, AttrNumber *atts, int ncols, TupleDesc tupdesc); /* Names an ordering rewrite records as its key (defined later, #415) */ @@ -2112,6 +2114,7 @@ pgcolumnar_expire(PG_FUNCTION_ARGS) TypeCacheEntry *tce; uint64 storageId; List *groups; + bool anyDeletes; ListCell *lc; int64 retired = 0; int i; @@ -2199,6 +2202,15 @@ pgcolumnar_expire(PG_FUNCTION_ARGS) storageId = PgColumnarStorageId(rel); groups = PgColumnarReadRowGroupList(storageId, GetActiveSnapshot()); + /* + * One probe for the whole storage, not one per group. With no delete vector + * anywhere, a zone map's write-time nullCount is still exact, so expire + * keeps its metadata-only path and reads nothing at all -- which is what it + * promises. Only a table that has deletes can have a stale nullCount, and + * only groups in such a table are looked at. + */ + anyDeletes = PgColumnarStorageHasDeleteVector(storageId, GetActiveSnapshot()); + foreach(lc, groups) { NativeRowGroupMetadata *rg = (NativeRowGroupMetadata *) lfirst(lc); @@ -2219,12 +2231,26 @@ pgcolumnar_expire(PG_FUNCTION_ARGS) continue; /* - * Keep any group that stored a NULL in the retention column. The - * maximum cannot speak for those rows, and treating "every timestamp - * we can see is expired" as "the group is expired" drops them. + * Keep any group that holds a LIVE NULL in the retention column. The + * maximum cannot speak for those rows, and treating "every timestamp we + * can see is expired" as "the group is expired" drops them. + * + * z->nullCount is recorded at WRITE time and never revised, so it still + * counts rows a later DELETE marked. Reading it as the live count keeps + * a group whose every live row is past retention and whose NULL rows + * have all been deleted -- and keeps it FOREVER, because nothing + * rewrites a zone map on delete. That trades data loss for permanent + * over-retention, which is quieter and not better. + * + * With no deletes the two counts agree, so the metadata answer stands + * and expire still reads nothing. Only a group with recorded NULLs AND + * deletes is ambiguous, and only that group is looked at. */ if (z->nullCount > 0) - continue; + { + if (!anyDeletes || group_has_live_null(rel, rg, attno)) + continue; + } cur = (char *) z->maximum; maxv = PgColumnarDecodeValue(att, &cur, z->maximum + z->maximumLen, @@ -2246,6 +2272,47 @@ pgcolumnar_expire(PG_FUNCTION_ARGS) PG_RETURN_INT64(retired); } +/* + * group_has_live_null + * Does any LIVE row of this group hold a NULL in the retention column? + * + * The zone map's nullCount is recorded at WRITE time and is never revised, so it + * still counts rows that a later DELETE marked. It answers "were any NULLs ever + * written here", which is not the question expire has to ask. + * + * Nothing in the metadata says WHICH rows were null, so when the group has both + * recorded NULLs and deletes the only way to tell is to look. This reads one + * column of one group through the ordinary reader, which merges the delete + * vector, so every row it yields is live. + */ +static bool +group_has_live_null(Relation rel, NativeRowGroupMetadata *rg, AttrNumber attno) +{ + TupleDesc tupdesc = RelationGetDescr(rel); + Datum *values = palloc(sizeof(Datum) * tupdesc->natts); + bool *isnull = palloc(sizeof(bool) * tupdesc->natts); + PgColumnarReadState *rs; + uint64 rowNumber; + bool found = false; + + rs = PgColumnarBeginRead(rel, GetActiveSnapshot(), NULL, NULL, 0, NULL); + PgColumnarReadRestrictToGroups(rs, &rg->groupNumber, 1); + while (PgColumnarReadNextRow(rs, values, isnull, &rowNumber)) + { + CHECK_FOR_INTERRUPTS(); + if (isnull[attno - 1]) + { + found = true; + break; + } + } + PgColumnarEndRead(rs); + + pfree(values); + pfree(isnull); + return found; +} + Datum pgcolumnar_compact(PG_FUNCTION_ARGS) { diff --git a/test/ttl_expire.sh b/test/ttl_expire.sh index 888feff4..15919b7b 100755 --- a/test/ttl_expire.sh +++ b/test/ttl_expire.sh @@ -253,4 +253,77 @@ check "control: and a positive one is still accepted" \ check "control: and the rows are all still there" \ "$(q 'SELECT count(*) FROM ttl_neg')" "100" +# ---- a NULL that has been DELETED must stop pinning its group --------------- +# +# z->nullCount is recorded at WRITE time and never revised, so it still counts +# rows a later DELETE marked. Reading it as the live count keeps a group whose +# every live row is past retention and whose NULL rows have all been deleted -- +# and keeps it FOREVER, because nothing rewrites a zone map on delete. That +# trades data loss for permanent over-retention, which is quieter and not +# better. +# +# Measured on both trees before the guard read a live-row property: +# +# main 53224e4 expire 1, rows left 0 (right, but by dropping the +# live NULL rows in arm L too) +# the null-count guard expire 0, rows left 810 (REFUSED, and would never +# retire this group again) +# +# ONE INSERT statement, not two. Two statements flush two row groups, the NULLs +# never share a group with the rows under test, and every arm below answers a +# question nobody asked. The group count is asserted rather than assumed for +# exactly that reason. + +psql_run "CREATE TABLE ttl_dn (id int, ts timestamptz, v text) USING pgcolumnar;" +psql_run "INSERT INTO ttl_dn + SELECT g, + CASE WHEN g % 10 = 0 THEN NULL + ELSE now() - interval '400 days' END, + 'v' || g + FROM generate_series(1,900) g;" +psql_run "SELECT pgcolumnar.set_options('ttl_dn', ttl_column => 'ts', + ttl_interval => '90 days');" + +check "premise: the fixture is ONE row group, so the NULLs share it" \ + "$(q "SELECT count(*) FROM pgcolumnar.row_group rg + JOIN pgcolumnar.storage s ON s.storage_id = rg.storage_id + WHERE s.relation_oid = 'ttl_dn'::regclass")" "1" + +# Delete every NULL row. The zone map is not rewritten, so nullCount still +# counts them -- which is the whole point. +psql_run "DELETE FROM ttl_dn WHERE ts IS NULL;" + +check "premise: no LIVE row holds a NULL retention value any more" \ + "$(q 'SELECT count(*) FROM ttl_dn WHERE ts IS NULL')" "0" +check "premise: but the zone map still records the deleted NULLs" \ + "$(q "SELECT CASE WHEN sum(z.null_count) > 0 THEN 'still recorded' ELSE 'gone' END + FROM pgcolumnar.zone_map z + JOIN pgcolumnar.storage s ON s.storage_id = z.storage_id + WHERE s.relation_oid = 'ttl_dn'::regclass")" "still recorded" +check "premise: and every live row is past the retention" \ + "$(q "SELECT count(*) FROM ttl_dn WHERE ts >= now() - interval '90 days'")" "0" + +DN_RETIRED="$(q "SELECT pgcolumnar.expire('ttl_dn')")" +check "a group whose only NULLs have been deleted is retired" "$DN_RETIRED" "1" +check "and its rows are gone" "$(q 'SELECT count(*) FROM ttl_dn')" "0" + +# Control: the live-NULL case must still be refused, or the fix above is just +# "retire everything" wearing a delete. +psql_run "CREATE TABLE ttl_dl (id int, ts timestamptz, v text) USING pgcolumnar;" +psql_run "INSERT INTO ttl_dl + SELECT g, + CASE WHEN g % 10 = 0 THEN NULL + ELSE now() - interval '400 days' END, + 'v' || g + FROM generate_series(1,900) g;" +psql_run "SELECT pgcolumnar.set_options('ttl_dl', ttl_column => 'ts', + ttl_interval => '90 days');" +# One row deleted, so the table HAS a delete vector and takes the same path as +# ttl_dn above -- but the NULLs are still live. +psql_run "DELETE FROM ttl_dl WHERE id = 1;" +DL_RETIRED="$(q "SELECT pgcolumnar.expire('ttl_dl')")" +check "control: a group whose NULLs are still live is NOT retired" "$DL_RETIRED" "0" +check "control: and those NULL rows survive" \ + "$(q 'SELECT count(*) FROM ttl_dl WHERE ts IS NULL')" "90" + pgc_summary From 4b58e308ab5971d587adfb58470eb312fc141090 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Tue, 1 Sep 2026 21:17:10 -0600 Subject: [PATCH 4/6] docs: record what the coarse delete probe costs, so switching it is a decision The storage-wide PgColumnarStorageHasDeleteVector probe is deliberately coarse: one delete anywhere in the table sends every null-bearing group down the read path, where a per-group deleted count would skip the ones carrying no deletes. That trade was made without a number beside it, which is how a deliberate choice becomes something a later reader has to rediscover. Measured on PG 17.10, 200,000 rows at stripe_row_limit 10000, arms interleaved and both retiring all 20 groups so they differ only in the path: read path 16 ms 11 ms 11 ms metadata path 1 ms 2 ms 3 ms about +0.5 ms per row group. An independent run at 40 groups gave +0.3 ms per group, so the figure is the right order and not exact. Neither transfers to the shipped stripe_row_limit of 150000, where a group holds fifteen times these rows, and the comment says so rather than letting the number look general. The probe asserts BOTH arms retired the same number of groups. Without it an arm that refused every group would time the refusal path and report six plausible milliseconds about nothing -- which is exactly what happened to the first version of this measurement on the other side, and what the check caught on mine when a stray psql meta-command made every reading the string "is off.". Kept coarse because of what kind of path this is. relation_estimate_size runs on every plan of every query and a per-group fold there was worth removing; expire is a maintenance function called by name. The per-group count is also only exposed in a header by #868, so using it would couple this fix to that PR landing first. Comment only. No behaviour change; ttl_expire 34/34 on PG 17.10. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WDbfRym2V1sYFmMZ5gnsQL --- src/columnar_vacuum.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/columnar_vacuum.c b/src/columnar_vacuum.c index cebbc6d5..97f10533 100644 --- a/src/columnar_vacuum.c +++ b/src/columnar_vacuum.c @@ -2208,6 +2208,29 @@ pgcolumnar_expire(PG_FUNCTION_ARGS) * keeps its metadata-only path and reads nothing at all -- which is what it * promises. Only a table that has deletes can have a stale nullCount, and * only groups in such a table are looked at. + * + * This is COARSE on purpose, and the cost is recorded here so that changing + * it later is a decision rather than a rediscovery. A per-group deleted + * count would skip the read for a null-bearing group carrying no deletes; + * this probe makes one delete anywhere in the storage send every such group + * down the read path. Measured on PG 17.10, 200,000 rows at + * stripe_row_limit 10000, both arms retiring all 20 groups so they differ + * only in the path: + * + * read path 16 ms 11 ms 11 ms + * metadata path 1 ms 2 ms 3 ms + * + * about +0.5 ms per row group. An independent run at 40 groups gave +0.3 ms + * per group, so the figure is the right order and not exact. NEITHER + * transfers to the shipped stripe_row_limit of 150000, where a group holds + * fifteen times these rows; nobody has measured it there. + * + * It is kept because of what kind of path this is. relation_estimate_size + * runs on every plan of every query, and a per-group fold there was worth + * removing. expire is a maintenance function called by name, where a few + * milliseconds is a different class of problem -- and the per-group count is + * only exposed in a header by a separate PR, so using it would couple this + * fix to that one landing first. */ anyDeletes = PgColumnarStorageHasDeleteVector(storageId, GetActiveSnapshot()); From 346f4659fe258cc241cc3dcd6f8a67a6506bc847 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Tue, 1 Sep 2026 21:22:09 -0600 Subject: [PATCH 5/6] docs: the probe's cost is a total, not a rate, and the shape is undetermined My previous commit put "+0.5 ms per row group" in that comment. That is a rate, and I did not measure a rate. I divided one total by one group count. The tell was there and I explained it away. An independent run at the SAME geometry gave +0.3 ms per group. Same rows per group, same box, same code -- a real per-group cost would have agreed. I wrote "the right order and not exact" and moved on, which is the thing I have spent this session catching elsewhere: a disagreement between two instruments is evidence about the frame, not a range to average. Swept the group count at fixed group size, six repetitions, arm order alternated, every point asserting both arms retired all N groups: 5 groups read 3-7 ms metadata 1-2 ms 40 groups read 21-37 ms metadata 2-5 ms Fitting a + b*groups over my points gives about 0.7 ms fixed and 0.59 ms per group. The same fit over the other sweep's points gives 2.6 ms fixed and 0.27 ms per group. Same box, same geometry, opposite decompositions -- so the shape is NOT determined by either dataset, and the reason is visible in the spread: 21 to 37 ms across repetitions at 40 groups is comparable to the difference being fitted. So the comment now states the totals and says explicitly not to divide by the group count. The concrete harm it prevents: a reader multiplying 0.5 ms/group by a table of several thousand groups gets seconds, and nothing measured supports that. What survives is what the decision actually needed: at these sizes the whole thing is milliseconds, and the read arm is under 40 ms for a 400,000-row table. Still nothing at the shipped stripe_row_limit of 150000. Comment only. ttl_expire 34/34 on PG 17.10. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WDbfRym2V1sYFmMZ5gnsQL --- src/columnar_vacuum.c | 38 +++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/src/columnar_vacuum.c b/src/columnar_vacuum.c index 97f10533..03025b2b 100644 --- a/src/columnar_vacuum.c +++ b/src/columnar_vacuum.c @@ -2209,21 +2209,33 @@ pgcolumnar_expire(PG_FUNCTION_ARGS) * promises. Only a table that has deletes can have a stale nullCount, and * only groups in such a table are looked at. * - * This is COARSE on purpose, and the cost is recorded here so that changing - * it later is a decision rather than a rediscovery. A per-group deleted - * count would skip the read for a null-bearing group carrying no deletes; - * this probe makes one delete anywhere in the storage send every such group - * down the read path. Measured on PG 17.10, 200,000 rows at - * stripe_row_limit 10000, both arms retiring all 20 groups so they differ - * only in the path: + * This is COARSE on purpose, and what it costs is recorded here so that + * changing it later is a decision rather than a rediscovery. A per-group + * deleted count would skip the read for a null-bearing group carrying no + * deletes; this probe makes one delete anywhere in the storage send every + * such group down the read path. * - * read path 16 ms 11 ms 11 ms - * metadata path 1 ms 2 ms 3 ms + * Measured on PG 17.10 at 10000 rows a group, six repetitions, arm order + * alternated, every point asserting both arms retired all N groups so they + * differ only in the path: * - * about +0.5 ms per row group. An independent run at 40 groups gave +0.3 ms - * per group, so the figure is the right order and not exact. NEITHER - * transfers to the shipped stripe_row_limit of 150000, where a group holds - * fifteen times these rows; nobody has measured it there. + * 5 groups read 3-7 ms metadata 1-2 ms + * 40 groups read 21-37 ms metadata 2-5 ms + * + * THOSE ARE TOTALS, AND THEY ARE NOT A RATE. Do not divide by the group + * count and multiply back up. Two independent sweeps on this same box, at + * this same geometry, agree on the totals and disagree on the decomposition: + * fitting a + b*groups gives roughly 0.7 ms fixed with 0.59 ms per group + * from one, and 2.6 ms fixed with 0.27 ms per group from the other. Neither + * dataset determines which, because the repetition spread (21 to 37 ms at 40 + * groups) is comparable to the difference being fitted. A reader who took + * 0.5 ms per group and multiplied by a table of several thousand groups + * would get seconds, and nothing here supports that. + * + * What IS supported: at these sizes the whole thing is milliseconds, and the + * read arm is under 40 ms for a 400,000-row table. Nothing has been measured + * at the shipped stripe_row_limit of 150000, where a group holds fifteen + * times these rows. * * It is kept because of what kind of path this is. relation_estimate_size * runs on every plan of every query, and a per-group fold there was worth From 41eb5852d218997154de1735c28fda2a4c228723 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Tue, 1 Sep 2026 21:24:11 -0600 Subject: [PATCH 6/6] docs: name the instrument, because wall clock on this host is not a fair one The two sweeps in the comment above disagreed about the decomposition. The likeliest reason is not the subject, it is the environment: the same 40 groups timed 21-37 ms in one sweep and 12-21 ms in the other, taken while a second tenant was building and running suites on the same eight cores. That difference is larger than the effect either fit was resolving. So the comment now names the instrument and its limit. The numbers bound the magnitude; they cannot support a shape. If the shape ever matters, the fair instrument on this host is instructions retired by the backend, because contention moves the clock and does not move the instruction count. This one is worth recording plainly. There is a note in my own working memory, written a week ago, that says wall-clock A/B on this machine is not a fair instrument, that a host process pegging a core produced a 10-43% swing that REVERSED when the arm order was flipped, and that instruction counts are the answer. I used wall clock anyway, on a contended host, with another agent actively running suites, and committed the numbers to a source comment. Having the rule did not fire the rule. What kept it survivable was someone else asking why two runs of the same thing differed, twice: first about the quotients, which found that I had reported a total as a rate, and then about the absolute numbers, which found that the frame was contention all along. Comment only. ttl_expire 34/34 on PG 17.10. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WDbfRym2V1sYFmMZ5gnsQL --- src/columnar_vacuum.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/columnar_vacuum.c b/src/columnar_vacuum.c index 03025b2b..a0d737f0 100644 --- a/src/columnar_vacuum.c +++ b/src/columnar_vacuum.c @@ -2237,6 +2237,16 @@ pgcolumnar_expire(PG_FUNCTION_ARGS) * at the shipped stripe_row_limit of 150000, where a group holds fifteen * times these rows. * + * AND THE INSTRUMENT IS WALL CLOCK ON A SHARED, CONTENDED HOST, which is the + * likeliest reason the two sweeps decomposed differently at all: the same 40 + * groups timed 21-37 ms in one and 12-21 ms in the other, taken while a + * second tenant was building and running suites on the same eight cores. + * That difference is larger than the effect either fit was resolving. These + * numbers bound the MAGNITUDE and cannot support a shape. If the shape ever + * matters, measure instructions retired by the backend (perf stat -p on the + * backend pid) rather than elapsed time: contention moves the clock and does + * not move the instruction count. + * * It is kept because of what kind of path this is. relation_estimate_size * runs on every plan of every query, and a per-group fold there was worth * removing. expire is a maintenance function called by name, where a few