On 2013-09-23 14:41:16 +0300, Heikki Linnakangas wrote:
> On 06.06.2013 17:22, Robert Haas wrote:
> >On Thu, May 30, 2013 at 2:29 AM, Andres Freund<and...@2ndquadrant.com>
> >wrote:
> >>>Yeah, I think it's fine. The patch also looks fine, although I think
> >>>the comments could use a bit of tidying. I guess we need to
> >>>back-patch this all the way back to 8.4? It will require some
> >>>adjustments for the older branches.
> >>
> >>I think 9.2 is actually far enough and it should apply there. Before
> >>that we only logged the unsetting of all_visible via
> >>heap_(inset|update|delete)'s wal records not the setting as far as I can
> >>tell. So I don't immediately see a danger< 9.2.
> >
> >OK. I have committed this. For 9.2, I had to backport
> >log_newpage_buffer() and use XLByteEQ rather than ==.
>
> I'm afraid this patch was a few bricks shy of a load. The
> log_newpage_buffer() function asserts that:
>
> > /* We should be in a critical section. */
> > Assert(CritSectionCount > 0);
>
> But the call in vacuumlazy.c is not inside a critical section. Also, the
> comments in log_newpage_buffer() say that the caller should mark the buffer
> dirty *before* calling log_newpage_buffer(), but in vacuumlazy.c, it's
> marked dirty afterwards. I'm not sure what consequences that might have, but
> at least it contradicts the comment.
>
> (spotted this while working on a patch, and ran into the assertion on crash
> recovery)
What about the attached patches (one for 9.3 and master, the other for
9.2)? I've tested that I can trigger the assert before and not after by
inserting faults...
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
>From 4fe13d241062c3aa47ddfe3cfb967d80809aa826 Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Tue, 24 Sep 2013 11:58:34 +0200
Subject: [PATCH] Use critical section when ensuring empty pages are
initialized during vacuum.
a6370fd9 tried to fix the problem that replay of XLOG_HEAP2_VISIBLE
records can fail when unitialized page are referenced during
replay. Unfortunately the patch/I missed the fact that log_newpage()
should be used inside a critical section leading to assertion failure
during WAL replay when those are enabled while working otherwise.
Also fix the issue that pages should be marked dirty before calling
log_newpage_buffer() (check src/backend/access/transam/README for
reasons).
Both issues noticed Heikki
---
src/backend/commands/vacuumlazy.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
index bb4e03e..af7cd59 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -663,6 +663,11 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
/* empty pages are always all-visible */
if (!PageIsAllVisible(page))
{
+ START_CRIT_SECTION();
+
+ /* dirty page before any possible XLogInsert()s */
+ MarkBufferDirty(buf);
+
/*
* It's possible that another backend has extended the heap,
* initialized the page, and then failed to WAL-log the page
@@ -682,9 +687,9 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
log_newpage_buffer(buf);
PageSetAllVisible(page);
- MarkBufferDirty(buf);
visibilitymap_set(onerel, blkno, buf, InvalidXLogRecPtr,
vmbuffer, InvalidTransactionId);
+ END_CRIT_SECTION();
}
UnlockReleaseBuffer(buf);
--
1.8.4.21.g992c386.dirty
>From 2aee405f62207bd7691fd13225ed5be62cc1033a Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Tue, 24 Sep 2013 11:58:34 +0200
Subject: [PATCH] Use critical section when ensuring empty pages are
initialized during vacuum.
a6370fd9 tried to fix the problem that replay of XLOG_HEAP2_VISIBLE
records can fail when unitialized page are referenced during
replay. Unfortunately the patch/I missed the fact that log_newpage()
should be used inside a critical section leading to assertion failure
during WAL replay when those are enabled while working otherwise.
Also fix the issue that pages should be marked dirty before calling
log_newpage_buffer() (check src/backend/access/transam/README for
reasons).
Both issues noticed Heikki
---
src/backend/commands/vacuumlazy.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
index ff8764b..87757aa 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -650,6 +650,11 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
/* empty pages are always all-visible */
if (!PageIsAllVisible(page))
{
+ START_CRIT_SECTION();
+
+ /* dirty page before any possible XLogInsert()s */
+ MarkBufferDirty(buf);
+
/*
* It's possible that another backend has extended the heap,
* initialized the page, and then failed to WAL-log the page
@@ -669,9 +674,9 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
log_newpage_buffer(buf);
PageSetAllVisible(page);
- MarkBufferDirty(buf);
visibilitymap_set(onerel, blkno, InvalidXLogRecPtr, vmbuffer,
InvalidTransactionId);
+ END_CRIT_SECTION();
}
UnlockReleaseBuffer(buf);
--
1.8.4.21.g992c386.dirty
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
