v2-0001-Add-instrumentation-for-xmin-horizon-validation.patch

From 9bd76028a10a1bb9a55136a95344d72ca3e11b84 Mon Sep 17 00:00:00 2001
From: Matthias van de Meent <boekewurm+postgres@gmail.com>
Date: Fri, 5 Nov 2021 12:23:59 +0100
Subject: [PATCH v2] Add instrumentation for xmin horizon validation

We have an unwritten rule that a backend's xmin may not go back, but
this is not enforced. The added check is for assertion checking only, and
allows one to check before writing this xmin to the global procarray that
this xmin is actually valid and may be written with the current state.

Checks include consistency at the cluster- and database level, locking,
and status flags.
---
 src/backend/replication/logical/snapbuild.c |  13 +-
 src/backend/replication/walsender.c         |   6 +
 src/backend/storage/ipc/procarray.c         | 162 +++++++++++++++++++-
 src/backend/utils/time/snapmgr.c            |   3 +
 src/include/storage/procarray.h             |   6 +
 5 files changed, 171 insertions(+), 19 deletions(-)

diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index 6df602485b..5fd126748b 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -545,18 +545,7 @@ SnapBuildInitialSnapshot(SnapBuild *builder)
 	 * mechanism. Due to that we can do this without locks, we're only
 	 * changing our own value.
 	 */
-#ifdef USE_ASSERT_CHECKING
-	{
-		TransactionId safeXid;
-
-		LWLockAcquire(ProcArrayLock, LW_SHARED);
-		safeXid = GetOldestSafeDecodingTransactionId(false);
-		LWLockRelease(ProcArrayLock);
-
-		Assert(TransactionIdPrecedesOrEquals(safeXid, snap->xmin));
-	}
-#endif
-
+	CheckXminIsConsistent(snap->xmin, false);
 	MyProc->xmin = snap->xmin;
 
 	/* allocate in transaction context */
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 3224536356..212c455eaf 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -2171,9 +2171,15 @@ ProcessStandbyHSFeedbackMessage(void)
 	{
 		if (TransactionIdIsNormal(feedbackCatalogXmin)
 			&& TransactionIdPrecedes(feedbackCatalogXmin, feedbackXmin))
+		{
+			CheckXminIsConsistent(feedbackCatalogXmin, false);
 			MyProc->xmin = feedbackCatalogXmin;
+		}
 		else
+		{
+			CheckXminIsConsistent(feedbackXmin, false);
 			MyProc->xmin = feedbackXmin;
+		}
 	}
 }
 
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index 9fcd12020a..2395ee856d 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -355,6 +355,7 @@ static void MaintainLatestCompletedXidRecovery(TransactionId latestXid);
 static inline FullTransactionId FullXidRelativeTo(FullTransactionId rel,
 												  TransactionId xid);
 static void GlobalVisUpdateApply(ComputeXidHorizonsResult *horizons);
+static void ComputeXidHorizons(ComputeXidHorizonsResult *h, bool already_locked);
 
 /*
  * Report shared-memory space needed by CreateSharedProcArray.
@@ -1698,14 +1699,15 @@ TransactionIdIsActive(TransactionId xid)
  * doesn't expect (breaking HOT).
  */
 static void
