amcheck: Tighten up validation of redirect line pointers.

Commit bbc1376b39627c6bddd8a0dc0a7dda24c91a97a0 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
index 10dd31477b0afd5b21874b7b891f79422bea3334..1dc9d89f306cd253860208e8692d68d9b7420bf5 100644 (file)
--- 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);