Re: Setting pd_lower in GIN metapage

On Mon, Sep 25, 2017 at 3:48 PM, Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Trying to catch up.

Glad to see you back.

> 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.

Thanks for summarizing all that happened.

> 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.

Yep.

> 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?

Not that I think of.

Buffer metabuffer;
+ Page metapage;
SpGistMetaPageData *metadata;

metabuffer = ReadBuffer(index, SPGIST_METAPAGE_BLKNO);
+ metapage = BufferGetPage(metabuffer);
No need to define metapage here and to call BufferGetPage() as long as
the lock on the buffer is not taken.

Except that small thing, the patches do their duty.
--
Michael