]> git.ipfire.org Git - thirdparty/postgresql.git/commitdiff
Combine visibilitymap_set() cases in lazy_scan_prune()
authorMelanie Plageman <melanieplageman@gmail.com>
Mon, 26 Jan 2026 20:57:51 +0000 (15:57 -0500)
committerMelanie Plageman <melanieplageman@gmail.com>
Mon, 26 Jan 2026 21:03:32 +0000 (16:03 -0500)
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 <melanieplageman@gmail.com>
Reviewed-by: Andres Freund <andres@anarazel.de>
Reviewed-by: Chao Li <li.evan.chao@gmail.com>
Reviewed-by: Srinath Reddy Sadipiralla <srinath2133@gmail.com>
Reviewed-by: Kirill Reshke <reshkekirill@gmail.com>
Reviewed-by: Xuneng Zhou <xunengzhou@gmail.com>
Reviewed-by: Andrey Borodin <x4mmm@yandex-team.ru>
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

contrib/pg_visibility/Makefile
contrib/pg_visibility/expected/pg_visibility.out
contrib/pg_visibility/sql/pg_visibility.sql
src/backend/access/heap/vacuumlazy.c

index d3cb411cc90d9eac9436b2464f30287c0002d686..e5a74f32c486d92e70d89cac71bfe72642da71cd 100644 (file)
@@ -10,6 +10,7 @@ DATA = pg_visibility--1.1.sql pg_visibility--1.1--1.2.sql \
        pg_visibility--1.0--1.1.sql
 PGFILEDESC = "pg_visibility - page visibility information"
 
+EXTRA_INSTALL = contrib/pageinspect
 REGRESS = pg_visibility
 TAP_TESTS = 1
 
index 09fa5933a35b29d2847a31943546b012a18c409d..e10f1706015f499e433cafb147b472f547db39e7 100644 (file)
@@ -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
index 5af06ec5b76008efbf823440567bb72078a41f85..57af8a0c5b64a0bd9e1d39c9b7aedf5724313138 100644 (file)
@@ -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));
 
index 1fcb212ab3d48574f595138c5a50ed856f9b98b2..e0f35655a0843863844a2c7f6c3f116878567d33 100644 (file)
@@ -2120,16 +2120,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
@@ -2137,15 +2135,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,
@@ -2217,65 +2225,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;
 }