Thread: Few comments on commit 857f9c36 (skip full index scans )

Hi,

Review comments on commit 857f9c36:
1.
@@ -2049,6 +2055,10 @@ _bt_newroot(Relation rel, Buffer lbuf, Buffer rbuf)
    metapg = BufferGetPage(metabuf);
    metad = BTPageGetMeta(metapg);

+   /* upgrade metapage if needed */
+   if (metad->btm_version < BTREE_VERSION)
+       _bt_upgrademetapage(metapg);
+

The metapage upgrade should be performed under critical section.

2.
@@ -191,6 +304,10 @@ _bt_getroot(Relation rel, int access)
        LockBuffer(metabuf, BUFFER_LOCK_UNLOCK);
        LockBuffer(metabuf, BT_WRITE);

+       /* upgrade metapage if needed */
+       if (metad->btm_version < BTREE_VERSION)
+           _bt_upgrademetapage(metapg);
+

Same as above.

In other cases like _bt_insertonpg, the upgrade is done inside the
critical section.  It seems the above cases are missed.

3.
+   TransactionId btm_oldest_btpo_xact; /* oldest btpo_xact among of
+                                        * deleted pages */

/among of/among all

Attached patch to fix the above comments.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Mon, May 28, 2018 at 4:52 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> Hi,
>
> Review comments on commit 857f9c36:
> 1.
> @@ -2049,6 +2055,10 @@ _bt_newroot(Relation rel, Buffer lbuf, Buffer rbuf)
>     metapg = BufferGetPage(metabuf);
>     metad = BTPageGetMeta(metapg);
>
> +   /* upgrade metapage if needed */
> +   if (metad->btm_version < BTREE_VERSION)
> +       _bt_upgrademetapage(metapg);
> +
>
> The metapage upgrade should be performed under critical section.
>
> 2.
> @@ -191,6 +304,10 @@ _bt_getroot(Relation rel, int access)
>         LockBuffer(metabuf, BUFFER_LOCK_UNLOCK);
>         LockBuffer(metabuf, BT_WRITE);
>
> +       /* upgrade metapage if needed */
> +       if (metad->btm_version < BTREE_VERSION)
> +           _bt_upgrademetapage(metapg);
> +
>
> Same as above.
>
> In other cases like _bt_insertonpg, the upgrade is done inside the
> critical section.  It seems the above cases are missed.
>
> 3.
> +   TransactionId btm_oldest_btpo_xact; /* oldest btpo_xact among of
> +                                        * deleted pages */
>
> /among of/among all
>
> Attached patch to fix the above comments.
>

Thank you for reviewing and the patch. All changes looks good to me.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

> The metapage upgrade should be performed under critical section.

Agree. But after close look I found that btm_version change isn't wal-logged 
(near line 2251 in _bt_newroot). So btm_version is not propagated to 
replica/backup/etc.
I believe it should be fixed.


-- 
Teodor Sigaev                                   E-mail: teodor@sigaev.ru
                                                    WWW: http://www.sigaev.ru/

On Wed, May 30, 2018 at 9:09 PM, Teodor Sigaev <teodor@sigaev.ru> wrote:
>> The metapage upgrade should be performed under critical section.
>
>
> Agree. But after close look I found that btm_version change isn't wal-logged
> (near line 2251 in _bt_newroot). So btm_version is not propagated to
> replica/backup/etc.
>

I think it will always be set to BTREE_VERSION (See _bt_restore_meta).
I see that other places like _bt_insertonpg,
_bt_update_meta_cleanup_info, etc. that log meta page contents don't
log version number, so if we want to log it, all such places also need
to be modified, but I don't see the need for same.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

> I think it will always be set to BTREE_VERSION (See _bt_restore_meta).
You are right, pushed. Thank you!

-- 
Teodor Sigaev                                   E-mail: teodor@sigaev.ru
                                                    WWW: http://www.sigaev.ru/