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(dot)lubennikova(at)postgrespro(dot)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