Обсуждение: 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

On 24 May 2013 18:40, Andres Freund <andres@2ndquadrant.com> wrote:

> 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.

Can you be more specific about the danger you see?

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

On 2013-05-24 19:09:57 +0100, Simon Riggs wrote:
> On 24 May 2013 18:40, Andres Freund <andres@2ndquadrant.com> wrote:
> 
> > 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.
> 
> Can you be more specific about the danger you see?

CHECKPOINT at lsn 0/10;
vacuum starts
vacuum finds page which is all visible
vacuum sets all_visible   PageSetAllVisible(page);   MarkBufferDirty(buf);   visibilitymap_set(onerel, blkno, buf,
InvalidXLogRecPtr,                    vmbuffer, visibility_cutoff_xid);       recptr = log_heap_visible(rel->rd_node,
heapBuf,vmBuf,                                 cutoff_xid);       if (DataChecksumsEnabled())
PageSetLSN(heapPage,recptr);
 

So at this point the *heap* page will have the lsn of the
xl_heap_visible record. Which I thought to be rather dangerous because I
somewow missed the fact that log_heap_visible does:if (DataChecksumsEnabled()){    rdata[1].next = &(rdata[2]);
    rdata[2].data = NULL;    rdata[2].len = 0;    rdata[2].buffer = heap_buffer;    rdata[2].buffer_std = true;
rdata[2].next= NULL;}
 

So. Forget what I said, I just was confused.

Greetings,

Andres Freund

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

On 24 May 2013 20:26, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2013-05-24 19:09:57 +0100, Simon Riggs wrote:
>> On 24 May 2013 18:40, Andres Freund <andres@2ndquadrant.com> wrote:
>>
>> > 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.
>>
>> Can you be more specific about the danger you see?
>
> CHECKPOINT at lsn 0/10;
> vacuum starts
> vacuum finds page which is all visible
> vacuum sets all_visible
>     PageSetAllVisible(page);
>     MarkBufferDirty(buf);
>     visibilitymap_set(onerel, blkno, buf, InvalidXLogRecPtr,
>                       vmbuffer, visibility_cutoff_xid);
>         recptr = log_heap_visible(rel->rd_node, heapBuf, vmBuf,
>                                   cutoff_xid);
>         if (DataChecksumsEnabled())
>                 PageSetLSN(heapPage, recptr);
>
> So at this point the *heap* page will have the lsn of the
> xl_heap_visible record. Which I thought to be rather dangerous because I
> somewow missed the fact that log_heap_visible does:
>         if (DataChecksumsEnabled())
>         {
>                 rdata[1].next = &(rdata[2]);
>
>                 rdata[2].data = NULL;
>                 rdata[2].len = 0;
>                 rdata[2].buffer = heap_buffer;
>                 rdata[2].buffer_std = true;
>                 rdata[2].next = NULL;
>         }
>
> So. Forget what I said, I just was confused.

I think its perfectly understandable. Robert, Jeff and I discussed
that for a while before we passed it. I'm still not happy with it, and
think its a pretty confusing section of code with multiple paths
through it, but I just can't see a better way.

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

On Fri, 2013-05-24 at 22:16 +0100, Simon Riggs wrote: 
> I think its perfectly understandable. Robert, Jeff and I discussed
> that for a while before we passed it. I'm still not happy with it, and
> think its a pretty confusing section of code with multiple paths
> through it, but I just can't see a better way.

Agreed on all counts. Comment patches are welcome, of course.

I'll add that if we remove PD_ALL_VISIBLE, this complexity disappears.

Regards,Jeff Davis