Re: GIN pageinspect support for entry tree and posting tree - Mailing list pgsql-hackers

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.

> 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