Re: [HACKERS] Crash on promotion when recovery.conf is renamed

[Michael Paquier]

On Fri, Jul 06, 2018 at 09:09:27PM +0900, Michael Paquier wrote:
> Sure.  I think I can finish the set on Monday JST then.

So, I have been able to back-patch things down to 9.5, but further down
I am not really convinced that it is worth meddling with that,
particularly in light of 7cbee7c which has reworked the way partial
segments on older timelines are handled at the end of promotion.  The
portability issues I have found is related to the timeline number
exitArchiveRecovery uses which comes for the WAL reader hence this gets
set to the timeline from the last page read by the startup process.
This can actually cause quite a bit of trouble at the end of recovery
as we would get a failure when trying to copy the last segment from the
old timeline to the new timeline.  Well, it could be possible to fix
things properly by roughly back-porting 7cbee7c down to REL9_4_STABLE
but that's not really worth the risk, and moving exitArchiveRecovery()
and PrescanPreparedTransactions() around is a straight-forward move with
9.5~ thanks to this commit.

I have also done nothing yet for the detection of corrupted 2PC files
which get ignored at recovery.  While looking again at the patch I sent
upthread, the thing was actually missing some more error handling in
ReadTwoPhaseFile().  In particular, with the proposed patch we would
still not report an error if a 2PC file cannot be opened because of for
example EPERM.  In FinishPreparedTransaction, one would example get a
simply a *crash* with no hints about what happened.  I have a patch for
that which still needs some polishing, and I will start a new thread on
the matter.
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] Crash on promotion when recovery.conf is renamed

[Michael Paquier]

On Fri, Jul 06, 2018 at 08:05:50AM -0400, Andrew Dunstan wrote:
> On 07/06/2018 07:18 AM, Michael Paquier wrote:
> > For what it's worth, I volunteer to finish the work :)
> > 
> > The 2PC patch is really simple, and fixes a data loss issue.  The second
> > patch has been looked up by Heikki, Magnus and me at least once by each,
> > and there is visibly an agreement on having it.  Having reviews after
> > a new patch version is sent, by somebody else than the one who sent the
> > patches is of course always nice..
> 
> If you're comfortable committing it then go for it. It will be good to have
> the CF item resolved.

Sure.  I think I can finish the set on Monday JST then.
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] Crash on promotion when recovery.conf is renamed

[Andrew Dunstan]

On 07/06/2018 07:18 AM, Michael Paquier wrote:

Robert Haas wrote:

Although the state is now back to "Needs Review", I echo those
sentiments. This issue has now been hanging around for about 18
months.




No, those are my words, not Robert's :-)



For what it's worth, I volunteer to finish the work :)

The 2PC patch is really simple, and fixes a data loss issue.  The second
patch has been looked up by Heikki, Magnus and me at least once by each,
and there is visibly an agreement on having it.  Having reviews after
a new patch version is sent, by somebody else than the one who sent the
patches is of course always nice..


If you're comfortable committing it then go for it. It will be good to 
have the CF item resolved.


cheers

andrew

--
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Crash on promotion when recovery.conf is renamed

[Michael Paquier]

Robert Haas wrote:
> Although the state is now back to "Needs Review", I echo those
> sentiments. This issue has now been hanging around for about 18
> months.

For what it's worth, I volunteer to finish the work :)

The 2PC patch is really simple, and fixes a data loss issue.  The second
patch has been looked up by Heikki, Magnus and me at least once by each,
and there is visibly an agreement on having it.  Having reviews after
a new patch version is sent, by somebody else than the one who sent the
patches is of course always nice.. 
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] Crash on promotion when recovery.conf is renamed

[Andrew Dunstan]

On Thu, Jun 28, 2018 at 1:39 AM, Michael Paquier  wrote:
> On Thu, Jan 11, 2018 at 10:35:22PM -0500, Stephen Frost wrote:
>> Magnus, this was your thread to begin with, though I know others have
>> been involved, any chance you'll be able to review this for commit
>> during this CF?  I agree that this is certainly a good thing to have
>> too, though I've not looked at the patch itself in depth.  Is there
>> anything we can do to help move it along?
>
> As an effort to move on with bug items in the commit fest, attached are
> two patches with a proposed commit message as well as polished comments
> Those are proposed for a back-patched.  The 2PC issue is particularly
> bad in my opinion because having any 2PC file on-disk and corrupted
> means that a transaction is lost.  I have been playing a bit with
> hexedit and changed a couple of bytes in one of them...  If trying to
> use a base backup which includes one, then the standby reading it would
> be similarly confused.

