On Tue, 16 Dec 2025 at 22:17, 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. > > Adding to the confusion, in the fourth branch of the if statement for > setting the VM in lazy_scan_prune(): > > else if (all_visible_according_to_vm && presult.all_frozen && > !VM_ALL_FROZEN(vacrel->rel, blkno, &vmbuffer)) > > we conditionally mark the heap buffer dirty > > if (!PageIsAllVisible(page)) > { > PageSetAllVisible(page); > MarkBufferDirty(buf); > } > > This doesn't trip the assert because we heap buffer is always already > marked dirty when we enter this branch. However, I think that it is a > coincidence that this works out and was not intended by the author. > > And because the heap buffer is always dirty anyway, we don't save > anything with this if statement and only create confusion. > > As such, I've proposed in that thread that we refactor this code to a > single visibilitymap_set() case > > if ((presult.all_visible && !all_visible_according_to_vm) || > (presult.all_frozen && !VM_ALL_FROZEN(rel, blkno, &vmbuffer))) > > which unconditionally sets PD_ALL_VISIBLE and marks the heap buffer dirty. > > That patch is 0001 in the v27 posted here [1]. > > - Melanie > > [1] > https://www.postgresql.org/message-id/CAAKRu_ayWLg%3DWDGZZfSPWf0KjPM8u%3DLBb0D6XaEWyx2_YFFwAQ%40mail.gmail.com > >
Hi! I did run a test and this indeed triggers assertion if somebody writes something like [0]. Code at [0] works (although never testes) only because is passed InvalidXLogRecPtr as recptr to visibilitymap_set. Maybe it is worth to add comment nearby this ``` /* * Avoid relying on all_visible_according_to_vm as a proxy for the * page-level PD_ALL_VISIBLE bit being set, since it might have become * stale -- even when all_visible is set */ ``` To explain why is it OK to make conditional MarkBufferDirty? [0] https://coverage.postgresql.org/src/backend/access/heap/vacuumlazy.c.gcov.html#2209 -- Best regards, Kirill Reshke
