Re: WIP: Covering + unique indexes.

28.08.2016 09:13, Amit Kapila:
> On Mon, Aug 15, 2016 at 8:15 PM, Anastasia Lubennikova
> <a.lubennikova@postgrespro.ru>  wrote:
> @@ -590,7 +622,14 @@ _bt_buildadd(BTWriteState *wstate, BTPageState
> *state, IndexTuple itup)
>    if (last_off == P_HIKEY)
>    {
>    Assert(state->btps_minkey == NULL);
> - state->btps_minkey = CopyIndexTuple(itup);
> + /*
> + * Truncate the tuple that we're going to insert
> + * into the parent page as a downlink
> + */
>
> + if (indnkeyatts != indnatts && P_ISLEAF(pageop))
> + state->btps_minkey = index_truncate_tuple(wstate->index, itup);
> + else
> + state->btps_minkey = CopyIndexTuple(itup);
>
> It seems that above code always ensure that for leaf pages, high key
> is a truncated tuple.  What is less clear is if that is true, why you
> need to re-ensure it again for the old  page in below code:

Thank you for the question. Investigation took a long time)

As far as I understand, the code above only applies to
the first tuple of each level. While the code you have quoted below
truncates high keys for all other pages.

There is a comment that clarifies situation:
     /*
      * If the new item is the first for its page, stash a copy for 
later. Note
      * this will only happen for the first item on a level; on later pages,
      * the first item for a page is copied from the prior page in the code
      * above.
      */


So the patch is correct.
We can go further and remove this index_truncate_tuple() call, because
the first key of any inner (or root) page doesn't need any key at all.
It simply points out to the leftmost page of the level below.
But it's not a bug, because truncation of one tuple per level doesn't
add any considerable overhead. So I want to leave the patch in its 
current state.

> @@ -510,6 +513,8 @@ _bt_buildadd(BTWriteState *wstate, BTPageState
> *state, IndexTuple itup)
> {
> ..
> + if (indnkeyatts != indnatts && P_ISLEAF(opageop))
> + {
> + /*
> + * It's essential to truncate High key here.
> + * The purpose is not just to save more space on this particular page,
> + * but to keep whole b-tree structure consistent. Subsequent insertions
> + * assume that hikey is already truncated, and so they should not
> + * worry about it, when copying the high key into the parent page
> + * as a downlink.
> + * NOTE It is not crutial for reliability in present,
> + * but maybe it will be that in the future.
> + */
> + keytup = index_truncate_tuple(wstate->index, oitup);
> +
> + /*  delete "wrong" high key, insert keytup as P_HIKEY. */
> + PageIndexTupleDelete(opage, P_HIKEY);
> +
> + if (!_bt_pgaddtup(opage, IndexTupleSize(keytup), keytup, P_HIKEY))
> + elog(ERROR, "failed to rewrite compressed item in index \"%s\"",
> + RelationGetRelationName(wstate->index));
> + }
> +
> ..
> ..


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