Re: Improve search for missing parent downlinks in amcheck - Mailing list pgsql-hackers

Hi, Peter!

On Tue, Mar 3, 2020 at 3:04 AM Peter Geoghegan <pg@bowt.ie> wrote:
> Apologies for the delayed response. I was a little tired from the
> deduplication project.

No problem.  Apologies for the delayed revision as well.

> I taught pageinspect to display a "htid" field for pivot tuples
> recently, making it easier to visualize this example.

Great!

> I think that you should say more about how "lowkey" is used here:
>
> >         /*
> > -        * Record if page that is about to become target is the right half of
> > -        * an incomplete page split.  This can go stale immediately in
> > -        * !readonly case.
> > +        * Copy current target low key as the high key of right sibling.
> > +        * Allocate memory in upper level context, so it would be cleared
> > +        * after reset of target context.
> > +        *
> > +        * We only need low key for parent check.
> >          */
> > -       state->rightsplit = P_INCOMPLETE_SPLIT(opaque);
> > +       if (state->readonly && !P_RIGHTMOST(opaque))
> > +       {
>
> Say something about concurrent page splits, since they're the only
> case where we actually use lowkey. Maybe say something like: "We
> probably won't end up doing anything with lowkey, but it's simpler for
> readonly verification to always have it available".

I've revised this comment.  Hopefully it's better now.

> * I think that these comments could still be clearer:
>
> > +               /*
> > +                * We're going to try match child high key to "negative
> > +                * infinity key".  This normally happens when the last child
> > +                * we visited for target's left sibling was an incomplete
> > +                * split. So, we must be still on the child of target's left
> > +                * sibling. Thus, we should match to target's left sibling
> > +                * high key. Thankfully we saved it, it's called a "low key".
> > +                */
>
> Maybe start with "We cannot try to match child's high key to a
> negative infinity key in target, since there is nothing to compare.
> However...". Perhaps use terms like "cousin page" and "subtree", which
> can be useful. Alternatively, mention this case in the diagram example
> at the top of bt_child_highkey_check(). It's tough to write comments
> like this, but I think it's worth it.

I've updated this comment using terms "cousin page" and "subtree".  I
didn't refer the diagram example, because it doesn't contain
appropriate case.  And I wouldn't like this diagram to contain such
case, because that probably makes this diagram too complex.  I've also
invented term "uncle page". BTW, should it be "aunt page"?  I don't
know.

> Note that a high key is also a pivot tuple, so I wouldn't mention high
> keys here:
>
> > +/*
> > + * Check if two tuples are binary identical except the block number.  So,
> > + * this function is capable to compare high keys with pivot keys.
> > + */
> > +static bool
> > +bt_pivot_tuple_identical(IndexTuple itup1, IndexTuple itup2)
> > +{

Sure, this comment is revised.

> v7 looks pretty close to being commitable, though I'll probably want
> to update some comments that you haven't touched when you commit this.
> I should probably wait until you've committed the patch to go do that.
> I'm thinking of things like old comments in bt_downlink_check().
>
> I will test the patch properly one more time when you produce a new
> revision. I haven't really tested it since the last time.

Attached patch also has revised commit message.  I'll wait for your
response before commit.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company