Re: HOT chain validation in verify_heapam()

On Wed, Mar 8, 2023 at 7:06 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

>
> > + /* HOT chains should not intersect. */
> > + if (predecessor[nextoffnum] != InvalidOffsetNumber)
> > + {
> > + report_corruption(&ctx,
> > + psprintf("redirect line pointer
> > points to offset %u, but offset %u also points there",
> > + (unsigned) nextoffnum,
> > (unsigned) predecessor[nextoffnum]));
> > + continue;
> > + }
> > ```
> >
> > This type of corruption doesn't seem to be test-covered.
>
> Himanshu, would you be able to try to write a test case for this? I
> think you need something like this: update a tuple with a lower TID to
> produce a tuple with a higher TID, e.g. (0,10) is updated to produce
> (0,11). But then have a redirect line pointer that also points to the
> result of the update, in this case (0,11).
>
> Sure Robert, I will work on this.

> > ```
> > + /*
> > + * If the next line pointer is a redirect, or if it's a
> tuple
> > + * but the XMAX of this tuple doesn't match the XMIN of the
> next
> > + * tuple, then the two aren't part of the same update chain
> and
> > + * there is nothing more to do.
> > + */
> > + if (ItemIdIsRedirected(next_lp))
> > + continue;
> > ```
> >
> > lcov shows that the `continue` path is never executed. This is
> > probably not a big deal however.
>
> It might be good to have a negative test case for this, though. Let's
> say we, e.g. update (0,1) to produce (0,2), but then abort. The page
> is HOT-pruned. Then we add insert a new tuple at (0,2), HOT-update it
> to produce (0,3), and commit. Then we HOT-prune again. Possibly we
> could try to write a test case that verifies that this does NOT
> produce any corruption indication.
>
> will work on this too.

> > ```
> > +$node->append_conf('postgresql.conf','max_prepared_transactions=100');
> > ```
> >
> > From what I can tell this line is not needed.
>
> That surprises me, because the new test cases involve preparing a
> transaction, and by default max_prepared_transactions=0. So it seems
> to me (without testing) that this ought to be required. Did you test
> that it works without this setting?
>
> The value of 100 seems a bit excessive, though. Most TAP tests seem to use
> 10.
>
> We need this for prepare transaction, will change it to 10.

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