> On Dec 11, 2025, at 07:35, Melanie Plageman <[email protected]> wrote:
>
> On Tue, Dec 9, 2025 at 12:48 PM Melanie Plageman
> <[email protected]> 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 reviewing this 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 comment far
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 visibility hint”, 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/
