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

On Mon, Sep 18, 2017 at 11:16 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Sat, Sep 16, 2017 at 7:15 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Fri, Sep 15, 2017 at 4:22 PM, Amit Langote
>> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>>> On 2017/09/14 16:00, Michael Paquier wrote:
>>>> 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.
>>>
>>> Thanks for the review.
>>>
>>>> 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.
>>
>> Check.
>>
>>> 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?
>>
>> Yeah, I am just reading the code again and there is no alarm here.
>>
>>> 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.
>>
>> No problem. Enjoy your vacations.
>>
>> I have spent some time looking at the XLOG insert code, and tested the
>> compressibility of the meta pages. And I have noticed that all pages
>> registered with REGBUF_WILL_INIT will force XLogRecordAssemble to not
>> take a FPW of the block registered because the page will be
>> reinitialized at replay, and in such cases the zero'ed page is filled
>> with the data from the record. log_newpage is used to initialize init
>> forks for unlogged relations, which is where this patch allows page
>> compression to take effect correctly. Still setting REGBUF_STANDARD
>> with REGBUF_WILL_INIT is actually a no-op, except if
>> wal_checking_consistency is used when registering a buffer for WAL
>> insertion. There is one code path though where things are useful all
>> the time: revmap_physical_extend for BRIN.
>>
>> The patch is using the correct logic, still such comments are in my
>> opinion incorrect because of the reason written above:
>> +    * 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.
>> Here REGBUF_STANDARD is actually a no-op for btree.
>>
>
> 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 agree that
this is a minor point, so if the wave of this thread is that I am too
noisy, please feel free to ignore me: the logic of the patches is
still correct, still having those comments feels like cheating a bit
;p
-- 
Michael


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