Re: Setting pd_lower in GIN metapage

On Fri, Sep 15, 2017 at 4:22 PM, Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> On 2017/09/14 16:00, Michael Paquier 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.
>>
>> OK, I took a closer look at all patches, but did not run any manual
>> tests to test the compression except some stuff with
>> wal_consistency_checking.
>
> Thanks for the review.
>
>> For SpGist, I think that there are two missing: spgbuild() and spgGetCache().
>
> spgbuild() calls SpGistInitMetapage() before marking the metapage buffer
> dirty. The latter already sets pd_lower correctly, so we don't need to do
> it explicitly in spgbuild() itself.

Check.

> spgGetCache() doesn't write the metapage, only reads it:
>
> /* Last, get the lastUsedPages data from the metapage */
> metabuffer = ReadBuffer(index, SPGIST_METAPAGE_BLKNO);
> LockBuffer(metabuffer, BUFFER_LOCK_SHARE);
>
> metadata = SpGistPageGetMeta(BufferGetPage(metabuffer));
>
> if (metadata->magicNumber != SPGIST_MAGIC_NUMBER)
> elog(ERROR, "index \"%s\" is not an SP-GiST index",
> RelationGetRelationName(index));
>
> cache->lastUsedPages = metadata->lastUsedPages;
>
> UnlockReleaseBuffer(metabuffer);
>
> So, I think it won't be correct to set pd_lower here, no?

Yeah, I am just reading the code again and there is no alarm here.

> Updated patch attached, which implements your suggested changes to the
> masking functions.
>
> By the way, as I noted on another unrelated thread, I will not be able to
> respond to emails from tomorrow until 9/23.

No problem. Enjoy your vacations.

I have spent some time looking at the XLOG insert code, and tested the
compressibility of the meta pages. And I have noticed that all pages
registered with REGBUF_WILL_INIT will force XLogRecordAssemble to not
take a FPW of the block registered because the page will be
reinitialized at replay, and in such cases the zero'ed page is filled
with the data from the record. log_newpage is used to initialize init
forks for unlogged relations, which is where this patch allows page
compression to take effect correctly. Still setting REGBUF_STANDARD
with REGBUF_WILL_INIT is actually a no-op, except if
wal_checking_consistency is used when registering a buffer for WAL
insertion. There is one code path though where things are useful all
the time: revmap_physical_extend for BRIN.

The patch is using the correct logic, still such comments are in my
opinion incorrect because of the reason written above:
+ * 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.
Here REGBUF_STANDARD is actually a no-op for btree.

+ /*
+ * Set pd_lower just past the end of the metadata. This is not
+ * essential but it makes the page look compressible to xlog.c,
+ * because we pass the buffer containing this page to
+ * XLogRegisterBuffer() as page with standard layout.
+ */
And here as well because of REGBUF_WILL_INIT is used.
--
Michael