From ed8807b5099a0066881c8b8e1690100fa71f2e90 Mon Sep 17 00:00:00 2001 From: Melanie Plageman Date: Mon, 8 Dec 2025 15:49:54 -0500 Subject: [PATCH v24 03/16] Combine visibilitymap_set() cases in lazy_scan_prune() The heap buffer is unconditionally added to the WAL chain when setting the VM, so it must always be marked dirty. In one of the cases in lazy_scan_prune(), we try to avoid setting PD_ALL_VISIBLE and marking the buffer dirty again if PD_ALL_VISIBLE is already set. There is little gain here, and if we eliminate that condition, we can easily combine the two cases which set the VM in lazy_scan_prune(). This is more straightforward and makes it clear that the heap buffer must be marked dirty since it is added to the WAL chain. In the previously separate second VM set case, the heap buffer would always be dirty anyway -- either because we just froze a tuple and marked the buffer dirty or because we modified the buffer between find_next_unskippable_block() and heap_page_prune_and_freeze() and then pruned it in heap_page_prune_and_freeze(). This commit also adds a test case to ensure we don't add code resulting in the heap buffer not being marked dirty before being added to the WAL chain. XXX: is it okay to do a checkpoint in the pg_visibility test? --- .../pg_visibility/expected/pg_visibility.out | 13 +++ contrib/pg_visibility/sql/pg_visibility.sql | 9 ++ src/backend/access/heap/vacuumlazy.c | 95 ++++--------------- 3 files changed, 43 insertions(+), 74 deletions(-) diff --git a/contrib/pg_visibility/expected/pg_visibility.out b/contrib/pg_visibility/expected/pg_visibility.out index 09fa5933a35..adc01162895 100644 --- a/contrib/pg_visibility/expected/pg_visibility.out +++ b/contrib/pg_visibility/expected/pg_visibility.out @@ -204,6 +204,19 @@ select pg_truncate_visibility_map('test_partition'); (1 row) +-- the heap buffer must be marked dirty before adding it to the WAL chain when +-- setting the VM +create table test_heap_buffer_dirty(a int); +insert into test_heap_buffer_dirty values (1); +vacuum (freeze) test_heap_buffer_dirty; +checkpoint; +select pg_truncate_visibility_map('test_heap_buffer_dirty'); + pg_truncate_visibility_map +---------------------------- + +(1 row) + +vacuum test_heap_buffer_dirty; -- 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..0cdd087badb 100644 --- a/contrib/pg_visibility/sql/pg_visibility.sql +++ b/contrib/pg_visibility/sql/pg_visibility.sql @@ -94,6 +94,15 @@ 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'); +-- the heap buffer must be marked dirty before adding it to the WAL chain when +-- setting the VM +create table test_heap_buffer_dirty(a int); +insert into test_heap_buffer_dirty values (1); +vacuum (freeze) test_heap_buffer_dirty; +checkpoint; +select pg_truncate_visibility_map('test_heap_buffer_dirty'); +vacuum test_heap_buffer_dirty; + -- 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 984d5879947..14040552e48 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -2080,15 +2080,21 @@ 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; + uint8 new_vmbits = VISIBILITYMAP_ALL_VISIBLE; if (presult.all_frozen) { + /* + * 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)); - flags |= VISIBILITYMAP_ALL_FROZEN; + new_vmbits |= VISIBILITYMAP_ALL_FROZEN; } /* @@ -2097,36 +2103,36 @@ 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. + * The heap page is added to the WAL chain even if it wasn't modified, + * so we still need to mark it dirty. The only scenario where it isn't + * modified in phase I is when the VM was truncated or removed, which + * isn't worth optimizing for. */ PageSetAllVisible(page); MarkBufferDirty(buf); old_vmbits = visibilitymap_set(vacrel->rel, blkno, buf, InvalidXLogRecPtr, vmbuffer, presult.vm_conflict_horizon, - flags); + new_vmbits); /* - * If the page wasn't already set all-visible and/or all-frozen in the - * VM, count it as newly set for logging. + * For the purposes of logging, count whether or not the page was + * newly set all-visible and, potentially, all-frozen. */ - if ((old_vmbits & VISIBILITYMAP_ALL_VISIBLE) == 0) + if ((old_vmbits & VISIBILITYMAP_ALL_VISIBLE) == 0 && + (new_vmbits & VISIBILITYMAP_ALL_VISIBLE) != 0) { vacrel->vm_new_visible_pages++; - if (presult.all_frozen) + if ((new_vmbits & VISIBILITYMAP_ALL_FROZEN) != 0) { vacrel->vm_new_visible_frozen_pages++; *vm_page_frozen = true; } } else if ((old_vmbits & VISIBILITYMAP_ALL_FROZEN) == 0 && - presult.all_frozen) + (new_vmbits & VISIBILITYMAP_ALL_FROZEN) != 0) { + Assert((new_vmbits & VISIBILITYMAP_ALL_VISIBLE) != 0); vacrel->vm_new_frozen_pages++; *vm_page_frozen = true; } @@ -2177,65 +2183,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