From 8e1286c1a6dbfe3309d111aaa21af5a8e6237bb8 Mon Sep 17 00:00:00 2001 From: Melanie Plageman Date: Mon, 8 Dec 2025 15:49:54 -0500 Subject: [PATCH v33 01/16] Combine visibilitymap_set() cases in lazy_scan_prune() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit lazy_scan_prune() previously had two separate cases that called visibilitymap_set() after pruning and freezing. These branches were nearly identical except that one attempted to avoid dirtying the heap buffer. However, that situation can never occur — the heap buffer cannot be clean at that point (and we would hit an assertion if it were). In lazy_scan_prune(), when we change a previously all-visible page to all-frozen and the page was recorded as all-visible in the visibility map by find_next_unskippable_block(), the heap buffer will always be dirty. Either we have just frozen a tuple and already dirtied the buffer, or the buffer was modified between find_next_unskippable_block() and heap_page_prune_and_freeze() and then pruned in heap_page_prune_and_freeze(). Additionally, XLogRegisterBuffer() asserts that the buffer is dirty, so attempting to add a clean heap buffer to the WAL chain would assert out anyway. Since the “clean heap buffer with already set VM” case is impossible, the two visibilitymap_set() branches in lazy_scan_prune() can be merged. Doing so makes the intent clearer and emphasizes that the heap buffer must always be marked dirty before being added to the WAL chain. This commit also adds a test case for vacuuming when no heap modifications are required. Currently this ensures that the heap buffer is marked dirty before it is added to the WAL chain, but if we later remove the heap buffer from the VM-set WAL chain or pass it with the REGBUF_NO_CHANGES flag, this test would guard that behavior. Author: Melanie Plageman Reviewed-by: Chao Li Reviewed-by: Srinath Reddy Sadipiralla Reviewed-by: Kirill Reshke Reviewed-by: Xuneng Zhou Discussion: https://postgr.es/m/5CEAA162-67B1-44DA-B60D-8B65717E8B05%40gmail.com Discussion: https://postgr.es/m/flat/CAAKRu_ZWx5gCbeCf7PWCv8p5%3D%3Db7EEws0VD2wksDxpXCvCyHvQ%40mail.gmail.com --- .../pg_visibility/expected/pg_visibility.out | 44 ++++++++++ contrib/pg_visibility/sql/pg_visibility.sql | 20 +++++ src/backend/access/heap/vacuumlazy.c | 87 ++++--------------- 3 files changed, 82 insertions(+), 69 deletions(-) diff --git a/contrib/pg_visibility/expected/pg_visibility.out b/contrib/pg_visibility/expected/pg_visibility.out index 09fa5933a35..e10f1706015 100644 --- a/contrib/pg_visibility/expected/pg_visibility.out +++ b/contrib/pg_visibility/expected/pg_visibility.out @@ -1,4 +1,5 @@ CREATE EXTENSION pg_visibility; +CREATE EXTENSION pageinspect; -- -- recently-dropped table -- @@ -204,6 +205,49 @@ select pg_truncate_visibility_map('test_partition'); (1 row) +-- test the case where vacuum phase I does not need to modify the heap buffer +-- and only needs to set the VM +create table test_vac_unmodified_heap(a int); +insert into test_vac_unmodified_heap values (1); +vacuum (freeze) test_vac_unmodified_heap; +select pg_visibility_map_summary('test_vac_unmodified_heap'); + pg_visibility_map_summary +--------------------------- + (1,1) +(1 row) + +-- the checkpoint cleans the buffer dirtied by freezing the sole tuple +checkpoint; +-- truncating the VM ensures that the next vacuum will need to set it +select pg_truncate_visibility_map('test_vac_unmodified_heap'); + pg_truncate_visibility_map +---------------------------- + +(1 row) + +select pg_visibility_map_summary('test_vac_unmodified_heap'); + pg_visibility_map_summary +--------------------------- + (0,0) +(1 row) + +-- though the VM is truncated, the heap page-level visibility hint, +-- PD_ALL_VISIBLE should still be set +SELECT (flags & x'0004'::int) <> 0 + FROM page_header(get_raw_page('test_vac_unmodified_heap', 0)); + ?column? +---------- + t +(1 row) + +-- vacuum sets the VM +vacuum test_vac_unmodified_heap; +select pg_visibility_map_summary('test_vac_unmodified_heap'); + pg_visibility_map_summary +--------------------------- + (1,1) +(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 5af06ec5b76..57af8a0c5b6 100644 --- a/contrib/pg_visibility/sql/pg_visibility.sql +++ b/contrib/pg_visibility/sql/pg_visibility.sql @@ -1,4 +1,5 @@ CREATE EXTENSION pg_visibility; +CREATE EXTENSION pageinspect; -- -- recently-dropped table @@ -94,6 +95,25 @@ select count(*) > 0 from pg_visibility_map_summary('test_partition'); select * from pg_check_frozen('test_partition'); -- hopefully none select pg_truncate_visibility_map('test_partition'); +-- test the case where vacuum phase I does not need to modify the heap buffer +-- and only needs to set the VM +create table test_vac_unmodified_heap(a int); +insert into test_vac_unmodified_heap values (1); +vacuum (freeze) test_vac_unmodified_heap; +select pg_visibility_map_summary('test_vac_unmodified_heap'); +-- the checkpoint cleans the buffer dirtied by freezing the sole tuple +checkpoint; +-- truncating the VM ensures that the next vacuum will need to set it +select pg_truncate_visibility_map('test_vac_unmodified_heap'); +select pg_visibility_map_summary('test_vac_unmodified_heap'); +-- though the VM is truncated, the heap page-level visibility hint, +-- PD_ALL_VISIBLE should still be set +SELECT (flags & x'0004'::int) <> 0 + FROM page_header(get_raw_page('test_vac_unmodified_heap', 0)); +-- vacuum sets the VM +vacuum test_vac_unmodified_heap; +select pg_visibility_map_summary('test_vac_unmodified_heap'); + -- test copy freeze create table copyfreeze (a int, b char(1500)); diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 2086a577199..2da35c85e76 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -2122,16 +2122,14 @@ lazy_scan_prune(LVRelState *vacrel, * of last heap_vac_scan_next_block() call), and from all_visible and * all_frozen variables */ - if (!all_visible_according_to_vm && presult.all_visible) + if ((presult.all_visible && !all_visible_according_to_vm) || + (presult.all_frozen && !VM_ALL_FROZEN(rel, blkno, &vmbuffer))) { uint8 old_vmbits; uint8 flags = VISIBILITYMAP_ALL_VISIBLE; if (presult.all_frozen) - { - Assert(!TransactionIdIsValid(presult.vm_conflict_horizon)); flags |= VISIBILITYMAP_ALL_FROZEN; - } /* * It should never be the case that the visibility map page is set @@ -2139,15 +2137,25 @@ lazy_scan_prune(LVRelState *vacrel, * checksums are not enabled). Regardless, set both bits so that we * get back in sync. * - * NB: If the heap page is all-visible but the VM bit is not set, we - * don't need to dirty the heap page. However, if checksums are - * enabled, we do need to make sure that the heap page is dirtied - * before passing it to visibilitymap_set(), because it may be logged. - * Given that this situation should only happen in rare cases after a - * crash, it is not worth optimizing. + * Even if PD_ALL_VISIBLE is already set, we don't need to worry about + * unnecessarily dirtying the heap buffer. Nearly the only scenario + * where PD_ALL_VISIBLE is set but the VM is not is if the VM was + * removed -- and that isn't worth optimizing for. And if we add the + * heap buffer to the WAL chain (without passing REGBUF_NO_CHANGES), + * it must be marked dirty. */ PageSetAllVisible(page); MarkBufferDirty(buf); + + /* + * If the page is being set all-frozen, we pass InvalidTransactionId + * as the cutoff_xid, since a snapshot conflict horizon sufficient to + * make everything safe for REDO was logged when the page's tuples + * were frozen. + */ + Assert(!presult.all_frozen || + !TransactionIdIsValid(presult.vm_conflict_horizon)); + old_vmbits = visibilitymap_set(vacrel->rel, blkno, buf, InvalidXLogRecPtr, vmbuffer, presult.vm_conflict_horizon, @@ -2219,65 +2227,6 @@ lazy_scan_prune(LVRelState *vacrel, VISIBILITYMAP_VALID_BITS); } - /* - * If the all-visible page is all-frozen but not marked as such yet, mark - * it as all-frozen. - */ - else if (all_visible_according_to_vm && presult.all_frozen && - !VM_ALL_FROZEN(vacrel->rel, blkno, &vmbuffer)) - { - uint8 old_vmbits; - - /* - * Avoid relying on all_visible_according_to_vm as a proxy for the - * page-level PD_ALL_VISIBLE bit being set, since it might have become - * stale -- even when all_visible is set - */ - if (!PageIsAllVisible(page)) - { - PageSetAllVisible(page); - MarkBufferDirty(buf); - } - - /* - * Set the page all-frozen (and all-visible) in the VM. - * - * We can pass InvalidTransactionId as our cutoff_xid, since a - * snapshotConflictHorizon sufficient to make everything safe for REDO - * was logged when the page's tuples were frozen. - */ - Assert(!TransactionIdIsValid(presult.vm_conflict_horizon)); - old_vmbits = visibilitymap_set(vacrel->rel, blkno, buf, - InvalidXLogRecPtr, - vmbuffer, InvalidTransactionId, - VISIBILITYMAP_ALL_VISIBLE | - VISIBILITYMAP_ALL_FROZEN); - - /* - * The page was likely already set all-visible in the VM. However, - * there is a small chance that it was modified sometime between - * setting all_visible_according_to_vm and checking the visibility - * during pruning. Check the return value of old_vmbits anyway to - * ensure the visibility map counters used for logging are accurate. - */ - if ((old_vmbits & VISIBILITYMAP_ALL_VISIBLE) == 0) - { - vacrel->vm_new_visible_pages++; - vacrel->vm_new_visible_frozen_pages++; - *vm_page_frozen = true; - } - - /* - * We already checked that the page was not set all-frozen in the VM - * above, so we don't need to test the value of old_vmbits. - */ - else - { - vacrel->vm_new_frozen_pages++; - *vm_page_frozen = true; - } - } - return presult.ndeleted; } -- 2.43.0