Re: HOT chain validation in verify_heapam()

On Wed, Feb 8, 2023 at 11:17 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Sun, Feb 5, 2023 at 3:57 AM Himanshu Upadhyaya
> <upadhyaya(dot)himanshu(at)gmail(dot)com> wrote:
> > Thanks, yes it's working fine with Prepared Transaction.
> > Please find attached the v9 patch incorporating all the review comments.
>
> I don't know quite how we're still going around in circles about this,
> but this code makes no sense to me at all:
>
> /*
> * Add data to the predecessor array even if the current or
> * successor's LP is not valid. We will not process/validate
> these
> * offset entries while looping over the predecessor array but
> * having all entries in the predecessor array will help in
> * identifying(and validating) the Root of a chain.
> */
> if (!lp_valid[ctx.offnum] || !lp_valid[nextoffnum])
> {
> predecessor[nextoffnum] = ctx.offnum;
> continue;
> }
>
> If the current offset number is not for a valid line pointer, then it
> makes no sense to talk about the successor. An invalid redirected line
> pointer is one that points off the end of the line pointer array, or
> to before the beginning of the line pointer array, or to a line
> pointer that is unused. An invalid line pointer that is LP_USED is one
> which points to a location outside the page, or to a location inside
> the page. In none of these cases does it make any sense to talk about
> the next tuple. If the line pointer isn't valid, it's pointing to some
> invalid location where there cannot possibly be a tuple. In other
> words, if lp_valid[ctx.offnum] is false, then nextoffnum is a garbage
> value, and therefore referencing predecessor[nextoffnum] is useless
> and dangerous.
>
> If the next offset number is not for a valid line pointer, we could in
> theory still assign to the predecessor array, as you propose here. In
> that case, the tuple or line pointer at ctx.offnum is pointing to the
> line pointer at nextoffnum and that is all fine. But what is the
> point? The comment claims that the point is that it will help us
> identify and validate the root of the hot chain. But if the line
> pointer at nextoffnum is not valid, it can't be the root of a hot
> chain. When we're talking about the root of a HOT chain, we're
> speaking about a tuple. If lp_valid[nextoffnum] is false, there is no
> tuple. Instead of pointing to a tuple, that line pointer is pointing
> to garbage.
>
>
Initially while implementing logic to identify the root of the HOT chain
I was getting crash and regression failure's that time I thought of having
this check along with a few other changes that were required,
but you are right, it's unnecessary to add data to the predecessor
array(in this case) and is not required. I am removing this from the patch.

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