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 (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
