[Mark Dilger <mark.dilger@enterprisedb.com>]

> On Apr 1, 2021, at 1:08 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> 
> 
> 
> - There are a total of two (2) calls in the current source code to
> palloc0fast, and hundreds of calls to palloc0. So I think you should
> forget about using the fast variant and just do what almost every
> other caller does.

Done.

> - If you want to make this code faster, a better idea would be to
> avoid doing all of this allocate and free work and just allocate an
> array that's guaranteed to be big enough, and then keep track of how
> many elements of that array are actually in use.

Sounds like premature optimization to me.  I only used palloc0fast because the argument is compile-time known.  I wasn't specifically attempting to speed anything up.

> - #ifdef DECOMPRESSION_CORRUPTION_CHECKING is not a useful way of
> introducing such a feature. Either we do it for real and expose it via
> SQL and pg_amcheck as an optional behavior, or we rip it out and
> revisit it later. Given the nearness of feature freeze, my vote is for
> the latter.
> 
> - I'd remove the USE_LZ4 bit, too. Let's not define the presence of
> LZ4 data in a non-LZ4-enabled cluster as corruption. If we do, then
> people will expect to be able to use this to find places where they
> are dependent on LZ4 if they want to move away from it -- and if we
> don't recurse into composite datums, that will not actually work.

Ok, I have removed this bit.  I also removed the part of the patch that introduced a new corruption check, decompressing the data to see if it decompresses without error.

> - check_toast_tuple() has an odd and slightly unclear calling
> convention for which there are no comments. I wonder if it would be
> better to reverse things and make bool *error the return value and
> what is now the return value into a pointer argument, but whatever we
> do I think it needs a few words in the comments. We don't need to
> slavishly explain every argument -- I think toasttup and ctx and tctx
> are reasonably clear -- but this is not.
...
> - Is there a reason we need a cross-check on both the number of chunks
> and on the total size? It seems to me that we should check that each
> individual chunk has the size we expect, and that the total number of
> chunks is what we expect. The overall size is then necessarily
> correct.

Good point.  I've removed the extra check on the total size, since it cannot be wrong if the checks on the individual chunk sizes were all correct.  This eliminates the need for the odd calling convention for check_toast_tuple(), so I've changed that to return void and not take any return-by-reference arguments.

> - To me it would be more logical to reverse the order of the
> toast_pointer.va_toastrelid != ctx->rel->rd_rel->reltoastrelid and
> VARATT_EXTERNAL_GET_EXTSIZE(toast_pointer) > toast_pointer.va_rawsize
> - VARHDRSZ checks. Whether we're pointing at the correct relation
> feels more fundamental.

Done.

> - If we moved the toplevel foreach loop in check_toasted_attributes()
> out to the caller, say renaming the function to just
> check_toasted_attribute(), we'd save a level of indentation in that
> whole function and probably add a tad of clarity, too. You wouldn't
> feel the need to Assert(ctx.toasted_attributes == NIL) in the caller
> if the caller had just done list_free(ctx->toasted_attributes);
> ctx->toasted_attributes = NIL.

You're right.  It looks nicer that way.  Changed.

> - Why are all the changes to the tests in this patch? What do they
> have to do with getting the TOAST checks out from under the buffer
> lock? I really need you to structure the patch series so that each
> patch is about one topic and, equally, so that each topic is only
> covered by one patch. Otherwise it's just way too confusing.

v18-0001 - Finishes work started in commit 3b6c1259f9 that was overlooked owing to how I had separated the changes in v17-0002 vs. v17-0003

v18-0002 - Postpones the toast checks for a page until after the main table page lock is released

v18-0003 - Improves the corruption messages in ways already discussed earlier in this thread.  Changes the tests to expect the new messages, but adds no new checks

v18-0004 - Adding corruption checks of toast pointers.  Extends the regression tests to cover the new checks.

> 
> - I think some of these messages need a bit of word-smithing, too, but
> we can leave that for when we're closer to being done with this.

Ok.


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Attachments:

  [application/octet-stream] v18-0001-amcheck-remove-duplicate-xid-bounds-checks.patch (5.4K, ../AC5479E4-6321-473D-AC92-5EC36299FBC2@enterprisedb.com/2-v18-0001-amcheck-remove-duplicate-xid-bounds-checks.patch)
  download | inline diff:
From 5ff174db4de8c35f73c3bb9ffc51d854686cb1c5 Mon Sep 17 00:00:00 2001
From: Mark Dilger <mark.dilger@enterprisedb.com>
Date: Fri, 2 Apr 2021 15:03:12 -0700
Subject: [PATCH v18 1/4] amcheck: remove duplicate xid bounds checks

Commit 3b6c1259f9 resulted in the same xmin and xmax bounds checking
being performed in both check_tuple() and check_tuple_visibility().
Leaving the ones in check_tuple_visibility() and removing the
others.

While at it, adjusting some code comments that should have been
changed in 3b6c1259f9 but were not.
---
 contrib/amcheck/verify_heapam.c | 130 ++------------------------------
 1 file changed, 6 insertions(+), 124 deletions(-)

diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index 11ace483d0..1d769035f1 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -1390,136 +1390,18 @@ check_tuple_attribute(HeapCheckContext *ctx)
 static void
 check_tuple(HeapCheckContext *ctx)
 {
-	TransactionId xmin;
-	TransactionId xmax;
-	bool		fatal = false;
-	uint16		infomask = ctx->tuphdr->t_infomask;
-
-	/* If xmin is normal, it should be within valid range */
-	xmin = HeapTupleHeaderGetXmin(ctx->tuphdr);
-	switch (get_xid_status(xmin, ctx, NULL))
-	{
-		case XID_INVALID:
-		case XID_BOUNDS_OK:
-			break;
-		case XID_IN_FUTURE:
-			report_corruption(ctx,
-							  psprintf("xmin %u equals or exceeds next valid transaction ID %u:%u",
-									   xmin,
-									   EpochFromFullTransactionId(ctx->next_fxid),
-									   XidFromFullTransactionId(ctx->next_fxid)));
-			fatal = true;
-			break;
-		case XID_PRECEDES_CLUSTERMIN:
-			report_corruption(ctx,
-							  psprintf("xmin %u precedes oldest valid transaction ID %u:%u",
-									   xmin,
-									   EpochFromFullTransactionId(ctx->oldest_fxid),
-									   XidFromFullTransactionId(ctx->oldest_fxid)));
-			fatal = true;
-			break;
-		case XID_PRECEDES_RELMIN:
-			report_corruption(ctx,
-							  psprintf("xmin %u precedes relation freeze threshold %u:%u",
-									   xmin,
-									   EpochFromFullTransactionId(ctx->relfrozenfxid),
-									   XidFromFullTransactionId(ctx->relfrozenfxid)));
-			fatal = true;
-			break;
-	}
-
-	xmax = HeapTupleHeaderGetRawXmax(ctx->tuphdr);
-
-	if (infomask & HEAP_XMAX_IS_MULTI)
-	{
-		/* xmax is a multixact, so it should be within valid MXID range */
-		switch (check_mxid_valid_in_rel(xmax, ctx))
-		{
-			case XID_INVALID:
-				report_corruption(ctx,
-								  pstrdup("multitransaction ID is invalid"));
-				fatal = true;
-				break;
-			case XID_PRECEDES_RELMIN:
-				report_corruption(ctx,
-								  psprintf("multitransaction ID %u precedes relation minimum multitransaction ID threshold %u",
-										   xmax, ctx->relminmxid));
-				fatal = true;
-				break;
-			case XID_PRECEDES_CLUSTERMIN:
-				report_corruption(ctx,
-								  psprintf("multitransaction ID %u precedes oldest valid multitransaction ID threshold %u",
-										   xmax, ctx->oldest_mxact));
-				fatal = true;
-				break;
-			case XID_IN_FUTURE:
-				report_corruption(ctx,
-								  psprintf("multitransaction ID %u equals or exceeds next valid multitransaction ID %u",
-										   xmax,
-										   ctx->next_mxact));
-				fatal = true;
-				break;
-			case XID_BOUNDS_OK:
-				break;
-		}
-	}
-	else
-	{
-		/*
-		 * xmax is not a multixact and is normal, so it should be within the
-		 * valid XID range.
-		 */
-		switch (get_xid_status(xmax, ctx, NULL))
-		{
-			case XID_INVALID:
-			case XID_BOUNDS_OK:
-				break;
-			case XID_IN_FUTURE:
-				report_corruption(ctx,
-								  psprintf("xmax %u equals or exceeds next valid transaction ID %u:%u",
-										   xmax,
-										   EpochFromFullTransactionId(ctx->next_fxid),
-										   XidFromFullTransactionId(ctx->next_fxid)));
-				fatal = true;
-				break;
-			case XID_PRECEDES_CLUSTERMIN:
-				report_corruption(ctx,
-								  psprintf("xmax %u precedes oldest valid transaction ID %u:%u",
-										   xmax,
-										   EpochFromFullTransactionId(ctx->oldest_fxid),
-										   XidFromFullTransactionId(ctx->oldest_fxid)));
-				fatal = true;
-				break;
-			case XID_PRECEDES_RELMIN:
-				report_corruption(ctx,
-								  psprintf("xmax %u precedes relation freeze threshold %u:%u",
-										   xmax,
-										   EpochFromFullTransactionId(ctx->relfrozenfxid),
-										   XidFromFullTransactionId(ctx->relfrozenfxid)));
-				fatal = true;
-		}
-	}
-
 	/*
-	 * Cannot process tuple data if tuple header was corrupt, as the offsets
-	 * within the page cannot be trusted, leaving too much risk of reading
-	 * garbage if we continue.
-	 *
-	 * We also cannot process the tuple if the xmin or xmax were invalid
-	 * relative to relfrozenxid or relminmxid, as clog entries for the xids
-	 * may already be gone.
+	 * Check various forms of tuple header corruption, and if the header is too
+	 * corrupt, do not continue with other checks.
 	 */
-	if (fatal)
+	if (!check_tuple_header(ctx))
 		return;
 
 	/*
-	 * Check various forms of tuple header corruption.  If the header is too
-	 * corrupt to continue checking, or if the tuple is not visible to anyone,
-	 * we cannot continue with other checks.
+	 * Check tuple visibility.  If the inserting transaction aborted, we
+	 * cannot assume our relation description matches the tuple structure, and
+	 * therefore cannot check it.
 	 */
-	if (!check_tuple_header(ctx))
-		return;
-
 	if (!check_tuple_visibility(ctx))
 		return;
 
-- 
2.21.1 (Apple Git-122.3)

=

  [application/octet-stream] v18-0002-amcheck-avoid-extra-work-while-holding-buffer-lo.patch (16.3K, ../AC5479E4-6321-473D-AC92-5EC36299FBC2@enterprisedb.com/3-v18-0002-amcheck-avoid-extra-work-while-holding-buffer-lo.patch)
  download | inline diff:
From e33e49f1e1325a4439991550789c47fdf54b7220 Mon Sep 17 00:00:00 2001
From: Mark Dilger <mark.dilger@enterprisedb.com>
Date: Fri, 2 Apr 2021 15:05:07 -0700
Subject: [PATCH v18 2/4] amcheck: avoid extra work while holding buffer locks

Refactoring verify_heapam so it does not look up attributes in the
toast table while holding a buffer lock on the main table page.
Instead, check all toasted attributes found in tuples on a main
table page after the lock has been released.

This changes the user visible behavior slightly, in that when both
toasted attributes and non-toasted attributes from the same page are
corrupt, the corruption reports about non-toasted attributes will
all be emitted prior to any corruption reports about the toasted
attributes.  This shouldn't matter, though, as this function was
first committed in this development cycle, and no users should be
familiar with the old behavior.
---
 contrib/amcheck/verify_heapam.c  | 236 +++++++++++++++++++++----------
 src/tools/pgindent/typedefs.list |   1 +
 2 files changed, 165 insertions(+), 72 deletions(-)

diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index 1d769035f1..d8a3e966c4 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -58,6 +58,19 @@ typedef enum SkipPages
 	SKIP_PAGES_NONE
 } SkipPages;
 
