Re: Setting pd_lower in GIN metapage

On Thu, Sep 14, 2017 at 12:30 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Wed, Sep 13, 2017 at 4:43 PM, Amit Langote
> <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> Sure, no problem.
>

>
> + *
> + * This won't be of any help unless we use option like REGBUF_STANDARD
> + * while registering the buffer for metapage as otherwise, it won't try to
> + * remove the hole while logging the full page image.
> */
> This comment is in the btree code. But you actually add
> REGBUF_STANDARD. So I think that this could be just removed.
>
> * Set pd_lower just past the end of the metadata. This is not essential
> - * but it makes the page look compressible to xlog.c.
> + * but it makes the page look compressible to xlog.c. See
> + * _bt_initmetapage.
> This reference could be removed as well as _bt_initmetapage does not
> provide any information, the existing comment is wrong without your
> patch, and then becomes right with this patch.
>

I have added this comment just to add some explanation as to why we
are setting pd_lower and what makes it useful. We can change it or
remove it, but I am not sure what is the right thing to do here, may
be we can defer this to the committer.

>
> It seems to me that an update of pd_lower is missing in _bt_getroot(),
> just before marking the buffer as dirty I think. And there is a second
> in _bt_unlink_halfdead_page(), a third in _bt_insertonpg() and finally
> one in _bt_newroot().
>
>
> For hash, hashbulkdelete(), _hash_vacuum_one_page(),
> _hash_addovflpage(), _hash_freeovflpage() and _hash_doinsert() are
> missing the shot, no? We could have a meta page of a hash index
> upgraded from PG10.
>

Why do we need to change metapage at every place for btree or hash?
Any index that is upgraded should have pd_lower set, do you have any
case in mind where it won't be set? For hash, if someone upgrades
from a version lower than 9.6, it might not have set, but we already
give warning to reindex the hash indexes upgraded from a version lower
than 10.

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