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