Re: Setting pd_lower in GIN metapage

On Fri, Sep 8, 2017 at 1:54 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> On Thu, Sep 7, 2017 at 2:40 PM, Amit Langote
>> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>>> On 2017/09/07 13:09, Michael Paquier wrote:
>>>> pd_lower should remain at 0 on pre-10 servers.
>
>>> Doesn't PageInit(), which is where any page gets initialized, has always
>>> set pd_lower to SizeOfPageHeaderData?
>
>> Yes, sorry. I had a mind slippery here..
>
> Indeed, which raises the question of how did any of this code work at all?
> Up to now, these pages have been initialized with pd_lower =
> SizeOfPageHeaderData, *not* zero.  Which means that the FPI code would
> have considered the metadata to be part of a valid "hole" and not stored
> it, if we'd ever told the FPI code that the metadata page was of standard
> format.  I've just looked through the code for these three index types and
> can't find any place where they do that --- for instance, they don't pass
> REGBUF_STANDARD to XLogRegisterBuffer when writing their metapages, and
> they pass page_std = false to log_newpage when using that for metapages.
> Good thing, because they'd be completely broken otherwise.
>
> This means that the premise of this patch is wrong.  Adjusting pd_lower,
> by itself, would accomplish precisely zip for WAL compression, because
> we'd still not be telling the WAL code to compress out the hole.
>
> To get any benefit, I think we'd need to do all of the following:
>
> 1. Initialize pd_lower correctly in the metapage init functions, as here.
>
> 2. Any place we are about to write the metapage, set its pd_lower to the
> correct value, in case it's an old index that doesn't have that set
> correctly.  Fortunately this is cheap enough that we might as well just
> do it unconditionally.
>
> 3. Adjust all the xlog calls to tell them the page is of standard format.
>
> Now, one advantage we'd get from this is that we could say confidently
> that any index metapage appearing in a WAL stream generated by v11 or
> later has got the right pd_lower; therefore we could dispense with
> checking for wrong pd_lower in the mask functions (unless they're used
> in some way I don't know about, which is surely possible).
>
> BTW, while nbtree correctly initializes pd_lower, it looks to me like it
> is not exploiting that, because it seems never to pass REGBUF_STANDARD for
> the metapage anyway.
>

During the creation of the index, it uses log_newpage to log metapage
and there it uses REGBUF_STANDARD.  So, there seems to be some use of
it.

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