Re: [WIP] Effective storage of duplicates in B-tree index.

04.02.2016 20:16, Peter Geoghegan:
> On Fri, Jan 29, 2016 at 8:50 AM, Anastasia Lubennikova
> <a.lubennikova@postgrespro.ru> wrote:
>> I fixed it in the new version (attached).

Thank you for the review.
At last, there is a new patch version 3.0. After some refactoring it 
looks much better.
I described all details of the compression in this document 
https://goo.gl/50O8Q0 
<https://vk.com/away.php?to=https%3A%2F%2Fgoo.gl%2F50O8Q0> (the same 
text without pictures is attached in btc_readme_1.0.txt).
Consider it as a rough copy of readme. It contains some notes about 
tricky moments of implementation and questions about future work.
Please don't hesitate to comment it.

> Some quick remarks on your V2.0:
>
> * Seems unnecessary that _bt_binsrch() is passed a real pointer by all
> callers. Maybe the one current posting list caller
> _bt_findinsertloc(), or its caller, _bt_doinsert(), should do this
> work itself:
>
> @@ -373,7 +377,17 @@ _bt_binsrch(Relation rel,
>       * scan key), which could be the last slot + 1.
>       */
>      if (P_ISLEAF(opaque))
> +   {
> +       if (low <= PageGetMaxOffsetNumber(page))
> +       {
> +           IndexTuple oitup = (IndexTuple) PageGetItem(page,
> PageGetItemId(page, low));
> +           /* one excessive check of equality. for possible posting
> tuple update or creation */
> +           if ((_bt_compare(rel, keysz, scankey, page, low) == 0)
> +               && (IndexTupleSize(oitup) + sizeof(ItemPointerData) <
> BTMaxItemSize(page)))
> +               *updposing = true;
> +       }
>          return low;
> +   }
>
> * ISTM that you should not use _bt_compare() above, in any case. Consider this:
>
> postgres=# select 5.0 = 5.000;
>   ?column?
> ──────────
>   t
> (1 row)
>
> B-Tree operator class indicates equality here. And yet, users will
> expect to see the original value in an index-only scan, including the
> trailing zeroes as they were originally input. So this should be a bit
> closer to HeapSatisfiesHOTandKeyUpdate() (actually,
> heap_tuple_attr_equals()), which looks for strict binary equality for
> similar reasons.
Thank you for the notice. Fixed.
> * Is this correct?:
>
> @@ -555,7 +662,9 @@ _bt_buildadd(BTWriteState *wstate, BTPageState
> *state, IndexTuple itup)
>           * it off the old page, not the new one, in case we are not at leaf
>           * level.
>           */
> -       state->btps_minkey = CopyIndexTuple(oitup);
> +       ItemId iihk = PageGetItemId(opage, P_HIKEY);
> +       IndexTuple hikey = (IndexTuple) PageGetItem(opage, iihk);
> +       state->btps_minkey = CopyIndexTuple(hikey);
>
> How this code has changed from the master branch is not clear to me.
Yes, it is. I completed the comment above.
> I understand that this code in incomplete/draft:
>
> +#define MaxPackedIndexTuplesPerPage    \
> +   ((int) ((BLCKSZ - SizeOfPageHeaderData) / \
> +           (sizeof(ItemPointerData))))
>
> But why is it different to the old (actually unchanged)
> MaxIndexTuplesPerPage? I would like to see comments explaining your
> understanding, even if they are quite rough. Why did GIN never require
> this change to a generic header (itup.h)? Should such a change live in
> that generic header file, and not another one more localized to
> nbtree?
I agree.

-- 
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company