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

Hi,

To start with: I think this is an extremely helpful and important
feature. Both for checking production systems and for finding problems during
development.


> From 08fe01f5073c0a850541265494bb4a875bec7d3f Mon Sep 17 00:00:00 2001
> From: Himanshu Upadhyaya <himanshu.upadhyaya@enterprisedb.com>
> Date: Fri, 30 Sep 2022 17:44:56 +0530
> Subject: [PATCH v6] Implement HOT chain validation in verify_heapam()
> 
> Himanshu Upadhyaya, reviewed by Robert Haas, Aleksander Alekseev
> 
> Discussion: https://postgr.es/m/CAPF61jBBR2-iE-EmN_9v0hcQEfyz_17e5Lbb0%2Bu2%3D9ukA9sWmQ%40mail.gmail.com
> ---
>  contrib/amcheck/verify_heapam.c           | 207 ++++++++++++++++++++++
>  src/bin/pg_amcheck/t/004_verify_heapam.pl | 192 ++++++++++++++++++--
>  2 files changed, 388 insertions(+), 11 deletions(-)
> 
> diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
> index c875f3e5a2..007f7b2f37 100644
> --- a/contrib/amcheck/verify_heapam.c
> +++ b/contrib/amcheck/verify_heapam.c
> @@ -399,6 +399,9 @@ verify_heapam(PG_FUNCTION_ARGS)
>      for (ctx.blkno = first_block; ctx.blkno <= last_block; ctx.blkno++)
>      {
>          OffsetNumber maxoff;
> +        OffsetNumber predecessor[MaxOffsetNumber] = {0};
> +        OffsetNumber successor[MaxOffsetNumber] = {0};
> +        bool        lp_valid[MaxOffsetNumber] = {false};
>  
>          CHECK_FOR_INTERRUPTS();
>  
> @@ -433,6 +436,8 @@ verify_heapam(PG_FUNCTION_ARGS)
>          for (ctx.offnum = FirstOffsetNumber; ctx.offnum <= maxoff;
>               ctx.offnum = OffsetNumberNext(ctx.offnum))
>          {
> +            OffsetNumber nextoffnum;
> +
>              ctx.itemid = PageGetItemId(ctx.page, ctx.offnum);
>  
>              /* Skip over unused/dead line pointers */
> @@ -469,6 +474,13 @@ verify_heapam(PG_FUNCTION_ARGS)
>                      report_corruption(&ctx,
>                                        psprintf("line pointer redirection to unused item at offset %u",
>                                                 (unsigned) rdoffnum));
> +
> +                /*
> +                 * make entry in successor array, redirected tuple will be
> +                 * validated at the time when we loop over successor array
> +                 */
> +                successor[ctx.offnum] = rdoffnum;
> +                lp_valid[ctx.offnum] = true;
>                  continue;
>              }
>  
> @@ -504,9 +516,197 @@ verify_heapam(PG_FUNCTION_ARGS)
>              /* It should be safe to examine the tuple's header, at least */
>              ctx.tuphdr = (HeapTupleHeader) PageGetItem(ctx.page, ctx.itemid);
>              ctx.natts = HeapTupleHeaderGetNatts(ctx.tuphdr);
> +            lp_valid[ctx.offnum] = true;
>  
>              /* Ok, ready to check this next tuple */
>              check_tuple(&ctx);
> +
> +            /*
> +             * Add the data to the successor array if next updated tuple is in
> +             * the same page. It will be used later to generate the
> +             * predecessor array.
> +             *
> +             * We need to access the tuple's header to populate the
> +             * predecessor array. However the tuple is not necessarily sanity
> +             * checked yet so delaying construction of predecessor array until
> +             * all tuples are sanity checked.
> +             */
> +            nextoffnum = ItemPointerGetOffsetNumber(&(ctx.tuphdr)->t_ctid);
> +            if (ItemPointerGetBlockNumber(&(ctx.tuphdr)->t_ctid) == ctx.blkno &&
> +                nextoffnum != ctx.offnum)
> +            {
> +                successor[ctx.offnum] = nextoffnum;
> +            }

I don't really understand this logic - why can't we populate the predecessor
array, if we can construct a successor entry?


> +        }
> +
> +        /*
> +         * 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?


> +            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.


> +                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
atoffset %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.


> 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.

I don't immediately see what prevents the frozen tuple being from an entirely
different HOT chain than the two tuples pointing to it.




> +        }
> +
> +        /* 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?


> +                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
%uwith 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?


> +            }
> +
> +            /*
> +             * If the predecessor's xmin is not frozen, then current tuple's
> +             * shouldn't be either.
> +             */
> +            if (pred_xmin != FrozenTransactionId && curr_xmin == FrozenTransactionId)
> +            {
> +                report_corruption(&ctx,
> +                                  psprintf("unfrozen tuple was updated to produce a tuple at offset %u which is
frozen",
> +                                           (unsigned) currentoffnum));
> +            }

Can't we have a an update chain that is e.g.
xmin 10, xmax 5 -> xmin 5, xmax invalid

and a vacuum cutoff of 7? That'd preent the first tuple from being removed,
but would allow 5 to be frozen.

I think there were recent patches proposing we don't freeze in that case, but
we'll having done that in the past....


Greetings,

Andres Freund