Hi Melanie, On Wed, Dec 10, 2025 at 11:21 PM Melanie Plageman <[email protected]> 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/
