Re: eliminate xl_heap_visible to reduce WAL (and eventually set VM on-access) - Mailing list pgsql-hackers

> On Dec 11, 2025, at 07:35, Melanie Plageman <melanieplageman@gmail.com> wrote:
>
> On Tue, Dec 9, 2025 at 12:48 PM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
>>
>> In this set 0001 and 0002 are independent. 0003-0007 are all small
>> steps toward the single change in 0007 which combines the VM updates
>> into the same WAL record as pruning and freezing. 0008 and 0009 are
>> removing the rest of XLOG_HEAP2_VISIBLE. 0010 - 0012 are refactoring
>> needed to set the VM during on-access pruning. 0013 - 0015 are small
>> steps toward setting the VM on-access. And 0016 sets the prune xid on
>> insert so we may set the VM on-access for pages that have only new
>> data.
>
> I committed 0001 and 0002. attached v25 reflects that.
> 0001-0004 refactoring steps for eliminate visible record from phase I
> (not probably independent commits in the end)
> 0005 eliminate XLOG_HEAP2_VISIBLE from phase I vac
> 0006-0007 removing the rest of XLOG_HEAP2_VISIBLE
> 0008-0010 refactoring for setting VM on-access
> 0011-0013 setting the VM on-access
> 0014 - setting pd_prune_xid on insert
>
> - Melanie
>
<v25-0001-Combine-visibilitymap_set-cases-in-lazy_scan_pru.patch><v25-0002-Refactor-lazy_scan_prune-VM-set-logic-into-helpe.patch><v25-0003-Set-the-VM-in-heap_page_prune_and_freeze.patch><v25-0004-Move-VM-assert-into-prune-freeze-code.patch><v25-0005-Eliminate-XLOG_HEAP2_VISIBLE-from-vacuum-phase-I.patch><v25-0006-Eliminate-XLOG_HEAP2_VISIBLE-from-empty-page-vac.patch><v25-0007-Remove-XLOG_HEAP2_VISIBLE-entirely.patch><v25-0008-Rename-GlobalVisTestIsRemovableXid-to-GlobalVisX.patch><v25-0009-Use-GlobalVisState-in-vacuum-to-determine-page-l.patch><v25-0011-Track-which-relations-are-modified-by-a-query.patch><v25-0012-Pass-down-information-on-table-modification-to-s.patch><v25-0013-Allow-on-access-pruning-to-set-pages-all-visible.patch><v25-0014-Set-pd_prune_xid-on-insert.patch>

A few more small comments. Sorry for keeping come out new comments. Actually I learned a lot about vacuum from
reviewingthis patch. 

1 - 0001
```
+-- 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');
+-- vacuum sets the VM but does not need to set PD_ALL_VISIBLE so no heap page
+-- modification
+vacuum test_vac_unmodified_heap;
```

The last vacuum is expected to set vm bits, but the test doesn’t verify that. Should we verify that like:
```
evantest=# SELECT blkno, all_visible, all_frozen FROM pg_visibility_map('test_vac_unmodified_heap');
 blkno | all_visible | all_frozen
-------+-------------+------------
     0 | t           | t
(1 row)
```

As you have been using the extension pg_visibility, adding the verification with pg_visibility_map() should not be a
burden.

2 - 0001
```
         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;
         }
```

The comment here is a little confusing. In the old code, the Assert() as immediately above the call
visibilitymap_set(),and cutoff_xid is a parameter to the call. But the new code moves the Assert() as well as the
commentfar away from the call visibilitymap_set(), so I think the comment should stay together with the call of
visibilitymap_set().

3 - 0002
```
 * If it finds that the page-level visibility hint or VM is corrupted, it will
* fix them by clearing the VM bits and visibility page hint. This does not
```

In the second line, “visibility page hint” is understandable but feels not quite good. I know it’s actually “page-level
visibilityhint”, so how about just “visibility hint”. 

