From 2d6b372cf61782e0fd52590b57b1c914b0ed7a4c Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathandbossart@gmail.com>
Date: Tue, 22 Mar 2022 15:35:34 -0700
Subject: [PATCH v4 1/1] disallow XMAX_COMMITTED + XMAX_LOCK_ONLY

---
 contrib/amcheck/verify_heapam.c             | 19 ++++
 src/backend/access/heap/README.tuplock      |  2 +-
 src/backend/access/heap/heapam.c            | 10 +++
 src/backend/access/heap/heapam_visibility.c | 97 ++++++++++++++-------
 src/backend/access/heap/hio.c               |  2 +
 5 files changed, 96 insertions(+), 34 deletions(-)

diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index e5f7355dcb..f30b9c234f 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -665,6 +665,25 @@ check_tuple_header(HeapCheckContext *ctx)
 		 */
 	}
 
+	if ((ctx->tuphdr->t_infomask & HEAP_XMAX_COMMITTED) &&
+		(ctx->tuphdr->t_infomask & HEAP_XMAX_LOCK_ONLY))
+	{
+		report_corruption(ctx,
+						  pstrdup("locked-only should not be marked committed"));
+
+		/* As above, do not skip further checks. */
+	}
+
+	/* also check for pre-v9.3 lock-only bit pattern */
+	if ((ctx->tuphdr->t_infomask & HEAP_XMAX_COMMITTED) &&
+		HEAP_XMAX_IS_LOCKED_ONLY(ctx->tuphdr->t_infomask))
+	{
+		report_corruption(ctx,
+						  pstrdup("tuple with HEAP_XMAX_EXCL_LOCK set and HEAP_XMAX_IS_MULTI unset should not be marked committed"));
+
+		/* As above, do not skip further checks. */
+	}
+
 	if (infomask & HEAP_HASNULL)
 		expected_hoff = MAXALIGN(SizeofHeapTupleHeader + BITMAPLEN(ctx->natts));
 	else
diff --git a/src/backend/access/heap/README.tuplock b/src/backend/access/heap/README.tuplock
index 6441e8baf0..4e565e60ab 100644
--- a/src/backend/access/heap/README.tuplock
+++ b/src/backend/access/heap/README.tuplock
@@ -152,4 +152,4 @@ The following infomask bits are applicable:
   whether the XMAX is a TransactionId or a MultiXactId.
 
 We currently never set the HEAP_XMAX_COMMITTED when the HEAP_XMAX_IS_MULTI bit
-is set.
+or the HEAP_XMAX_LOCK_ONLY bit is set.
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 3746336a09..3f0b4cd754 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -5151,6 +5151,16 @@ l5:
 		MultiXactStatus status;
 		MultiXactStatus new_status;
 
+		/*
+		 * Currently we don't allow XMAX_LOCK_ONLY and XMAX_COMMITTED to both be
+		 * set in a tuple header, so cross-check.
+		 *
+		 * Note that this also checks for the special locked-only bit pattern
+		 * that may be present on servers that were pg_upgraded from pre-v9.3
+		 * versions.
+		 */
+		Assert(!HEAP_XMAX_IS_LOCKED_ONLY(old_infomask));
+
 		if (old_infomask2 & HEAP_KEYS_UPDATED)
 			status = MultiXactStatusUpdate;
 		else
diff --git a/src/backend/access/heap/heapam_visibility.c b/src/backend/access/heap/heapam_visibility.c
index ff0b8a688d..7d6fb66b0d 100644
--- a/src/backend/access/heap/heapam_visibility.c
+++ b/src/backend/access/heap/heapam_visibility.c
@@ -78,6 +78,31 @@
 #include "utils/snapmgr.h"
 
 
+/*
+ * InfomaskAssertions()
+ *
+ * Checks for invalid infomask bit patterns.
+ */
+static inline void
+InfomaskAssertions(HeapTupleHeader tuple)
+{
+	/*
+	 * XMAX_COMMITTED and XMAX_LOCK_ONLY cannot both be set in a tuple header.
+	 *
+	 * Note that this also checks for the special locked-only bit pattern that
+	 * may be present on servers that were pg_upgraded from pre-v9.3 versions.
+	 */
+	Assert(!((tuple->t_infomask & HEAP_XMAX_COMMITTED) &&
+			 HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask)));
+
+	/*
+	 * XMAX_COMMITTED and XMAX_IS_MULTI cannot be be set in a tuple header.
+	 */
+	Assert(!((tuple->t_infomask & HEAP_XMAX_COMMITTED) &&
+			 (tuple->t_infomask & HEAP_XMAX_IS_MULTI)));
+}
+
+
 /*
  * SetHintBits()
  *
@@ -113,6 +138,8 @@ static inline void
 SetHintBits(HeapTupleHeader tuple, Buffer buffer,
 			uint16 infomask, TransactionId xid)
 {
+	InfomaskAssertions(tuple);
+
 	if (TransactionIdIsValid(xid))
 	{
 		/* NB: xid must be known committed here! */
