Rework handling of subtransactions in 2PC recovery

The bug fixed by 0874d4f
caused us to question and rework the handling of
subtransactions in 2PC during and at end of recovery.
Patch adds checks and tests to ensure no further bugs.

This effectively removes the temporary measure put in place
by 546c13e.

Author: Simon Riggs
Reviewed-by: Tom Lane, Michael Paquier
Discussion: http://postgr.es/m/CANP8+j+vvXmruL_i2buvdhMeVv5TQu0Hm2+C5N+kdVwHJuor8w@mail.gmail.com

Author: simonat2ndQuadrant

src/backend/access/transam/subtrans.c

@@ -68,32 +68,35 @@ static int	ZeroSUBTRANSPage(int64 pageno);
 
 /*
  * Record the parent of a subtransaction in the subtrans log.
- *
- * In some cases we may need to overwrite an existing value.
  */
 void
-SubTransSetParent(TransactionId xid, TransactionId parent, bool overwriteOK)
+SubTransSetParent(TransactionId xid, TransactionId parent)
 {
 	int64		pageno = TransactionIdToPage(xid);
 	int			entryno = TransactionIdToEntry(xid);
 	int			slotno;
 	TransactionId *ptr;
 
 	Assert(TransactionIdIsValid(parent));
+	Assert(TransactionIdFollows(xid, parent));
 
 	LWLockAcquire(SubtransControlLock, LW_EXCLUSIVE);
 
 	slotno = SimpleLruReadPage(SubTransCtl, pageno, true, xid);
 	ptr = (TransactionId *) SubTransCtl->shared->page_buffer[slotno];
 	ptr += entryno;
 
-	/* Current state should be 0 */
-	Assert(*ptr == InvalidTransactionId ||
-		   (*ptr == parent && overwriteOK));
-
-	*ptr = parent;
-
-	SubTransCtl->shared->page_dirty[slotno] = true;
+	/*
+	 * It's possible we'll try to set the parent xid multiple times
+	 * but we shouldn't ever be changing the xid from one valid xid
+	 * to another valid xid, which would corrupt the data structure.
+	 */
+	if (*ptr != parent)
+	{
+		Assert(*ptr == InvalidTransactionId);
+		*ptr = parent;
+		SubTransCtl->shared->page_dirty[slotno] = true;
+	}
 
 	LWLockRelease(SubtransControlLock);
 }
@@ -160,6 +163,15 @@ SubTransGetTopmostTransaction(TransactionId xid)
 		if (TransactionIdPrecedes(parentXid, TransactionXmin))
 			break;
 		parentXid = SubTransGetParent(parentXid);
+
+		/*
+		 * By convention the parent xid gets allocated first, so should
+		 * always precede the child xid. Anything else points to a corrupted
+		 * data structure that could lead to an infinite loop, so exit.
+		 */
+		if (!TransactionIdPrecedes(parentXid, previousXid))
+			elog(ERROR, "pg_subtrans contains invalid entry: xid %u points to parent xid %u",
+							previousXid, parentXid);
 	}
 
 	Assert(TransactionIdIsValid(previousXid));

src/backend/access/transam/twophase.c

@@ -303,8 +303,7 @@ static void RemoveGXact(GlobalTransaction gxact);
 static void XlogReadTwoPhaseData(XLogRecPtr lsn, char **buf, int *len);
 static char *ProcessTwoPhaseBuffer(TransactionId xid,
 							XLogRecPtr	prepare_start_lsn,
-							bool fromdisk, bool overwriteOK, bool setParent,
-							bool setNextXid);
+							bool fromdisk, bool setParent, bool setNextXid);
 static void MarkAsPreparingGuts(GlobalTransaction gxact, TransactionId xid,
 				const char *gid, TimestampTz prepared_at, Oid owner,
 				Oid databaseid);
@@ -1930,8 +1929,7 @@ restoreTwoPhaseData(void)
 			xid = (TransactionId) strtoul(clde->d_name, NULL, 16);
 
 			buf = ProcessTwoPhaseBuffer(xid, InvalidXLogRecPtr,
-										true, false, false,
-										false);
+										true, false, false);
 			if (buf == NULL)
 				continue;
 
