Re: HOT chain validation in verify_heapam() - Mailing list pgsql-hackers

On Wed, Mar 22, 2023 at 5:56 PM Andres Freund <andres@anarazel.de> wrote:
> Why are redirections now checked in two places? There already was a
> ItemIdIsUsed() check in the "/* Perform tuple checks */" loop, but now there's
> the ItemIdIsRedirected() check in the "Update chain validation." loop as well
> - and the output of that is confusing, because it'll just mention the target
> of the redirect.

ctx.offnum is reported as part of the context message, and doesn't
need to be duplicate in the message itself. Any other offset numbers
do need to be mentioned in the message itself. I have attempted to
make sure that the code consistently follows this rule but if you see
a case where I have failed to do so, let me know.

> I also think it's not quite right that some of checks inside if
> (ItemIdIsRedirected()) continue in case of corruption, others don't. While
> there's a later continue, that means the corrupt tuples get added to the
> predecessor array.  Similarly, in the non-redirect portion, the successor
> array gets filled with corrupt tuples, which doesn't seem quite right to me.

I'm not entirely sure I agree with this. I mean, we should skip
further checks of the data is so corrupt that further checks are not
sensible e.g. if the line pointer is just gibberish we can't validate
anything about the tuple, because we can't even find the tuple.
However, we don't want to skip further checks as soon as we see any
kind of a problem whatsoever. For instance, even if the tuple data is
utter gibberish, that should not and does not keep us from checking
whether the update chain looks sane. If the tuple header is garbage
(e.g. xmin and xmax are outside the bounds of clog) then at least some
of the update-chain checks are not possible, because we can't know the
commit status of the tuples, but garbage in the tuple data isn't
really a problem for update chain validation per se.

So I think the places that lack a continue are making a judgement that
continuing with further checks is reasonable in those scenarios, and
it's not obvious to me that those judgements are wrong. For instance,
suppose we reach the "Can only redirect to a HOT tuple." case. If we
continue there, we won't check whether the HEAP_UPDATED is set on the
successor tuple, and we won't perform the intersecting-HOT-chain
check, and we won't set the predecessor[] array, and I don't really
see why we shouldn't do those things. In my view, the fact that the
redirected line pointer points to a tuple that is not marked as HOT
does mean we have corruption, but it doesn't mean that we should give
up on thinking of those two offset numbers as part of an update chain.

> A patch addressing some, but not all, of those is attached. With that I don't
> see any crashes or false-positives anymore.

+                               else if (ItemIdIsDead(rditem))
+                                       report_corruption(&ctx,
+
   psprintf("line pointer redirection to dead item at offset %u",
+
                    (unsigned) rdoffnum));
+                               else if (ItemIdIsRedirected(rditem))
+                                       report_corruption(&ctx,
+
   psprintf("line pointer redirection to another redirect at offset
%u",
+
                    (unsigned) rdoffnum));

Hmm, the first of these definitely seems like a good additional check.
The second one moves an existing check from the subsequent loop to
this loop, which if we're going to add that additional check makes
sense to do for consistency.

+            /* the current line pointer may not have a successor */
+            if (nextoffnum == InvalidOffsetNumber)
+                continue;
+
             /*
-             * The current line pointer may not have a successor, either
-             * because it's not valid or because it didn't point to anything.
-             * In either case, we have to give up.
-             *
-             * If the current line pointer does point to something, it's
-             * possible that the target line pointer isn't valid. We have to
-             * give up in that case, too.
+             * The successor is located beyond the end of the line item array,
+             * which can happen when the array is truncated.
              */
-            if (nextoffnum == InvalidOffsetNumber || !lp_valid[nextoffnum])
+            if (nextoffnum > maxoff)
+                continue;
+
+            /* the successor is not valid, have to give up */
+            if (!lp_valid[nextoffnum])
                 continue;

I don't agree with this part -- I think we should instead do what I
proposed before and prevent the successor[] array from ever getting
entries that are out of bounds.

-                 * Redirects are created by updates, so successor should be
-                 * the result of an update.
+                 * Redirects are created by HOT updates, so successor should
+                 * be the result of an HOT update.
+                 *
+                 * XXX: HeapTupleHeaderIsHeapOnly() should always imply
+                 * HEAP_UPDATED. This should be checked even when the tuple
+                 * isn't a target of a redirect.

Hmm, OK. So the question is where to put this check. Maybe inside
check_tuple_header(), making it independent of the update chain
validation stuff?

+             *
+             * NB: Can't use HeapTupleHeaderIsHotUpdated() as it checks if
+             * hint bits indicate xmin/xmax aborted.
              */
-            if (!HeapTupleHeaderIsHotUpdated(curr_htup) &&
+            if (!(curr_htup->t_infomask2 & HEAP_HOT_UPDATED) &&
                 HeapTupleHeaderIsHeapOnly(next_htup))
             {
                 report_corruption(&ctx,
                                   psprintf("non-heap-only update
produced a heap-only tuple at offset %u",
                                            (unsigned) nextoffnum));
             }
-            if (HeapTupleHeaderIsHotUpdated(curr_htup) &&
+            if ((curr_htup->t_infomask2 & HEAP_HOT_UPDATED) &&

Makes sense.

--
Robert Haas
EDB: http://www.enterprisedb.com