On Sat, 10 Jan 2026 at 12:41, Kirill Reshke <[email protected]> wrote: > 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. >
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.
