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

On Fri, 9 Jan 2026 at 14:43, Chao Li <li.evan.chao@gmail.com> 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.cformatting 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 x4mmm@yandex-team.ru
> ```
>
> I think the correct form is: Reviewed-by: Andrey Borodin <x4mmm@yandex-team.ru <mailto:x4mmm@yandex-team.ru>>
>
> I believe the committer will fix that before pushing. But to make committer’s life easier, you’d better fix that
ratherthan leaving to the committer. 
>
> Otherwise, code changes of 0001 is straightforward and LGTM.

Yes, fixed.

> 4 - 0002
> ```
> Reviewed-by: Andrey Borodin x4mmm@yandex-team.ru
> Reviewed-by: Roman Khapov rkhapov@yandex-team.ru <mailto:rkhapov@yandex-team.ru>
> ```
>
> 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
following3 “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().
Maybechange 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