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
v12-0001-Modernize-coding-in-GIN-pageinspect-functions.patch
Description: Binary data
v12-0002-GIN-pageinspect-support-for-entry-tree-and-posti.patch
Description: Binary data
