Re: [HACKERS] all_visible replay aborting due to uninitialized pages
Bruce Momjian writes: > On Mon, Nov 11, 2013 at 01:42:07AM +0100, Andres Freund wrote: >> The fix is included in 9.2.5, it's just not noted in the release notes. > Yes, I missed it because I didn't understand the importance of these > commit messages: > commit 17fa4c321ccf9693de406faffe6b235e949aa25f > Author: Robert Haas > Date: Thu Jun 6 10:15:45 2013 -0400 > Ensure that XLOG_HEAP2_VISIBLE always targets an initialized page. > Andres Freund I would say that's not your fault, it's the fault of the author of the commit message. These messages are supposed to provide enough information for release note writing. This one seems a bit ... um ... terse. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] all_visible replay aborting due to uninitialized pages
On Mon, Nov 11, 2013 at 01:42:07AM +0100, Andres Freund wrote: > Hi, > > On 2013-11-10 17:40:31 -0700, Noah Yetter wrote: > > Like your customer, this bug has blown up my standby servers, twice in the > > last month: the first time all 4 replicas, the second time (mysteriously > > but luckily) only 1 of them. > > > > At any rate, since the fix isn't available yet, is/are there any > > configuration changes that can be made or maintenance procedures that can > > be undertaken to prevent or reduce the probability of this bug popping up > > again in the meantime? I really can't afford to be without my standby > > servers during the holidays, even for the few hours it takes to build a new > > one. > > The fix is included in 9.2.5, it's just not noted in the release notes. Yes, I missed it because I didn't understand the importance of these commit messages: commit 17fa4c321ccf9693de406faffe6b235e949aa25f Author: Robert Haas Date: Thu Jun 6 10:15:45 2013 -0400 Ensure that XLOG_HEAP2_VISIBLE always targets an initialized page. Andres Freund commit 4c641d994e19676ef2fec574d52d2156ffc2b3ce Author: Robert Haas Date: Thu Jun 6 10:14:46 2013 -0400 Backport log_newpage_buffer. Andres' fix for XLOG_HEAP2_VISIBLE on unitialized pages requires this. Sorry. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] all_visible replay aborting due to uninitialized pages
Hi, On 2013-11-10 17:40:31 -0700, Noah Yetter wrote: > Like your customer, this bug has blown up my standby servers, twice in the > last month: the first time all 4 replicas, the second time (mysteriously > but luckily) only 1 of them. > > At any rate, since the fix isn't available yet, is/are there any > configuration changes that can be made or maintenance procedures that can > be undertaken to prevent or reduce the probability of this bug popping up > again in the meantime? I really can't afford to be without my standby > servers during the holidays, even for the few hours it takes to build a new > one. The fix is included in 9.2.5, it's just not noted in the release notes. 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
Re: [HACKERS] all_visible replay aborting due to uninitialized pages
Like your customer, this bug has blown up my standby servers, twice in the last month: the first time all 4 replicas, the second time (mysteriously but luckily) only 1 of them. At any rate, since the fix isn't available yet, is/are there any configuration changes that can be made or maintenance procedures that can be undertaken to prevent or reduce the probability of this bug popping up again in the meantime? I really can't afford to be without my standby servers during the holidays, even for the few hours it takes to build a new one. On Tue, May 28, 2013 at 11:58 AM, Andres Freund wrote: > Hi, > > A customer of ours reporting a standby loosing sync with the primary due > to the following error: > CONTEXT: xlog redo visible: rel 1663/XXX/XXX; blk 173717 > WARNING: page 173717 of relation base/XXX/XXX is uninitialized > ... > PANIC: WAL contains references to invalid pages > > Guessing around I looked and noticed the following problematic pattern: > 1) A: wants to do an update, doesn't have enough freespace > 2) A: extends the relation on the filesystem level > (RelationGetBufferForTuple) > 3) A: does PageInit (RelationGetBufferForTuple) > 4) A: aborts, e.g. due to a serialization failure (heap_update) > > At this point the page is initialized in memory, but not wal logged. It > isn't pinned or locked either. > > 5) B: vacuum finds that page and it's empty. So it marks it all > visible. But since the page wasn't written out (we haven't even marked > it dirty in 3.) the standby doesn't know that and reports the page as > being uninitialized. > > ISTM the best backbranchable fix for this is to teach lazy_scan_heap to > log an FPI for the heap page via visibilitymap_set in that rather > limited case. > > Happy to provide a patch unless somebody has a better idea? > > 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 >
Re: [HACKERS] all_visible replay aborting due to uninitialized pages
On 22.10.2013 14:14, Andres Freund wrote: Hi Robert, Heikki, On 2013-09-24 13:25:41 +0200, Andres Freund wrote: 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... Yould either of you commit those patches to the corresponding branches? Committed, thanks. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] all_visible replay aborting due to uninitialized pages
Hi Robert, Heikki, On 2013-09-24 13:25:41 +0200, Andres Freund wrote: > > 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... Yould either of you commit those patches to the corresponding branches? 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
Re: [HACKERS] all_visible replay aborting due to uninitialized pages
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
> >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
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
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 th
Re: [HACKERS] all_visible replay aborting due to uninitialized pages
Andres Freund wrote: >On 2013-09-23 14:41:16 +0300, Heikki Linnakangas wrote: >> (spotted this while working on a patch, and ran into the assertion on >crash >> recovery) > >You got the assertion failure about CritSectionCount during recovery? >If so, I do not understand, that code shouldn't be executed there? Or >do >you mean you patched a version that didn't include that patch and it >Asserted during recovery because of the missing lsn? Sorry, I was imprecise. I ran into it after recovery, on vacuum. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] all_visible replay aborting due to uninitialized pages
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 > >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. Hrmpf. Sorry for that. Will provide a patch. > 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. We generally should do that for wal logging - I am not sure why log_newpage is not doing that itself, but whatever. > (spotted this while working on a patch, and ran into the assertion on crash > recovery) You got the assertion failure about CritSectionCount during recovery? If so, I do not understand, that code shouldn't be executed there? Or do you mean you patched a version that didn't include that patch and it Asserted during recovery because of the missing lsn? 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
Re: [HACKERS] all_visible replay aborting due to uninitialized pages
On 06.06.2013 17:22, Robert Haas wrote: On Thu, May 30, 2013 at 2:29 AM, Andres Freund 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) - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] all_visible replay aborting due to uninitialized pages
On 2013-06-06 10:22:14 -0400, Robert Haas wrote: > On Thu, May 30, 2013 at 2:29 AM, Andres Freund 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 ==. Thanks! 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
Re: [HACKERS] all_visible replay aborting due to uninitialized pages
On Thu, May 30, 2013 at 2:29 AM, Andres Freund 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 ==. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] all_visible replay aborting due to uninitialized pages
On 2013-05-29 23:01:31 -0400, Robert Haas wrote: > On Wed, May 29, 2013 at 9:57 AM, Andres Freund wrote: > >> Thought about that, but given that 9.3's visibilitymap_set already will > >> already FPI heap pages I concluded it wouldn't really be an improvement > >> since it's only one ||log_heap_page or so there. Not sure what's > >> better. Will write the patch and see how it goes. > > > > Ended up using log_newpage_buffer since reusing visibilitymap_set's > > record would break the wal format as we currently do not accept an FPI > > on the heap pages during replay when < 9.3. Forcing to upgrade the > > client first would be rather unfriendly... > > > > That has the disadvantage of logging a full heap page since it doesn't > > use the hole optimization but this happens really infrequently, so ... > > 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. 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
Re: [HACKERS] all_visible replay aborting due to uninitialized pages
On Wed, May 29, 2013 at 9:57 AM, Andres Freund wrote: >> Thought about that, but given that 9.3's visibilitymap_set already will >> already FPI heap pages I concluded it wouldn't really be an improvement >> since it's only one ||log_heap_page or so there. Not sure what's >> better. Will write the patch and see how it goes. > > Ended up using log_newpage_buffer since reusing visibilitymap_set's > record would break the wal format as we currently do not accept an FPI > on the heap pages during replay when < 9.3. Forcing to upgrade the > client first would be rather unfriendly... > > That has the disadvantage of logging a full heap page since it doesn't > use the hole optimization but this happens really infrequently, so ... 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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] all_visible replay aborting due to uninitialized pages
On 2013-05-29 03:56:38 +0200, Andres Freund wrote:
> On 2013-05-28 21:36:17 -0400, Robert Haas wrote:
> > On Tue, May 28, 2013 at 1:58 PM, Andres Freund
> > wrote:
> > > Guessing around I looked and noticed the following problematic pattern:
> > > 1) A: wants to do an update, doesn't have enough freespace
> > > 2) A: extends the relation on the filesystem level
> > > (RelationGetBufferForTuple)
> > > 3) A: does PageInit (RelationGetBufferForTuple)
> > > 4) A: aborts, e.g. due to a serialization failure (heap_update)
> > >
> > > At this point the page is initialized in memory, but not wal logged. It
> > > isn't pinned or locked either.
> > >
> > > 5) B: vacuum finds that page and it's empty. So it marks it all
> > > visible. But since the page wasn't written out (we haven't even marked
> > > it dirty in 3.) the standby doesn't know that and reports the page as
> > > being uninitialized.
> > >
> > > ISTM the best backbranchable fix for this is to teach lazy_scan_heap to
> > > log an FPI for the heap page via visibilitymap_set in that rather
> > > limited case.
> > >
> > > Happy to provide a patch unless somebody has a better idea?
> >
> > Good catch. However, I would suggest using log_newpage() before
> > visibilitymap_set() rather than trying to stick extra logic into
> > visibilitymap_set(). I think that will be cleaner and simpler.
>
> Thought about that, but given that 9.3's visibilitymap_set already will
> already FPI heap pages I concluded it wouldn't really be an improvement
> since it's only one ||log_heap_page or so there. Not sure what's
> better. Will write the patch and see how it goes.
Ended up using log_newpage_buffer since reusing visibilitymap_set's
record would break the wal format as we currently do not accept an FPI
on the heap pages during replay when < 9.3. Forcing to upgrade the
client first would be rather unfriendly...
That has the disadvantage of logging a full heap page since it doesn't
use the hole optimization but this happens really infrequently, so ...
I don't think this can happen < 9.2 in at least this code path. I wonder
though if there are other codepaths were this can happen although I
haven't found any on a cursory inspection.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
>From 62e1c6fc679af80f8f4d26f1fe9d2fae8f430100 Mon Sep 17 00:00:00 2001
From: Andres Freund
Date: Wed, 29 May 2013 15:37:05 +0200
Subject: [PATCH] Ensure that all_visible WAL records operate on an intialized
page
There was a race condition were lazy_scan_heap could cause all_visible records
to be emitted although the page marked all visible wasn't logged in an
initialized state yet causing a PANIC during WAL replay.
---
src/backend/commands/vacuumlazy.c | 25 +
1 file changed, 25 insertions(+)
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
index 9d30415..4eff3c1 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -663,6 +663,31 @@ lazy_scan_heap(Relation onerel, LVRelStats *vacrelstats,
/* empty pages are always all-visible */
if (!PageIsAllVisible(page))
{
+/*
+ * There's a race condition here we need to prevent: Another
+ * backend might have extended the heap and PageInit'ed but
+ * ERRORed before filling the page with actual content.
+ *
+ * Since relation extension is not WAL logged (and we don't
+ * even dirty the buffer) it's quite possible that the page's
+ * content hasn't made it to disk yet, especially on a
+ * standby. During WAL replay we then could find an
+ * uninitialized page when trying to set the all visible flag
+ * which will cause a PANIC lateron.
+ *
+ * If there already is a WAL record covering this page
+ * (i.e. its LSN is valid) the respective record it will
+ * initialize the page, so we don't need to do anything in that
+ * case.
+ *
+ * XXX: It would be nice to use a logging method supporting
+ * standard buffers here since log_newpage_buffer() will write
+ * the full block instead of omitting the hole.
+ */
+if (RelationNeedsWAL(onerel) &&
+ PageGetLSN(page) == InvalidXLogRecPtr)
+ log_newpage_buffer(buf);
+
PageSetAllVisible(page);
MarkBufferDirty(buf);
visibilitymap_set(onerel, blkno, buf, InvalidXLogRecPtr,
--
1.7.10.4
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] all_visible replay aborting due to uninitialized pages
On 2013-05-28 21:36:17 -0400, Robert Haas wrote: > On Tue, May 28, 2013 at 1:58 PM, Andres Freund wrote: > > Guessing around I looked and noticed the following problematic pattern: > > 1) A: wants to do an update, doesn't have enough freespace > > 2) A: extends the relation on the filesystem level > > (RelationGetBufferForTuple) > > 3) A: does PageInit (RelationGetBufferForTuple) > > 4) A: aborts, e.g. due to a serialization failure (heap_update) > > > > At this point the page is initialized in memory, but not wal logged. It > > isn't pinned or locked either. > > > > 5) B: vacuum finds that page and it's empty. So it marks it all > > visible. But since the page wasn't written out (we haven't even marked > > it dirty in 3.) the standby doesn't know that and reports the page as > > being uninitialized. > > > > ISTM the best backbranchable fix for this is to teach lazy_scan_heap to > > log an FPI for the heap page via visibilitymap_set in that rather > > limited case. > > > > Happy to provide a patch unless somebody has a better idea? > > Good catch. However, I would suggest using log_newpage() before > visibilitymap_set() rather than trying to stick extra logic into > visibilitymap_set(). I think that will be cleaner and simpler. Thought about that, but given that 9.3's visibilitymap_set already will already FPI heap pages I concluded it wouldn't really be an improvement since it's only one ||log_heap_page or so there. Not sure what's better. Will write the patch and see how it goes. 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
Re: [HACKERS] all_visible replay aborting due to uninitialized pages
On Tue, May 28, 2013 at 1:58 PM, Andres Freund wrote: > Guessing around I looked and noticed the following problematic pattern: > 1) A: wants to do an update, doesn't have enough freespace > 2) A: extends the relation on the filesystem level (RelationGetBufferForTuple) > 3) A: does PageInit (RelationGetBufferForTuple) > 4) A: aborts, e.g. due to a serialization failure (heap_update) > > At this point the page is initialized in memory, but not wal logged. It > isn't pinned or locked either. > > 5) B: vacuum finds that page and it's empty. So it marks it all > visible. But since the page wasn't written out (we haven't even marked > it dirty in 3.) the standby doesn't know that and reports the page as > being uninitialized. > > ISTM the best backbranchable fix for this is to teach lazy_scan_heap to > log an FPI for the heap page via visibilitymap_set in that rather > limited case. > > Happy to provide a patch unless somebody has a better idea? Good catch. However, I would suggest using log_newpage() before visibilitymap_set() rather than trying to stick extra logic into visibilitymap_set(). I think that will be cleaner and simpler. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] all_visible replay aborting due to uninitialized pages
Hi, A customer of ours reporting a standby loosing sync with the primary due to the following error: CONTEXT: xlog redo visible: rel 1663/XXX/XXX; blk 173717 WARNING: page 173717 of relation base/XXX/XXX is uninitialized ... PANIC: WAL contains references to invalid pages Guessing around I looked and noticed the following problematic pattern: 1) A: wants to do an update, doesn't have enough freespace 2) A: extends the relation on the filesystem level (RelationGetBufferForTuple) 3) A: does PageInit (RelationGetBufferForTuple) 4) A: aborts, e.g. due to a serialization failure (heap_update) At this point the page is initialized in memory, but not wal logged. It isn't pinned or locked either. 5) B: vacuum finds that page and it's empty. So it marks it all visible. But since the page wasn't written out (we haven't even marked it dirty in 3.) the standby doesn't know that and reports the page as being uninitialized. ISTM the best backbranchable fix for this is to teach lazy_scan_heap to log an FPI for the heap page via visibilitymap_set in that rather limited case. Happy to provide a patch unless somebody has a better idea? 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
