From ade86fa8692f4ffc92e05bd419c45e9fdb2fb4b1 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Wed, 2 Sep 2026 11:36:39 -0600 Subject: [PATCH 1/2] test: the visibility map is cleared wherever LIVE rows are renumbered (#877) Three paths retire a row group and give its live rows new row numbers, and each must clear the all-visible bits over the OLD numbers, or an index-only scan answers from the index for a TID whose group no longer exists. Only expire's clear was held by a test, because only its absence had been reported as data loss. Deleting the other two left 319 checks across 14 suites green: src/columnar_vacuum.c:346 rewrite_one_group, via compact_rewrite src/columnar_vacuum.c:765 recluster_online, via recluster Removal proof, one .so per arm and the same suite file in all three trees: unmutated 17 passed + 0 failed :346 neutered 16 passed + 1 failed only rewrite_one_group's arm :765 neutered 15 passed + 2 failed only recluster_online's arms Neither mutation reddens the other's arms, and every arm is paired with a control -- an untouched group for the rewrite, an un-reclustered relation for recluster -- so a clear that wiped the whole fork fails too. THE INSTRUMENT IS WHY THIS WAS NOT COVERED EARLIER. pg_class.relallvisible is a statistic VACUUM refreshes; clearing a bit does not touch it, so an arm reading it reports the same number with the clear and without. An earlier attempt at these arms was correctly abandoned for that reason. pgcolumnar.vm_is_visible() reads the fork through visibilitymap_get_status, the call the index-only-scan executor makes, and it was already in the tree. AND A DELETE CLEARS THE BLOCKS HOLDING THE DELETED ROWS, so a group made compactable is already not-all-visible there and an arm placed on it cannot fail. The first fixture I built had exactly that shape. These arms VACUUM first, delete only the FRONT of the target group, and assert over the group's LATER blocks: all-visible on the way in, reachable only by the rewrite's clear. Two premises are gated rather than printed: block_size must be 8192, because the block arithmetic is MaxHeapTuplesPerPage; and the chosen blocks must lie wholly inside the relation's row range, because the first and last blocks are partially covered -- block 0 spans row 0, which no relation has -- and read f on a correct tree, where an arm could not fail. That gate caught a bug in my own fixture. This does not fix anything. The status of the two clears is uncovered, not defective; whether a defect exists behind them is still unknown. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017V7PhZ1TzoVVNsACFXTbdT --- CHANGELOG.md | 30 ++++++ test/run_all_versions.sh | 1 + test/vm_clear_on_renumber.sh | 189 +++++++++++++++++++++++++++++++++++ 3 files changed, 220 insertions(+) create mode 100755 test/vm_clear_on_renumber.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c1095d2..6425ea7e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,36 @@ true until the next version shipped. ### Added +- The two visibility-map clears that no test held are now held (#877). + + **Three paths retire a row group and give its live rows new row numbers**, and + each must clear the all-visible bits over the OLD numbers, or an index-only + scan answers from the index for a TID whose group no longer exists. Only + `pgcolumnar.expire`'s clear was covered, because only its absence had been + reported as data loss. Deleting the other two -- `rewrite_one_group` + (`src/columnar_vacuum.c:346`, reached through `pgcolumnar.compact_rewrite`) and + `pgcolumnar_recluster_online` (`:765`, through `pgcolumnar.recluster`) -- left + 319 checks across 14 suites green. + + `test/vm_clear_on_renumber.sh` is the two missing arms. Neuter the clear at + `:346` and one arm reddens; neuter the one at `:765` and two redden; neither + mutation reddens the other's arms. + + **The instrument is why this was not covered earlier.** + `pg_class.relallvisible` is a statistic `VACUUM` refreshes, and clearing a bit + does not touch it, so an arm reading it reports the same number with the clear + and without. `pgcolumnar.vm_is_visible(rel, blk)` reads the fork through + `visibilitymap_get_status`, which is the call the index-only-scan executor + makes. + + **And a `DELETE` clears the bits of the blocks holding the deleted rows**, so a + group made compactable is already not-all-visible there and an arm placed on it + cannot fail. These arms `VACUUM` first, delete only the front of the target + group, and assert over the group's later blocks, which are all-visible on the + way in and reachable only by the rewrite. Each is paired with a control -- an + untouched group for the rewrite, an un-reclustered relation for recluster -- so + a clear that wiped the whole fork fails too. + - `MERGE` is documented as working, which it has been all along. It needs no index on the columnar target and takes every arm, including `WHEN MATCHED ... DELETE`, `WHEN NOT MATCHED BY SOURCE`, and diff --git a/test/run_all_versions.sh b/test/run_all_versions.sh index 6682bb93..39ecc7df 100755 --- a/test/run_all_versions.sh +++ b/test/run_all_versions.sh @@ -276,6 +276,7 @@ SUITES=( vacuum_stripe_count vector_agg_rescan_memory vector_agg_tlist_shape + vm_clear_on_renumber vm_privilege wal_envelope write_fsst_compressed diff --git a/test/vm_clear_on_renumber.sh b/test/vm_clear_on_renumber.sh new file mode 100755 index 00000000..4464e323 --- /dev/null +++ b/test/vm_clear_on_renumber.sh @@ -0,0 +1,189 @@ +#!/usr/bin/env bash +# +# pgColumnar: the visibility map is cleared wherever LIVE rows are RENUMBERED. +# +# Three paths retire a row group and give its live rows new row numbers, and each +# must clear the all-visible bits over the OLD numbers. Otherwise an index-only +# scan answers from the index for a TID whose group no longer exists: +# +# src/columnar_vacuum.c:2310 pgcolumnar_expire -- held by ttl_expire.sh +# src/columnar_vacuum.c:346 rewrite_one_group -- held by NOTHING before this file +# src/columnar_vacuum.c:765 pgcolumnar_recluster_online -- held by NOTHING before this file +# +# Only expire's clear was covered, because only expire's was reported as data +# loss. Deleting the other two left 319 checks across 14 suites green. This file +# is the two missing arms, and it exists as its own suite because the rule is one +# rule -- the comment at :765 states it as "wherever LIVE rows are renumbered +# rather than only where they expire" -- while the operations that break it live +# in different suites. +# +# THE INSTRUMENT, and why the obvious one does not work. `pg_class.relallvisible` +# is a statistic VACUUM refreshes; clearing a VM bit does not touch it, so an arm +# reading it reports the same number on a tree with the clear and a tree without. +# `pg_visibility` is not built under these prefixes. What does work is already in +# the extension: `pgcolumnar.vm_is_visible(rel, blk)` reads the fork through +# visibilitymap_get_status -- the same call the index-only-scan executor makes. +# +# THE FIXTURE'S ONE SUBTLETY. A DELETE clears the bits of the blocks holding the +# deleted rows, so a group that has been made compactable is ALREADY not +# all-visible over those blocks and an arm there cannot fail. The arms below +# therefore VACUUM first, delete only the FRONT of the target group, and assert +# over the group's LATER blocks -- all-visible on the way in, and reachable only +# by the rewrite's clear. Every arm is paired with a control block in a group the +# operation does not touch, so a clear that wiped the whole fork fails too. +# +# Usage: test/vm_clear_on_renumber.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}" + +# premise: the block arithmetic below is written for MaxHeapTuplesPerPage at an +# 8 kB block size. A synthetic block covers COLUMNAR_VALID_ITEMPOINTER_OFFSETS +# row numbers (src/columnar.h), which is MaxHeapTuplesPerPage, 291 at BLCKSZ +# 8192. At another block size every block number below names a different row +# range and the arms would measure nothing. +PGC_BLOCK_SIZE="$(q "SELECT current_setting('block_size');")" +if [ "$PGC_BLOCK_SIZE" != "8192" ]; then + echo "PREMISE FAILED: block_size is [$PGC_BLOCK_SIZE]; these arms are written for 8192" >&2 + exit 1 +fi +SPAN=291 + +# Block holding a given row number, the same division the extension does. +blk_of() { echo $(( $1 / SPAN )); } + +# t/f for one synthetic block. +vis() { q "SELECT pgcolumnar.vm_is_visible('$1', $2);"; } + +# --------------------------------------------------------------------------- +# Part 1: rewrite_one_group, reached through pgcolumnar.compact_rewrite +# --------------------------------------------------------------------------- + +psql_run "CREATE TABLE cw (id int, v int) USING pgcolumnar;" +psql_run "SELECT pgcolumnar.set_options('cw', stripe_row_limit => 2048, chunk_group_row_limit => 1024);" +psql_run "INSERT INTO cw SELECT g, g FROM generate_series(1,20480) g;" +psql_run "CREATE INDEX cw_id ON cw (id);" +psql_run "VACUUM cw;" + +# Take the geometry from the catalog rather than from the INSERT, so a change to +# the group size is a premise failure here instead of an arm that silently moves +# to a different part of the table. +CW_G3_FIRST="$(q "SELECT first_row_number FROM pgcolumnar.row_group + WHERE storage_id = pgcolumnar.get_storage_id('cw') AND group_number = 3;")" +CW_G3_COUNT="$(q "SELECT row_count FROM pgcolumnar.row_group + WHERE storage_id = pgcolumnar.get_storage_id('cw') AND group_number = 3;")" +CW_G5_FIRST="$(q "SELECT first_row_number FROM pgcolumnar.row_group + WHERE storage_id = pgcolumnar.get_storage_id('cw') AND group_number = 5;")" +case "$CW_G3_FIRST$CW_G3_COUNT$CW_G5_FIRST" in + *[!0-9]*|"") echo "PREMISE FAILED: group 3/5 geometry unreadable [$CW_G3_FIRST/$CW_G3_COUNT/$CW_G5_FIRST]" >&2; exit 1 ;; +esac + +CW_LAST=$(( CW_G3_FIRST + CW_G3_COUNT - 1 )) +CW_FRONT_END=$(( CW_G3_FIRST + 600 )) # deleted; ~29% of a 2048 group +CW_TARGET_BLK=$(blk_of $(( CW_LAST - 100 ))) # late in group 3, no deleted row +CW_FRONT_BLK=$(blk_of "$CW_G3_FIRST") # early in group 3, deleted rows +CW_CTRL_BLK=$(blk_of $(( CW_G5_FIRST + 100 ))) # group 5, untouched throughout + +# The arm is only meaningful if the target block holds no deleted row, otherwise +# the DELETE below clears it and the rewrite is not what the arm measures. +if [ "$CW_TARGET_BLK" -le "$(blk_of "$CW_FRONT_END")" ]; then + echo "PREMISE FAILED: target block $CW_TARGET_BLK is inside the deleted range" >&2 + exit 1 +fi + +check "premise: VACUUM set all-visible on group 3's late block" \ + "$(vis cw "$CW_TARGET_BLK")" "t" +check "premise: and on the untouched control group's block" \ + "$(vis cw "$CW_CTRL_BLK")" "t" + +psql_run "DELETE FROM cw WHERE id BETWEEN $CW_G3_FIRST AND $CW_FRONT_END;" + +# These two are what make the arm attributable. The DELETE clears the blocks it +# touches; if it also cleared the target block there would be nothing left for +# the rewrite to do and the arm could not fail. +check "the delete cleared the block holding the deleted rows" \ + "$(vis cw "$CW_FRONT_BLK")" "f" +check "premise: but left group 3's late block all-visible" \ + "$(vis cw "$CW_TARGET_BLK")" "t" +check "premise: and left the control block all-visible" \ + "$(vis cw "$CW_CTRL_BLK")" "t" + +# Measure the work, not the intent: the call must report that it rewrote exactly +# the one qualifying group. A zero here would leave every arm below green for a +# rewrite that never ran. +CW_REWROTE="$(q "SELECT pgcolumnar.compact_rewrite('cw', 0.2);")" +check "compact_rewrite rewrote exactly the one qualifying group" "$CW_REWROTE" "1" +check "premise: group 3 is gone from the catalog, so its rows were renumbered" \ + "$(q "SELECT count(*) FROM pgcolumnar.row_group + WHERE storage_id = pgcolumnar.get_storage_id('cw') AND group_number = 3;")" "0" + +# THE ARM. Revert src/columnar_vacuum.c:346 and this goes green-to-red: the block +# stays all-visible over row numbers whose group no longer exists. +check "rewrite_one_group cleared the retired group's late block (#877)" \ + "$(vis cw "$CW_TARGET_BLK")" "f" + +# THE CONTROL. A clear that wiped the whole fork would satisfy the arm above. +check "control: a group the rewrite did not touch keeps its all-visible bit" \ + "$(vis cw "$CW_CTRL_BLK")" "t" + +# --------------------------------------------------------------------------- +# Part 2: pgcolumnar_recluster_online, reached through pgcolumnar.recluster +# --------------------------------------------------------------------------- +# +# Recluster renumbers every live row and takes no group limit, so there is no +# untouched group to use as an in-table control. A second columnar table, +# vacuumed the same way and never reclustered, carries that half: it fails a +# clear that reached beyond the relation it was called on. + +for t in rc rcctl; do + psql_run "CREATE TABLE $t (id int, x int, y int) USING pgcolumnar;" + psql_run "SELECT pgcolumnar.set_options('$t', stripe_row_limit => 2048, chunk_group_row_limit => 1024);" + psql_run "INSERT INTO $t SELECT g, ((g::bigint*7919)%200)::int, ((g::bigint*104729)%200)::int + FROM generate_series(1,20480) g;" + psql_run "VACUUM $t;" +done + +RC_FIRST="$(q "SELECT min(first_row_number) FROM pgcolumnar.row_group + WHERE storage_id = pgcolumnar.get_storage_id('rc');")" +RC_LAST="$(q "SELECT max(first_row_number + row_count - 1) FROM pgcolumnar.row_group + WHERE storage_id = pgcolumnar.get_storage_id('rc');")" +case "$RC_FIRST$RC_LAST" in *[!0-9]*|"") echo "PREMISE FAILED: rc geometry unreadable" >&2; exit 1 ;; esac + +# A synthetic block is all-visible only when every row number it spans exists. +# The first and last blocks of a relation are PARTIALLY covered -- block 0 spans +# row 0, which no relation has, and the top block runs past the last row -- so +# both read `f` on a correct tree and an arm placed there cannot fail. Step three +# whole blocks in from each end, and gate that the result is genuinely interior. +RC_EARLY_BLK=$(( $(blk_of "$RC_FIRST") + 3 )) +RC_LATE_BLK=$(( $(blk_of "$RC_LAST") - 3 )) +if [ "$RC_EARLY_BLK" -ge "$RC_LATE_BLK" ] || \ + [ $(( RC_EARLY_BLK * SPAN )) -lt "$RC_FIRST" ] || \ + [ $(( (RC_LATE_BLK + 1) * SPAN - 1 )) -gt "$RC_LAST" ]; then + echo "PREMISE FAILED: blocks $RC_EARLY_BLK/$RC_LATE_BLK are not wholly inside rows $RC_FIRST..$RC_LAST" >&2 + exit 1 +fi + +check "premise: VACUUM set all-visible early in rc" "$(vis rc "$RC_EARLY_BLK")" "t" +check "premise: and late in rc" "$(vis rc "$RC_LATE_BLK")" "t" +check "premise: and on the control relation" "$(vis rcctl "$RC_EARLY_BLK")" "t" + +RC_GROUPS="$(q "SELECT pgcolumnar.recluster('rc','x','y');")" +check "recluster reported reclustering groups" \ + "$([ "${RC_GROUPS:-0}" -gt 0 ] 2>/dev/null && echo yes || echo no)" "yes" + +# THE ARMS. Revert src/columnar_vacuum.c:765 and both go red: recluster gives +# every live row a new number, so every old block must lose its bit. +check "recluster_online cleared the old numbers' early block (#877)" \ + "$(vis rc "$RC_EARLY_BLK")" "f" +check "recluster_online cleared the old numbers' late block (#877)" \ + "$(vis rc "$RC_LATE_BLK")" "f" + +# THE CONTROL, the half that fails an over-broad clear. +check "control: a relation that was not reclustered keeps its all-visible bits" \ + "$(vis rcctl "$RC_EARLY_BLK")" "t" +check "control: and its late block too" \ + "$(vis rcctl "$RC_LATE_BLK")" "t" + +pgc_summary From 355dbf437e7d7a7dcf599af360116a6f76896c81 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Wed, 2 Sep 2026 11:47:18 -0600 Subject: [PATCH 2/2] test: say what each control actually catches, and what it does not (#877) The reviewer widened each clear to (rel, 1, 100000000) and measured: :346 over-broad 16 passed + 1 failed the control reddens :765 over-broad 17 passed + 0 failed the control does NOT So the header's "a clear that wiped the whole fork fails too" was true of Part 1 and false of Part 2, stated over both. PgColumnarVMClearForRowRange takes `rel`, so no widening of the RANGE can make a clear on rc touch rcctl; that control catches a clear escaping its RELATION, which is a different and narrower thing. Recluster renumbers every group, so no in-table control exists for that half. That is a real limit and the file now states it, with the measurement, instead of carrying a sentence borrowed from the half where it holds. Nothing about the arms changed. This is the same overclaiming shape as the "All three clear now" line this suite exists to retire, which is why it is worth its own commit rather than a quiet reword. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017V7PhZ1TzoVVNsACFXTbdT --- CHANGELOG.md | 12 +++++++++--- test/vm_clear_on_renumber.sh | 32 +++++++++++++++++++++++++------- 2 files changed, 34 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6425ea7e..d1742a1c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,9 +44,15 @@ true until the next version shipped. group made compactable is already not-all-visible there and an arm placed on it cannot fail. These arms `VACUUM` first, delete only the front of the target group, and assert over the group's later blocks, which are all-visible on the - way in and reachable only by the rewrite. Each is paired with a control -- an - untouched group for the rewrite, an un-reclustered relation for recluster -- so - a clear that wiped the whole fork fails too. + way in and reachable only by the rewrite. + + **The two controls differ in strength and the suite says so.** The rewrite's + control is a group in the same table, and widening that clear to the whole + relation reddens it. Recluster renumbers every group, so that half has no + in-table control; its control is a second relation, which catches a clear + reaching beyond the relation it was called on but NOT an over-broad range -- + the same widening leaves it green. That limit is measured and recorded in the + file rather than left for a reader to infer. - `MERGE` is documented as working, which it has been all along. It needs no index on the columnar target and takes every arm, including diff --git a/test/vm_clear_on_renumber.sh b/test/vm_clear_on_renumber.sh index 4464e323..3389d480 100755 --- a/test/vm_clear_on_renumber.sh +++ b/test/vm_clear_on_renumber.sh @@ -29,8 +29,18 @@ # all-visible over those blocks and an arm there cannot fail. The arms below # therefore VACUUM first, delete only the FRONT of the target group, and assert # over the group's LATER blocks -- all-visible on the way in, and reachable only -# by the rewrite's clear. Every arm is paired with a control block in a group the -# operation does not touch, so a clear that wiped the whole fork fails too. +# by the rewrite's clear. +# +# THE TWO CONTROLS DIFFER IN STRENGTH, and the difference is measured rather than +# assumed. Part 1's control is a group in the SAME table the rewrite does not +# touch, so widening that clear to the whole relation reddens it -- measured, 16 +# passed + 1 failed with the range replaced by (rel, 1, 100000000). Part 2 has NO +# in-table control available, because recluster renumbers every group and leaves +# nothing untouched to compare against. Its control is a second relation, which +# catches a clear reaching beyond the relation it was called on, NOT an +# over-broad range: the same widening at :765 leaves the suite 17 passed + 0 +# failed. That is a real limit of this half, stated here rather than left for a +# reader to infer from an arm that looks like Part 1's. # # Usage: test/vm_clear_on_renumber.sh [PG_CONFIG] # Written fresh for pgColumnar. @@ -124,7 +134,8 @@ check "premise: group 3 is gone from the catalog, so its rows were renumbered" \ check "rewrite_one_group cleared the retired group's late block (#877)" \ "$(vis cw "$CW_TARGET_BLK")" "f" -# THE CONTROL. A clear that wiped the whole fork would satisfy the arm above. +# THE CONTROL, and it is load-bearing: replace this site's range with the whole +# relation and this arm goes red while the arm above stays green. check "control: a group the rewrite did not touch keeps its all-visible bit" \ "$(vis cw "$CW_CTRL_BLK")" "t" @@ -133,9 +144,15 @@ check "control: a group the rewrite did not touch keeps its all-visible bit" \ # --------------------------------------------------------------------------- # # Recluster renumbers every live row and takes no group limit, so there is no -# untouched group to use as an in-table control. A second columnar table, -# vacuumed the same way and never reclustered, carries that half: it fails a -# clear that reached beyond the relation it was called on. +# untouched group to use as an in-table control, and this half has none. A second +# columnar table, vacuumed the same way and never reclustered, carries what is +# left: it fails a clear that reached beyond the RELATION it was called on. +# +# It does NOT fail an over-broad RANGE, and the arms below must not be read as if +# it did. PgColumnarVMClearForRowRange takes `rel`, so widening the range cannot +# make a clear on rc touch rcctl -- measured: with this site's range replaced by +# (rel, 1, 100000000) the suite is 17 passed + 0 failed. A helper rewritten to +# clear globally WOULD be caught here; a wrong row range would not. for t in rc rcctl; do psql_run "CREATE TABLE $t (id int, x int, y int) USING pgcolumnar;" @@ -180,7 +197,8 @@ check "recluster_online cleared the old numbers' early block (#877)" \ check "recluster_online cleared the old numbers' late block (#877)" \ "$(vis rc "$RC_LATE_BLK")" "f" -# THE CONTROL, the half that fails an over-broad clear. +# THE CONTROL for this half: a clear that escaped its relation. See the note +# above for what it does not cover. check "control: a relation that was not reclustered keeps its all-visible bits" \ "$(vis rcctl "$RC_EARLY_BLK")" "t" check "control: and its late block too" \