-ComputeXidHorizons(ComputeXidHorizonsResult *h)
+ComputeXidHorizons(ComputeXidHorizonsResult *h, bool already_locked)
 {
 	ProcArrayStruct *arrayP = procArray;
 	TransactionId kaxmin;
 	bool		in_recovery = RecoveryInProgress();
 	TransactionId *other_xids = ProcGlobal->xids;
 
-	LWLockAcquire(ProcArrayLock, LW_SHARED);
+	if (!already_locked)
+		LWLockAcquire(ProcArrayLock, LW_SHARED);
 
 	h->latest_completed = ShmemVariableCache->latestCompletedXid;
 
@@ -1743,6 +1745,21 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
 			h->temp_oldest_nonremovable = MyProc->xid;
 		else
 			h->temp_oldest_nonremovable = initial;
+
+		/* Sanity checks: values are always >= our previous maybe_needed */
+		Assert(FullTransactionIdFollowsOrEquals(
+			FullXidRelativeTo(h->latest_completed, initial),
+			GlobalVisSharedRels.maybe_needed));
+		Assert(FullTransactionIdFollowsOrEquals(
+			FullXidRelativeTo(h->latest_completed, initial),
+			GlobalVisCatalogRels.maybe_needed));
+		Assert(FullTransactionIdFollowsOrEquals(
+			FullXidRelativeTo(h->latest_completed, initial),
+			GlobalVisDataRels.maybe_needed));
+		Assert(FullTransactionIdFollowsOrEquals(
+			FullXidRelativeTo(h->latest_completed,
+							  h->temp_oldest_nonremovable),
+			GlobalVisTempRels.maybe_needed));
 	}
 
 	/*
@@ -1795,6 +1812,11 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
 		if (statusFlags & (PROC_IN_VACUUM | PROC_IN_LOGICAL_DECODING))
 			continue;
 
+		/* Sanity check: xmin can not be lower than maybe_needed */
+		Assert(FullTransactionIdFollowsOrEquals(
+				   FullXidRelativeTo(h->latest_completed, xmin),
+				   GlobalVisSharedRels.maybe_needed));
+
 		/* shared tables need to take backends in all databases into account */
 		h->shared_oldest_nonremovable =
 			TransactionIdOlder(h->shared_oldest_nonremovable, xmin);
@@ -1822,8 +1844,19 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
 			 * only on vacuums of user-defined tables.
 			 */
 			if (!(statusFlags & PROC_IN_SAFE_IC))
+			{
+				/* Sanity check: xmin can not be lower than maybe_needed */
+				Assert(FullTransactionIdFollowsOrEquals(
+						   FullXidRelativeTo(h->latest_completed, xmin),
+						   GlobalVisDataRels.maybe_needed));
 				h->data_oldest_nonremovable =
 					TransactionIdOlder(h->data_oldest_nonremovable, xmin);
+			}
+
+			/* Sanity check: xmin can not be lower than maybe_needed */
+			Assert(FullTransactionIdFollowsOrEquals(
+				FullXidRelativeTo(h->latest_completed, xmin),
+				GlobalVisCatalogRels.maybe_needed));
 
 			/* Catalog tables need to consider all backends in this db */
 			h->catalog_oldest_nonremovable =
@@ -1847,7 +1880,8 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
 	 * No other information from shared state is needed, release the lock
 	 * immediately. The rest of the computations can be done without a lock.
 	 */
