On Mon, Sep 14, 2026 at 12:02 PM Melanie Plageman <[email protected]> wrote: > > I've also pushed a fix for this.
Continuing to probe these patches with an LLM found a few more minor oversights. 0001 adds an assert to protect against silently leaking a VM pin when the wrong VM page is passed to heap_page_prune_and_freeze(). This should be caught in development, so I think an assert is sufficient. 0002 downgrades the lock required in recovery to replay a VM-update-only record from cleanup to exclusive. Pruning and freezing always required a cleanup lock in recovery but now that you can end up just setting the VM with the same record type, you should still be able to do that with only an exclusive lock to avoid canceled queries on a standby. 0003 allows a few more cases to set the VM on-access than before. Usually this would be an enhancement. But, I argue it is backpatchable because in 19 we started setting pd_prune_xid on insert so that we could set the page all-visible in the VM during read-only queries, and there are a few cases where we will do all the work to do that and then not set it in the VM because we thought it would require an extra FPI but it turns out it actually will not. So we added a cycle of work for no gain without the patch. The details are described in the patch comment and commit message. 0004 I'm not really considering backpatching but it does seem like strictly an improvement. Without it, if a page has only live tuples but any of them are too new to be all-visible, we'll clear pd_prune_xid which means we won't revisit the page to try to set it all-visible in future queries. This patch instead updates pd_prune_xid with the newest live xid, so that on-access pruning will set it all-visible in the future once the visibility horizon is past that xid. I'll want review on 0002 and 0003 before pushing (and 0001 is assert only), so I'll probably let these slide until after beta 4 code freeze. - Melanie
From 4669d64ba13f8605b9a9ac4c7e59b846c76c7556 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Fri, 18 Sep 2026 15:45:35 -0400 Subject: [PATCH 1/4] Assert correct VM page passed to pruning Before pruning a heap page, we get the current status of the corresponding VM page. If the passed in vmbuffer isn't the right one, visibilitymap_get_status() will silently unpin it and pin the correct page. Pruning assumes the caller manages the vmbuffer lifecycle, so this would leave the caller with a stale VM reference and would leak the new VM pin. To avoid mistakes in development, assert that the correct VM page is pinned before beginning. --- src/backend/access/heap/pruneheap.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c index 50f810c8830..5778ba49a05 100644 --- a/src/backend/access/heap/pruneheap.c +++ b/src/backend/access/heap/pruneheap.c @@ -443,7 +443,13 @@ prune_freeze_setup(PruneFreezeParams *params, prstate->buffer = params->buffer; prstate->page = BufferGetPage(params->buffer); - Assert(BufferIsValid(params->vmbuffer)); + /* + * The caller must have pinned the VM page covering this heap block. If it + * hadn't, visibilitymap_get_status() below would silently release the + * caller's pin and take its own, leaving the caller holding a stale + * buffer and leaking ours. + */ + Assert(visibilitymap_pin_ok(prstate->block, params->vmbuffer)); prstate->vmbuffer = params->vmbuffer; prstate->new_vmbits = 0; prstate->old_vmbits = visibilitymap_get_status(prstate->relation, -- 2.43.0
From 8bfcac2621d3a5417fe02d9c778e02e61ea69804 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Fri, 18 Sep 2026 13:06:01 -0400 Subject: [PATCH 2/4] Avoid cleanup lock in recovery if only setting VM 1252a4ee286 folded visibility map updates into the XLOG_HEAP2_PRUNE_* records emitted by heap_page_prune_and_freeze(), and passed the flag requesting a cleanup lock on replay unconditionally. That is required when the record removes or redirects line pointers, but not for a record that only sets PD_ALL_VISIBLE and the VM bits. The former XLOG_HEAP2_VISIBLE record was replayed under an ordinary exclusive lock. This could be seen as a regression since cleanup locks require all pins to be released and eventually cancels the query holding them. This could lead to more conflicts on the standby. Fix by requesting the cleanup lock only when the record actually prunes or freezes. --- src/backend/access/heap/pruneheap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c index 5778ba49a05..2f23554dd90 100644 --- a/src/backend/access/heap/pruneheap.c +++ b/src/backend/access/heap/pruneheap.c @@ -1337,7 +1337,7 @@ heap_page_prune_and_freeze(PruneFreezeParams *params, do_set_vm ? prstate.vmbuffer : InvalidBuffer, do_set_vm ? prstate.new_vmbits : 0, conflict_xid, - true, /* cleanup lock */ + do_prune || do_freeze, /* cleanup lock */ params->reason, prstate.frozen, prstate.nfrozen, prstate.redirected, prstate.nredirected, -- 2.43.0
From de106b2fa35b8ed665d14dda4b5d168e92844655 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Fri, 18 Sep 2026 15:49:33 -0400 Subject: [PATCH 3/4] Correct on-access VM setting heuristic The heuristic to avoid setting the VM during on-access pruning when doing so would emit an FPI that would otherwise be avoidable missed a few cases. First it missed temp and unlogged tables. Those will, of course, never emit an FPI, so we can always set the VM on-access. It also missed that if hint bits are not WAL-logged, setting only the VM passes REGBUF_NO_IMAGE and doesn't include a heap FPI in the WAL record. The third is more subtle: If the page is all-visible, that means the new prune xid will be InvalidTransactionId. On-access pruning only happens when the current pd_prune_xid is valid and visible. So, that means if on-access pruning finds the page all-visible, it will always be modifying the page to clear pd_prune_xid. Knowing this means we can set the VM without emitting an extra heap FPI in more cases. When hint bits are WAL-logged, if the heap buffer is clean, modifying pd_prune_xid will emit an FPI if one is required. Then, there is no reason to try to avoid one by not setting the VM. When the heap buffer is dirty already, modifying pd_prune_xid can avoid an FPI, so if the page hasn't been logged since the current checkpoint again, we shouldn't set it all-visible because that will end up emitting a heap page FPI. These changes allow us to set the VM on-access in more cases, which is arguably an enhancement. However, it is being backpatched because we set pd_prune_xid on insert now, and if we go through the whole prune cycle and could set the VM cheaply and don't because of an incorrect heuristic, we are wasting that work. Add some tests covering these cases and update one of the temp table tests now that we may update temp tables VM and FSM on-access and end up using more pins for the same queries (running into the temp local buffers limit). --- .../pg_visibility/expected/pg_visibility.out | 76 +++++++++++++++++++ contrib/pg_visibility/sql/pg_visibility.sql | 34 +++++++++ src/backend/access/heap/pruneheap.c | 37 +++++++-- src/test/regress/expected/temp.out | 2 +- src/test/regress/sql/temp.sql | 2 +- 5 files changed, 142 insertions(+), 9 deletions(-) diff --git a/contrib/pg_visibility/expected/pg_visibility.out b/contrib/pg_visibility/expected/pg_visibility.out index d26f0ab7589..a7aa8487cc3 100644 --- a/contrib/pg_visibility/expected/pg_visibility.out +++ b/contrib/pg_visibility/expected/pg_visibility.out @@ -248,6 +248,82 @@ select pg_visibility_map_summary('test_vac_unmodified_heap'); (1,1) (1 row) +-- Test that on-access pruning during a read-only scan sets the VM. Temp tables +-- are used because their visibility horizon depends only on this backend and no +-- other process can pin their buffers, so the conditional cleanup lock needed for +-- pruning is always available. +create temp table test_on_access_vm(a int, b text) with (fillfactor = 90); +insert into test_on_access_vm select g, repeat('x', 99) + from generate_series(1, 500) g; +-- HOT-update a few rows on every page. The new versions fit in the space +-- reserved by the fillfactor, and afterwards each page has too little free +-- space to escape on-access pruning. +update test_on_access_vm set b = b where a % 20 = 0; +select pg_visibility_map_summary('test_on_access_vm'); + pg_visibility_map_summary +--------------------------- + (0,0) +(1 row) + +-- A read-only scan that prunes tuples sets the VM +select count(*) from test_on_access_vm; + count +------- + 500 +(1 row) + +select pg_visibility_map_summary('test_on_access_vm'); + pg_visibility_map_summary +--------------------------- + (9,0) +(1 row) + +select * from pg_check_visible('test_on_access_vm'); + t_ctid +-------- +(0 rows) + +-- A read-only scan of newly inserted data sets the VM +create temp table test_on_access_vm_insert_only(a int, b text); +insert into test_on_access_vm_insert_only select g, repeat('x', 99) + from generate_series(1, 500) g; +select pg_visibility_map_summary('test_on_access_vm_insert_only'); + pg_visibility_map_summary +--------------------------- + (0,0) +(1 row) + +select count(*) from test_on_access_vm_insert_only; + count +------- + 500 +(1 row) + +select pg_visibility_map_summary('test_on_access_vm_insert_only'); + pg_visibility_map_summary +--------------------------- + (8,0) +(1 row) + +select * from pg_check_visible('test_on_access_vm_insert_only'); + t_ctid +-------- +(0 rows) + +create temp table test_on_access_vm_modify(a int, b text) with (fillfactor = 90); +insert into test_on_access_vm_modify select g, repeat('x', 99) + from generate_series(1, 500) g; +-- Create some dead rows for the next update's on-access pruning to clean up +update test_on_access_vm_modify set b = b where a % 20 = 0; +-- A scan by a query that modifies the relation prunes but does not set the VM. +-- This matches no rows, but scans every page as the query's result relation. +update test_on_access_vm_modify set b = b where a = -1; +select pg_visibility_map_summary('test_on_access_vm_modify'); + pg_visibility_map_summary +--------------------------- + (0,0) +(1 row) + -- test copy freeze create table copyfreeze (a int, b char(1500)); -- load all rows via COPY FREEZE and ensure that all pages are set all-visible diff --git a/contrib/pg_visibility/sql/pg_visibility.sql b/contrib/pg_visibility/sql/pg_visibility.sql index 0888adb96a6..f292679bd58 100644 --- a/contrib/pg_visibility/sql/pg_visibility.sql +++ b/contrib/pg_visibility/sql/pg_visibility.sql @@ -114,6 +114,40 @@ SELECT (flags & x'0004'::int) <> 0 vacuum test_vac_unmodified_heap; select pg_visibility_map_summary('test_vac_unmodified_heap'); +-- Test that on-access pruning during a read-only scan sets the VM. Temp tables +-- are used because their visibility horizon depends only on this backend and no +-- other process can pin their buffers, so the conditional cleanup lock needed for +-- pruning is always available. +create temp table test_on_access_vm(a int, b text) with (fillfactor = 90); +insert into test_on_access_vm select g, repeat('x', 99) + from generate_series(1, 500) g; +-- HOT-update a few rows on every page. The new versions fit in the space +-- reserved by the fillfactor, and afterwards each page has too little free +-- space to escape on-access pruning. +update test_on_access_vm set b = b where a % 20 = 0; +select pg_visibility_map_summary('test_on_access_vm'); +-- A read-only scan that prunes tuples sets the VM +select count(*) from test_on_access_vm; +select pg_visibility_map_summary('test_on_access_vm'); +select * from pg_check_visible('test_on_access_vm'); +-- A read-only scan of newly inserted data sets the VM +create temp table test_on_access_vm_insert_only(a int, b text); +insert into test_on_access_vm_insert_only select g, repeat('x', 99) + from generate_series(1, 500) g; +select pg_visibility_map_summary('test_on_access_vm_insert_only'); +select count(*) from test_on_access_vm_insert_only; +select pg_visibility_map_summary('test_on_access_vm_insert_only'); +select * from pg_check_visible('test_on_access_vm_insert_only'); +create temp table test_on_access_vm_modify(a int, b text) with (fillfactor = 90); +insert into test_on_access_vm_modify select g, repeat('x', 99) + from generate_series(1, 500) g; +-- Create some dead rows for the next update's on-access pruning to clean up +update test_on_access_vm_modify set b = b where a % 20 = 0; +-- A scan by a query that modifies the relation prunes but does not set the VM. +-- This matches no rows, but scans every page as the query's result relation. +update test_on_access_vm_modify set b = b where a = -1; +select pg_visibility_map_summary('test_on_access_vm_modify'); + -- test copy freeze create table copyfreeze (a int, b char(1500)); diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c index 2f23554dd90..07ef563b874 100644 --- a/src/backend/access/heap/pruneheap.c +++ b/src/backend/access/heap/pruneheap.c @@ -991,16 +991,39 @@ heap_page_will_set_vm(PruneState *prstate, PruneReason reason, return false; /* - * If this is an on-access call and we're not actually pruning, avoid - * setting the visibility map if it would newly dirty the heap page or, if - * the page is already dirty, if doing so would require including a - * full-page image (FPI) of the heap page in the WAL. + * If this is an on-access call and we're not actually pruning or + * freezing, consider whether setting the VM would cost us an additional + * heap page FPI. If the relation isn't WAL-logged, or if hint bits are + * not WAL-logged, setting the VM won't include a heap page FPI (the + * latter passes REGBUF_NO_IMAGE for the heap page), apart from a page + * that has never been WAL-logged, which we don't bother about here. */ if (reason == PRUNE_ON_ACCESS && !do_prune && !do_freeze && - (!BufferIsDirty(prstate->buffer) || XLogCheckBufferNeedsBackup(prstate->buffer))) + RelationNeedsWAL(prstate->relation) && XLogHintBitIsNeeded()) { - prstate->set_all_visible = prstate->set_all_frozen = false; - return false; + /* + * Because the page is known to be all-visible, we will clear + * pd_prune_xid regardless of whether we actually set the page + * all-visible in the VM. That clear is a hint update which is not + * WAL-logged, other than an FPI for torn-page protection, so in some + * cases we want to avoid setting the VM if doing so would cost us a + * heap page FPI that clearing pd_prune_xid wouldn't have. + * + * Since hint bits are WAL-logged, if the buffer is clean, clearing + * pd_prune_xid will already emit a heap page FPI if one is needed, so + * there's no reason to avoid setting the VM. + * + * However, if the heap buffer is already dirty, clearing pd_prune_xid + * will never emit an FPI. So avoid setting the VM if the page hasn't + * been WAL-logged since the current checkpoint began, as the record + * setting the VM would then include a heap page FPI. + */ + if (BufferIsDirty(prstate->buffer) && + XLogCheckBufferNeedsBackup(prstate->buffer)) + { + prstate->set_all_visible = prstate->set_all_frozen = false; + return false; + } } prstate->new_vmbits = VISIBILITYMAP_ALL_VISIBLE; diff --git a/src/test/regress/expected/temp.out b/src/test/regress/expected/temp.out index a50c7ae88a9..ae96d4a0272 100644 --- a/src/test/regress/expected/temp.out +++ b/src/test/regress/expected/temp.out @@ -485,7 +485,7 @@ FETCH NEXT FROM c_3; (1 row) -- new cursors with pins can be created after subtrans rollback -SELECT test_temp_pin(10, 94); +SELECT test_temp_pin(10, 93); test_temp_pin --------------- diff --git a/src/test/regress/sql/temp.sql b/src/test/regress/sql/temp.sql index d50472ddced..708187c6e86 100644 --- a/src/test/regress/sql/temp.sql +++ b/src/test/regress/sql/temp.sql @@ -369,7 +369,7 @@ ROLLBACK TO SAVEPOINT rescue_me; FETCH NEXT FROM c_3; -- new cursors with pins can be created after subtrans rollback -SELECT test_temp_pin(10, 94); +SELECT test_temp_pin(10, 93); -- Check that read streams deal with lower number of pins available SELECT count(*), max(a) max_a, min(a) min_a, max(cnt) max_cnt FROM test_temp; -- 2.43.0
From 54dbd9cbe68cbe7f9aa2a7c4d8f262b15410d3e6 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Fri, 18 Sep 2026 15:45:03 -0400 Subject: [PATCH 4/4] Retain newest live xid as prune hint after visibility horizon rejection When a page contains committed live tuples that are not yet visible to all snapshots, pruning cannot mark it all-visible. These tuples do not contribute to new_prune_xid, so pruning can clear pd_prune_xid and prevent subsequent on-access scans from reconsidering the page after the horizon advances. Vacuum scans the page regardless of pd_prune_xid, so the only change to its behavior is that it may dirty the page to set pd_prune_xid when it otherwise wouldn't have. Record the newest live xmin as a retry hint when the visibility horizon prevents setting the VM. Preserve any earlier pruning opportunity already recorded so that dead tuples can still be reclaimed sooner. --- src/backend/access/heap/pruneheap.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c index 07ef563b874..4c8ddc4f743 100644 --- a/src/backend/access/heap/pruneheap.c +++ b/src/backend/access/heap/pruneheap.c @@ -1209,8 +1209,20 @@ heap_page_prune_and_freeze(PruneFreezeParams *params, GlobalVisTestXidConsideredRunning(prstate.vistest, prstate.newest_live_xid, true)) + { prstate.set_all_visible = prstate.set_all_frozen = false; + /* + * Preserve an opportunity to set the VM on-access once the newest + * live xmin is visible to everyone. Retain any earlier pruning + * opportunity already recorded, so that we can still reclaim dead + * tuples sooner. + */ + if (!TransactionIdIsValid(prstate.new_prune_xid) || + TransactionIdPrecedes(prstate.newest_live_xid, prstate.new_prune_xid)) + prstate.new_prune_xid = prstate.newest_live_xid; + } + /* * If checksums are enabled, calling heap_prune_satisfies_vacuum() while * checking tuple visibility information in prune_freeze_plan() may have -- 2.43.0
