Don’t push nextid too far forwards in recovery

simonat2ndQuadrant · kelvich · commit bfc79ee52ca8 · 2017-11-09T00:43:08.000+03:00
Doing so allows various crash possibilities. Fix by avoiding having PrescanPreparedTransactions() increment ShmemVariableCache->nextXid when it has no 2PC files Bug found by Jeff Janes, diagnosis and patch by Pavan Deolasee, then patch re-designed for clarity and full accuracy by Michael Paquier. Reported-by: Jeff Janes Author: Pavan Deolasee, Michael Paquier Discussion: https://postgr.es/m/CAMkU=1zMLnH_i1-PVQ-biZzvNx7VcuatriquEnh7HNk6K8Ss3Q@mail.gmail.com
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
@@ -304,7 +304,7 @@ static void XlogReadTwoPhaseData(XLogRecPtr lsn, char **buf, int *len);
 static char *ProcessTwoPhaseBuffer(TransactionId xid,
 							XLogRecPtr	prepare_start_lsn,
 							bool fromdisk, bool overwriteOK, bool setParent,
-							TransactionId *result, TransactionId *maxsubxid);
+							bool setNextXid);
 static void MarkAsPreparingGuts(GlobalTransaction gxact, TransactionId xid,
 				const char *gid, TimestampTz prepared_at, Oid owner,
 				Oid databaseid);
@@ -1931,7 +1931,7 @@ restoreTwoPhaseData(void)
 
 			buf = ProcessTwoPhaseBuffer(xid, InvalidXLogRecPtr,
 										true, false, false,
-										NULL, NULL);
+										false);
 			if (buf == NULL)
 				continue;
 
@@ -1974,7 +1974,6 @@ PrescanPreparedTransactions(TransactionId **xids_p, int *nxids_p)
 {
 	TransactionId origNextXid = ShmemVariableCache->nextXid;
 	TransactionId result = origNextXid;
-	TransactionId maxsubxid = origNextXid;
 	TransactionId *xids = NULL;
 	int			nxids = 0;
 	int			allocsize = 0;
@@ -1994,11 +1993,18 @@ PrescanPreparedTransactions(TransactionId **xids_p, int *nxids_p)
 		buf = ProcessTwoPhaseBuffer(xid,
 				gxact->prepare_start_lsn,
 				gxact->ondisk, false, false,
-				&result, &maxsubxid);
+				true);
 
 		if (buf == NULL)
 			continue;
 
+		/*
+		 * OK, we think this file is valid.  Incorporate xid into the
+		 * running-minimum result.
+		 */
+		if (TransactionIdPrecedes(xid, result))
+			result = xid;
+
 		if (xids_p)
 		{
 			if (nxids == allocsize)
@@ -2027,15 +2033,6 @@ PrescanPreparedTransactions(TransactionId **xids_p, int *nxids_p)
 		*nxids_p = nxids;
 	}
 
-	/* update nextXid if needed */
-	if (TransactionIdFollowsOrEquals(maxsubxid, ShmemVariableCache->nextXid))
-	{
-		LWLockAcquire(XidGenLock, LW_EXCLUSIVE);
-		ShmemVariableCache->nextXid = maxsubxid;
-		TransactionIdAdvance(ShmemVariableCache->nextXid);
-		LWLockRelease(XidGenLock);
-	}
-
 	return result;
 }
 
@@ -2072,7 +2069,7 @@ StandbyRecoverPreparedTransactions(bool overwriteOK)
 		buf = ProcessTwoPhaseBuffer(xid,
 				gxact->prepare_start_lsn,
 				gxact->ondisk, overwriteOK, true,
-				NULL, NULL);
+				false);
 		if (buf != NULL)
 			pfree(buf);
 	}
@@ -2118,7 +2115,7 @@ RecoverPreparedTransactions(void)
 		buf = ProcessTwoPhaseBuffer(xid,
 				gxact->prepare_start_lsn,
 				gxact->ondisk, false, false,
-				NULL, NULL);
+				false);
 		if (buf == NULL)
 			continue;
 
@@ -2209,20 +2206,16 @@ RecoverPreparedTransactions(void)
  * If setParent is true, then use the overwriteOK parameter to set up
  * subtransaction parent linkages.
  *
- * If result and maxsubxid are not NULL, fill them up with smallest
- * running transaction id (lesser than ShmemVariableCache->nextXid)
- * and largest subtransaction id for this transaction respectively.
+ * If setNextXid is true, set ShmemVariableCache->nextXid to the newest
+ * value scanned.
  */
 static char *
 ProcessTwoPhaseBuffer(TransactionId xid,
 					  XLogRecPtr prepare_start_lsn,
 					  bool fromdisk, bool overwriteOK,
-					  bool setParent, TransactionId *result,
-					  TransactionId *maxsubxid)
+					  bool setParent, bool setNextXid)
 {
 	TransactionId origNextXid = ShmemVariableCache->nextXid;
-	TransactionId res = InvalidTransactionId;
-	TransactionId maxsub = InvalidTransactionId;
 	TransactionId *subxids;
 	char	   *buf;
 	TwoPhaseFileHeader *hdr;
@@ -2233,11 +2226,6 @@ ProcessTwoPhaseBuffer(TransactionId xid,
 	if (!fromdisk)
 		Assert(prepare_start_lsn != InvalidXLogRecPtr);
 
-	if (result)
-		res = *result;
-	if (maxsubxid)
-		maxsub = *maxsubxid;
-
 	/* Already processed? */
 	if (TransactionIdDidCommit(xid) || TransactionIdDidAbort(xid))
 	{
@@ -2319,13 +2307,6 @@ ProcessTwoPhaseBuffer(TransactionId xid,
 		return NULL;
 	}
 
-	/*
-	 * OK, we think this buffer is valid.  Incorporate xid into the
-	 * running-minimum result.
-	 */
-	if (TransactionIdPrecedes(xid, res))
-		res = xid;
-
 	/*
 	 * Examine subtransaction XIDs ... they should all follow main
 	 * XID, and they may force us to advance nextXid.
@@ -2338,17 +2319,31 @@ ProcessTwoPhaseBuffer(TransactionId xid,
 		TransactionId subxid = subxids[i];
 
 		Assert(TransactionIdFollows(subxid, xid));
-		if (TransactionIdFollowsOrEquals(subxid, maxsub))
-			maxsub = subxid;
+
+		/* update nextXid if needed */
+		if (setNextXid &&
+			TransactionIdFollowsOrEquals(subxid,
+										 ShmemVariableCache->nextXid))
+		{
+			/*
+			 * We don't expect anyone else to modify nextXid, hence we don't
+			 * need to hold a lock while examining it.  We still acquire the
+			 * lock to modify it, though, so we recheck.
+			 */
+			LWLockAcquire(XidGenLock, LW_EXCLUSIVE);
+			if (TransactionIdFollowsOrEquals(subxid,
+										 ShmemVariableCache->nextXid))
+			{
+				ShmemVariableCache->nextXid = subxid;
+				TransactionIdAdvance(ShmemVariableCache->nextXid);
+			}
+			LWLockRelease(XidGenLock);
+		}
+
 		if (setParent)
 			SubTransSetParent(xid, subxid, overwriteOK);
 	}
 
-	if (result)
-		*result = res;
-	if (maxsubxid)
-		*maxsubxid = maxsub;
-
 	return buf;
 }
 
