On Tue, Dec 16, 2025 at 10:47 PM Melanie Plageman <[email protected]> wrote:
> Thanks for the review! > > On Tue, Dec 16, 2025 at 11:39 AM Srinath Reddy Sadipiralla > <[email protected]> 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. -- Thanks, Srinath Reddy Sadipiralla EDB: https://www.enterprisedb.com/
