|From:||Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>|
|To:||Michael Paquier <michael(dot)paquier(at)gmail(dot)com>|
|Cc:||Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||Re: Setting pd_lower in GIN metapage|
|Views:||Raw Message | Whole Thread | Download mbox|
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
Thanks for the review.
> + if (opaque->flags & GIN_DELETED)
> + mask_page_content(page);
> + else if (pagehdr->pd_lower != 0)
> + mask_unused_space(page);
> + /* Mask the unused space, provided the page's pd_lower is set. */
> + if (pagehdr->pd_lower != 0)
> For the masking functions, shouldn't those check use (pd_lower >
> SizeOfPageHeaderData) instead of 0? pd_lower gets set to a non-zero
> value on HEAD, so you would apply the masking even if the meta page is
> upgraded from an instance that did not enforce the value of pd_lower
> later on. Those conditions also definitely need comments. That will be
> a good reminder so as why it needs to be kept.
> + *
> + * 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.
> This comment is in the btree code. But you actually add
> REGBUF_STANDARD. So I think that this could be just removed.
> * Set pd_lower just past the end of the metadata. This is not essential
> - * but it makes the page look compressible to xlog.c.
> + * but it makes the page look compressible to xlog.c. See
> + * _bt_initmetapage.
> This reference could be removed as well as _bt_initmetapage does not
> provide any information, the existing comment is wrong without your
> patch, and then becomes right with this patch.
Amit K's reply may have addressed these comments.
> After that I have spotted a couple of places for btree, hash and
> SpGist where the updates of pd_lower are not happening. Let's keep in
> mind that updated meta pages could come from an upgraded version, so
> we need to be careful here about all code paths updating meta pages,
> and WAL-logging them.
> It seems to me that an update of pd_lower is missing in _bt_getroot(),
> just before marking the buffer as dirty I think. And there is a second
> in _bt_unlink_halfdead_page(), a third in _bt_insertonpg() and finally
> one in _bt_newroot().
Amit K's reply about btree and hash should've resolved any doubts for
those index types. About SP-Gist, see the comment below.
> 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);
metadata = SpGistPageGetMeta(BufferGetPage(metabuffer));
if (metadata->magicNumber != SPGIST_MAGIC_NUMBER)
elog(ERROR, "index \"%s\" is not an SP-GiST index",
cache->lastUsedPages = metadata->lastUsedPages;
So, I think it won't be correct to set pd_lower here, no?
> For hash, hashbulkdelete(), _hash_vacuum_one_page(),
> _hash_addovflpage(), _hash_freeovflpage() and _hash_doinsert() are
> missing the shot, no? We could have a meta page of a hash index
> upgraded from PG10.
Amit K's reply. :)
Updated patch attached, which implements your suggested changes to the
By the way, as I noted on another unrelated thread, I will not be able to
respond to emails from tomorrow until 9/23.
|Next Message||Gaddam Sai Ram||2017-09-15 07:51:33||Error: dsa_area could not attach to a segment that has been freed|
|Previous Message||Thomas Munro||2017-09-15 06:47:10||Re: pgsql: Add support for coordinating record typmods among parallel worke|