Re: WIP: Covering + unique indexes. - Mailing list pgsql-hackers

> First, there is the question of risks, or costs. I think that this
I hope that's acceptable risk.

> * It's possible that something was missed in the optimizer. I'm not sure.
> 
> I share the intuition that very little code is actually needed there,
> but I'm far from the best person to judge whether or not some subtle
> detail was missed.
Of course, it's possible but some variant of this patch is already used 
in production environment and we didn't face  with planer issues. Of 
course it could be, but if so then they are so deep that I doubt that 
they can be found easily.


> 
> * This seems out of date:
> 
>> +            * NOTE: It is not crucial for reliability in present, but maybe
>> +            * it will be that in the future. Now the purpose is just to save
>> +            * more space on inner pages of btree.
removed

> 
> * CheckIndexCompatible() didn't seem to get the memo about this patch.
> Maybe just a comment?
improved

> * I was wrong to suggest _bt_isequal() has an assertion against
> truncation. It is called for the highkey. Suggest you weaken the
> assertion, so it only applies when the offset isn't P_HIKEY on
> non-rightmost page.
Fixed
> 
> * Suggest adding a comment above BTStackData, about bts_btentry + offset.
see below

> 
> * Suggest breaking BTEntrySame() into 3 lines, not 2.
see below

> 
> * This comment needs to be updated:
> /* get high key from left page == lowest key on new right page */
> Suggest "get high key from left page == lower bound for new right page".
fixed

> 
> * This comment needs to be updated:
> 13th bit: unused
> 
> Suggest "13th bit: AM-defined meaning"
done

> * Suggest adding a note that the use of P_HIKEY here is historical,
> since it isn't used to match downlinks:
> 
>          /*
>           * Find the parent buffer and get the parent page.
>           *
>           * Oops - if we were moved right then we need to change stack item! We
>           * want to find parent pointing to where we are, right ?    - vadim
>           * 05/27/97
>           */
>          ItemPointerSet(&(stack->bts_btentry.t_tid), bknum, P_HIKEY);
>          pbuf = _bt_getstackbuf(rel, stack, BT_WRITE);
On close look, bts_btentry.ip_posid is not used anymore, I change 
bts_btentry type to BlockNumber. As result, BTEntrySame() is removed.


> * The docs need some more polishing. Didn't spend very much time on this at all.
Suppose, it should be some native English speaker, definitely not me.


I'm not very happy with massive usage of 
ItemPointerGetBlockNumberNoCheck(&(itup->t_tid)), suggest to  wrap it to 
macro something like this:
#define BTreeInnerTupleGetDownLink(itup) \
    ItemPointerGetBlockNumberNoCheck(&(itup->t_tid))

It will be nice to add assert checking in this macro about inner tuple 
or not, but, as I can see, it's impossible - inner and leaf tuples are 
indistinguishable. So I add pair 
BTreeInnerTupleGetDownLink/TreeInnerTupleSetDownLink except a few places.


If there isn't strong objection, I intend to push it this evening.

-- 
Teodor Sigaev                      E-mail: teodor@sigaev.ru
                                       WWW: http://www.sigaev.ru/