4 - 0002
```
     /*
-     * As of PostgreSQL 9.2, the visibility map bit should never be set if the
-     * page-level bit is clear.  However, it's possible that the bit got
-     * cleared after heap_vac_scan_next_block() was called, so we must recheck
-     * with buffer lock before concluding that the VM is corrupt.
+     * For the purposes of logging, count whether or not the page was newly
+     * set all-visible and, potentially, all-frozen.
      */
-    else if (all_visible_according_to_vm && !PageIsAllVisible(page) &&
-             visibilitymap_get_status(vacrel->rel, blkno, &vmbuffer) != 0)
+    if ((old_vmbits & VISIBILITYMAP_ALL_VISIBLE) == 0 &&
+        (new_vmbits & VISIBILITYMAP_ALL_VISIBLE) != 0)
     {
```

Without do_set_vm==true, old_vmbits will only be 0, thus this “if-elseif” that uses old_vmbits should be moved into “if
(do_set_vm)”.From this perspective, if not do_set_vm, this function can return early, like: 

```
Do_set_vm = heap_page_will_set_vm(&new_vmbits)
If (!do_set_vm)
   Return presult.ndeleted;

PageSetAllVisible(page);
MarkBufferDirty(buf);
old_vmbits = visibilitymap_set(new_vmbits);
If (old_vmbits..)
{
..
}
Else if (old_vmbits…)
{
…
}

Return presult.ndeleted;
```

5 - 0003
```
 /*
  *    lazy_scan_prune() -- lazy_scan_heap() pruning and freezing.
  *
@@ -2076,15 +1979,14 @@ lazy_scan_prune(LVRelState *vacrel,
                 bool *vm_page_frozen)
 {
     Relation    rel = vacrel->rel;
-    bool        do_set_vm = false;
-    uint8        new_vmbits = 0;
-    uint8        old_vmbits = 0;
     PruneFreezeResult presult;
     PruneFreezeParams params = {
         .relation = rel,
         .buffer = buf,
+        .vmbuffer = vmbuffer,
+        .blk_known_av = all_visible_according_to_vm,
         .reason = PRUNE_VACUUM_SCAN,
-        .options = HEAP_PAGE_PRUNE_FREEZE,
+        .options = HEAP_PAGE_PRUNE_FREEZE | HEAP_PAGE_PRUNE_UPDATE_VM,
         .vistest = vacrel->vistest,
         .cutoffs = &vacrel->cutoffs,
     };
```

This maybe a legacy bug. Here presult is not initialized, and it is immediately passed to heap_page_prune_and_freeze():

```
    heap_page_prune_and_freeze(¶ms,
                               &presult, <=== here
                               &vacrel->offnum,
                               &vacrel->NewRelfrozenXid, &vacrel->NewRelminMxid);
```

Then heap_page_prune_and_freeze() immediately calls prune_freeze_setup():
```
    /* Initialize prstate */
    prune_freeze_setup(params,
                       new_relfrozen_xid, new_relmin_mxid,
                       presult, &prstate);
```

And prune_freeze_setup() takes presult as a const pointer:
```
static void
prune_freeze_setup(PruneFreezeParams *params,
                   TransactionId *new_relfrozen_xid,
                   MultiXactId *new_relmin_mxid,
                   const PruneFreezeResult *presult, <=== here
                   PruneState *prstate)
{
    prstate->deadoffsets = (OffsetNumber *) presult->deadoffsets; <== here, presult->deadoffsets could be a random
value
}
```

As this is a separate issue off the current patch, I just filed a new patch to fix it. Please take a look at:
https://www.postgresql.org/message-id/CAEoWx2%3DjiD1nqch4JQN%2BodAxZSD7mRvdoHUGJYN2r6tQG_66yQ%40mail.gmail.com

6 - 0003
```
+ * Returns true if one or both VM bits should be set, along with returning the
+ * desired what bits should be set in the VM in *new_vmbits.
```

Looks like a typo: “returning the desired what bits should be set”, maybe change to “returning the desired bits to be
set”.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/