Re: Setting pd_lower in GIN metapage

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
> On Thu, Sep 7, 2017 at 2:40 PM, Amit Langote
> <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)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. I think this doesn't matter performance-wise for
nbtree, because it seems to always pass REGBUF_WILL_INIT instead.
But if we do likewise in other index types then they aren't really going
to win anyway. GIN at least looks like it might do that; I have not
gone through any of the index types in detail.

In short, this patch needs a significant rewrite, and more analysis than
you've done so far on whether there's actually any benefit to be gained.
It might not be worth messing with.

I'll set the CF entry back to Waiting on Author.

regards, tom lane