+/*
+ * Struct holding information about a toasted attribute sufficient to both
+ * check the toasted attribute and, if found to be corrupt, to report where it
+ * was encountered in the main table.
+ */
+typedef struct ToastedAttribute
+{
+	struct varatt_external toast_pointer;
+	BlockNumber blkno;			/* block in main table */
+	OffsetNumber offnum;		/* offset in main table */
+	AttrNumber	attnum;			/* attribute in main table */
+} ToastedAttribute;
+
 /*
  * Struct holding the running context information during
  * a lifetime of a verify_heapam execution.
@@ -119,11 +132,11 @@ typedef struct HeapCheckContext
 	/* True if tuple's xmax makes it eligible for pruning */
 	bool		tuple_could_be_pruned;
 
-	/* Values for iterating over toast for the attribute */
-	int32		chunkno;
-	int32		attrsize;
-	int32		endchunk;
-	int32		totalchunks;
+	/*
+	 * List of ToastedAttribute structs for toasted attributes which are not
+	 * eligible for pruning and should be checked
+	 */
+	List	   *toasted_attributes;
 
 	/* Whether verify_heapam has yet encountered any corrupt tuples */
 	bool		is_corrupt;
@@ -136,13 +149,20 @@ typedef struct HeapCheckContext
 /* Internal implementation */
 static void sanity_check_relation(Relation rel);
 static void check_tuple(HeapCheckContext *ctx);
-static void check_toast_tuple(HeapTuple toasttup, HeapCheckContext *ctx);
+static void check_toast_tuple(HeapTuple toasttup, HeapCheckContext *ctx,
+							  ToastedAttribute *ta, int32 chunkno,
+							  int32 endchunk);
 
 static bool check_tuple_attribute(HeapCheckContext *ctx);
+static void check_toasted_attribute(HeapCheckContext *ctx,
+									ToastedAttribute *ta);
+
 static bool check_tuple_header(HeapCheckContext *ctx);
 static bool check_tuple_visibility(HeapCheckContext *ctx);
 
 static void report_corruption(HeapCheckContext *ctx, char *msg);
+static void report_toast_corruption(HeapCheckContext *ctx,
+									ToastedAttribute *ta, char *msg);
 static TupleDesc verify_heapam_tupdesc(void);
 static FullTransactionId FullTransactionIdFromXidAndCtx(TransactionId xid,
 														const HeapCheckContext *ctx);
@@ -253,6 +273,7 @@ verify_heapam(PG_FUNCTION_ARGS)
 
 	memset(&ctx, 0, sizeof(HeapCheckContext));
 	ctx.cached_xid = InvalidTransactionId;
+	ctx.toasted_attributes = NIL;
 
 	/*
 	 * Any xmin newer than the xmin of our snapshot can't become all-visible
@@ -469,6 +490,19 @@ verify_heapam(PG_FUNCTION_ARGS)
 		/* clean up */
 		UnlockReleaseBuffer(ctx.buffer);
 
+		/*
+		 * Check any toast pointers from the page whose lock we just released
+		 */
+		if (ctx.toasted_attributes != NIL)
+		{
+			ListCell   *cell;
+
+			foreach(cell, ctx.toasted_attributes)
+				check_toasted_attribute(&ctx, lfirst(cell));
+			list_free(ctx.toasted_attributes);
+			ctx.toasted_attributes = NIL;
+		}
+
 		if (on_error_stop && ctx.is_corrupt)
 			break;
 	}
@@ -510,14 +544,13 @@ sanity_check_relation(Relation rel)
 }
 
 /*
- * Record a single corruption found in the table.  The values in ctx should
- * reflect the location of the corruption, and the msg argument should contain
- * a human-readable description of the corruption.
- *
- * The msg argument is pfree'd by this function.
+ * Shared internal implementation for report_corruption and
+ * report_toast_corruption.
  */
 static void
-report_corruption(HeapCheckContext *ctx, char *msg)
+report_corruption_internal(Tuplestorestate *tupstore, TupleDesc tupdesc,
+						   BlockNumber blkno, OffsetNumber offnum,
+						   AttrNumber attnum, char *msg)
 {
 	Datum		values[HEAPCHECK_RELATION_COLS];
 	bool		nulls[HEAPCHECK_RELATION_COLS];
@@ -525,10 +558,10 @@ report_corruption(HeapCheckContext *ctx, char *msg)
 
 	MemSet(values, 0, sizeof(values));
 	MemSet(nulls, 0, sizeof(nulls));
-	values[0] = Int64GetDatum(ctx->blkno);
-	values[1] = Int32GetDatum(ctx->offnum);
-	values[2] = Int32GetDatum(ctx->attnum);
-	nulls[2] = (ctx->attnum < 0);
+	values[0] = Int64GetDatum(blkno);
+	values[1] = Int32GetDatum(offnum);
+	values[2] = Int32GetDatum(attnum);
+	nulls[2] = (attnum < 0);
 	values[3] = CStringGetTextDatum(msg);
 
 	/*
@@ -541,8 +574,39 @@ report_corruption(HeapCheckContext *ctx, char *msg)
 	 */
 	pfree(msg);
 
-	tuple = heap_form_tuple(ctx->tupdesc, values, nulls);
-	tuplestore_puttuple(ctx->tupstore, tuple);
+	tuple = heap_form_tuple(tupdesc, values, nulls);
+	tuplestore_puttuple(tupstore, tuple);
+}
+
+/*
+ * Record a single corruption found in the main table.  The values in ctx should
+ * indicate the location of the corruption, and the msg argument should contain
+ * a human-readable description of the corruption.
+ *
+ * The msg argument is pfree'd by this function.
+ */
+static void
+report_corruption(HeapCheckContext *ctx, char *msg)
+{
+	report_corruption_internal(ctx->tupstore, ctx->tupdesc, ctx->blkno,
+							   ctx->offnum, ctx->attnum, msg);
+	ctx->is_corrupt = true;
+}
+
+/*
+ * Record corruption found in the toast table.  The values in ta should
+ * indicate the location in the main table where the toast pointer was
+ * encountered, and the msg argument should contain a human-readable
+ * description of the toast table corruption.
+ *
+ * As above, the msg argument is pfree'd by this function.
+ */
+static void
+report_toast_corruption(HeapCheckContext *ctx, ToastedAttribute *ta,
+						char *msg)
+{
+	report_corruption_internal(ctx->tupstore, ctx->tupdesc, ta->blkno,
+							   ta->offnum, ta->attnum, msg);
 	ctx->is_corrupt = true;
 }
 
