Re: Setting pd_lower in GIN metapage

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.

Thanks,
Amit

Thanks,
Amit