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 is set
* 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 VM bit is not set,
* we don't need to dirty the heap page. However, if checksums are
* enabled, we do need to make sure that the heap page is dirtied
* before passing it to visibilitymap_set(), because it may be
* logged. Given that 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers