v13-0001-Refactoring-function-check_tuple_header_and_visi.patch

From acee08646354ddb001d7b31a1b8932237bd40405 Mon Sep 17 00:00:00 2001
From: Mark Dilger <mark.dilger@enterprisedb.com>
Date: Wed, 24 Mar 2021 18:18:56 -0700
Subject: [PATCH v13 1/4] Refactoring function
 check_tuple_header_and_visibility

Extending enum XidCommitStatus to include XID_IS_CURRENT_XID.  The
visibility code for verify_heapam() was conflating XID_IN_PROGRESS
and XID_IS_CURRENT_XID under just one enum, making it harder to
compare the logic to that used by vacuum's visibility function,
which treats those two cases separately.

Simplifying check_tuple_header_and_visibilty signature.  It was
taking both tuphdr and ctx arguments, but the tuphdr is just
ctx->tuphdr, so it is a bit absurd to pass two arguments for this.

Splitting check_tuple_header_and_visibilty() into two functions.
check_tuple_header() and check_tuple_visibility() are split out as
separate functions, but otherwise behave exactly as before.
---
 contrib/amcheck/verify_heapam.c | 82 +++++++++++++++++++--------------
 1 file changed, 47 insertions(+), 35 deletions(-)

diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index 6f972e630a..9172b5fd81 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -46,6 +46,7 @@ typedef enum XidBoundsViolation
 typedef enum XidCommitStatus
 {
 	XID_COMMITTED,
+	XID_IS_CURRENT_XID,
 	XID_IN_PROGRESS,
 	XID_ABORTED
 } XidCommitStatus;
@@ -133,8 +134,8 @@ static void check_tuple(HeapCheckContext *ctx);
 static void check_toast_tuple(HeapTuple toasttup, HeapCheckContext *ctx);
 
 static bool check_tuple_attribute(HeapCheckContext *ctx);
-static bool check_tuple_header_and_visibilty(HeapTupleHeader tuphdr,
-											 HeapCheckContext *ctx);
+static bool check_tuple_header(HeapCheckContext *ctx);
+static bool check_tuple_visibility(HeapCheckContext *ctx);
 
 static void report_corruption(HeapCheckContext *ctx, char *msg);
 static TupleDesc verify_heapam_tupdesc(void);
@@ -555,16 +556,11 @@ verify_heapam_tupdesc(void)
 }
 
 /*
- * Check for tuple header corruption and tuple visibility.
- *
- * Since we do not hold a snapshot, tuple visibility is not a question of
- * whether we should be able to see the tuple relative to any particular
- * snapshot, but rather a question of whether it is safe and reasonable to
- * check the tuple attributes.
+ * Check for tuple header corruption.
  *
  * Some kinds of corruption make it unsafe to check the tuple attributes, for
  * example when the line pointer refers to a range of bytes outside the page.
- * In such cases, we return false (not visible) after recording appropriate
+ * In such cases, we return false (not checkable) after recording appropriate
  * corruption messages.
  *
  * Some other kinds of tuple header corruption confuse the question of where
@@ -576,27 +572,16 @@ verify_heapam_tupdesc(void)
  *
  * Other kinds of tuple header corruption do not bear on the question of
  * whether the tuple attributes can be checked, so we record corruption
- * messages for them but do not base our visibility determination on them.  (In
- * other words, we do not return false merely because we detected them.)
- *
- * For visibility determination not specifically related to corruption, what we
- * want to know is if a tuple is potentially visible to any running
- * transaction.  If you are tempted to replace this function's visibility logic
- * with a call to another visibility checking function, keep in mind that this
- * function does not update hint bits, as it seems imprudent to write hint bits
- * (or anything at all) to a table during a corruption check.  Nor does this
- * function bother classifying tuple visibility beyond a boolean visible vs.
- * not visible.
+ * messages for them but we do not return false merely because we detected
+ * them.
  *
- * The caller should already have checked that xmin and xmax are not out of
- * bounds for the relation.
- *
- * Returns whether the tuple is both visible and sufficiently sensible to
- * undergo attribute checks.
+ * Returns whether the tuple is sufficiently sensible to undergo visibility and
+ * attribute checks.
  */
 static bool
