Re: Setting pd_lower in GIN metapage

Hi.

Trying to catch up.

On 2017/09/25 13:43, Michael Paquier wrote:
> On Sun, Sep 24, 2017 at 2:25 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> Added and updated the comments for both btree and hash index patches.
>
> I don't have real complaints about this patch, this looks fine to me.
>
> + * Currently, the advantage of setting pd_lower is in limited cases like
> + * during wal_consistency_checking or while logging for unlogged relation
> + * as for all other purposes, we initialize the metapage. Note, it also
> + * helps in page masking by allowing to mask unused space.
> I would have reworked this comment a bit, say like that:
> Setting pd_lower is useful for two cases which make use of WAL
> compressibility even if the meta page is initialized at replay:
> - Logging of init forks for unlogged relations.
> - wal_consistency_checking logs extra full-page writes, and this
> allows masking of the unused space of the page.
>
> Now I often get complains that I suck at this exercise ;)

So, I think I understand the discussion so far and the arguments about
what we should write to explain why we're setting pd_lower to the correct
value.

Just to remind, I didn't actually start this thread [1] to address the
issue that the FPWs of meta pages written to WAL are not getting
compressed. An external backup tool relies on pd_lower to give the
correct starting offset of the hole to compress, provided the page's other
fields suggest it has the standard page layout. Since we discovered that
pd_lower is not set correctly in gin, brin, and spgist meta pages, I
created patches to do the same. You (Michael) pointed out that that
actually helps compress their FPW images in WAL as well, so we began
considering that. Also, you pointed out that WAL checking code masks
pages based on the respective masking functions' assumptions about the
page's layout properties, which the original patches forgot to consider.
So, we updated the patches to change the respective masking functions to
mask meta pages as pages with standard page layout, too.

But as Tom pointed out [2], WAL compressibility benefit cannot be obtained
unless we change how the meta page is passed to xlog.c to be written to
WAL. So, we found out all the places that write/register the meta page to
WAL and changed the respective XLogRegisterBuffer calls to include the
REGBUG_STANDARD flag. Some of these calls already passed
REGBUF_WILL_INIT, which would result in no FPW image to be written to the
WAL and so there would be no question of compressibility. But, passing
REGBUF_STANDARD would still help the case where WAL checking is enabled,
because FPW image *is* written in that case.

So, ISTM, comments that the patches add should all say that setting the
meta pages' pd_lower to the correct value helps to pass those pages to
xlog.c as compressible standard layout pages, regardless of whether they
are actually passed that way. Even if the patches do take care of the
latter as well.

Did I miss something?

Looking at Amit K's updated patches for btree and hash, it seems that he
updated the comment to say that setting pd_lower to the correct value is
*essential*, because those pages are passed as REGBUF_STANDARD pages and
hence will be compressed. That seems to contradict what I wrote above,
but I think that's not wrong, too. So, I updated the gin, brin, and
spgist patches to say the same, viz. the following:

/*
* Set pd_lower just past the end of the metadata. This is essential,
* because without doing so, metadata will be lost if xlog.c compresses
* the page.
*/

Attached updated patches.

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/0d273805-0e9e-ec1a-cb84-d4da400b8f85%40lab.ntt.co.jp

[2] https://www.postgresql.org/message-id/22268.1504815869%40sss.pgh.pa.us