From: Anastasia Lubennikova <a.lubennikova@postgrespro.ru>
Subject: Re: WIP: Covering + unique indexes.
To: Amit Kapila <amit.kapila16@gmail.com>
References: <56168952.4010101@postgrespro.ru>
 <CA+TgmoZ3SUbfGBFas7=G8Fs5S6uzbqgrWE_PFwQPD-=E7-2EOA@mail.gmail.com>
 <20160814171131.21390.75752.pgcf@coridan.postgresql.org>
 <f90aa60a-b67f-95b5-d9f5-f5d8ced178c6@postgrespro.ru>
 <CAA4eK1+0jLkz8P7MVsUAj+RHmXm4BWBjV8Cc+_-Hsyhgth9ELA@mail.gmail.com>
Cc: Andrey Borodin <amborodin@acm.org>,
 pgsql-hackers <pgsql-hackers@postgresql.org>,
 Anastasia Lubennikova <lubennikovaav@gmail.com>
Message-ID: <e3d49c6f-3d66-ffa3-29ad-c4719350f049@postgrespro.ru>
Date: Tue, 6 Sep 2016 19:48:31 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.2.0
MIME-Version: 1.0
In-Reply-To: 
 <CAA4eK1+0jLkz8P7MVsUAj+RHmXm4BWBjV8Cc+_-Hsyhgth9ELA@mail.gmail.com>
Content-Type: multipart/alternative;
 boundary="------------4F65F546808092FB37F761DB"
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

This is a multi-part message in MIME format.
--------------4F65F546808092FB37F761DB
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit

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


--------------4F65F546808092FB37F761DB
Content-Type: text/html; charset=utf-8
Content-Transfer-Encoding: 8bit

<html>
  <head>
    <meta content="text/html; charset=utf-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    28.08.2016 09:13, Amit Kapila:<br>
    <blockquote
cite="mid:CAA4eK1+0jLkz8P7MVsUAj+RHmXm4BWBjV8Cc+_-Hsyhgth9ELA@mail.gmail.com"
      type="cite">
      <pre wrap="">On Mon, Aug 15, 2016 at 8:15 PM, Anastasia Lubennikova
<a class="moz-txt-link-rfc2396E" href="mailto:a.lubennikova@postgrespro.ru">&lt;a.lubennikova@postgrespro.ru&gt;</a> wrote:
@@ -590,7 +622,14 @@ _bt_buildadd(BTWriteState *wstate, BTPageState
*state, IndexTuple itup)
  if (last_off == P_HIKEY)
  {
  Assert(state-&gt;btps_minkey == NULL);
- state-&gt;btps_minkey = CopyIndexTuple(itup);
+ /*
+ * Truncate the tuple that we're going to insert
+ * into the parent page as a downlink
+ */

+ if (indnkeyatts != indnatts &amp;&amp; P_ISLEAF(pageop))
+ state-&gt;btps_minkey = index_truncate_tuple(wstate-&gt;index, itup);
+ else
+ state-&gt;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:
</pre>
    </blockquote>
    <br>
    Thank you for the question. Investigation took a long time)<br>
    <br>
    As far as I understand, the code above only applies to<br>
    the first tuple of each level. While the code you have quoted below<br>
    truncates high keys for all other pages.<br>
    <br>
    There is a comment that clarifies situation:<br>
        /*<br>
         * If the new item is the first for its page, stash a copy for
    later. Note<br>
         * this will only happen for the first item on a level; on later
    pages,<br>
         * the first item for a page is copied from the prior page in
    the code<br>
         * above.<br>
         */<br>
    <br>
    <br>
    So the patch is correct.<br>
    We can go further and remove this index_truncate_tuple() call,
    because<br>
    the first key of any inner (or root) page doesn't need any key at
    all.<br>
    It simply points out to the leftmost page of the level below.<br>
    But it's not a bug, because truncation of one tuple per level
    doesn't<br>
    add any considerable overhead. So I want to leave the patch in its
    current state.<br>
    <br>
    <blockquote
cite="mid:CAA4eK1+0jLkz8P7MVsUAj+RHmXm4BWBjV8Cc+_-Hsyhgth9ELA@mail.gmail.com"
      type="cite">
      <pre wrap="">@@ -510,6 +513,8 @@ _bt_buildadd(BTWriteState *wstate, BTPageState
*state, IndexTuple itup)
{
..
+ if (indnkeyatts != indnatts &amp;&amp; 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-&gt;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-&gt;index));
+ }
+
..
..
</pre>
    </blockquote>
    <br>
    <br>
    <pre class="moz-signature" cols="72">-- 
Anastasia Lubennikova
Postgres Professional: <a class="moz-txt-link-freetext" href="http://www.postgrespro.com">http://www.postgrespro.com</a>
The Russian Postgres Company</pre>
  </body>
</html>

--------------4F65F546808092FB37F761DB--