Re: Setting pd_lower in GIN metapage

On Tue, Sep 19, 2017 at 12:40 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Mon, Sep 18, 2017 at 4:03 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> On Mon, Sep 18, 2017 at 11:16 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>> On Sat, Sep 16, 2017 at 7:15 PM, Michael Paquier
>>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>>>
>>>
>>> You have already noticed above that it will help when
>>> wal_checking_consistency is used and that was the main motivation to
>>> pass REGBUF_STANDARD apart from maintaining consistency. It is not
>>> clear to me what is bothering you. If your only worry about these
>>> patches is that you want this sentence to be removed from the comment
>>> because you think it is obvious or doesn't make much sense, then I
>>> think we can leave this decision to committer. I have added it based
>>> on Tom's suggestion above thread about explaining why it is
>>> inessential or essential to set pd_lower. I think Amit Langote just
>>> tried to mimic what I have done in hash and btree patches to maintain
>>> consistency. I am also not very sure if we should write some detailed
>>> comment or leave the existing comment as it is. I think it is just a
>>> matter of different perspective.
>>
>> What is disturbing me a bit is that the existing comments mention
>> something that could be supported (the compression of pages), but
>> that's actually not done and this is unlikely to happen because the
>> number of bytes associated to a meta page is going to be always
>> cheaper than a FPW, which would cost in CPU to store it for
>> compression is enabled. So I think that we should switch comments to
>> mention that pd_lower is set so as this helps with page masking, but
>> we don't take advantage of XLOG compression in the code.
>>
>
> I think that is not true because we do need FPW for certain usages of
> metapage. Consider a case in _hash_doinsert where register metabuf
> with just
> REGBUF_STANDARD, it can take advantage of removing the hole if
> pd_lower is set to its correct position.

I am not saying that no index AMs take advantage FPW compressibility
for their meta pages. There are cases like this one, as well as one
code path in BRIN where this is useful, and it is useful as well when
logging FPWs of the init forks for unlogged relations.

> There are other similar
> usages in hash index. For other indexes like btree, there is no such
> usage currently, but it can also take advantage for
> wal_consistency_checking. Now, probably there is an argument that we
> use different comments for different indexes as the usage varies, but
> I think someone looking at code after reading the comments can
> differentiate such cases.

I'd think about adjusting the comments the proper way for each AM so
as one can read those comments and catch any limitation immediately.
The fact this has not been pointed out on this thread before any
checks and the many mails exchanged on the matter on this thread make
me think that the current code does not outline the current code
properties appropriately.
--
Michael