Re: BUG #10533: 9.4 beta1 assertion failure in autovacuum process

On 2014-06-06 18:21:45 -0400, Tom Lane wrote:
> Andres Freund <andres@2ndquadrant.com> writes:
> > On 2014-06-06 18:03:53 -0400, Tom Lane wrote:
> >> The point here seems to be that lazy_vacuum_page does the visibility map
> >> ops inside its own critical section.  Why?  Setting a visibility bit
> >> doesn't seem like it's critical.  Why can't we just move the
> >> END_CRIT_SECTION() to before the PageIsAllVisible test?
>
> > Yea, that's what I am proposing upthread. If we move the visibility
> > tests out of the critical section this will get rid of the original
> > report as well.
>
> I went trolling for other critical sections ...
>
> lazy_scan_heap has same disease, but looks like it can be fixed same way.

Hm. I'm writing a fix for this issue right now and went to look into
lazy_scan_heap() - but I'm not seeing a problem atm. Do you mean that
bit:
        START_CRIT_SECTION();

        /* mark buffer dirty before writing a WAL record */
        MarkBufferDirty(buf);

        /*
         * It's possible that another backend has extended the heap,
         * initialized the page, and then failed to WAL-log the page
         * due to an ERROR.  Since heap extension is not WAL-logged,
         * recovery might try to replay our record setting the page
         * all-visible and find that the page isn't initialized, which
         * will cause a PANIC.  To prevent that, check whether the
         * page has been previously WAL-logged, and if not, do that
         * now.
         */
        if (RelationNeedsWAL(onerel) &&
                PageGetLSN(page) == InvalidXLogRecPtr)
                log_newpage_buffer(buf, true);

        PageSetAllVisible(page);
        visibilitymap_set(onerel, blkno, buf, InvalidXLogRecPtr,
                                          vmbuffer, InvalidTransactionId);
        END_CRIT_SECTION();
If so, I think that should actually be safe - visibilitymap_set()
shouldn't ever do IO - that's why it gets the vmbuffer argument passed
in.
Now, afaics there isn't really any need for the critical section in this
case but the assertion in log_newpage_buffer(). So, afaics, we could
just end the do the END_CRIT_SECTION() two lines earlier, to keep
it as short as possible.

Greetings,

Andres Freund

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