lazy_scan_heap() forgets to mark buffer dirty when setting all frozen?

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