Hi,
On 2023-03-22 14:56:22 -0700, Andres Freund wrote:
> A patch addressing some, but not all, of those is attached. With that I don't
> see any crashes or false-positives anymore.
That patch missed that, as committed, the first if (ItemIdIsRedirected())
check sets lp_valid[n] = true even if the target of the redirect is unused.
With that fixed, 004_verify_heapam doesn't cause crash anymore - it doesn't
pass though, because there's a bunch of unadjusted error messages.
Andres
diff --git i/contrib/amcheck/verify_heapam.c w/contrib/amcheck/verify_heapam.c
index 663fb65dee6..d0c462e28ea 100644
--- i/contrib/amcheck/verify_heapam.c
+++ w/contrib/amcheck/verify_heapam.c
@@ -486,14 +486,24 @@ verify_heapam(PG_FUNCTION_ARGS)
report_corruption(&ctx,
psprintf("line pointer redirection to unused item at offset %u",
(unsigned) rdoffnum));
-
- /*
- * Record the fact that this line pointer has passed basic
- * sanity checking, and also the offset number to which it
- * points.
- */
- lp_valid[ctx.offnum] = true;
- successor[ctx.offnum] = rdoffnum;
+ 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));
+ else
+ {
+ /*
+ * Record the fact that this line pointer has passed basic
+ * sanity checking, and also the offset number to which it
+ * points.
+ */
+ lp_valid[ctx.offnum] = true;
+ successor[ctx.offnum] = rdoffnum;
+ }
continue;
}
@@ -564,16 +574,19 @@ verify_heapam(PG_FUNCTION_ARGS)
TransactionId next_xmin;
OffsetNumber nextoffnum = successor[ctx.offnum];
+ /* 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;
/* We have two valid line pointers that we can examine. */
@@ -583,14 +596,8 @@ 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;
- }
+ /* should have filtered out all the other cases */
+ Assert(ItemIdIsNormal(next_lp));
/* Can only redirect to a HOT tuple. */
next_htup = (HeapTupleHeader) PageGetItem(ctx.page, next_lp);
@@ -602,8 +609,12 @@ verify_heapam(PG_FUNCTION_ARGS)
}
/*
- * 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.
*/
if ((next_htup->t_infomask & HEAP_UPDATED) == 0)
{
@@ -665,15 +676,18 @@ verify_heapam(PG_FUNCTION_ARGS)
* tuple should be marked as a heap-only tuple. Conversely, if the
* current tuple isn't marked as HOT-updated, then the next tuple
* shouldn't be marked as a heap-only tuple.
+ *
+ * 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) &&
!HeapTupleHeaderIsHeapOnly(next_htup))
{
report_corruption(&ctx,
