Re: visibilitymap_set and checksums

On 24 May 2013 20:26, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2013-05-24 19:09:57 +0100, Simon Riggs wrote:
>> On 24 May 2013 18:40, Andres Freund <andres(at)2ndquadrant(dot)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