Re: [HACKERS] Setting pd_lower in GIN metapage

On 2017/09/26 11:34, Amit Kapila wrote:
> On Mon, Sep 25, 2017 at 12:18 PM, Amit Langote wrote:
>> So, ISTM, comments that the patches add should all say that setting the
>> meta pages' pd_lower to the correct value helps to pass those pages to
>> xlog.c as compressible standard layout pages, regardless of whether they
>> are actually passed that way.  Even if the patches do take care of the
>> latter as well.
>>
>> Did I miss something?
>>
>> Looking at Amit K's updated patches for btree and hash, it seems that he
>> updated the comment to say that setting pd_lower to the correct value is
>> *essential*, because those pages are passed as REGBUF_STANDARD pages and
>> hence will be compressed.  That seems to contradict what I wrote above,
>> but I think that's not wrong, too.
>>
> 
> I think you are missing that there are many cases where we use only
> REGBUF_STANDARD for meta-pages (cf. hash index).  For btree, where the
> advantage is in fewer cases, I have explicitly stated those cases.

I do see that there are some places that use only REGBUF_STANDARD.  I also
understand that specifying this flag is necessary condition for
XLogRecordAssemble() to perform the hole compression, if it is to be
performed at all.  ISTM, the hole is compressed only if we write the FP
image.  However, reasons for why FP image needs to be written, AFAICS, are
independent of whether the hole is (should be) compressed or not.  Whether
compression should occur or not depends on whether the REGBUF_STANDARD
flag is passed, that is, whether a caller is sure that the page is of
standard layout.  The last part is only true if page's pd_lower is always
set correctly.

Perhaps, we should focus on just writing exactly why setting pd_lower to
the correct value is essential.  I think that's a good change, so I
adopted it from your patch.  The *only* reason why it's essential to set
pd_lower to the correct value, as I see it, is that xloginsert.c *might*
compress the hole in the page and its pd_lower better be correct if we
want compression to preserve the metadata content (in meta pages).  OTOH,
it's not clear to me why we should write in that comment *why* the
compression would occur or why the FP image would be written in the first
place.  Whether or not FP image is written has nothing to do with whether
we should set pd_lower correctly, does it?  Also, since we want to repeat
the same comment in multiple places where we now set pd_lower (gin, brin,
spgist patches have many more new places where meta page's pd_lower is
set), keeping it to the point would be good.

But as you said a few times, we should leave it to the committer to decide
the exact text to write in these comment, which I think is fine.

> Do you still have confusion?

This is an area of PG code I don't have much experience with and is also a
bit complex, so sorry if I'm saying things that are not quite right.

Thanks,
Amit



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