@@ -1094,9 +1158,12 @@ check_tuple_visibility(HeapCheckContext *ctx)
  * tuples that store the toasted value are retrieved and checked in order, with
  * each toast tuple being checked against where we are in the sequence, as well
  * as each toast tuple having its varlena structure sanity checked.
+ *
+ * Returns whether the toast tuple passed the corruption checks.
  */
 static void
-check_toast_tuple(HeapTuple toasttup, HeapCheckContext *ctx)
+check_toast_tuple(HeapTuple toasttup, HeapCheckContext *ctx,
+				  ToastedAttribute *ta, int32 chunkno, int32 endchunk)
 {
 	int32		curchunk;
 	Pointer		chunk;
@@ -1111,7 +1178,7 @@ check_toast_tuple(HeapTuple toasttup, HeapCheckContext *ctx)
 										 ctx->toast_rel->rd_att, &isnull));
 	if (isnull)
 	{
-		report_corruption(ctx,
+		report_toast_corruption(ctx, ta,
 						  pstrdup("toast chunk sequence number is null"));
 		return;
 	}
@@ -1119,7 +1186,7 @@ check_toast_tuple(HeapTuple toasttup, HeapCheckContext *ctx)
 										ctx->toast_rel->rd_att, &isnull));
 	if (isnull)
 	{
-		report_corruption(ctx,
+		report_toast_corruption(ctx, ta,
 						  pstrdup("toast chunk data is null"));
 		return;
 	}
@@ -1137,7 +1204,7 @@ check_toast_tuple(HeapTuple toasttup, HeapCheckContext *ctx)
 		/* should never happen */
 		uint32		header = ((varattrib_4b *) chunk)->va_4byte.va_header;
 
-		report_corruption(ctx,
+		report_toast_corruption(ctx, ta,
 						  psprintf("corrupt extended toast chunk has invalid varlena header: %0x (sequence number %d)",
 								   header, curchunk));
 		return;
@@ -1146,30 +1213,28 @@ check_toast_tuple(HeapTuple toasttup, HeapCheckContext *ctx)
 	/*
 	 * Some checks on the data we've found
 	 */
-	if (curchunk != ctx->chunkno)
+	if (curchunk != chunkno)
 	{
-		report_corruption(ctx,
+		report_toast_corruption(ctx, ta,
 						  psprintf("toast chunk sequence number %u does not match the expected sequence number %u",
-								   curchunk, ctx->chunkno));
+								   curchunk, chunkno));
 		return;
 	}
-	if (curchunk > ctx->endchunk)
+	if (curchunk > endchunk)
 	{
-		report_corruption(ctx,
+		report_toast_corruption(ctx, ta,
 						  psprintf("toast chunk sequence number %u exceeds the end chunk sequence number %u",
-								   curchunk, ctx->endchunk));
+								   curchunk, endchunk));
 		return;
 	}
 
-	expected_size = curchunk < ctx->totalchunks - 1 ? TOAST_MAX_CHUNK_SIZE
-		: ctx->attrsize - ((ctx->totalchunks - 1) * TOAST_MAX_CHUNK_SIZE);
+	expected_size = curchunk < endchunk ? TOAST_MAX_CHUNK_SIZE
+		: VARATT_EXTERNAL_GET_EXTSIZE(ta->toast_pointer) - (endchunk * TOAST_MAX_CHUNK_SIZE);
+
 	if (chunksize != expected_size)
-	{
-		report_corruption(ctx,
+		report_toast_corruption(ctx, ta,
 						  psprintf("toast chunk size %u differs from the expected size %u",
 								   chunksize, expected_size));
-		return;
-	}
 }
 
 /*
@@ -1177,17 +1242,17 @@ check_toast_tuple(HeapTuple toasttup, HeapCheckContext *ctx)
  * found in ctx->tupstore.
  *
  * This function follows the logic performed by heap_deform_tuple(), and in the
- * case of a toasted value, optionally continues along the logic of
- * detoast_external_attr(), checking for any conditions that would result in
- * either of those functions Asserting or crashing the backend.  The checks
- * performed by Asserts present in those two functions are also performed here.
- * In cases where those two functions are a bit cavalier in their assumptions
- * about data being correct, we perform additional checks not present in either
- * of those two functions.  Where some condition is checked in both of those
- * functions, we perform it here twice, as we parallel the logical flow of
- * those two functions.  The presence of duplicate checks seems a reasonable
- * price to pay for keeping this code tightly coupled with the code it
- * protects.
+ * case of a toasted value, optionally stores the toast pointer so later it can
+ * be checked following the logic of detoast_external_attr(), checking for any
+ * conditions that would result in either of those functions Asserting or
+ * crashing the backend.  The checks performed by Asserts present in those two
+ * functions are also performed here and in check_toasted_attribute.  In cases
+ * where those two functions are a bit cavalier in their assumptions about data
+ * being correct, we perform additional checks not present in either of those
+ * two functions.  Where some condition is checked in both of those functions,
+ * we perform it here twice, as we parallel the logical flow of those two
+ * functions.  The presence of duplicate checks seems a reasonable price to pay
+ * for keeping this code tightly coupled with the code it protects.
  *
  * Returns true if the tuple attribute is sane enough for processing to
  * continue on to the next attribute, false otherwise.
@@ -1195,12 +1260,6 @@ check_toast_tuple(HeapTuple toasttup, HeapCheckContext *ctx)
 static bool
 check_tuple_attribute(HeapCheckContext *ctx)
 {
-	struct varatt_external toast_pointer;
-	ScanKeyData toastkey;
-	SysScanDesc toastscan;
-	SnapshotData SnapshotToast;
-	HeapTuple	toasttup;
-	bool		found_toasttup;
 	Datum		attdatum;
 	struct varlena *attr;
 	char	   *tp;				/* pointer to the tuple data */