@@ -1992,8 +1990,7 @@ PrescanPreparedTransactions(TransactionId **xids_p, int *nxids_p)
 
 		buf = ProcessTwoPhaseBuffer(xid,
 				gxact->prepare_start_lsn,
-				gxact->ondisk, false, false,
-				true);
+				gxact->ondisk, false, true);
 
 		if (buf == NULL)
 			continue;
@@ -2046,12 +2043,12 @@ PrescanPreparedTransactions(TransactionId **xids_p, int *nxids_p)
  * This is never called at the end of recovery - we use
  * RecoverPreparedTransactions() at that point.
  *
- * Currently we simply call SubTransSetParent() for any subxids of prepared
- * transactions. If overwriteOK is true, it's OK if some XIDs have already
- * been marked in pg_subtrans.
+ * The lack of calls to SubTransSetParent() calls here is by design;
+ * those calls are made by RecoverPreparedTransactions() at the end of recovery
+ * for those xacts that need this.
  */
 void
-StandbyRecoverPreparedTransactions(bool overwriteOK)
+StandbyRecoverPreparedTransactions(void)
 {
 	int			i;
 
@@ -2068,8 +2065,7 @@ StandbyRecoverPreparedTransactions(bool overwriteOK)
 
 		buf = ProcessTwoPhaseBuffer(xid,
 				gxact->prepare_start_lsn,
-				gxact->ondisk, overwriteOK, true,
-				false);
+				gxact->ondisk, false, false);
 		if (buf != NULL)
 			pfree(buf);
 	}
@@ -2082,8 +2078,7 @@ StandbyRecoverPreparedTransactions(bool overwriteOK)
  * Scan the shared memory entries of TwoPhaseState and reload the state for
  * each prepared transaction (reacquire locks, etc).
  *
- * This is run at the end of recovery, but before we allow backends to write
- * WAL.
+ * This is run during database startup.
  *
  * At the end of recovery the way we take snapshots will change. We now need
  * to mark all running transactions with their full SubTransSetParent() info
@@ -2107,15 +2102,21 @@ RecoverPreparedTransactions(void)
 		TwoPhaseFileHeader *hdr;
 		TransactionId *subxids;
 		const char *gid;
-		bool		overwriteOK = false;
-		int			i;
 
 		xid = gxact->xid;
 
+		/*
+		 * Reconstruct subtrans state for the transaction --- needed
+		 * because pg_subtrans is not preserved over a restart.  Note that
+		 * we are linking all the subtransactions directly to the
+		 * top-level XID; there may originally have been a more complex
+		 * hierarchy, but there's no need to restore that exactly.
+		 * It's possible that SubTransSetParent has been set before, if
+		 * the prepared transaction generated xid assignment records.
+		 */
 		buf = ProcessTwoPhaseBuffer(xid,
 				gxact->prepare_start_lsn,
-				gxact->ondisk, false, false,
-				false);
+				gxact->ondisk, true, false);
 		if (buf == NULL)
 			continue;
 
@@ -2133,25 +2134,6 @@ RecoverPreparedTransactions(void)
 		bufptr += MAXALIGN(hdr->nabortrels * sizeof(RelFileNode));
 		bufptr += MAXALIGN(hdr->ninvalmsgs * sizeof(SharedInvalidationMessage));
 
