(()
On Wed, Apr 18, 2018 at 10:10 AM, Teodor Sigaev <teodor@sigaev.ru> wrote:
> I mostly agree with your patch, nice work, but I have some notices for your
> patch:
Thanks.
> 1)
> bt_target_page_check():
> if (!P_RIGHTMOST(topaque) &&
> !_bt_check_natts(state->rel, state->target, P_HIKEY))
>
> Seems not very obvious: it looks like we don't need to check nattrs on
> rightmost page. Okay, I remember that on rightmost page there is no hikey at
> all, but at least comment should added. Implicitly bt_target_page_check()
> already takes into account 'is page rightmost or not?' by using
> P_FIRSTDATAKEY, so, may be better to move rightmost check into
> bt_target_page_check() with some refactoring if-logic:
I don't understand. We do check the number of attributes on rightmost
pages, but we do so separately, in the main loop. For every item that
isn't the high key.
This code appears before the main bt_target_page_check() loop because
we're checking the high key itself, on its own, which is a new thing.
The high key is also involved in the loop (on non-rightmost pages),
but that's only because we check real items *against* the high key (we
assume the high key is good and that the item might be bad). The high
key is involved in every iteration of the main loop (on non-rightmost
pages), rather than getting its own loop.
That said, I am quite happy if you want to put a comment about this
being the rightmost page at the beginning of the check.
> 2)
> Style notice:
> ItemPointerSetInvalid(&trunctuple.t_tid);
> + BTreeTupSetNAtts(&trunctuple, 0);
> if (PageAddItem(page, (Item) &trunctuple, sizeof(IndexTupleData),
> P_HIKEY,
> It's better to have blank line between BTreeTupSetNAtts() and if clause.
Sure.
> 3) Naming BTreeTupGetNAtts/BTreeTupSetNAtts - several lines above we use
> full Tuple word in dowlink macroses, here we use just Tup. Seems, better to
> have Tuple in both cases. Or Tup, but still in both cases.
+1
> 4) BTreeTupSetNAtts - seems, it's better to add check of nattrs to fits to
> BT_N_KEYS_OFFSET_MASK mask, and it should not touch BT_RESERVED_OFFSET_MASK
> bits, now it will overwrite that bits.
An assertion sounds like it would be an improvement, though I don't
see that in the patch you posted.
> Attached patch is rebased to current head and contains some comment
> improvement in index_truncate_tuple() - you save some amount of memory with
> TupleDescCopy() call but didn't explain why pfree is enough to free all
> allocated memory.
Makes sense.
--
Peter Geoghegan