On Tue, Dec 16, 2025 at 11:39 AM Srinath Reddy Sadipiralla <srinath2133@gmail.com> wrote: > >> 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);
You're right. In the current code, it will correctly mark the buffer dirty -- even if PD_ALL_VISIBLE was already set. I'm suggesting we add the test to guard against someone trying to optimize this case and not set PD_ALL_VISIBLE and mark the buffer dirty if PD_ALL_VISIBLE is already set and the heap page requires no modification.
While writing another patch, I did try this optimization and didn't see any test failures. After a conversation off-list with Andres, he reminded me that buffers always must be marked dirty before registering them with XLogRegisterBuffer() (unless REGBUF_NO_CHANGES is passed) or an assert will be tripped. That is how I realized we didn't have coverage of the case where the heap buffer doesn't need to be modified.
Makes sense, after this clarification, I have tested the patch, LGTM.