-	LWLockRelease(ProcArrayLock);
+	if (!already_locked)
+		LWLockRelease(ProcArrayLock);
 
 	if (in_recovery)
 	{
@@ -2006,7 +2040,7 @@ GetOldestNonRemovableTransactionId(Relation rel)
 {
 	ComputeXidHorizonsResult horizons;
 
-	ComputeXidHorizons(&horizons);
+	ComputeXidHorizons(&horizons, false);
 
 	switch (GlobalVisHorizonKindForRel(rel))
 	{
@@ -2034,7 +2068,7 @@ GetOldestTransactionIdConsideredRunning(void)
 {
 	ComputeXidHorizonsResult horizons;
 
-	ComputeXidHorizons(&horizons);
+	ComputeXidHorizons(&horizons, false);
 
 	return horizons.oldest_considered_running;
 }
@@ -2047,7 +2081,7 @@ GetReplicationHorizons(TransactionId *xmin, TransactionId *catalog_xmin)
 {
 	ComputeXidHorizonsResult horizons;
 
-	ComputeXidHorizons(&horizons);
+	ComputeXidHorizons(&horizons, false);
 
 	/*
 	 * Don't want to use shared_oldest_nonremovable here, as that contains the
@@ -2153,7 +2187,10 @@ GetSnapshotDataReuse(Snapshot snapshot)
 	 * xmin.
 	 */
 	if (!TransactionIdIsValid(MyProc->xmin))
+	{
+		CheckXminIsConsistent(snapshot->xmin, true);
 		MyProc->xmin = TransactionXmin = snapshot->xmin;
+	}
 
 	RecentXmin = snapshot->xmin;
 	Assert(TransactionIdPrecedesOrEquals(TransactionXmin, RecentXmin));
@@ -2441,7 +2478,10 @@ GetSnapshotData(Snapshot snapshot)
 	replication_slot_catalog_xmin = procArray->replication_slot_catalog_xmin;
 
 	if (!TransactionIdIsValid(MyProc->xmin))
+	{
+		CheckXminIsConsistent(xmin, true);
 		MyProc->xmin = TransactionXmin = xmin;
+	}
 
 	LWLockRelease(ProcArrayLock);
 
@@ -2619,6 +2659,7 @@ ProcArrayInstallImportedXmin(TransactionId xmin,
 		 * GetSnapshotData first, we'll be overwriting a valid xmin here, so
 		 * we don't check that.)
 		 */
+		CheckXminIsConsistent(xmin, true);
 		MyProc->xmin = TransactionXmin = xmin;
 
 		result = true;
@@ -2662,6 +2703,8 @@ ProcArrayInstallRestoredXmin(TransactionId xmin, PGPROC *proc)
 		TransactionIdIsNormal(xid) &&
 		TransactionIdPrecedesOrEquals(xid, xmin))
 	{
+		/* restore xmin */
+		CheckXminIsConsistent(xmin, true);
 		MyProc->xmin = TransactionXmin = xmin;
 		result = true;
 	}
@@ -4086,6 +4129,23 @@ GlobalVisTestShouldUpdate(GlobalVisState *state)
 static void
 GlobalVisUpdateApply(ComputeXidHorizonsResult *horizons)
 {
+	Assert(FullTransactionIdFollowsOrEquals(
+		FullXidRelativeTo(horizons->latest_completed,
+						  horizons->shared_oldest_nonremovable),
+		GlobalVisSharedRels.maybe_needed));
+	Assert(FullTransactionIdFollowsOrEquals(
+		FullXidRelativeTo(horizons->latest_completed,
+						  horizons->catalog_oldest_nonremovable),
+		GlobalVisCatalogRels.maybe_needed));
+	Assert(FullTransactionIdFollowsOrEquals(
+		FullXidRelativeTo(horizons->latest_completed,
+						  horizons->data_oldest_nonremovable),
+		GlobalVisDataRels.maybe_needed));
+	Assert(FullTransactionIdFollowsOrEquals(
+		FullXidRelativeTo(horizons->latest_completed,
+						  horizons->temp_oldest_nonremovable),
+		GlobalVisTempRels.maybe_needed));
+
 	GlobalVisSharedRels.maybe_needed =
 		FullXidRelativeTo(horizons->latest_completed,
 						  horizons->shared_oldest_nonremovable);
@@ -4128,7 +4188,7 @@ GlobalVisUpdate(void)
 	ComputeXidHorizonsResult horizons;
 
 	/* updates the horizons as a side-effect */
-	ComputeXidHorizons(&horizons);
+	ComputeXidHorizons(&horizons, false);
 }
 
 /*
@@ -5132,3 +5192,91 @@ KnownAssignedXidsReset(void)
 
 	LWLockRelease(ProcArrayLock);
 }
+
+#ifdef USE_ASSERT_CHECKING
+/*
+ * CheckXminIsConsistent
+ *		Checks that an xmin is consistent with the current state of
+ *		the PGPROC infrastructure.
+ *
+ * Any xmin that is set on MyProc (or any other proc, really) must be
+ * consistent with the current state of the pgproc infrastructure, i.e. it may
+ * not move the globally minimum considered running xid backwards. Results
+ * from transactions older than the oldest considered running transaction may
+ * be removed or otherwise modified through e.g. vacuum, whereas results from
+ * transactions that are considered running may not yet be removed and are
+ * thus safe from removal.
+ *
+ * Basically, we assert that with this xmin we would not move the GlobalVis*
+ * horizons backwards.
+ */
+void
+CheckXminIsConsistent(TransactionId xmin, bool already_locked)
+{
+	ComputeXidHorizonsResult result;
+
+	/*
+	 * There can be reasons for xmins to be incorrectly registered under
+	 * non-postmaster environments, whilst still retaining overall
+	 * consistency. However, when under postmaster, data consistency cannot
+	 * be guaranteed unless all xmins are correctly registered while
+	 * that xmin is still inside the oldest_considered_running window.
+	 */
+	if (!IsPostmasterEnvironment)
+		return;
+
+	/* Resetting xmin is generally safe */
+	if (!TransactionIdIsValid(xmin))
+	{
+		/* We cannot set unset xmin without also having no xid */
+		Assert(!TransactionIdIsValid(MyProc->xid));
+		return;
+	}
+	/* Moving our own xmin forward is always safe */
+	if (TransactionIdIsValid(MyProc->xmin) &&
+			TransactionIdFollowsOrEquals(xmin, MyProc->xmin))
+		return;
+
+	/*
+	 * For the following conditions, we can only update MyProc->xmin
+	 * while we hold the ProcArrayLock: we have to be certain that the
+	 * xmin that we're installing is still inside the visible range
+	 * for this cluster. For any caller, it is impossible to guarantee
+	 * that the origin of their xmin is still valid when they call
+	 * this function unless they hold a lock on the Proc array.
+	 *
+	 * Note that the lock held may be at both LW_SHARED and LW_EXCLUSIVE
+	 * granularity: The xmin we're installing will not lower the visibility
+	 * horizon for any running backend, and therefore shouldn't impact
+	 * concurrently generated results.
+	 */
+	Assert(already_locked);
+	Assert(LWLockHeldByMe(ProcArrayLock));
+
+	ComputeXidHorizons(&result, already_locked);
+
+	/*
+	 * xmin may not be older than the current oldest transaction
+	 */
+	Assert(TransactionIdFollowsOrEquals(xmin, result.oldest_considered_running));
+
+	/*
+	 * If we don't have a special status, xmin must also not be older
+	 * than the oldest removable xid for the database that this
+	 * backend is connected to.
+	 */
+	if (MyProc->databaseId == InvalidOid)
+		return;
+
+	if (MyProc->statusFlags & PROC_IN_VACUUM)
+	{
+		Assert(TransactionIdFollowsOrEquals(xmin, result.catalog_oldest_nonremovable));
+	}
+	else if (MyProc->statusFlags & PROC_IN_SAFE_IC)
+	{
+		Assert(TransactionIdFollowsOrEquals(xmin, result.catalog_oldest_nonremovable));
+	}
+	else
+		Assert(TransactionIdFollowsOrEquals(xmin, result.data_oldest_nonremovable));
+}
+#endif /* USE_ASSERT_CHECKING */
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index dca1bc8afc..3f3641298f 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -954,7 +954,10 @@ SnapshotResetXmin(void)
 										pairingheap_first(&RegisteredSnapshots));
 
 	if (TransactionIdPrecedes(MyProc->xmin, minSnapshot->xmin))
+	{
+		CheckXminIsConsistent(minSnapshot->xmin, false);
 		MyProc->xmin = minSnapshot->xmin;
+	}
 }
 
 /*
diff --git a/src/include/storage/procarray.h b/src/include/storage/procarray.h
index b01fa52139..cca407c859 100644
--- a/src/include/storage/procarray.h
+++ b/src/include/storage/procarray.h
@@ -94,4 +94,10 @@ extern void ProcArraySetReplicationSlotXmin(TransactionId xmin,
 extern void ProcArrayGetReplicationSlotXmin(TransactionId *xmin,
 											TransactionId *catalog_xmin);
 
+#ifdef USE_ASSERT_CHECKING
+extern void CheckXminIsConsistent(TransactionId xmin, bool already_locked);
+#else /* USE_ASSERT_CHECKING */
+#define CheckXminIsConsistent(xmin, already_locked) ((void) 0)
+#endif /* USE_ASSERT_CHECKING */
+
 #endif							/* PROCARRAY_H */
-- 
2.30.2