From 5b5bcc7422203e8b62a6cab2c3e72e3476f5dfb0 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <bossartn@amazon.com>
Date: Tue, 30 Nov 2021 16:33:39 -0800
Subject: [PATCH v2 1/1] Disallow HEAP_XMAX_COMMITTED and
 HEAP_XMAX_IS_LOCKED_ONLY.

---
 contrib/amcheck/verify_heapam.c             |  19 +++
 src/backend/access/heap/README.tuplock      |   2 +-
 src/backend/access/heap/heapam.c            |   6 +
 src/backend/access/heap/heapam_visibility.c | 149 ++++++++++++++++----
 src/backend/access/heap/hio.c               |   2 +
 5 files changed, 148 insertions(+), 30 deletions(-)

diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index bae5340111..63759516a6 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -705,6 +705,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 17afe1ea4c..e02a788060 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -5089,6 +5089,12 @@ 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.
+		 */
+		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 b72b03ea25..e5eaf74f85 100644
--- a/src/backend/access/heap/heapam_visibility.c
+++ b/src/backend/access/heap/heapam_visibility.c
@@ -127,6 +127,12 @@ SetHintBits(HeapTupleHeader tuple, Buffer buffer,
 	}
 
 	tuple->t_infomask |= infomask;
+
+	Assert(!((tuple->t_infomask & HEAP_XMAX_COMMITTED) &&
+			 HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask)));
+	Assert(!((tuple->t_infomask & HEAP_XMAX_COMMITTED) &&
+			 (tuple->t_infomask & HEAP_XMAX_IS_MULTI)));
+
 	MarkBufferDirtyHint(buffer, true);
 }
 
@@ -272,8 +278,28 @@ HeapTupleSatisfiesSelf(HeapTuple htup, Snapshot snapshot, Buffer buffer)
 
 	if (tuple->t_infomask & HEAP_XMAX_COMMITTED)
 	{
-		if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
-			return true;
+		if (unlikely(HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask)))
+		{
+			if (tuple->t_infomask & HEAP_XMAX_LOCK_ONLY)
+				ereport(ERROR,
+						(errcode(ERRCODE_DATA_CORRUPTED),
+						 errmsg_internal("found tuple with HEAP_XMAX_COMMITTED "
+										 "and HEAP_XMAX_LOCK_ONLY")));
+
+			/* pre-v9.3 lock-only bit pattern */
+			ereport(ERROR,
+					(errcode(ERRCODE_DATA_CORRUPTED),
+					 errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and"
+									 "HEAP_XMAX_EXCL_LOCK set and"
+									 "HEAP_XMAX_IS_MULTI unset")));
+		}
+
+		if (unlikely(tuple->t_infomask & HEAP_XMAX_IS_MULTI))
+			ereport(ERROR,
+					(errcode(ERRCODE_DATA_CORRUPTED),
+					 errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and "
+									 "HEAP_XMAX_IS_MULTI")));
+
 		return false;			/* updated by other */
 	}
 
@@ -605,8 +631,28 @@ 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 (unlikely(HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask)))
+		{
+			if (tuple->t_infomask & HEAP_XMAX_LOCK_ONLY)
+				ereport(ERROR,
+						(errcode(ERRCODE_DATA_CORRUPTED),
+						 errmsg_internal("found tuple with HEAP_XMAX_COMMITTED "
+										 "and HEAP_XMAX_LOCK_ONLY")));
+
+			/* pre-v9.3 lock-only bit pattern */
+			ereport(ERROR,
+					(errcode(ERRCODE_DATA_CORRUPTED),
+					 errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and"
+									 "HEAP_XMAX_EXCL_LOCK set and "
+									 "HEAP_XMAX_IS_MULTI unset")));
+		}
+
+		if (unlikely(tuple->t_infomask & HEAP_XMAX_IS_MULTI))
+			ereport(ERROR,
+					(errcode(ERRCODE_DATA_CORRUPTED),
+					 errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and "
+									 "HEAP_XMAX_IS_MULTI")));
+
 		if (!ItemPointerEquals(&htup->t_self, &tuple->t_ctid))
 			return TM_Updated;	/* updated by other */
 		else
@@ -867,8 +913,28 @@ HeapTupleSatisfiesDirty(HeapTuple htup, Snapshot snapshot,
 
 	if (tuple->t_infomask & HEAP_XMAX_COMMITTED)
 	{
-		if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
-			return true;
+		if (unlikely(HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask)))
+		{
+			if (tuple->t_infomask & HEAP_XMAX_LOCK_ONLY)
+				ereport(ERROR,
+						(errcode(ERRCODE_DATA_CORRUPTED),
+						 errmsg_internal("found tuple with HEAP_XMAX_COMMITTED "
+										 "and HEAP_XMAX_LOCK_ONLY")));
+
+			/* pre-v9.3 lock-only bit pattern */
+			ereport(ERROR,
+					(errcode(ERRCODE_DATA_CORRUPTED),
+					 errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and"
+									 "HEAP_XMAX_EXCL_LOCK set and "
+									 "HEAP_XMAX_IS_MULTI unset")));
+		}
+
+		if (unlikely(tuple->t_infomask & HEAP_XMAX_IS_MULTI))
+			ereport(ERROR,
+					(errcode(ERRCODE_DATA_CORRUPTED),
+					 errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and "
+									 "HEAP_XMAX_IS_MULTI")));
+
 		return false;			/* updated by other */
 	}
 
@@ -1304,33 +1370,52 @@ HeapTupleSatisfiesVacuumHorizon(HeapTuple htup, Buffer buffer, TransactionId *de
 
 	if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask))
 	{
+		if (unlikely(tuple->t_infomask & HEAP_XMAX_COMMITTED))
+		{
+			if (tuple->t_infomask & HEAP_XMAX_LOCK_ONLY)
+				ereport(ERROR,
+						(errcode(ERRCODE_DATA_CORRUPTED),
+						 errmsg_internal("found tuple with HEAP_XMAX_COMMITTED "
+										 "and HEAP_XMAX_LOCK_ONLY")));
+
+			/* pre-v9.3 lock-only bit pattern */
+			ereport(ERROR,
+					(errcode(ERRCODE_DATA_CORRUPTED),
+					 errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and"
+									 "HEAP_XMAX_EXCL_LOCK set and "
+									 "HEAP_XMAX_IS_MULTI unset")));
+		}
+
 		/*
 		 * "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 (unlikely(tuple->t_infomask & HEAP_XMAX_COMMITTED))
+				ereport(ERROR,
+						(errcode(ERRCODE_DATA_CORRUPTED),
+						 errmsg_internal("found tuple with HEAP_XMAX_COMMITTED "
+										 "and 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);
 		}
 
 		/*
@@ -1352,6 +1437,12 @@ HeapTupleSatisfiesVacuumHorizon(HeapTuple htup, Buffer buffer, TransactionId *de
 		/* not LOCKED_ONLY, so it has to have an xmax */
 		Assert(TransactionIdIsValid(xmax));
 
+		if (unlikely(tuple->t_infomask & HEAP_XMAX_COMMITTED))
+			ereport(ERROR,
+					(errcode(ERRCODE_DATA_CORRUPTED),
+					 errmsg_internal("found tuple with HEAP_XMAX_COMMITTED and "
+									 "HEAP_XMAX_IS_MULTI")));
+
 		if (TransactionIdIsInProgress(xmax))
 			return HEAPTUPLE_DELETE_IN_PROGRESS;
 		else if (TransactionIdDidCommit(xmax))
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index d34edb4190..990c336575 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