Thanks to Michael for progressing this.

Back in August, nearly a year ago, Robert Haas said upthread:

> This bug fix has been pending in "Ready for Committer" state for about
> 4.5 months. Three committers (Magnus, Heikki, Tom) have contributed
> to the thread to date. Maybe one of them would like to commit this?

Although the state is now back to "Needs Review", I echo those
sentiments. This issue has now been hanging around for about 18
months.

cheers

andrew

-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Crash on promotion when recovery.conf is renamed

[Michael Paquier]

On Thu, Jan 11, 2018 at 10:35:22PM -0500, Stephen Frost wrote:
> Magnus, this was your thread to begin with, though I know others have
> been involved, any chance you'll be able to review this for commit
> during this CF?  I agree that this is certainly a good thing to have
> too, though I've not looked at the patch itself in depth.  Is there
> anything we can do to help move it along?

As an effort to move on with bug items in the commit fest, attached are
two patches with a proposed commit message as well as polished comments
Those are proposed for a back-patched.  The 2PC issue is particularly
bad in my opinion because having any 2PC file on-disk and corrupted
means that a transaction is lost.  I have been playing a bit with
hexedit and changed a couple of bytes in one of them...  If trying to
use a base backup which includes one, then the standby reading it would
be similarly confused.
--
Michael
From d12621c1e03abf8876d944ea3e831213111f2909 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Thu, 28 Jun 2018 14:38:24 +0900
Subject: [PATCH 1/2] Fail hard when facing corrupted two-phase state files

When a corrupted file is found by WAL replay, be it for crash recovery
or archive recovery, then the file is simply skipped and a WARNING is
logged to the user.  Facing an on-disk WAL file which is corrupted is
more likely to happen than its pair recorded in dedicated WAL records,
but if that happens then the instance faces data loss as the transaction
is not around anymore as it is not possible to commit it.

Reported-by: Michael Paquier
Author: Michael Paquier
Discussion: https://postgr.es/m/20161216060832.gb17...@paquier.xyz
---
 src/backend/access/transam/twophase.c | 28 ++-
 src/backend/access/transam/xlog.c | 10 +++---
 2 files changed, 17 insertions(+), 21 deletions(-)

diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index a9ef1b3d73..2da3d93d87 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1873,6 +1873,10 @@ restoreTwoPhaseData(void)
  * write a WAL entry, and so there might be no evidence in WAL of those
  * subxact XIDs.
  *
+ * On corrupted two-phase files, fail immediately. Keeping around broken
+ * entries and let replay continue causes harm on the system, and a new
+ * backup should be rolled in.
+ *
  * Our other responsibility is to determine and return the oldest valid XID
  * among the prepared xacts (if none, return ShmemVariableCache->nextXid).
  * This is needed to synchronize pg_subtrans startup properly.
