Re: Adding vacuum test case of setting the VM when heap page is unmodified

Hi Melanie,

On Wed, Dec 10, 2025 at 11:21 PM Melanie Plageman <melanieplageman(at)gmail(dot)com>
wrote:

> Hi,
>
> While working on a patch to set the VM in the same WAL record as
> pruning and freezing [1], I discovered we have no test coverage of the
> case where vacuum phase I sets the VM but no modifications are made to
> the heap buffer (not even setting PD_ALL_VISIBLE). This can only
> happen when the VM was somehow removed or destroyed.
>
>
+1 for adding the test, but IIUC PD_ALL_VISIBLE is being set in this
case during the "vacuum test_vac_unmodified_heap;" because
VM bit is not set (as we truncated VM) and presult.all_visible is true as
well ,
so it goes in if (!all_visible_according_to_vm && presult.all_visible),
where its
doing these, this was the flow i observed while trying to understand the
patch by running the given test, please correct me if I'm wrong.

PageSetAllVisible(page);
MarkBufferDirty(buf);
old_vmbits = visibilitymap_set(vacrel->rel, blkno, buf,
InvalidXLogRecPtr,
vmbuffer, presult.vm_conflict_horizon,
flags);

> Currently, we require the heap buffer to be marked dirty even if it is
> unmodified because we add it to the WAL chain and do not pass
> REGBUF_NO_CHANGES. (And we require adding it to the WAL chain because
> we update the freespace map using the heap buffer in recovery). The VM
> being gone is an uncommon case, so I don't think it makes sense to add
> special logic to pass REGBUF_NO_CHANGES. However, I do think we should
> have a test for this case.
>

makes sense, i think this below comment supports your final decision
of not optimizing it.

* 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.
*/

--
Thanks,
Srinath Reddy Sadipiralla
EDB: https://www.enterprisedb.com/