-check_tuple_header_and_visibilty(HeapTupleHeader tuphdr, HeapCheckContext *ctx)
+check_tuple_header(HeapCheckContext *ctx)
 {
+	HeapTupleHeader tuphdr = ctx->tuphdr;
 	uint16		infomask = tuphdr->t_infomask;
 	bool		header_garbled = false;
 	unsigned	expected_hoff;
@@ -651,13 +636,34 @@ check_tuple_header_and_visibilty(HeapTupleHeader tuphdr, HeapCheckContext *ctx)
 	if (header_garbled)
 		return false;			/* checking of this tuple should not continue */
 
-	/*
-	 * Ok, we can examine the header for tuple visibility purposes, though we
-	 * still need to be careful about a few remaining types of header
-	 * corruption.  This logic roughly follows that of
-	 * HeapTupleSatisfiesVacuum.  Where possible the comments indicate which
-	 * HTSV_Result we think that function might return for this tuple.
-	 */
+	return true;				/* header ok */
+}
+
+/*
+ * Checks whether a tuple is visible for checking.
+ *
+ * Since we do not hold a snapshot, tuple visibility is not a question of
+ * whether we should be able to see the tuple relative to any particular
+ * snapshot, but rather a question of whether it is safe and reasonable to
+ * check the tuple attributes.
+ *
+ * For visibility determination not specifically related to corruption, what we
+ * want to know is if a tuple is potentially visible to any running
+ * transaction.  If you are tempted to replace this function's visibility logic
+ * with a call to another visibility checking function, keep in mind that this
+ * function does not update hint bits, as it seems imprudent to write hint bits
+ * (or anything at all) to a table during a corruption check.  Nor does this
+ * function bother classifying tuple visibility beyond a boolean visible vs.
+ * not visible.
+ *
+ * Returns whether the tuple is visible for checking.
+ */
+static bool
+check_tuple_visibility(HeapCheckContext *ctx)
+{
+	HeapTupleHeader tuphdr = ctx->tuphdr;
+	uint16		infomask = tuphdr->t_infomask;
+
 	if (!HeapTupleHeaderXminCommitted(tuphdr))
 	{
 		TransactionId raw_xmin = HeapTupleHeaderGetRawXmin(tuphdr);
@@ -704,6 +710,7 @@ check_tuple_header_and_visibilty(HeapTupleHeader tuphdr, HeapCheckContext *ctx)
 					switch (status)
 					{
 						case XID_IN_PROGRESS:
+						case XID_IS_CURRENT_XID:
 							return true;	/* HEAPTUPLE_DELETE_IN_PROGRESS */
 						case XID_COMMITTED:
 						case XID_ABORTED:
@@ -748,6 +755,7 @@ check_tuple_header_and_visibilty(HeapTupleHeader tuphdr, HeapCheckContext *ctx)
 						case XID_COMMITTED:
 							break;
 						case XID_IN_PROGRESS:
+						case XID_IS_CURRENT_XID:
 							return true;	/* insert or delete in progress */
 						case XID_ABORTED:
 							return false;	/* HEAPTUPLE_DEAD */
@@ -795,6 +803,7 @@ check_tuple_header_and_visibilty(HeapTupleHeader tuphdr, HeapCheckContext *ctx)
 					switch (status)
 					{
 						case XID_IN_PROGRESS:
+						case XID_IS_CURRENT_XID:
 							return true;	/* HEAPTUPLE_DELETE_IN_PROGRESS */
 						case XID_COMMITTED:
 						case XID_ABORTED:
@@ -1247,7 +1256,10 @@ check_tuple(HeapCheckContext *ctx)
 	 * corrupt to continue checking, or if the tuple is not visible to anyone,
 	 * we cannot continue with other checks.
 	 */
-	if (!check_tuple_header_and_visibilty(ctx->tuphdr, ctx))
+	if (!check_tuple_header(ctx))
+		return;
+
+	if (!check_tuple_visibility(ctx))
 		return;
 
 	/*
@@ -1448,7 +1460,7 @@ get_xid_status(TransactionId xid, HeapCheckContext *ctx,
 	if (FullTransactionIdPrecedesOrEquals(clog_horizon, fxid))
 	{
 		if (TransactionIdIsCurrentTransactionId(xid))
-			*status = XID_IN_PROGRESS;
+			*status = XID_IS_CURRENT_XID;
 		else if (TransactionIdDidCommit(xid))
 			*status = XID_COMMITTED;
 		else if (TransactionIdDidAbort(xid))
-- 
2.21.1 (Apple Git-122.3)