-		/*
-		 * It's possible that SubTransSetParent has been set before, if
-		 * the prepared transaction generated xid assignment records. Test
-		 * here must match one used in AssignTransactionId().
-		 */
-		if (InHotStandby && (hdr->nsubxacts >= PGPROC_MAX_CACHED_SUBXIDS ||
-							 XLogLogicalInfoActive()))
-			overwriteOK = true;
-
-		/*
-		 * Reconstruct subtrans state for the transaction --- needed
-		 * because pg_subtrans is not preserved over a restart.  Note that
-		 * we are linking all the subtransactions directly to the
-		 * top-level XID; there may originally have been a more complex
-		 * hierarchy, but there's no need to restore that exactly.
-		 */
-		for (i = 0; i < hdr->nsubxacts; i++)
-			SubTransSetParent(subxids[i], xid, true);
-
 		/*
 		 * Recreate its GXACT and dummy PGPROC. But, check whether
 		 * it was added in redo and already has a shmem entry for
@@ -2203,16 +2185,15 @@ RecoverPreparedTransactions(void)
  * Given a transaction id, read it either from disk or read it directly
  * via shmem xlog record pointer using the provided "prepare_start_lsn".
  *
- * If setParent is true, then use the overwriteOK parameter to set up
- * subtransaction parent linkages.
+ * If setParent is true, set up subtransaction parent linkages.
  *
  * 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 fromdisk,
 					  bool setParent, bool setNextXid)
 {
 	TransactionId origNextXid = ShmemVariableCache->nextXid;
@@ -2341,7 +2322,7 @@ ProcessTwoPhaseBuffer(TransactionId xid,
 		}
 
 		if (setParent)
-			SubTransSetParent(subxid, xid, overwriteOK);
+			SubTransSetParent(subxid, xid);
 	}
 
 	return buf;

src/backend/access/transam/xact.c

@@ -577,7 +577,7 @@ AssignTransactionId(TransactionState s)
 		XactTopTransactionId = s->transactionId;
 
 	if (isSubXact)
-		SubTransSetParent(s->transactionId, s->parent->transactionId, false);
+		SubTransSetParent(s->transactionId, s->parent->transactionId);
 
 	/*
 	 * If it's a top-level transaction, the predicate locking system needs to

src/backend/access/transam/xlog.c

@@ -6690,7 +6690,7 @@ StartupXLOG(void)
 
 				ProcArrayApplyRecoveryInfo(&running);
 
-				StandbyRecoverPreparedTransactions(false);
+				StandbyRecoverPreparedTransactions();
 			}
 		}
 
@@ -9356,7 +9356,7 @@ xlog_redo(XLogReaderState *record)
 
 			ProcArrayApplyRecoveryInfo(&running);
 
-			StandbyRecoverPreparedTransactions(true);
+			StandbyRecoverPreparedTransactions();
 		}
 
 		/* ControlFile->checkPointCopy always tracks the latest ckpt XID */

src/backend/storage/ipc/procarray.c

@@ -1012,7 +1012,7 @@ ProcArrayApplyXidAssignment(TransactionId topxid,
 	 * have attempted to SubTransSetParent().
 	 */
 	for (i = 0; i < nsubxids; i++)
-		SubTransSetParent(subxids[i], topxid, false);
+		SubTransSetParent(subxids[i], topxid);
 
 	/* KnownAssignedXids isn't maintained yet, so we're done for now */
 	if (standbyState == STANDBY_INITIALIZED)

src/include/access/subtrans.h

@@ -14,7 +14,7 @@
 /* Number of SLRU buffers to use for subtrans */
 #define NUM_SUBTRANS_BUFFERS	32
 
-extern void SubTransSetParent(TransactionId xid, TransactionId parent, bool overwriteOK);
+extern void SubTransSetParent(TransactionId xid, TransactionId parent);
 extern TransactionId SubTransGetParent(TransactionId xid);
 extern TransactionId SubTransGetTopmostTransaction(TransactionId xid);
 

src/include/access/twophase.h

@@ -57,7 +57,7 @@ extern TransactionId PrescanPreparedTransactions(TransactionId **xids_p,
 							int *nxids_p);
 extern void ParsePrepareRecord(uint8 info, char *xlrec,
 							xl_xact_parsed_prepare *parsed);
-extern void StandbyRecoverPreparedTransactions(bool overwriteOK);
+extern void StandbyRecoverPreparedTransactions(void);
 extern void RecoverPreparedTransactions(void);
 
 extern void CheckPointTwoPhase(XLogRecPtr redo_horizon);