Re: [HACKERS] Setting pd_lower in GIN metapage

On Fri, Sep 15, 2017 at 4:22 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.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@lab.ntt.co.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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers