Re: Setting pd_lower in GIN metapage

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