Hi, Peter!
On Tue, Mar 3, 2020 at 3:04 AM Peter Geoghegan <p...@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
