From 557434b7fd0397cc33cd87b66a002c833d79986d Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <pg@bowt.ie>
Date: Sat, 30 Oct 2021 09:33:50 -0700
Subject: [PATCH v3] Add hardening to catch invalid TIDs from indexes.

In passing, add some similar assertions to heap pruning code, that
document our assumptions.
---
 src/include/access/tableam.h          |  2 +
 src/backend/access/heap/heapam.c      | 69 ++++++++++++++++++++++
 src/backend/access/heap/pruneheap.c   | 84 +++++++++++++++++++++++++--
 src/backend/access/index/genam.c      |  4 +-
 src/backend/access/nbtree/nbtdedup.c  |  2 +
 src/backend/access/nbtree/nbtinsert.c |  2 +
 6 files changed, 158 insertions(+), 5 deletions(-)

diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index 4aff18215..ed1856c42 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -220,6 +220,8 @@ typedef struct TM_IndexStatus
  */
 typedef struct TM_IndexDeleteOp
 {
+	Relation	irel;			/* Target index relation */
+	BlockNumber	iblknum;		/* Can be used for error reporting */
 	bool		bottomup;		/* Bottom-up (not simple) deletion? */
 	int			bottomupfreespace;	/* Bottom-up space target */
 
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index fecb97286..bf6d0ce18 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -7252,6 +7252,67 @@ index_delete_prefetch_buffer(Relation rel,
 }
 #endif
 
+/*
+ * Helper function for heap_index_delete_tuples.  Checks for index corruption
+ * involving an invalid TID in index AM caller's index.
+ *
+ * This is an ideal place for these checks.  The index AM caller must hold a
+ * buffer lock on the index page containing the TIDs we examine here, so we
+ * don't have to worry about concurrent VACUUMs at all.  We can be sure that
+ * the index is corrupt when htid points directly to an LP_UNUSED item or
+ * heap-only tuple, which is not generally the case when performing standard
+ * index scans.
+ *
+ * It's not certain that the index is corrupt (it might actually be the heap
+ * page).  We blame the index here all the same, since the true underlying
+ * problem at least has a decent chance of being in index vacuuming.
+ */
+static inline void
+index_delete_check_htid(TM_IndexDeleteOp *delstate,
+						Page page, OffsetNumber maxoff,
+						ItemPointer htid, TM_IndexStatus *istatus)
+{
+	OffsetNumber indexpagehoffnum = ItemPointerGetOffsetNumber(htid);
+	ItemId		iid;
+	HeapTupleHeader htup;
+
+	Assert(OffsetNumberIsValid(istatus->idxoffnum));
+
+	if (indexpagehoffnum > maxoff)
+		ereport(ERROR,
+				(errcode(ERRCODE_INDEX_CORRUPTED),
+				 errmsg_internal("heap tid from index tuple (%u,%u) points past end of heap page line pointer array at offset %u of block %u in index \"%s\"",
+								 ItemPointerGetBlockNumber(htid),
+								 indexpagehoffnum,
+								 istatus->idxoffnum, delstate->iblknum,
+								 RelationGetRelationName(delstate->irel))));
+
+	iid = PageGetItemId(page, indexpagehoffnum);
+	if (!ItemIdIsUsed(iid))
+		ereport(ERROR,
+				(errcode(ERRCODE_INDEX_CORRUPTED),
+				 errmsg_internal("heap tid from index tuple (%u,%u) points to unused heap page item at offset %u of block %u in index \"%s\"",
+								 ItemPointerGetBlockNumber(htid),
+								 indexpagehoffnum,
+								 istatus->idxoffnum, delstate->iblknum,
+								 RelationGetRelationName(delstate->irel))));
+
+	if (ItemIdHasStorage(iid))
+	{
+		Assert(ItemIdIsNormal(iid));
+		htup = (HeapTupleHeader) PageGetItem(page, iid);
+
+		if (HeapTupleHeaderIsHeapOnly(htup))
+			ereport(ERROR,
+					(errcode(ERRCODE_INDEX_CORRUPTED),
+					 errmsg_internal("heap tid from index tuple (%u,%u) points to heap-only tuple at offset %u of block %u in index \"%s\"",
+									 ItemPointerGetBlockNumber(htid),
+									 indexpagehoffnum,
+									 istatus->idxoffnum, delstate->iblknum,
+									 RelationGetRelationName(delstate->irel))));
+	}
+}
+
 /*
  * heapam implementation of tableam's index_delete_tuples interface.
  *
@@ -7446,6 +7507,14 @@ heap_index_delete_tuples(Relation rel, TM_IndexDeleteOp *delstate)
 			maxoff = PageGetMaxOffsetNumber(page);
 		}
 
+		/*
+		 * In passing, detect index corruption involving an index page with a
+		 * TID that point to a location in the heap that couldn't possibly be
+		 * correct.  We only do this with actual TIDs from caller's index page
+		 * (not items reached by traversing through a HOT chain).
+		 */
+		index_delete_check_htid(delstate, page, maxoff, htid, istatus);
+
 		if (istatus->knowndeletable)
 			Assert(!delstate->bottomup && !istatus->promising);
 		else
diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
index db6912e9f..43549be04 100644
--- a/src/backend/access/heap/pruneheap.c
+++ b/src/backend/access/heap/pruneheap.c
@@ -844,39 +844,115 @@ heap_page_prune_execute(Buffer buffer,
 {
 	Page		page = (Page) BufferGetPage(buffer);
 	OffsetNumber *offnum;
-	int			i;
+	HeapTupleHeader htup PG_USED_FOR_ASSERTS_ONLY;
 
 	/* Shouldn't be called unless there's something to do */
 	Assert(nredirected > 0 || ndead > 0 || nunused > 0);
 
 	/* Update all redirected line pointers */
 	offnum = redirected;
-	for (i = 0; i < nredirected; i++)
+	for (int i = 0; i < nredirected; i++)
 	{
 		OffsetNumber fromoff = *offnum++;
 		OffsetNumber tooff = *offnum++;
 		ItemId		fromlp = PageGetItemId(page, fromoff);
+		ItemId		tolp PG_USED_FOR_ASSERTS_ONLY;
+
+#ifdef USE_ASSERT_CHECKING
+
+		/*
+		 * The existing items that we set as redirects must be the first tuple
+		 * of a HOT chain that has not be pruned before now (can't be a
+		 * heap-only tuple) when target item has tuple storage.  When the item
+		 * has no storage, then we must just be maintaining the LP_REDIRECT of
+		 * a HOT chain that has been pruned at least once before now.
+		 */
+		if (!ItemIdIsRedirected(fromlp))
+		{
+			Assert(ItemIdHasStorage(fromlp) && ItemIdIsNormal(fromlp));
+
+			htup = (HeapTupleHeader) PageGetItem(page, fromlp);
+			Assert(!HeapTupleHeaderIsHeapOnly(htup));
+		}
+		else
+		{
+			/* We shouldn't need to redundantly set the redirect */
+			Assert(ItemIdGetRedirect(fromlp) != tooff);
+		}
+
+		/*
+		 * Redirect links must point to heap-only tuples.  There can be at
+		 * most one LP_REDIRECT item per HOT chain.
+		 *
+		 * We need to keep around an LP_REDIRECT item (after original
+		 * non-heap-only root tuple gets pruned away) so that it's always
+		 * possible for VACUUM to easily figure out what TID to delete from
+		 * indexes when an entire HOT chain becomes dead.  A heap-only tuple
+		 * can never become LP_DEAD; an LP_REDIRECT item or a regular heap
+		 * tuple can.
+		 */
+		tolp = PageGetItemId(page, tooff);
+		Assert(ItemIdHasStorage(tolp) && ItemIdIsNormal(tolp));
+		htup = (HeapTupleHeader) PageGetItem(page, tolp);
+		Assert(HeapTupleHeaderIsHeapOnly(htup));
+#endif
 
 		ItemIdSetRedirect(fromlp, tooff);
 	}
 
 	/* Update all now-dead line pointers */
 	offnum = nowdead;
-	for (i = 0; i < ndead; i++)
+	for (int i = 0; i < ndead; i++)
 	{
 		OffsetNumber off = *offnum++;
 		ItemId		lp = PageGetItemId(page, off);
 
+#ifdef USE_ASSERT_CHECKING
+
+		/*
+		 * LP_DEAD line pointers must be left behind when they could be
+		 * pointed to by TIDs from indexes.  Index scans expect TIDs to always
+		 * work as stable identifiers of a tuple version (or HOT chain).
+		 */
+		if (ItemIdHasStorage(lp))
+		{
+			Assert(ItemIdIsNormal(lp));
+			htup = (HeapTupleHeader) PageGetItem(page, lp);
+			Assert(!HeapTupleHeaderIsHeapOnly(htup));
+		}
+		else
+		{
+			/*
+			 * Whole HOT chain becomes dead. (We shouldn't need to redundantly
+			 * mark existing LP_DEAD items LP_DEAD.)
+			 */
+			Assert(ItemIdIsRedirected(lp));
+		}
+#endif
+
 		ItemIdSetDead(lp);
 	}
 
 	/* Update all now-unused line pointers */
 	offnum = nowunused;
-	for (i = 0; i < nunused; i++)
+	for (int i = 0; i < nunused; i++)
 	{
 		OffsetNumber off = *offnum++;
 		ItemId		lp = PageGetItemId(page, off);
 
+#ifdef USE_ASSERT_CHECKING
+
+		/*
+		 * Only heap-only tuples can become LP_UNUSED during pruning.  They
+		 * don't need to be left in place as LP_DEAD items until VACUUM gets
+		 * around to doing index vacuuming.  We are already sure that there
+		 * cannot be a TID in an index that points to the items.
+		 */
+		Assert(ItemIdHasStorage(lp) && ItemIdIsNormal(lp));
+		htup = (HeapTupleHeader) PageGetItem(page, lp);
+		Assert(HeapTupleHeaderIsHeapOnly(htup));
+#endif
+
 		ItemIdSetUnused(lp);
 	}
 
diff --git a/src/backend/access/index/genam.c b/src/backend/access/index/genam.c
index b93288a6f..4fe4514fb 100644
--- a/src/backend/access/index/genam.c
+++ b/src/backend/access/index/genam.c
@@ -303,6 +303,8 @@ index_compute_xid_horizon_for_tuples(Relation irel,
 
 	Assert(nitems > 0);
 
+	delstate.irel = irel;
+	delstate.iblknum = BufferGetBlockNumber(ibuf);
 	delstate.bottomup = false;
 	delstate.bottomupfreespace = 0;
 	delstate.ndeltids = 0;
@@ -321,7 +323,7 @@ index_compute_xid_horizon_for_tuples(Relation irel,
 
 		ItemPointerCopy(&itup->t_tid, &delstate.deltids[i].tid);
 		delstate.deltids[i].id = delstate.ndeltids;
-		delstate.status[i].idxoffnum = InvalidOffsetNumber; /* unused */
+		delstate.status[i].idxoffnum = itemnos[i];
 		delstate.status[i].knowndeletable = true;	/* LP_DEAD-marked */
 		delstate.status[i].promising = false;	/* unused */
 		delstate.status[i].freespace = 0;	/* unused */
diff --git a/src/backend/access/nbtree/nbtdedup.c b/src/backend/access/nbtree/nbtdedup.c
index 6401fce57..c88dc6eed 100644
--- a/src/backend/access/nbtree/nbtdedup.c
+++ b/src/backend/access/nbtree/nbtdedup.c
@@ -348,6 +348,8 @@ _bt_bottomupdel_pass(Relation rel, Buffer buf, Relation heapRel,
 	 * concerning ourselves with avoiding work during the tableam call.  Our
 	 * role in costing the bottom-up deletion process is strictly advisory.
 	 */
+	delstate.irel = rel;
+	delstate.iblknum = BufferGetBlockNumber(buf);
 	delstate.bottomup = true;
 	delstate.bottomupfreespace = Max(BLCKSZ / 16, newitemsz);
 	delstate.ndeltids = 0;
diff --git a/src/backend/access/nbtree/nbtinsert.c b/src/backend/access/nbtree/nbtinsert.c
index ccddb0378..0fe8c7093 100644
--- a/src/backend/access/nbtree/nbtinsert.c
+++ b/src/backend/access/nbtree/nbtinsert.c
@@ -2810,6 +2810,8 @@ _bt_simpledel_pass(Relation rel, Buffer buffer, Relation heapRel,
 								&ndeadblocks);
 
 	/* Initialize tableam state that describes index deletion operation */
+	delstate.irel = rel;
+	delstate.iblknum = BufferGetBlockNumber(buffer);
 	delstate.bottomup = false;
 	delstate.bottomupfreespace = 0;
 	delstate.ndeltids = 0;
-- 
2.30.2

