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

Hi,

On 2019-04-08 10:59:32 -0400, Robert Haas wrote:
> On Mon, Apr 8, 2019 at 12:55 AM Andres Freund <andres@anarazel.de> wrote:
> > > Apparently the initial commit a892234f830e had MarkBufferDirty, but it
> > > was removed one week later by 77a1d1e79892.
> >
> > Good catch. Kinda looks like it could have been an accidental removal?
> > Robert?
>
> So you're talking about this hunk?
>
> -            /* Page is marked all-visible but should be all-frozen */
> -            PageSetAllFrozen(page);
> -            MarkBufferDirty(buf);
> -
>
> I don't remember exactly, but I am pretty sure that I assumed from the
> way that hunk looked that the MarkBufferDirty() was only needed there
> because of the call to PageSetAllFrozen().  Perhaps I should've
> figured it out from the "NB: If the heap page is all-visible..."
> comment, but I unfortunately don't find that comment to be very clear
> -- it basically says we don't need to do it, and then immediately
> contradicts itself by saying we sometimes do need to do it "because it
> may be logged."  But that's hardly an explanation, because why should
> the fact that the page is going to be logged require that it be
> dirtied?  We could improve the comment, but before we go there...

> Why the heck does visibilitymap_set() require callers to do
> MarkBufferDirty() instead of doing it itself?  Or at least, if it's
> got to work that way, can't it Assert() something?  It seems crazy to
> me that it calls PageSetLSN() without calling MarkBufferDirty() or
> asserting that the buffer is dirty or having a header comment that
> says that the buffer must be dirty. Ugh.

I think the visibilitymap_set() has incrementally gotten worse, to the
point that it should just be redone. Initially, before you made it
crashsafe, it indeed didn't do any I/O (like the header claims), and
didn't touch the heap. To make it crashsafe it started to call back into
heap. Then to support checksums, most of its callers had to take be
adapted around marking buffers dirty. And then the all-frozen stuff
complicated it a bit further.

I don't quite know what the right answer is, but I think
visibilitymap_set (or whatever it's successor is), shouldn't do any WAL
logging of its own, and it shouldn't call into heap - we want to be able
to reuse the vm for things like zheap after all. It's also just an
extremely confusing interface, especially because say
visibilitymap_clear() doesn't do WAL logging.

I think we should have a heap_visibilitymap_set() that does the WAL
logging, which internally calls into visibilitymap.c.

Perhaps it could look something roughly like:

static void
heap_visibilitymap_set(...)
{
    Assert(LWLockIsHeld(BufferDescriptorGetIOLock(heapBuf))

    Assert(PageIsAllVisible(heapPage));
    /* other checks */

    START_CRIT_SECTION();

    /* if change required, this locks VM buffer */
    if (visibilitmap_start_set(rel, heapBlk, &vmBuf))
    {
        XLogRecPtr recptr;

        MarkBufferDirty(heapBuf)

        if (RelationNeedsWAL(rel))
        {
            /* inlined body of log_heap_visible */
            recptr = XLogInsert(XLOG_HEAP2_VISIBLE);

            /*
             * 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);
            }
        }


        /* actually change bits, set page LSN, release vmbuf lock */
        visibilitmap_finish_set(rel, heapBlk, vmbuf, recptr);
    }

    END_CRIT_SECTION();
}

The replay routines would then not use heap_visibilitymap_set (they
right now call visibilitymap_set with a valid XLogRecPtr), but instead
visibilitmap_redo_set() or such, which can check the LSNs etc.

Greetings,

Andres Freund