@@ -127,6 +154,7 @@ SetHintBits(HeapTupleHeader tuple, Buffer buffer,
 	}
 
 	tuple->t_infomask |= infomask;
+	InfomaskAssertions(tuple);	/* we check again after infomask updates */
 	MarkBufferDirtyHint(buffer, true);
 }
 
@@ -172,6 +200,7 @@ HeapTupleSatisfiesSelf(HeapTuple htup, Snapshot snapshot, Buffer buffer)
 
 	Assert(ItemPointerIsValid(&htup->t_self));
 	Assert(htup->t_tableOid != InvalidOid);
+	InfomaskAssertions(tuple);
 
 	if (!HeapTupleHeaderXminCommitted(tuple))
 	{
@@ -271,11 +300,7 @@ HeapTupleSatisfiesSelf(HeapTuple htup, Snapshot snapshot, Buffer buffer)
 		return true;
 
 	if (tuple->t_infomask & HEAP_XMAX_COMMITTED)
-	{
-		if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
-			return true;
 		return false;			/* updated by other */
-	}
 
 	if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
 	{
@@ -365,6 +390,7 @@ HeapTupleSatisfiesToast(HeapTuple htup, Snapshot snapshot,
 
 	Assert(ItemPointerIsValid(&htup->t_self));
 	Assert(htup->t_tableOid != InvalidOid);
+	InfomaskAssertions(tuple);
 
 	if (!HeapTupleHeaderXminCommitted(tuple))
 	{
@@ -461,6 +487,7 @@ HeapTupleSatisfiesUpdate(HeapTuple htup, CommandId curcid,
 
 	Assert(ItemPointerIsValid(&htup->t_self));
 	Assert(htup->t_tableOid != InvalidOid);
+	InfomaskAssertions(tuple);
 
 	if (!HeapTupleHeaderXminCommitted(tuple))
 	{
@@ -605,8 +632,6 @@ HeapTupleSatisfiesUpdate(HeapTuple htup, CommandId curcid,
 
 	if (tuple->t_infomask & HEAP_XMAX_COMMITTED)
 	{
-		if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
-			return TM_Ok;
 		if (!ItemPointerEquals(&htup->t_self, &tuple->t_ctid))
 			return TM_Updated;	/* updated by other */
 		else
@@ -746,6 +771,7 @@ HeapTupleSatisfiesDirty(HeapTuple htup, Snapshot snapshot,
 
 	Assert(ItemPointerIsValid(&htup->t_self));
 	Assert(htup->t_tableOid != InvalidOid);
+	InfomaskAssertions(tuple);
 
 	snapshot->xmin = snapshot->xmax = InvalidTransactionId;
 	snapshot->speculativeToken = 0;
@@ -866,11 +892,7 @@ HeapTupleSatisfiesDirty(HeapTuple htup, Snapshot snapshot,
 		return true;
 
 	if (tuple->t_infomask & HEAP_XMAX_COMMITTED)
-	{
-		if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
-			return true;
 		return false;			/* updated by other */
-	}
 
 	if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
 	{
@@ -963,6 +985,7 @@ HeapTupleSatisfiesMVCC(HeapTuple htup, Snapshot snapshot,
 
 	Assert(ItemPointerIsValid(&htup->t_self));
 	Assert(htup->t_tableOid != InvalidOid);
+	InfomaskAssertions(tuple);
 
 	if (!HeapTupleHeaderXminCommitted(tuple))
 	{
@@ -1164,6 +1187,8 @@ HeapTupleSatisfiesVacuum(HeapTuple htup, TransactionId OldestXmin,
 	TransactionId dead_after = InvalidTransactionId;
 	HTSV_Result res;
 
+	InfomaskAssertions(htup->t_data);
+
 	res = HeapTupleSatisfiesVacuumHorizon(htup, buffer, &dead_after);
 
 	if (res == HEAPTUPLE_RECENTLY_DEAD)
@@ -1199,6 +1224,7 @@ HeapTupleSatisfiesVacuumHorizon(HeapTuple htup, Buffer buffer, TransactionId *de
 	Assert(ItemPointerIsValid(&htup->t_self));
 	Assert(htup->t_tableOid != InvalidOid);
 	Assert(dead_after != NULL);
+	InfomaskAssertions(tuple);
 
 	*dead_after = InvalidTransactionId;
 
@@ -1306,31 +1332,28 @@ HeapTupleSatisfiesVacuumHorizon(HeapTuple htup, Buffer buffer, TransactionId *de
 	{
 		/*
 		 * "Deleting" xact really only locked it, so the tuple is live in any
-		 * case.  However, we should make sure that either XMAX_COMMITTED or
-		 * XMAX_INVALID gets set once the xact is gone, to reduce the costs of
-		 * examining the tuple for future xacts.
+		 * case.  However, we should make sure that XMAX_INVALID gets set once
+		 * the xact is gone, to reduce the costs of examining the tuple for
+		 * future xacts.
 		 */
-		if (!(tuple->t_infomask & HEAP_XMAX_COMMITTED))
+		if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
 		{
-			if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
-			{
-				/*
-				 * If it's a pre-pg_upgrade tuple, the multixact cannot
-				 * possibly be running; otherwise have to check.
-				 */
-				if (!HEAP_LOCKED_UPGRADED(tuple->t_infomask) &&
-					MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple),
-										 true))
-					return HEAPTUPLE_LIVE;
-				SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
-			}
-			else
-			{
-				if (TransactionIdIsInProgress(HeapTupleHeaderGetRawXmax(tuple)))
-					return HEAPTUPLE_LIVE;
-				SetHintBits(tuple, buffer, HEAP_XMAX_INVALID,
-							InvalidTransactionId);
-			}
+			/*
+			 * If it's a pre-pg_upgrade tuple, the multixact cannot possibly be
+			 * running; otherwise have to check.
+			 */
+			if (!HEAP_LOCKED_UPGRADED(tuple->t_infomask) &&
+				MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple),
+									 true))
+				return HEAPTUPLE_LIVE;
+			SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
+		}
+		else
+		{
+			if (TransactionIdIsInProgress(HeapTupleHeaderGetRawXmax(tuple)))
+				return HEAPTUPLE_LIVE;
+			SetHintBits(tuple, buffer, HEAP_XMAX_INVALID,
+						InvalidTransactionId);
 		}
 
 		/*
@@ -1431,6 +1454,8 @@ HeapTupleSatisfiesNonVacuumable(HeapTuple htup, Snapshot snapshot,
 	TransactionId dead_after = InvalidTransactionId;
 	HTSV_Result res;
 
+	InfomaskAssertions(htup->t_data);
+
 	res = HeapTupleSatisfiesVacuumHorizon(htup, buffer, &dead_after);
 
 	if (res == HEAPTUPLE_RECENTLY_DEAD)
@@ -1467,6 +1492,7 @@ HeapTupleIsSurelyDead(HeapTuple htup, GlobalVisState *vistest)
 
 	Assert(ItemPointerIsValid(&htup->t_self));
 	Assert(htup->t_tableOid != InvalidOid);
+	InfomaskAssertions(tuple);
 
 	/*
 	 * If the inserting transaction is marked invalid, then it aborted, and
@@ -1520,6 +1546,8 @@ HeapTupleHeaderIsOnlyLocked(HeapTupleHeader tuple)
 {
 	TransactionId xmax;
 
+	InfomaskAssertions(tuple);
+
 	/* if there's no valid Xmax, then there's obviously no update either */
 	if (tuple->t_infomask & HEAP_XMAX_INVALID)
 		return true;
@@ -1592,6 +1620,7 @@ HeapTupleSatisfiesHistoricMVCC(HeapTuple htup, Snapshot snapshot,
 
 	Assert(ItemPointerIsValid(&htup->t_self));
 	Assert(htup->t_tableOid != InvalidOid);
+	InfomaskAssertions(tuple);
 
 	/* inserting transaction aborted */
 	if (HeapTupleHeaderXminInvalid(tuple))
@@ -1765,6 +1794,8 @@ HeapTupleSatisfiesHistoricMVCC(HeapTuple htup, Snapshot snapshot,
 bool
 HeapTupleSatisfiesVisibility(HeapTuple tup, Snapshot snapshot, Buffer buffer)
 {
+	InfomaskAssertions(tup->t_data);
+
 	switch (snapshot->snapshot_type)
 	{
 		case SNAPSHOT_MVCC:
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index ae2e2ce37a..d6274617b8 100644
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -55,6 +55,8 @@ RelationPutHeapTuple(Relation relation,
 	 */
 	Assert(!((tuple->t_data->t_infomask & HEAP_XMAX_COMMITTED) &&
 			 (tuple->t_data->t_infomask & HEAP_XMAX_IS_MULTI)));
+	Assert(!((tuple->t_data->t_infomask & HEAP_XMAX_COMMITTED) &&
+			 HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_data->t_infomask)));
 
 	/* Add the tuple to the page */
 	pageHeader = BufferGetPage(buffer);
-- 
2.25.1

