lazy_scan_heap() forgets to mark buffer dirty when setting allfrozen?

Hi,

lazy_scan_heap() contains the following block:

        /*
         * If the all-visible page is turned out to be all-frozen but not
         * marked, we should so mark it.  Note that all_frozen is only valid
         * if all_visible is true, so we must check both.
         */
        else if (all_visible_according_to_vm && all_visible && all_frozen &&
                 !VM_ALL_FROZEN(onerel, blkno, &vmbuffer))
        {
            /*
             * We can pass InvalidTransactionId as the cutoff XID here,
             * because setting the all-frozen bit doesn't cause recovery
             * conflicts.
             */
            visibilitymap_set(onerel, blkno, buf, InvalidXLogRecPtr,
                              vmbuffer, InvalidTransactionId,
                              VISIBILITYMAP_ALL_FROZEN);
        }

but I'm afraid that's not quite enough. As an earlier comment explains:


             * 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.
             */
            PageSetAllVisible(page);
            MarkBufferDirty(buf);
            visibilitymap_set(onerel, blkno, buf, InvalidXLogRecPtr,
                              vmbuffer, visibility_cutoff_xid, flags);

don't we need to do that here too? visibilitymap_set() does:

                /*
                 * If data checksums are enabled (or wal_log_hints=on), we
                 * need to protect the heap page from being torn.
                 */
                if (XLogHintBitIsNeeded())
                {
                    Page        heapPage = BufferGetPage(heapBuf);

                    /* caller is expected to set PD_ALL_VISIBLE first */
                    Assert(PageIsAllVisible(heapPage));
                    PageSetLSN(heapPage, recptr);
                }

i.e. it actually modifies the page when checksums/wal hint bits are
enabled, setting a different LSN. Without it being dirtied that won't
persist. Which doesn't seem good?


visibilitymap_set()'s comment header doesn't explain this well. Nor is
 * Call visibilitymap_pin first to pin the right one. This function doesn't do
 * any I/O.
actually true, given it does XLogInsert(). I think that should've been
adjusted in 503c7305a1e3 ("Make the visibility map crash-safe.").

Greetings,

Andres Freund