On Fri, 09 Jan 2026 at 21:39, Kirill Reshke <[email protected]> wrote:
> On Fri, 9 Jan 2026 at 14:43, Chao Li <[email protected]> wrote:
>>
>> Hi Kirill,
>>
>> Thanks for the patch. I have some small comments:
>>
>> 1 - 0001
>> ```
>> Subject: [PATCH v11 1/2] Preliminary cleanup
>> ```
>>
>> 0001 does some cleanup work, the commit subject "Preliminary cleanup” is a
>> bit vague. Maybe: pageinspect: clean up ginfuncs.c formatting and
>> allocations, or pageinspect: cleanup in ginfuncs.c
>>
>
> Yes, the commit message is indeed vague. PFA v12 with reworked commit message.
>
>> 2 - 0001
>> ```
>> Per Peter Eisentraut's patch in thread.
>> ```
>>
>> From what I have seen so far, instead of mention a name, it’s more common to
>> mention a commit id.
>
> In this case, I am referring to this patch, which is never committed
> anywhere [0]
>
>> 3 - 0001
>> ```
>> Reviewed-by: Andrey Borodin [email protected]
>> ```
>>
>> I think the correct form is: Reviewed-by: Andrey Borodin
>> <[email protected] <mailto:[email protected]>>
>>
>> I believe the committer will fix that before pushing. But to make
>> committer’s life easier, you’d better fix that rather than leaving to the
>> committer.
>>
>> Otherwise, code changes of 0001 is straightforward and LGTM.
>
> Yes, fixed.
>
>> 4 - 0002
>> ```
>> Reviewed-by: Andrey Borodin [email protected]
>> Reviewed-by: Roman Khapov [email protected]
>> <mailto:[email protected]>
>> ```
>>
>> Add <> to email addresses.
>
> Thanks, fixed.
>
>> 5 - 0002
>> ```
>> + if (!IS_INDEX(indexRel) || !IS_GIN(indexRel))
>> + ereport(ERROR,
>> + errcode(ERRCODE_WRONG_OBJECT_TYPE),
>> + errmsg("\"%s\" is not a %s index",
>> +
>> RelationGetRelationName(indexRel), "GIN"));
>> ```
>>
>> I don’t see a test covering this error case.
>
> Hmm. I dont see any tests for this in nearby pageinspect modules (for
> btree, hash, etc), so I leave it as-is for now. I am not sure the
> value of testing this is worth testing cycles.
>
>> 6 - 0002
>> ```
>> + /* we only support entry tree in this function, check that */
>> + if (opaq->flags & GIN_META)
>> + ereport(ERROR,
>> + errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>> + errmsg("gin_entrypage_items is unsupported
>> for metapage"));
>> +
>> +
>> + if (opaq->flags & (GIN_LIST | GIN_LIST_FULLROW))
>> + ereport(ERROR,
>> + errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>> + errmsg("gin_entrypage_items is unsupported
>> for fast list pages"));
>> +
>> +
>> + if (opaq->flags & GIN_DATA)
>> + ereport(ERROR,
>> + errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>> + errmsg("input page is not a GIN entry tree
>> page"),
>> + errhint("This appears to be a GIN posting
>> tree page. Please use gin_datapage_items."));
>> ```
>>
>> I just feel the comment is unclear. It’s easy to read it as only for
>> the immediate “if”, but it’s actually for the following 3
>> “if”. Maybe change to: Reject non-entry-tree GIN pages (meta, fast
>> list, and data pages)
>>
>> And the 3 error messages look in inconsistent forms. I would suggest change
>> the first 2 error messages to:
>>
>> * gin_entrypage_items does not support metapages
>> * gin_entrypage_items does not support fast list pages
>
> Thank you, I changed all 3 error messages to this form and updated tests.
>
>> 7 - 0002
>> ```
>> + if (!ItemIdIsValid(iid))
>> + elog(ERROR, "invalid ItemId”);
>> ```
>>
>> Why this is elog but ereport?
>
> I copied this from gistfuncs.c. However, this is user-facing change,
> so this is indeed something where ereport should be used.
>
>> Also, the error message is too simple. Maybe change to:
>> ```
>> errmsg("invalid ItemId at offset %u", offset))
>> ```
>>
>> 8 - 0002
>> ```
>> + /*
>> + * here we can safely reuse pg_class's tuple
>> descriptor.
>> + */
>> + attrVal = index_getattr(idxtuple, FirstOffsetNumber,
>> tupdesc, &isnull);
>> ```
>>
>> I think this comment is wrong. tupdesc is the index relation’s descriptor.
>>
>> Also, this can be a one-line comment.
>
> Thank you, I have updated this comment.
>
>> 9 - 0002
>> ```
>> + ereport(ERROR,
>> +
>> errcode(ERRCODE_INDEX_CORRUPTED),
>> + errmsg("invalid gin entry
>> page tuple at offset %d", offset));
>> ```
>>
>> Offset is unsigned, so %u should be used.
>
> Fixed.
>
>> 10 - 0002
>> ```
>> + /* Most of this is copied from record_out(). */
>> + typoid = TupleDescAttr(tupdesc, indAtt -
>> 1)->atttypid;
>> ```
>>
>> This comment is confusing. I understand that you meant to say the following
>> code piece is copied from record_out(). Maybe change to:
>> ```
>> typoid = TupleDescAttr(tupdesc, indAtt - 1)->atttypid;
>> getTypeOutputInfo(typoid, &foutoid, &typisvarlena);
>> value = OidOutputFunctionCall(foutoid, attrVal);
>>
>> /*
>> * The following value output and quoting logic is copied
>> * from record_out().
>> */
>> ```
>
> Applied.
>
>> 11 - 0002
>> ```
>> + /* Get list of item pointers from the tuple. */
>> + if (GinItupIsCompressed(idxtuple))
>> ```
>>
>> Nit: Get list -> Get a list
>
> Applied.
>
>> Best regards,
>> --
>> Chao Li (Evan)
>> HighGo Software Co., Ltd.
>> https://www.highgo.com/
>>
>>
>>
>>
>
>
>
> PFA v12.
>
> [0]
> https://www.postgresql.org/message-id/921aa27c-e983-4577-b1dc-3adda3ce79da%40eisentraut.org
>
> --
> Best regards,
> Kirill Reshke
>
> [2. text/x-diff;
> v12-0001-Modernize-coding-in-GIN-pageinspect-functions.patch]...
>
> [3. text/x-diff;
> v12-0002-GIN-pageinspect-support-for-entry-tree-and-posti.patch]...
Just a few small nitpicks on v12:
1.
+ /* Open the relation */
+ indexRel = index_open(indexRelid, AccessShareLock);
"relation" is technically correct, but "index" feels more precise here.
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.
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 */
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.
--
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.
