Re: HOT chain validation in verify_heapam()

On Thu, Nov 10, 2022 at 3:38 AM Andres Freund <andres(at)anarazel(dot)de> wrote:

>
> > + }
> > +
> > + /*
> > + * Loop over offset and populate predecessor array from
> all entries
> > + * that are present in successor array.
> > + */
> > + ctx.attnum = -1;
> > + for (ctx.offnum = FirstOffsetNumber; ctx.offnum <= maxoff;
> > + ctx.offnum = OffsetNumberNext(ctx.offnum))
> > + {
> > + ItemId curr_lp;
> > + ItemId next_lp;
> > + HeapTupleHeader curr_htup;
> > + HeapTupleHeader next_htup;
> > + TransactionId curr_xmax;
> > + TransactionId next_xmin;
> > +
> > + OffsetNumber nextoffnum = successor[ctx.offnum];
> > +
> > + curr_lp = PageGetItemId(ctx.page, ctx.offnum);
>
> Why do we get the item when nextoffnum is 0?
>

Fixed by moving PageGetItemId() call after the 'if' check.

> > + if (nextoffnum == 0 || !lp_valid[ctx.offnum] ||
> !lp_valid[nextoffnum])
> > + {
> > + /*
> > + * This is either the last updated tuple
> in the chain or a
> > + * corruption raised for this tuple.
> > + */
>
> "or a corruption raised" isn't quite right grammatically.
>

done.

>
> > + continue;
> > + }
> > + if (ItemIdIsRedirected(curr_lp))
> > + {
> > + next_lp = PageGetItemId(ctx.page,
> nextoffnum);
> > + if (ItemIdIsRedirected(next_lp))
> > + {
> > + report_corruption(&ctx,
> > +
> psprintf("redirected line pointer pointing to another redirected line
> pointer at offset %u",
> > +
> (unsigned) nextoffnum));
> > + continue;
> > + }
> > + next_htup = (HeapTupleHeader) PageGetItem(
> ctx.page, next_lp);
> > + if (!HeapTupleHeaderIsHeapOnly(next_htup))
> > + {
> > + report_corruption(&ctx,
> > +
> psprintf("redirected tuple at line pointer offset %u is not heap only
> tuple",
> > +
> (unsigned) nextoffnum));
> > + }
> > + if ((next_htup->t_infomask & HEAP_UPDATED)
> == 0)
> > + {
> > + report_corruption(&ctx,
> > +
> psprintf("redirected tuple at line pointer offset %u is not heap updated
> tuple",
> > +
> (unsigned) nextoffnum));
> > + }
> > + continue;
> > + }
> > +
> > + /*
> > + * Add a line pointer offset to the predecessor
> array if xmax is
> > + * matching with xmin of next tuple (reaching via
> its t_ctid).
> > + * Prior to PostgreSQL 9.4, we actually changed
> the xmin to
> > + * FrozenTransactionId
>
> I'm doubtful it's a good idea to try to validate the 9.4 case. The
> likelihood
> of getting that right seems low and I don't see us gaining much by even
> trying.
>
>
>
removed code with regards to frozen tuple checks.

> so we must add offset to predecessor
> > + * array(irrespective of xmax-xmin matching) if
> updated tuple xmin
> > + * is frozen, so that we can later do validation
> related to frozen
> > + * xmin. Raise corruption if we have two tuples
> having the same
> > + * predecessor.
> > + * We add the offset to the predecessor array
> irrespective of the
> > + * transaction (t_xmin) status. We will do
> validation related to
> > + * the transaction status (and also all other
> validations) when we
> > + * loop over the predecessor array.
> > + */
> > + curr_htup = (HeapTupleHeader) PageGetItem(ctx.page,
> curr_lp);
> > + curr_xmax = HeapTupleHeaderGetUpdateXid(curr_htup);
> > + next_lp = PageGetItemId(ctx.page, nextoffnum);
> > + next_htup = (HeapTupleHeader) PageGetItem(ctx.page,
> next_lp);
> > + next_xmin = HeapTupleHeaderGetXmin(next_htup);
> > + if (TransactionIdIsValid(curr_xmax) &&
> > + (TransactionIdEquals(curr_xmax, next_xmin)
> ||
> > + next_xmin == FrozenTransactionId))
> > + {
> > + if (predecessor[nextoffnum] != 0)
> > + {
> > + report_corruption(&ctx,
> > +
> psprintf("updated version at offset %u is also the updated version of
> tuple at offset %u",
> > +
> (unsigned) nextoffnum, (unsigned) predecessor[nextoffnum]));
> > + continue;
>
> I doubt it is correct to enter this path with next_xmin ==
> FrozenTransactionId. This is following a ctid chain that we normally
> wouldn't
> follow, because it doesn't satisfy the t_self->xmax == t_ctid->xmin
> condition.
>
> removed this frozen check.

> + }
> > +
> > + /* Loop over offsets and validate the data in the
> predecessor array. */
> > + for (OffsetNumber currentoffnum = FirstOffsetNumber;
> currentoffnum <= maxoff;
> > + currentoffnum = OffsetNumberNext(currentoffnum))
> > + {
> > + HeapTupleHeader pred_htup;
> > + HeapTupleHeader curr_htup;
> > + TransactionId pred_xmin;
> > + TransactionId curr_xmin;
> > + ItemId pred_lp;
> > + ItemId curr_lp;
> > +
> > + ctx.offnum = predecessor[currentoffnum];
> > + ctx.attnum = -1;
> > +
> > + if (ctx.offnum == 0)
> > + {
> > + /*
> > + * Either the root of the chain or an
> xmin-aborted tuple from
> > + * an abandoned portion of the HOT chain.
> > + */
>
> Hm - couldn't we check that the tuple could conceivably be at the root of a
> chain? I.e. isn't HEAP_HOT_UPDATED? Or alternatively has an aborted xmin?
>
> Done, I have added code to identify cases of missing offset in the
predecessor[] array and added validation that root of the chain must not be
HEAP_ONLY_TUPLE.

>
> > + continue;
> > + }
> > +
> > + curr_lp = PageGetItemId(ctx.page, currentoffnum);
> > + curr_htup = (HeapTupleHeader) PageGetItem(ctx.page,
> curr_lp);
> > + curr_xmin = HeapTupleHeaderGetXmin(curr_htup);
> > +
> > + ctx.itemid = pred_lp = PageGetItemId(ctx.page,
> ctx.offnum);
> > + pred_htup = (HeapTupleHeader) PageGetItem(ctx.page,
> pred_lp);
> > + pred_xmin = HeapTupleHeaderGetXmin(pred_htup);
> > +
> > + /*
> > + * If the predecessor's xmin is aborted or in
> progress, the
> > + * current tuples xmin should be aborted or in
> progress
> > + * respectively. Also both xmin's must be equal.
> > + */
> > + if (!TransactionIdEquals(pred_xmin, curr_xmin) &&
> > + !TransactionIdDidCommit(pred_xmin))
> > + {
> > + report_corruption(&ctx,
> > +
> psprintf("tuple with uncommitted xmin %u was updated to produce a tuple at
> offset %u with differing xmin %u",
> > +
> (unsigned) pred_xmin, (unsigned) currentoffnum, (unsigned)
> curr_xmin));
>
> Is this necessarily true? What about a tuple that was inserted in a
> subtransaction and then updated in another subtransaction of the same
> toplevel
> transaction?
>
>
patch has been updated to handle cases of sub-transaction.

--
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com