@@ -1335,13 +1394,44 @@ check_tuple_attribute(HeapCheckContext *ctx)
 		return true;
 
 	/*
-	 * Must copy attr into toast_pointer for alignment considerations
+	 * If this tuple is eligible to be pruned, we cannot check the toast.
+	 * Otherwise, we push a copy of the toast tuple so we can check it after
+	 * releasing the main table buffer lock.
 	 */
-	VARATT_EXTERNAL_GET_POINTER(toast_pointer, attr);
+	if (!ctx->tuple_could_be_pruned)
+	{
+		ToastedAttribute *ta;
+
+		ta = (ToastedAttribute *) palloc0(sizeof(ToastedAttribute));
+
+		VARATT_EXTERNAL_GET_POINTER(ta->toast_pointer, attr);
+		ta->blkno = ctx->blkno;
+		ta->offnum = ctx->offnum;
+		ta->attnum = ctx->attnum;
+		ctx->toasted_attributes = lappend(ctx->toasted_attributes, ta);
+	}
+
+	return true;
+}
+
+/*
+ * For each attribute collected in ctx->toasted_attributes, look up the value
+ * in the toast table and perform checks on it.  This function should only be
+ * called on toast pointers which cannot be vacuumed away during our
+ * processing.
+ */
+static void
+check_toasted_attribute(HeapCheckContext *ctx, ToastedAttribute *ta)
+{
+	SnapshotData SnapshotToast;
+	ScanKeyData toastkey;
+	SysScanDesc toastscan;
+	bool		found_toasttup;
+	HeapTuple	toasttup;
+	int32		chunkno;
+	int32		endchunk;
 
-	ctx->attrsize = VARATT_EXTERNAL_GET_EXTSIZE(toast_pointer);
-	ctx->endchunk = (ctx->attrsize - 1) / TOAST_MAX_CHUNK_SIZE;
-	ctx->totalchunks = ctx->endchunk + 1;
+	endchunk = (VARATT_EXTERNAL_GET_EXTSIZE(ta->toast_pointer) - 1) / TOAST_MAX_CHUNK_SIZE;
 
 	/*
 	 * Setup a scan key to find chunks in toast table with matching va_valueid
@@ -1349,7 +1439,7 @@ check_tuple_attribute(HeapCheckContext *ctx)
 	ScanKeyInit(&toastkey,
 				(AttrNumber) 1,
 				BTEqualStrategyNumber, F_OIDEQ,
-				ObjectIdGetDatum(toast_pointer.va_valueid));
+				ObjectIdGetDatum(ta->toast_pointer.va_valueid));
 
 	/*
 	 * Check if any chunks for this toasted object exist in the toast table,
@@ -1360,27 +1450,26 @@ check_tuple_attribute(HeapCheckContext *ctx)
 										   ctx->valid_toast_index,
 										   &SnapshotToast, 1,
 										   &toastkey);
-	ctx->chunkno = 0;
+	chunkno = 0;
 	found_toasttup = false;
 	while ((toasttup =
 			systable_getnext_ordered(toastscan,
 									 ForwardScanDirection)) != NULL)
 	{
 		found_toasttup = true;
-		check_toast_tuple(toasttup, ctx);
-		ctx->chunkno++;
+		check_toast_tuple(toasttup, ctx, ta, chunkno, endchunk);
+		chunkno++;
 	}
-	if (!found_toasttup)
-		report_corruption(ctx,
-						  psprintf("toasted value for attribute %u missing from toast table",
-								   ctx->attnum));
-	else if (ctx->chunkno != (ctx->endchunk + 1))
-		report_corruption(ctx,
-						  psprintf("final toast chunk number %u differs from expected value %u",
-								   ctx->chunkno, (ctx->endchunk + 1)));
 	systable_endscan_ordered(toastscan);
 
-	return true;
+	if (!found_toasttup)
+		report_toast_corruption(ctx, ta,
+								psprintf("toasted value for attribute %u missing from toast table",
+										 ta->attnum));
+	else if (chunkno != (endchunk + 1))
+		report_toast_corruption(ctx, ta,
+								psprintf("final toast chunk number %u differs from expected value %u",
+										 chunkno, (endchunk + 1)));
 }
 
 /*
@@ -1391,8 +1480,8 @@ static void
 check_tuple(HeapCheckContext *ctx)
 {
 	/*
-	 * Check various forms of tuple header corruption, and if the header is too
-	 * corrupt, do not continue with other checks.
+	 * Check various forms of tuple header corruption, and if the header is
+	 * too corrupt, do not continue with other checks.
 	 */
 	if (!check_tuple_header(ctx))
 		return;
@@ -1423,7 +1512,10 @@ check_tuple(HeapCheckContext *ctx)
 	 * Check each attribute unless we hit corruption that confuses what to do
 	 * next, at which point we abort further attribute checks for this tuple.
 	 * Note that we don't abort for all types of corruption, only for those
-	 * types where we don't know how to continue.
+	 * types where we don't know how to continue.  We also don't abort the
+	 * checking of toasted attributes collected from the tuple prior to
+	 * aborting.  Those will still be checked later along with other toasted
+	 * attributes collected from the page.
 	 */
 	ctx->offset = 0;
 	for (ctx->attnum = 0; ctx->attnum < ctx->natts; ctx->attnum++)
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 9e6777e9d0..862a6df949 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2558,6 +2558,7 @@ TmFromChar
 TmToChar
 ToastAttrInfo
 ToastTupleContext
+ToastedAttribute
 TocEntry
 TokenAuxData
 TokenizedLine
-- 
2.21.1 (Apple Git-122.3)

=

  [application/octet-stream] v18-0003-amcheck-improving-corruption-messages.patch (8.0K, ../AC5479E4-6321-473D-AC92-5EC36299FBC2@enterprisedb.com/4-v18-0003-amcheck-improving-corruption-messages.patch)
  download | inline diff:
From 4195bb5d6e39ec0cc2b323b0ad5b24c4c843df88 Mon Sep 17 00:00:00 2001
From: Mark Dilger <mark.dilger@enterprisedb.com>
Date: Fri, 2 Apr 2021 15:06:50 -0700
Subject: [PATCH v18 3/4] amcheck: improving corruption messages.

Removing redundant mention of attnum in the corruption message text,
as the attnum is already its own separate column.

When reporting toast corruption, mentioning the toast value in the
message since that information is not otherwise reported.
---
 contrib/amcheck/verify_heapam.c           | 61 +++++++++++++----------
 src/bin/pg_amcheck/t/004_verify_heapam.pl |  4 +-
 2 files changed, 37 insertions(+), 28 deletions(-)

diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index d8a3e966c4..cd1f2c4113 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -1179,7 +1179,8 @@ check_toast_tuple(HeapTuple toasttup, HeapCheckContext *ctx,
 	if (isnull)
 	{
 		report_toast_corruption(ctx, ta,
-						  pstrdup("toast chunk sequence number is null"));
+								psprintf("toast value %u has toast chunk with null sequence number",
+										 ta->toast_pointer.va_valueid));
 		return;
 	}
 	chunk = DatumGetPointer(fastgetattr(toasttup, 3,
@@ -1187,7 +1188,8 @@ check_toast_tuple(HeapTuple toasttup, HeapCheckContext *ctx,
 	if (isnull)
 	{
 		report_toast_corruption(ctx, ta,
-						  pstrdup("toast chunk data is null"));
+								psprintf("toast value %u chunk data is null",
+										 ta->toast_pointer.va_valueid));
 		return;
 	}
 	if (!VARATT_IS_EXTENDED(chunk))
@@ -1205,8 +1207,9 @@ check_toast_tuple(HeapTuple toasttup, HeapCheckContext *ctx,
 		uint32		header = ((varattrib_4b *) chunk)->va_4byte.va_header;
 
 		report_toast_corruption(ctx, ta,
-						  psprintf("corrupt extended toast chunk has invalid varlena header: %0x (sequence number %d)",
-								   header, curchunk));
+								psprintf("toast value %u corrupt extended chunk has invalid varlena header: %0x (sequence number %d)",
+										 ta->toast_pointer.va_valueid,
+										 header, curchunk));
 		return;
 	}
 
@@ -1216,15 +1219,17 @@ check_toast_tuple(HeapTuple toasttup, HeapCheckContext *ctx,
 	if (curchunk != chunkno)
 	{
 		report_toast_corruption(ctx, ta,
-						  psprintf("toast chunk sequence number %u does not match the expected sequence number %u",
-								   curchunk, chunkno));
+								psprintf("toast value %u chunk sequence number %u does not match the expected sequence number %u",
+										 ta->toast_pointer.va_valueid,
+										 curchunk, chunkno));
 		return;
 	}
 	if (curchunk > endchunk)
 	{
 		report_toast_corruption(ctx, ta,
-						  psprintf("toast chunk sequence number %u exceeds the end chunk sequence number %u",
-								   curchunk, endchunk));
+								psprintf("toast value %u chunk sequence number %u exceeds the end chunk sequence number %u",
+										 ta->toast_pointer.va_valueid,
+										 curchunk, endchunk));
 		return;
 	}
 
@@ -1233,8 +1238,9 @@ check_toast_tuple(HeapTuple toasttup, HeapCheckContext *ctx,
 
 	if (chunksize != expected_size)
 		report_toast_corruption(ctx, ta,
-						  psprintf("toast chunk size %u differs from the expected size %u",
-								   chunksize, expected_size));
+								psprintf("toast value %u chunk size %u differs from the expected size %u",
+										 ta->toast_pointer.va_valueid,
+										 chunksize, expected_size));
 }
 
 /*
@@ -1265,6 +1271,7 @@ check_tuple_attribute(HeapCheckContext *ctx)
 	char	   *tp;				/* pointer to the tuple data */
 	uint16		infomask;
 	Form_pg_attribute thisatt;
