Re: [HACKERS] Setting pd_lower in GIN metapage - Mailing list pgsql-hackers

On Wed, Sep 13, 2017 at 4:43 PM, Amit Langote
<Langote_Amit_f8@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.

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

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

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

For SpGist, I think that there are two missing: spgbuild() and spgGetCache().

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


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