On Sat, 10 Jan 2026 at 07:20, Japin Li <[email protected]> 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
v13-0001-Modernize-coding-in-GIN-pageinspect-functions.patch
Description: Binary data
v13-0002-GIN-pageinspect-support-for-entry-tree-and-posti.patch
Description: Binary data
