Hi,
On 2023-03-23 11:41:52 -0400, Robert Haas wrote:
> On Wed, Mar 22, 2023 at 5:56 PM Andres Freund <andres@anarazel.de> wrote:
> > 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.
The cases I was complaining about where metadata that's important. We
shouldn't enter a tuple into lp_valid[] or successor[] if it failed validity
checks - the subsequent error reports that that generates won't be helpful -
or we'll crash.
E.g. continuing after:
rditem = PageGetItemId(ctx.page, rdoffnum);
if (!ItemIdIsUsed(rditem))
report_corruption(&ctx,
psprintf("line pointer redirection to unused item at offset %u",
(unsigned) rdoffnum));
means we'll look into the tuple in the "update chain validation" loop for
unused items. Where it afaict will lead to a crash or bogus results, because:
/* Can only redirect to a HOT tuple. */
next_htup = (HeapTupleHeader) PageGetItem(ctx.page, next_lp);
if (!HeapTupleHeaderIsHeapOnly(next_htup))
{
report_corruption(&ctx,
psprintf("redirected line pointer points to a non-heap-only tuple at offset %u",
(unsigned) nextoffnum));
}
will just dereference the unused item.
> - * 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?
Yes, check_tuple_header sounds sensible to me.
Greetings,
Andres Freund
