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


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

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

Reply via email to