Re: GIN pageinspect support for entry tree and posting tree

> On Jan 9, 2026, at 02:00, Kirill Reshke <reshkekirill(at)gmail(dot)com> wrote:
>
> On Thu, 8 Jan 2026 at 22:11, Kirill Reshke <reshkekirill(at)gmail(dot)com> wrote:
>>
>> On Thu, 8 Jan 2026 at 21:49, Andrey Borodin <x4mmm(at)yandex-team(dot)ru> wrote:
>>>> On 8 Jan 2026, at 01:57, Kirill Reshke <reshkekirill(at)gmail(dot)com> wrote:
>>>>
>>>> PFA v10
>>>
>>> Also it seems that you used something that is not pgindent.
>>
>> Why
>>
>>> Looks like clang-format with default settings.
>>
>> I use "./src/tools/pg_bsd_indent/pg_bsd_indent -l79 -di12 -nfc1 -nlp
>> -sac ./contrib/pageinspect/ginfuncs.c"
>>
>>
>>
>>
>> --
>> Best regards,
>> Kirill Reshke
>
> Okay, after off-list discussion looks like my options to pg_bsd_indent
> are bad, better is:
>
> "./src/tools/pg_bsd_indent/pg_bsd_indent -bad -bap -bbb -bc -bl -cli1
> -cp33 -cdb -nce -d0 -di12 -nfc1 -i4 -l79 -lp -lpl -nip -npro -sac -tpg
> -ts4 ./contrib/pageinspect/ginfuncs.c"
>
> And still this is not OK to use plain pg_bsd_indent and thus I used
> " ./src/tools/pgindent/pgindent ./contrib/pageinspect/ginfuncs.c"
>
> v11 with this and commit message polishing
>
> --
> Best regards,
> Kirill Reshke
> <v11-0002-GIN-pageinspect-support-for-entry-tree-and-posti.patch><v11-0001-Preliminary-cleanup.patch>

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

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.

3 - 0001
```
Reviewed-by: Andrey Borodin x4mmm(at)yandex-team(dot)ru
```

I think the correct form is: Reviewed-by: Andrey Borodin <x4mmm(at)yandex-team(dot)ru <mailto:x4mmm(at)yandex-team(dot)ru>>

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.

4 - 0002
```
Reviewed-by: Andrey Borodin x4mmm(at)yandex-team(dot)ru
Reviewed-by: Roman Khapov rkhapov(at)yandex-team(dot)ru <mailto:rkhapov(at)yandex-team(dot)ru>
```

Add <> to email addresses.

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.

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

7 - 0002
```
+ if (!ItemIdIsValid(iid))
+ elog(ERROR, "invalid ItemId”);
```

Why this is elog but ereport?

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.

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.

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().
*/
```

11 - 0002
```
+ /* Get list of item pointers from the tuple. */
+ if (GinItupIsCompressed(idxtuple))
```

Nit: Get list -> Get a list

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/