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 |
Date: | 2017-09-15 07:22:23 |
Message-ID: | e68458c3-9f6c-21f9-e45d-5bfadc303746@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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.
> + 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)
> mask_unused_space(page);
>
> 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.
Agreed, done.
> + *
> + * 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);
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?
> 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
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.
Thanks,
Amit
Attachment | Content-Type | Size |
---|---|---|
0001-Set-pd_lower-correctly-in-the-GIN-metapage.patch | text/plain | 7.2 KB |
0002-Set-pd_lower-correctly-in-the-BRIN-index-metapage.patch | text/plain | 4.6 KB |
0003-Set-pd_lower-correctly-in-the-SP-GiST-index-metapage.patch | text/plain | 3.9 KB |
From | Date | Subject | |
---|---|---|---|
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 |