@@ -2165,13 +2169,9 @@ ProcessTwoPhaseBuffer(TransactionId xid,
 		/* Read and validate file */
 		buf = ReadTwoPhaseFile(xid, true);
 		if (buf == NULL)
-		{
-			ereport(WARNING,
-	(errmsg("removing corrupt two-phase state file for transaction %u",
+			ereport(FATAL,
+	(errmsg("corrupted two-phase state file for \"%u\"",
 			xid)));
-			RemoveTwoPhaseFile(xid, true);
-			return NULL;
-		}
 	}
 	else
 	{
@@ -2184,21 +2184,13 @@ ProcessTwoPhaseBuffer(TransactionId xid,
 	if (!TransactionIdEquals(hdr->xid, xid))
 	{
 		if (fromdisk)
-		{
-			ereport(WARNING,
-	(errmsg("removing corrupt two-phase state file for transaction %u",
+			ereport(FATAL,
+	(errmsg("corrupted two-phase state file for \"%u\"",
 			xid)));
-			RemoveTwoPhaseFile(xid, true);
-		}
 		else
-		{
-			ereport(WARNING,
-	(errmsg("removing corrupt two-phase state from memory for transaction %u",
+			ereport(FATAL,
+	(errmsg("corrupted two-phase state in memory for \"%u\"",
 			xid)));
-			PrepareRedoRemove(xid, true);
-		}
-		pfree(buf);
-		return NULL;
 	}
 
 	/*
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 1a419aa49b..3695258e6f 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -7462,6 +7462,13 @@ StartupXLOG(void)
 		}
 	}
 
+	/*
+	 * Pre-scan prepared transactions to find out the range of XIDs present.
+	 * This information is not quite needed yet, but it is positioned here so
+	 * as potential problems are detected before any on-disk change is done.
+	 */
+	oldestActiveXID = PrescanPreparedTransactions(NULL, NULL);
+
 	/*
 	 * Consider whether we need to assign a new timeline ID.
 	 *
@@ -7585,9 +7592,6 @@ StartupXLOG(void)
 	XLogCtl->LogwrtRqst.Write = EndOfLog;
 	XLogCtl->LogwrtRqst.Flush = EndOfLog;
 
-	/* Pre-scan prepared transactions to find out the range of XIDs present */
-	oldestActiveXID = PrescanPreparedTransactions(NULL, NULL);
-
 	/*
 	 * Update full_page_writes in shared memory and write an XLOG_FPW_CHANGE
 	 * record before resource manager writes cleanup WAL records or checkpoint
-- 
2.18.0

From b2862294f52b7262d7ec0cb2379688b2176169ea Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Thu, 28 Jun 2018 14:38:50 +0900
Subject: [PATCH 2/2] Minimize window between history file and end-of-recovery
 record

Once a standby node is promoted, this makes the 

Re: [HACKERS] Crash on promotion when recovery.conf is renamed

[Thomas Munro]

On Fri, Jan 12, 2018 at 4:35 PM, Stephen Frost  wrote:
> Appears to compile and pass make check-world still (not sure why Thomas'
> bot currently thinks the build fails, since it worked here..).

It looks good now.  There was a brief window when all Travis CI builds
said this while updating various packages at startup:

W: GPG error: http://repo.mongodb.org/apt/ubuntu
trusty/mongodb-org/3.4 Release: The following signatures were invalid:
KEYEXPIRED 1515625755
W: The repository 'http://repo.mongodb.org/apt/ubuntu
trusty/mongodb-org/3.4 Release' is not signed.

That's because the apt.sources happens to have that repository in it.
I didn't look into it because it fixed itself at the next rebuild.  I
speculate that MongoDB's package repository is eventually consistent.

-- 
Thomas Munro
http://www.enterprisedb.com

Re: [HACKERS] Crash on promotion when recovery.conf is renamed

[Stephen Frost]

Magnus,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Mon, Oct 2, 2017 at 3:29 PM, Michael Paquier
>  wrote:
> > On Mon, Oct 2, 2017 at 8:13 AM, Daniel Gustafsson  wrote:
> >> I’ve moved this to the next CF, but since this no longer applies cleanly 
> >> I’ve
> >> reset it to Waiting for author.
> >
> > Thanks Daniel for the reminder. Attached are rebased patches. This
> > thing rots easily...
> 
> This set of patches still applies, and is marked as ready for
> committer. Are any of the committers cited on this thread, aka Magnus,
> Heikki, Tom interested in this patch set? Or not? We are close to the
> end of CF 2017-11, so I am bumping it to the next one.

Magnus, this was your thread to begin with, though I know others have
been involved, any chance you'll be able to review this for commit
during this CF?  I agree that this is certainly a good thing to have
too, though I've not looked at the patch itself in depth.  Is there
anything we can do to help move it along?

Appears to compile and pass make check-world still (not sure why Thomas'
bot currently thinks the build fails, since it worked here..).

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] Crash on promotion when recovery.conf is renamed

[Michael Paquier]

On Mon, Oct 2, 2017 at 3:29 PM, Michael Paquier
 wrote:
> On Mon, Oct 2, 2017 at 8:13 AM, Daniel Gustafsson  wrote:
>> I’ve moved this to the next CF, but since this no longer applies cleanly I’ve
>> reset it to Waiting for author.
>
> Thanks Daniel for the reminder. Attached are rebased patches. This
> thing rots easily...

This set of patches still applies, and is marked as ready for
committer. Are any of the committers cited on this thread, aka Magnus,
Heikki, Tom interested in this patch set? Or not? We are close to the
end of CF 2017-11, so I am bumping it to the next one.
-- 
Michael