+	struct varatt_external toast_pointer;
 
 	infomask = ctx->tuphdr->t_infomask;
 	thisatt = TupleDescAttr(RelationGetDescr(ctx->rel), ctx->attnum);
@@ -1274,8 +1281,7 @@ check_tuple_attribute(HeapCheckContext *ctx)
 	if (ctx->tuphdr->t_hoff + ctx->offset > ctx->lp_len)
 	{
 		report_corruption(ctx,
-						  psprintf("attribute %u with length %u starts at offset %u beyond total tuple length %u",
-								   ctx->attnum,
+						  psprintf("attribute with length %u starts at offset %u beyond total tuple length %u",
 								   thisatt->attlen,
 								   ctx->tuphdr->t_hoff + ctx->offset,
 								   ctx->lp_len));
@@ -1295,8 +1301,7 @@ check_tuple_attribute(HeapCheckContext *ctx)
 		if (ctx->tuphdr->t_hoff + ctx->offset > ctx->lp_len)
 		{
 			report_corruption(ctx,
-							  psprintf("attribute %u with length %u ends at offset %u beyond total tuple length %u",
-									   ctx->attnum,
+							  psprintf("attribute with length %u ends at offset %u beyond total tuple length %u",
 									   thisatt->attlen,
 									   ctx->tuphdr->t_hoff + ctx->offset,
 									   ctx->lp_len));
@@ -1328,8 +1333,7 @@ check_tuple_attribute(HeapCheckContext *ctx)
 		if (va_tag != VARTAG_ONDISK)
 		{
 			report_corruption(ctx,
-							  psprintf("toasted attribute %u has unexpected TOAST tag %u",
-									   ctx->attnum,
+							  psprintf("toasted attribute has unexpected TOAST tag %u",
 									   va_tag));
 			/* We can't know where the next attribute begins */
 			return false;
@@ -1343,8 +1347,7 @@ check_tuple_attribute(HeapCheckContext *ctx)
 	if (ctx->tuphdr->t_hoff + ctx->offset > ctx->lp_len)
 	{
 		report_corruption(ctx,
-						  psprintf("attribute %u with length %u ends at offset %u beyond total tuple length %u",
-								   ctx->attnum,
+						  psprintf("attribute with length %u ends at offset %u beyond total tuple length %u",
 								   thisatt->attlen,
 								   ctx->tuphdr->t_hoff + ctx->offset,
 								   ctx->lp_len));
@@ -1371,12 +1374,17 @@ check_tuple_attribute(HeapCheckContext *ctx)
 
 	/* It is external, and we're looking at a page on disk */
 
+	/*
+	 * Must copy attr into toast_pointer for alignment considerations
+	 */
+	VARATT_EXTERNAL_GET_POINTER(toast_pointer, attr);
+
 	/* The tuple header better claim to contain toasted values */
 	if (!(infomask & HEAP_HASEXTERNAL))
 	{
 		report_corruption(ctx,
-						  psprintf("attribute %u is external but tuple header flag HEAP_HASEXTERNAL not set",
-								   ctx->attnum));
+						  psprintf("toast value %u is external but tuple header flag HEAP_HASEXTERNAL not set",
+								   toast_pointer.va_valueid));
 		return true;
 	}
 
@@ -1384,8 +1392,8 @@ check_tuple_attribute(HeapCheckContext *ctx)
 	if (!ctx->rel->rd_rel->reltoastrelid)
 	{
 		report_corruption(ctx,
-						  psprintf("attribute %u is external but relation has no toast relation",
-								   ctx->attnum));
+						  psprintf("toast value %u is external but relation has no toast relation",
+								   toast_pointer.va_valueid));
 		return true;
 	}
 
@@ -1464,12 +1472,13 @@ check_toasted_attribute(HeapCheckContext *ctx, ToastedAttribute *ta)
 
 	if (!found_toasttup)
 		report_toast_corruption(ctx, ta,
-								psprintf("toasted value for attribute %u missing from toast table",
-										 ta->attnum));
+								psprintf("toast value %u not found in toast table",
+										 ta->toast_pointer.va_valueid));
 	else if (chunkno != (endchunk + 1))
 		report_toast_corruption(ctx, ta,
-								psprintf("final toast chunk number %u differs from expected value %u",
-										 chunkno, (endchunk + 1)));
+								psprintf("toast value %u was expected to end at chunk %u, but ended at chunk %u",
+										 ta->toast_pointer.va_valueid,
+										 (endchunk + 1), chunkno));
 }
 
 /*
diff --git a/src/bin/pg_amcheck/t/004_verify_heapam.pl b/src/bin/pg_amcheck/t/004_verify_heapam.pl
index 36607596b1..307f14611c 100644
--- a/src/bin/pg_amcheck/t/004_verify_heapam.pl
+++ b/src/bin/pg_amcheck/t/004_verify_heapam.pl
@@ -480,7 +480,7 @@ for (my $tupidx = 0; $tupidx < ROWCOUNT; $tupidx++)
 
 		$header = header(0, $offnum, 1);
 		push @expected,
-			qr/${header}attribute \d+ with length \d+ ends at offset \d+ beyond total tuple length \d+/;
+			qr/${header}attribute with length \d+ ends at offset \d+ beyond total tuple length \d+/;
 	}
 	elsif ($offnum == 13)
 	{
@@ -489,7 +489,7 @@ for (my $tupidx = 0; $tupidx < ROWCOUNT; $tupidx++)
 
 		$header = header(0, $offnum, 2);
 		push @expected,
-			qr/${header}toasted value for attribute 2 missing from toast table/;
+			qr/${header}toast value \d+ not found in toast table/;
 	}
 	elsif ($offnum == 14)
 	{
-- 
2.21.1 (Apple Git-122.3)

=

  [application/octet-stream] v18-0004-amcheck-adding-toast-pointer-corruption-checks.patch (6.0K, ../AC5479E4-6321-473D-AC92-5EC36299FBC2@enterprisedb.com/5-v18-0004-amcheck-adding-toast-pointer-corruption-checks.patch)
  download | inline diff:
From c3645b0af895767c40b1ead2f2e076c7c33e482b Mon Sep 17 00:00:00 2001
From: Mark Dilger <mark.dilger@enterprisedb.com>
Date: Fri, 2 Apr 2021 15:07:52 -0700
Subject: [PATCH v18 4/4] amcheck: adding toast pointer corruption checks

Verifying that toast pointer va_toastrelid fields match their heap
table's reltoastrelid.

Checking the extsize for a toast pointer against the raw size.  This
check could fail if buggy compression logic fails to notice that
compressing the attribute makes it bigger.  But assuming the logic
for that is correct, overlarge extsize indicates a corrupted toast
pointer.

Checking the toast is not too large to be allocated.  No such
toasted value should ever be stored, but a corrupted toast pointer
could record an unreasonbly large size, so check that.
---
 contrib/amcheck/verify_heapam.c           | 34 +++++++++++++++++++-
 src/bin/pg_amcheck/t/004_verify_heapam.pl | 39 +++++++++++++++++++++--
 2 files changed, 69 insertions(+), 4 deletions(-)

diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
index cd1f2c4113..b2e121ed38 100644
--- a/contrib/amcheck/verify_heapam.c
+++ b/contrib/amcheck/verify_heapam.c
@@ -1397,6 +1397,28 @@ check_tuple_attribute(HeapCheckContext *ctx)
 		return true;
 	}
 
+	/* The toast pointer had better point at the relation's toast table */
+	if (toast_pointer.va_toastrelid != ctx->rel->rd_rel->reltoastrelid)
+	{
+		report_corruption(ctx,
+						  psprintf("toast value %u toast relation oid %u differs from expected oid %u",
+								   toast_pointer.va_valueid,
+								   toast_pointer.va_toastrelid,
+								   ctx->rel->rd_rel->reltoastrelid));
+		return true;
+	}
+
+	/* Compression should never expand the attribute */
+	if (VARATT_EXTERNAL_GET_EXTSIZE(toast_pointer) > toast_pointer.va_rawsize - VARHDRSZ)
+	{
+		report_corruption(ctx,
+						  psprintf("toast value %u external size %u exceeds maximum expected for rawsize %u",
+								   toast_pointer.va_valueid,
+								   VARATT_EXTERNAL_GET_EXTSIZE(toast_pointer),
+								   toast_pointer.va_rawsize));
+		return true;
+	}
+
 	/* If we were told to skip toast checking, then we're done. */
 	if (ctx->toast_rel == NULL)
 		return true;
@@ -1471,14 +1493,24 @@ check_toasted_attribute(HeapCheckContext *ctx, ToastedAttribute *ta)
 	systable_endscan_ordered(toastscan);
 
 	if (!found_toasttup)
+	{
 		report_toast_corruption(ctx, ta,
 								psprintf("toast value %u not found in toast table",
 										 ta->toast_pointer.va_valueid));
-	else if (chunkno != (endchunk + 1))
+		return;
+	}
+
+	if (chunkno != (endchunk + 1))
 		report_toast_corruption(ctx, ta,
 								psprintf("toast value %u was expected to end at chunk %u, but ended at chunk %u",
 										 ta->toast_pointer.va_valueid,
 										 (endchunk + 1), chunkno));
+
+	if (!AllocSizeIsValid(ta->toast_pointer.va_rawsize))
+		report_toast_corruption(ctx, ta,
+								psprintf("toast value %u rawsize %u too large to be allocated",
+										 ta->toast_pointer.va_valueid,
+										 ta->toast_pointer.va_rawsize));
 }
 
 /*
diff --git a/src/bin/pg_amcheck/t/004_verify_heapam.pl b/src/bin/pg_amcheck/t/004_verify_heapam.pl
index 307f14611c..de525fbdd8 100644
--- a/src/bin/pg_amcheck/t/004_verify_heapam.pl
+++ b/src/bin/pg_amcheck/t/004_verify_heapam.pl
@@ -224,7 +224,7 @@ my $rel = $node->safe_psql('postgres', qq(SELECT pg_relation_filepath('public.te
 my $relpath = "$pgdata/$rel";
 
 # Insert data and freeze public.test
-use constant ROWCOUNT => 16;
+use constant ROWCOUNT => 19;
 $node->safe_psql('postgres', qq(
 	INSERT INTO public.test (a, b, c)
 		VALUES (
@@ -240,6 +240,13 @@ my $relfrozenxid = $node->safe_psql('postgres',
 my $datfrozenxid = $node->safe_psql('postgres',
 	q(select datfrozenxid from pg_database where datname = 'postgres'));
 
+# Find our toast relation id
+my $toastrelid = $node->safe_psql('postgres', qq(
+	SELECT c.reltoastrelid
+		FROM pg_catalog.pg_class c
+		WHERE c.oid = 'public.test'::regclass
+		));
+
 # Sanity check that our 'test' table has a relfrozenxid newer than the
 # datfrozenxid for the database, and that the datfrozenxid is greater than the
 # first normal xid.  We rely on these invariants in some of our tests.
@@ -296,7 +303,7 @@ close($file)
 $node->start;
 
 # Ok, Xids and page layout look ok.  We can run corruption tests.
-plan tests => 19;
+plan tests => 22;
 
 # Check that pg_amcheck runs against the uncorrupted table without error.
 $node->command_ok(['pg_amcheck', '-p', $port, 'postgres'],
@@ -501,7 +508,7 @@ for (my $tupidx = 0; $tupidx < ROWCOUNT; $tupidx++)
 		push @expected,
 			qr/${header}multitransaction ID 4 equals or exceeds next valid multitransaction ID 1/;
 	}
-	elsif ($offnum == 15)	# Last offnum must equal ROWCOUNT
+	elsif ($offnum == 15)
 	{
 		# Set both HEAP_XMAX_COMMITTED and HEAP_XMAX_IS_MULTI
 		$tup->{t_infomask} |= HEAP_XMAX_COMMITTED;
@@ -511,6 +518,32 @@ for (my $tupidx = 0; $tupidx < ROWCOUNT; $tupidx++)
 		push @expected,
 			qr/${header}multitransaction ID 4000000000 precedes relation minimum multitransaction ID threshold 1/;
 	}
+	elsif ($offnum == 16)
+	{
+		# Corrupt column c's toast pointer va_toastrelid field
+		my $otherid = $toastrelid + 1;
+		$tup->{c_va_toastrelid} = $otherid;
+		$header = header(0, $offnum, 2);
+		push @expected,
+			qr/${header}toast value \d+ toast relation oid $otherid differs from expected oid $toastrelid/;
+	}
+	elsif ($offnum == 17)
+	{
+		# Corrupt column c's toast pointer va_extinfo field
+		$tup->{c_va_extinfo} = 7654321;
+		$header = header(0, $offnum, 2);
+		push @expected,
+			qr/${header}toast value \d+ external size 7654321 exceeds maximum expected for rawsize 10004/;
+	}
+	elsif ($offnum == 18)	# Last offnum should equal ROWCOUNT-1
+	{
+		# Corrupt column c's toast pointer va_rawsize field with a value
+		# exceeding maximum allowable allocation size
+		$tup->{c_va_rawsize} = 0x40000000;
+		$header = header(0, $offnum, 2);
+		push @expected,
+			qr/${header}toast value \d+ rawsize 1073741824 too large to be allocated/;
+	}
 	write_tuple($file, $offset, $tup);
 }
 close($file)
-- 
2.21.1 (Apple Git-122.3)

=