Re: Setting pd_lower in GIN metapage

On Thu, Jun 22, 2017 at 6:55 PM, Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> On 2017/06/22 16:56, Michael Paquier wrote:
>> On Wed, Jun 21, 2017 at 9:42 AM, Amit Langote
>> <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>> On 2017/06/20 20:37, Amit Kapila wrote:
>>>> On Tue, Jun 20, 2017 at 1:50 PM, Amit Langote
>>>> <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>>>> On 2017/06/19 23:31, Tom Lane wrote:
>>>>>> I'd suggest a rule like "if pd_lower is smaller than SizeOfPageHeaderData
>>>>>> then don't trust it, but assume all of the page is valid data".
>>>>>
>>>>> Actually, such a check is already in place in the tool, whose condition
>>>>> looks like:
>>>>>
>>>>> if (PageGetPageSize(header) == BLCKSZ &&
>>>>> PageGetPageLayoutVersion(header) == PG_PAGE_LAYOUT_VERSION &&
>>>>> (header->pd_flags & ~PD_VALID_FLAG_BITS) == 0 &&
>>>>> header->pd_lower >= SizeOfPageHeaderData &&
>>>>> header->pd_lower <= header->pd_upper &&
>>>>> header->pd_upper <= header->pd_special &&
>>>>> header->pd_special <= BLCKSZ &&
>>>>> header->pd_special == MAXALIGN(header->pd_special) && ...
>>>>>
>>>>> which even GIN metapage passes, making it an eligible data page and hence
>>>>> for omitting the hole between pd_lower and pd_upper.
>>>>>
>>>>
>>>> Won't checking for GIN_META in header->pd_flags gives you what you want?
>>>
>>> GIN_META flag is not written into pd_flags but GinPageOpaqueData.flags,
>>> which still requires including GIN's private header.
>>
>> Did you check this patch with wal_consistency_checking? I am getting
>> failures so your patch does not have the masking of GIN pages
>> completely right:
>> FATAL: inconsistent page found, rel 1663/16385/28133, forknum 0, blkno 0
>> CONTEXT: WAL redo at 0/39379EB8 for Gin/UPDATE_META_PAGE:
>> That's easily reproducible with installcheck and a standby replaying
>> the changes. I did not look at the code in details to see what you may
>> be missing here.
>
> Oh, wasn't sure about the gin_mask() changes myself. Thanks for checking.
>
> Actually, the WAL consistency check fails even without patching
> gin_mask(), so the problem may be with the main patch itself. That is,
> the patch needs to do something else other than just teaching
> GinInitMetabuffer() to initialize pd_lower. Will look into that.
>

I've not read the code deeply but I guess we should use
GinInitMetabuffer() in ginRedoUpdateMetapage() instead of
GinInitPage(). Maybe also GinInitPage() in ginRedoDeleteListPages() is
the same.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center