amcheck: Tighten up validation of redirect line pointers.

robertmhaas · robertmhaas · commit 80d5e3a61551 · 2023-03-27T13:36:11.000-04:00
Commit bbc1376 added a new lp_valid[] array which records whether or not a line pointer was thought to be valid, but entries could sometimes get set to true in cases where that wasn't actually safe. Fix that. Suppose A is a redirect line pointer and B is the other line pointer to which it points. The old code could mishandle this situation in a couple of different ways. First, if B was unused, we'd complain about corruption but still set lp_valid[A] = true, causing later code to try to access the B as if it were pointing to a tuple. Second, if B was dead, we wouldn't complain about corruption at all, which is an oversight, and would also set lp_valid[A] = true, which would again confuse later code. Fix all that. In the case where B is a redirect, the old code was correct, but refactor things a bit anyway so that all of these cases are handled more symmetrically. Also add an Assert() and some comments. Andres Freund and Robert Haas Discussion: http://postgr.es/m/20230323172607.y3lejpntjnuis5vv%40awork3.anarazel.de
diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
@@ -481,11 +481,35 @@ verify_heapam(PG_FUNCTION_ARGS)
 											   (unsigned) maxoff));
 					continue;
 				}
+
+				/*
+				 * Since we've checked that this redirect points to a line
+				 * pointer between FirstOffsetNumber and maxoff, it should
+				 * now be safe to fetch the referenced line pointer. We expect
+				 * it to be LP_NORMAL; if not, that's corruption.
+				 */
 				rditem = PageGetItemId(ctx.page, rdoffnum);
 				if (!ItemIdIsUsed(rditem))
+				{
 					report_corruption(&ctx,
-									  psprintf("line pointer redirection to unused item at offset %u",
+									  psprintf("redirected line pointer points to an unused item at offset %u",
 											   (unsigned) rdoffnum));
+					continue;
+				}
+				else if (ItemIdIsDead(rditem))
+				{
+					report_corruption(&ctx,
+									  psprintf("redirected line pointer points to a dead item at offset %u",
+											   (unsigned) rdoffnum));
+					continue;
+				}
+				else if (ItemIdIsRedirected(rditem))
+				{
+					report_corruption(&ctx,
+									  psprintf("redirected line pointer points to another redirected line pointer at offset %u",
+											   (unsigned) rdoffnum));
+					continue;
+				}
 
 				/*
 				 * Record the fact that this line pointer has passed basic
@@ -584,14 +608,12 @@ verify_heapam(PG_FUNCTION_ARGS)
 			/* Handle the cases where the current line pointer is a redirect. */
 			if (ItemIdIsRedirected(curr_lp))
 			{
-				/* Can't redirect to another redirect. */
-				if (ItemIdIsRedirected(next_lp))
-				{
-					report_corruption(&ctx,
-									  psprintf("redirected line pointer points to another redirected line pointer at offset %u",
-											   (unsigned) nextoffnum));
-					continue;
-				}
+				/*
+				 * We should not have set successor[ctx.offnum] to a value
+				 * other than InvalidOffsetNumber unless that line pointer
+				 * is LP_NORMAL.
+				 */
+				Assert(ItemIdIsNormal(next_lp));
 
 				/* Can only redirect to a HOT tuple. */
 				next_htup = (HeapTupleHeader) PageGetItem(ctx.page, next_lp);
