Re: GIN pageinspect support for entry tree and posting tree

On Sat, 10 Jan 2026 at 12:41, Kirill Reshke <reshkekirill@gmail.com> wrote:
> On Sat, 10 Jan 2026 at 07:20, Japin Li <japinli@hotmail.com> wrote:
>> >
>> > PFA v12.
>> >
>>
>> Just a few small nitpicks on v12:
>
>
> Hi!
> Thank you for your review.
>
>
>> 1.
>> +       /* Open the relation */
>> +       indexRel = index_open(indexRelid, AccessShareLock);
>>
>> "relation" is technically correct, but "index" feels more precise here.
>
> I have updated this comment, thanks
>
>> 2.
>> +       if (PageGetSpecialSize(page) != MAXALIGN(sizeof(GinPageOpaqueData)))
>> +               ereport(ERROR,
>> +                               errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>> +                               errmsg("input page is not a valid GIN entry tree page"),
>> +                               errdetail("Expected special size %d, got %d.",
>> +                                                 (int) MAXALIGN(sizeof(GinPageOpaqueData)),
>> +                                                 PageGetSpecialSize(page)));
>>
>> Cast the second PageGetSpecialSize(page) to int as well, for consistency.
>
> Fixed.
>
>> 3.
>> +                       /* orig tuple reuse is safe */
>> +
>> +                       res = index_getattr(idxtuple, FirstOffsetNumber, tupdesc, &isnull);
>>
>> Does "orig tuple" refer to `idxtuple` here? If so, maybe the comment could be
>> slightly clearer, e.g.:
>>
>>    /* Safe to reuse the original index tuple */
>
> I have rephrased this comment.
>

Thanks for updating the patches.

>> 4.
>> +
>> +       return (Datum) 0;
>>
>> Since this appears to be a void-returning function, consider using_RETURN_VOID()
>> instead — it's the conventional way and slightly more self-documenting.
>>
>
> No, this function returns SET OF RECORD.  So, `return (Datum) 0;`  is
> not exactly VOID or NULL, it is rather the end of the output marker.
> You can also see `
> gist_page_items`

Thanks for pointing that out!

>
> Your comments refer to v12-0002. For the record, did you review 0001,
> if yes, do you think it is good? I have included you as a reviewer to
> v12-0002 commit message.
>

Yeah, patch 0001 looks good to me.

I noticed that the IS_INDEX macro is currently defined in btreefunc.c,
hashfuncs.c, and now also in ginfuncs.c.  Would it be possible to move it
to a shared header file so all these modules can include it from one place?

--
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.