visibilitymap_set and checksums

Hi,

while thinking about vacuum freeze I noticed that since the checksums
patch visibilitymap_set() does:/* * If data checksums are enabled, we need to protect the heap * page from being torn.
*/if(DataChecksumsEnabled()){    Page heapPage = BufferGetPage(heapBuf);
 
    /* caller is expected to set PD_ALL_VISIBLE first */    Assert(PageIsAllVisible(heapPage));    PageSetLSN(heapPage,
recptr);}

That pattern looks dangerous. Setting the lsn of the heap page will
prevent the next action from doing a FPI even if it would be required.

Its e.g. called like this from lazy_scan_heap:
if (all_visible && !all_visible_according_to_vm){    /*     * It should never be the case that the visibility map page
isset     * while the page-level bit is clear, but the reverse is allowed     * (if checksums are not enabled).
Regardless,set the both bits     * so that we get back in sync.     *     * NB: If the heap page is all-visible but the
VMbit is not set,     * we don't need to dirty the heap page.  However, if checksums are     * enabled, we do need to
makesure that the heap page is dirtied     * before passing it to visibilitymap_set(), because it may be     * logged.
Giventhat 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);}
 

other callers look similarly dangerous.

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services