Re: GIN pageinspect support for entry tree and posting tree

On Sat, 10 Jan 2026 at 07:20, Japin Li <japinli(at)hotmail(dot)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.

> 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`

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.

--
Best regards,
Kirill Reshke