GIN pageinspect support for entry tree and posting tree
Hi!
I do frequently interact with gin indexes. I also frequently use
pageinspect to check how data is stored in indixies. The latter is
beneficial in both education and corruption fixing purposes.
I was always wondering on why GIN pageinspect module lacks support for
anything rather that Posting Tree leaf pages. So, I implemented
extended GIN support.
Patch includes two functions, gin_entrypage_items and gin_datapage_items.
gin_entrypage_items is a function to work with entry tree. It supports
both entry tree leaf and internal pages.
GIN entry tree stores an index tuple with a single indexed column in
single-index-column case.
Otherwise, each tuple on GIN entry tree page is tuple of (attrnum,
attrvalue), where the first item in tuple shows which indexes column
this tuple refers to. GIN internal pages also contains downlink to
other entry tree pages, while entry tree leaf page may contain
compressed TIDs of the posting list, or downlink to posting tree.
example of output on entry tree internal page of multi-column index:
```
reshke=# select * from gin_entrypage_items(get_raw_page('x_i_j_idx',
1), 'x_i_j_idx'::regclass);
itemoffset | downlink | tids | keys
------------+----------+------+------------------------------------
1 | (3,0) | {} | i=113
2 | (5,0) | {} | j=34173cb38f07f89ddbebc2ac9128303f
3 | (2,0) | {} | j=a0a080f42e6f13b3a2df133f073095dd
4 | (4,0) | {} | j=fc490ca45c00b1249bbe3554a4fdf6fb
(4 rows)
```
example of output on entry tree leaf page of multi-column index:
```
reshke=# select * from gin_entrypage_items(get_raw_page('x_i_j_idx',
2), 'x_i_j_idx'::regclass);
itemoffset | downlink | tids |
keys
------------+----------------+------------------------------+------------------------------------
1 | (2147483696,3) | {"(1,39)","(1,40)","(2,1)"} |
j=35f4a8d465e6e1edc05f3d8ab658c551
2 | (2147483696,3) | {"(4,10)","(4,11)","(4,12)"} |
j=3636638817772e42b59d74cff571fbb3
3 | (2147483696,3) | {"(5,1)","(5,2)","(5,3)"} |
j=3644a684f98ea8fe223c713b77189a77
4 | (2147483696,3) | {"(0,25)","(0,26)","(0,27)"} |
j=37693cfc748049e45d87b8c7d8b9aacd
5 | (2147483696,3) | {"(3,33)","(3,34)","(3,35)"} |
j=37a749d808e46495a8da1e5352d03cae
```
downlink on the leaf page has a different meaning than on the internal
page, but I didn't handle it any differently. In the example ouput,
(2147483696,3) = ((1<<31) + 48, 3), meaning the next 48 bytes is the
entry tree key, and after that there is 3 posting items,
varbyte-encoded.
I also tested this function on GIN index with nulls, it works. Let me
know if I'm wrong, I know that GIN handles NULLS very differently, but
I had trouble with it, which makes me think that I'm missing
something.
Also turns out this gin_entrypage_items actually works for fast lists
(GIN_LIST and GIN_LIST_FULLROW pages). But output is something
strange, I cannot validate is output is sane. For tids, I get values
like
```
1 | (1,19) |
{"(16782080,4096)","(16777216,14336)","(1280,0)","(469774336,0)","(65536,0)","(2734686208,57344)","(8389120,0)","(3372220424,65280)","(4294967295,65535)","(16711680,0)","(0,0)","(
33554688,0)","(614,0)","(67108864,1024)","(16777216,0)","(73793536,0)","(0,0)","(0,0)","(0,0)"}
```
gin_datapage_items is for posting trees, but not leaf pages. For leaf
pages, users are expected to still use the gin_leafpage_items
function.
Example output for gin_datapage_items:
```
reshke=# select * from gin_datapage_items(get_raw_page('x_i_j_idx',
43), 'x_i_j_idx'::regclass);
itemoffset | downlink | item_tid
------------+----------+----------
1 | 124 | (162,12)
2 | 123 | (314,37)
3 | 251 | (467,23)
4 | 373 | (0,0)
(4 rows)
```
Patch still is very raw, many things to improve.
Comments?
--
Best regards,
Kirill Reshke
Attachments:
v1-0001-GIN-pageinspect-support-for-entry-tree-and-postin.patchapplication/octet-stream; name=v1-0001-GIN-pageinspect-support-for-entry-tree-and-postin.patchDownload+388-8
On Tue, 14 Oct 2025 at 01:43, Kirill Reshke <reshkekirill@gmail.com> wrote:
Hi!
I do frequently interact with gin indexes. I also frequently use
pageinspect to check how data is stored in indixies. The latter is
beneficial in both education and corruption fixing purposes.I was always wondering on why GIN pageinspect module lacks support for
anything rather that Posting Tree leaf pages. So, I implemented
extended GIN support.Patch includes two functions, gin_entrypage_items and gin_datapage_items.
gin_entrypage_items is a function to work with entry tree. It supports
both entry tree leaf and internal pages.GIN entry tree stores an index tuple with a single indexed column in
single-index-column case.
Otherwise, each tuple on GIN entry tree page is tuple of (attrnum,
attrvalue), where the first item in tuple shows which indexes column
this tuple refers to. GIN internal pages also contains downlink to
other entry tree pages, while entry tree leaf page may contain
compressed TIDs of the posting list, or downlink to posting tree.example of output on entry tree internal page of multi-column index:
```
reshke=# select * from gin_entrypage_items(get_raw_page('x_i_j_idx',
1), 'x_i_j_idx'::regclass);
itemoffset | downlink | tids | keys
------------+----------+------+------------------------------------
1 | (3,0) | {} | i=113
2 | (5,0) | {} | j=34173cb38f07f89ddbebc2ac9128303f
3 | (2,0) | {} | j=a0a080f42e6f13b3a2df133f073095dd
4 | (4,0) | {} | j=fc490ca45c00b1249bbe3554a4fdf6fb
(4 rows)```
example of output on entry tree leaf page of multi-column index:
```
reshke=# select * from gin_entrypage_items(get_raw_page('x_i_j_idx',
2), 'x_i_j_idx'::regclass);
itemoffset | downlink | tids |
keys
------------+----------------+------------------------------+------------------------------------
1 | (2147483696,3) | {"(1,39)","(1,40)","(2,1)"} |
j=35f4a8d465e6e1edc05f3d8ab658c551
2 | (2147483696,3) | {"(4,10)","(4,11)","(4,12)"} |
j=3636638817772e42b59d74cff571fbb3
3 | (2147483696,3) | {"(5,1)","(5,2)","(5,3)"} |
j=3644a684f98ea8fe223c713b77189a77
4 | (2147483696,3) | {"(0,25)","(0,26)","(0,27)"} |
j=37693cfc748049e45d87b8c7d8b9aacd
5 | (2147483696,3) | {"(3,33)","(3,34)","(3,35)"} |
j=37a749d808e46495a8da1e5352d03cae```
downlink on the leaf page has a different meaning than on the internal
page, but I didn't handle it any differently. In the example ouput,
(2147483696,3) = ((1<<31) + 48, 3), meaning the next 48 bytes is the
entry tree key, and after that there is 3 posting items,
varbyte-encoded.I also tested this function on GIN index with nulls, it works. Let me
know if I'm wrong, I know that GIN handles NULLS very differently, but
I had trouble with it, which makes me think that I'm missing
something.Also turns out this gin_entrypage_items actually works for fast lists
(GIN_LIST and GIN_LIST_FULLROW pages). But output is something
strange, I cannot validate is output is sane. For tids, I get values
like```
1 | (1,19) |
{"(16782080,4096)","(16777216,14336)","(1280,0)","(469774336,0)","(65536,0)","(2734686208,57344)","(8389120,0)","(3372220424,65280)","(4294967295,65535)","(16711680,0)","(0,0)","(
33554688,0)","(614,0)","(67108864,1024)","(16777216,0)","(73793536,0)","(0,0)","(0,0)","(0,0)"}
```gin_datapage_items is for posting trees, but not leaf pages. For leaf
pages, users are expected to still use the gin_leafpage_items
function.
Example output for gin_datapage_items:
In v2 I decided to simply reject these pages.
```
reshke=# select * from gin_datapage_items(get_raw_page('x_i_j_idx',
43), 'x_i_j_idx'::regclass);
itemoffset | downlink | item_tid
------------+----------+----------
1 | 124 | (162,12)
2 | 123 | (314,37)
3 | 251 | (467,23)
4 | 373 | (0,0)
(4 rows)```
Patch still is very raw, many things to improve.
Comments?--
Best regards,
Kirill Reshke
Attached v2 with minor fixes and new test cases in gin.sql.
--
Best regards,
Kirill Reshke
Attachments:
v2-0001-GIN-pageinspect-support-for-entry-tree-and-postin.patchapplication/octet-stream; name=v2-0001-GIN-pageinspect-support-for-entry-tree-and-postin.patchDownload+426-8
On 29 Dec 2025, at 17:51, Kirill Reshke <reshkekirill@gmail.com> wrote:
Attached v2
I've looked into the patch.
The functionality is useful and seems to reflect pageinspect style.
Patch still is very raw, many things to improve.
Yup, but let's work on it!
Please update the documentation here [0]https://www.postgresql.org/docs/current/pageinspect.html#PAGEINSPECT-GIN-FUNCS.
Other AM's seems to defend from each other: if (!IS_INDEX(rel) || !IS_BTREE(rel)) or if (!IS_GIST(indexRel)). I don't see such check in new functions. B-tree also protects from temp tables of other sessions: if (RELATION_IS_OTHER_TEMP(rel)).
gin_datapage_items() seem to ignore reloid, did you have some ideas how to use it?
In gin_entrypage_items() buf and tmpTupdesc seem to be recreated for every offset, can we reuse them?
gin_entrypage_items() errors out with "input page is not a valid GIN data leaf page", but function is for entry pages.
There's no tests for gin_datapage_items().
There's a typo "unsuppoerted" and "rejrect" in gin.sql.
gin_entrypage_items() emits elog(NOTICE, "page is deleted"), but gin_datapage_items() does not.
Also, note that corresponding GiST code does "OffsetNumber maxoff = InvalidOffsetNumber;", but gin_entrypage_items() has no this initialization.
I'd convert Assert(!isnull) into if+elog: inspected data might be corrupted.
Thanks!
Best regards, Andrey Borodin.
[0]: https://www.postgresql.org/docs/current/pageinspect.html#PAGEINSPECT-GIN-FUNCS
On Thu, 1 Jan 2026 at 19:00, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
On 29 Dec 2025, at 17:51, Kirill Reshke <reshkekirill@gmail.com> wrote:
Attached v2
I've looked into the patch.
The functionality is useful and seems to reflect pageinspect style.
Thank you for taking a look.
Patch still is very raw, many things to improve.
Yup, but let's work on it!
Please update the documentation here [0].
Other AM's seems to defend from each other: if (!IS_INDEX(rel) || !IS_BTREE(rel)) or if (!IS_GIST(indexRel)). I don't see such check in new functions. B-tree also protects from temp tables of other sessions: if (RELATION_IS_OTHER_TEMP(rel)).
I have added protections for functions that accept regclass.
RELATION_IS_OTHER_TEMP is unnecessary here, because generic
get_raw_page functions already take care of that.
gin_datapage_items() seem to ignore reloid, did you have some ideas how to use it?
Yes, I removed that 'reloid'. It was of no use.
In gin_entrypage_items() buf and tmpTupdesc seem to be recreated for every offset, can we reuse them?
Well, I did not get it, how we can reuse them, but v3 now releases
memory after use.
For tmpTupdesc, the issue here is that GIN entry tree pages can have
tuples which need different tuple descriptors to access data, on the
same page. That's why I re-evaluate on every offset. I think caching
tuple descriptors here would be a cure worse than a disease.
Example from my first email here:
```
reshke=# select * from gin_entrypage_items(get_raw_page('x_i_j_idx',
1), 'x_i_j_idx'::regclass);
itemoffset | downlink | tids | keys
------------+----------+------+------------------------------------
1 | (3,0) | {} | i=113
2 | (5,0) | {} | j=34173cb38f07f89ddbebc2ac9128303f
3 | (2,0) | {} | j=a0a080f42e6f13b3a2df133f073095dd
4 | (4,0) | {} | j=fc490ca45c00b1249bbe3554a4fdf6fb
(4 rows)
```
To display that, we need different tupdescs for offset 1 & 2. for
offsets 3&4 we can reuse tupdesc from offset 2, but this would
(unnecessary?) increase patch complexity.
WDYT?
gin_entrypage_items() errors out with "input page is not a valid GIN data leaf page", but function is for entry pages.
I have added more sanity checks and regression tests for them.
There's no tests for gin_datapage_items().
I have added them in v3. My main concern here was that we have to
generate many tuples to trigger GIN to create an internal posting tree
page,
It is also block-size-dependent, and will not work for values other
than 8192, actually. Maybe we need to compute the number of inserted
tuples in-flight?
There's a typo "unsuppoerted" and "rejrect" in gin.sql.
Fxd
gin_entrypage_items() emits elog(NOTICE, "page is deleted"), but gin_datapage_items() does not.
This is stupid copy-paste from GiST pageinspect. My fault, removed.
(GiST deleted page do not store info to compute max offset correctly,
while GIN does store maxOffset in its Opaque info)
Also, note that corresponding GiST code does "OffsetNumber maxoff = InvalidOffsetNumber;", but gin_entrypage_items() has no this initialization.
It actually does but in a separate line, is this a problem?
I'd convert Assert(!isnull) into if+elog: inspected data might be corrupted.
I do not have a strong opinion here, but we are implementing
pageinspect here, not amcheck? I can only see one ereport with
corruption error code in pageinspect, and it is for HASH indexes.
Anyway, I changed assert to if-elog
Thanks!
Best regards, Andrey Borodin.
[0] https://www.postgresql.org/docs/current/pageinspect.html#PAGEINSPECT-GIN-FUNCS
PFA v3. I added you and Romka (cc-ed) as reviewers. Romka did a review
of v1 off-list which triggered me to actually bump this thread.
--
Best regards,
Kirill Reshke
Attachments:
v3-0001-GIN-pageinspect-support-for-entry-tree-and-postin.patchapplication/octet-stream; name=v3-0001-GIN-pageinspect-support-for-entry-tree-and-postin.patchDownload+481-7
Sorry for noise, new version with minor polishing.
On Sat, 3 Jan 2026 at 00:10, Kirill Reshke <reshkekirill@gmail.com> wrote:
It actually does but in a separate line, is this a problem?
I removed this cause we unconditionally assign maxoff =
PageGetMaxOffsetNumber(page);.
PFA v3.
PFA v4 with typo fixes.
--
Best regards,
Kirill Reshke
Attachments:
v4-0001-GIN-pageinspect-support-for-entry-tree-and-postin.patchapplication/octet-stream; name=v4-0001-GIN-pageinspect-support-for-entry-tree-and-postin.patchDownload+479-7
On 3 Jan 2026, at 00:16, Kirill Reshke <reshkekirill@gmail.com> wrote:
PFA v4 with typo fixes.
Thanks!
I think block size dependency is not a problem. There are no pageinspect tests that pass with 1Kb pages.
I have some more nits:
Meson build needs to be updated.
Documentation is on a TODO list.
+ relation_close(indexRel, AccessShareLock);
Perhaps, index_close() is what you actually wanted?
StringInfoData buf is leaked still, you can init it once and reset in the loop.
tmpTupdesc recreation worth commenting on.
Thanks!
Best regards, Andrey Borodin.
On Sat, 3 Jan 2026 at 00:16, Kirill Reshke <reshkekirill@gmail.com> wrote:
PFA v3.
PFA v4 with typo fixes.
PFA v5 with pageinspect documentation updates.
--
Best regards,
Kirill Reshke
Attachments:
v5-0001-GIN-pageinspect-support-for-entry-tree-and-postin.patchapplication/octet-stream; name=v5-0001-GIN-pageinspect-support-for-entry-tree-and-postin.patchDownload+533-7
On Sat, 3 Jan 2026 at 23:58, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
On 3 Jan 2026, at 00:16, Kirill Reshke <reshkekirill@gmail.com> wrote:
PFA v4 with typo fixes.
Thanks!
I think block size dependency is not a problem. There are no pageinspect tests that pass with 1Kb pages.
I have some more nits:
Meson build needs to be updated.
Documentation is on a TODO list.
Done
+ relation_close(indexRel, AccessShareLock);
Perhaps, index_close() is what you actually wanted?
Ok
StringInfoData buf is leaked still, you can init it once and reset in the loop.
Ok
tmpTupdesc recreation worth commenting on.
Ok
Thanks!
Best regards, Andrey Borodin.
PFA v6
--
Best regards,
Kirill Reshke
Attachments:
v6-0001-GIN-pageinspect-support-for-entry-tree-and-postin.patchapplication/octet-stream; name=v6-0001-GIN-pageinspect-support-for-entry-tree-and-postin.patchDownload+539-7
On 4 Jan 2026, at 00:25, Kirill Reshke <reshkekirill@gmail.com> wrote:
PFA v6
Would it be theoretically possible to unite functions for different GIN page types?
e.g. merge gin_entrypage_items + gin_datapage_items -> gin_tree_items? Or is it an awkward API?
The patch adds whitespace errors.
x4mmm@x4mmm-osx postgres % git am ~/Downloads/v6-0001-GIN-pageinspect-support-for-entry-tree-and-postin.patch
Applying: GIN pageinspect support for entry tree and posting tree internal pages
.git/rebase-apply/patch:236: trailing whitespace.
.git/rebase-apply/patch:242: trailing whitespace.
.git/rebase-apply/patch:243: trailing whitespace.
.git/rebase-apply/patch:249: trailing whitespace.
.git/rebase-apply/patch:284: trailing whitespace.
warning: squelched 8 whitespace errors
warning: 13 lines add whitespace errors.
Docs build fail [0]https://github.com/x4m/postgres_g/runs/59444268144:
[08:01:48.343] pageinspect.sgml:775: parser error : Opening and ending tag mismatch: varlistentry line 722 and variablelist
[08:01:48.343] </variablelist>
[08:01:48.343] ^
[08:01:48.343] pageinspect.sgml:776: parser error : Opening and ending tag mismatch: variablelist line 637 and sect2
[08:01:48.343] </sect2>
[08:01:48.343] ^
[08:01:48.343] pageinspect.sgml:1016: parser error : Opening and ending tag mismatch: sect2 line 634 and sect1
[08:01:48.343] </sect1>
[08:01:48.343] ^
[08:01:48.343] pageinspect.sgml:1017: parser error : Premature end of data in tag sect1 line 3
[08:01:48.343]
[08:01:48.343] ^
[08:01:48.343] pageinspect.sgml:1017: parser error : chunk is not well balanced
[08:01:48.343]
[08:01:48.343] ^
[08:01:48.343] contrib.sgml:152: parser error : Entity 'pageinspect' failed to parse
[08:01:48.343] &pageinspect;
[08:01:48.343] ^
[08:01:48.343] contrib.sgml:239: parser error : chunk is not well balanced
[08:01:48.343]
[08:01:48.343] ^
[08:01:48.344] postgres.sgml:279: parser error : Entity 'contrib' failed to parse
[08:01:48.344] &contrib;
And 32-bit build fail [1]https://cirrus-ci.com/task/6255878661210112:
SELECT * FROM gin_entrypage_items(get_raw_page('test1_y_idx', 1), 'test1_y_idx'::regclass);
-[ RECORD 1 ]--------------
itemoffset | 1
-downlink | (2147483664,1)
+downlink | (2147483660,1)
tids | {"(0,1)"}
keys | y=11
-[ RECORD 2 ]--------------
itemoffset | 2
-downlink | (2147483664,1)
+downlink | (2147483660,1)
tids | {"(0,1)"}
keys | y=111
Thanks!
Best regards, Andrey Borodin.
[0]: https://github.com/x4m/postgres_g/runs/59444268144
[1]: https://cirrus-ci.com/task/6255878661210112
On Mon, 5 Jan 2026 at 13:39, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
On 4 Jan 2026, at 00:25, Kirill Reshke <reshkekirill@gmail.com> wrote:
PFA v6
Would it be theoretically possible to unite functions for different GIN page types?
e.g. merge gin_entrypage_items + gin_datapage_items -> gin_tree_items? Or is it an awkward API?The patch adds whitespace errors.
x4mmm@x4mmm-osx postgres % git am ~/Downloads/v6-0001-GIN-pageinspect-support-for-entry-tree-and-postin.patch
Applying: GIN pageinspect support for entry tree and posting tree internal pages
.git/rebase-apply/patch:236: trailing whitespace.
.git/rebase-apply/patch:242: trailing whitespace.
.git/rebase-apply/patch:243: trailing whitespace.
.git/rebase-apply/patch:249: trailing whitespace.
.git/rebase-apply/patch:284: trailing whitespace.
warning: squelched 8 whitespace errors
warning: 13 lines add whitespace errors.
Thanks
v7 should not suffer from this.
Docs build fail [0]:
[08:01:48.343] pageinspect.sgml:775: parser error : Opening and ending tag mismatch: varlistentry line 722 and variablelist
[08:01:48.343] </variablelist>
[08:01:48.343] ^
[08:01:48.343] pageinspect.sgml:776: parser error : Opening and ending tag mismatch: variablelist line 637 and sect2
[08:01:48.343] </sect2>
[08:01:48.343] ^
[08:01:48.343] pageinspect.sgml:1016: parser error : Opening and ending tag mismatch: sect2 line 634 and sect1
[08:01:48.343] </sect1>
[08:01:48.343] ^
[08:01:48.343] pageinspect.sgml:1017: parser error : Premature end of data in tag sect1 line 3
[08:01:48.343]
[08:01:48.343] ^
[08:01:48.343] pageinspect.sgml:1017: parser error : chunk is not well balanced
[08:01:48.343]
[08:01:48.343] ^
[08:01:48.343] contrib.sgml:152: parser error : Entity 'pageinspect' failed to parse
[08:01:48.343] &pageinspect;
[08:01:48.343] ^
[08:01:48.343] contrib.sgml:239: parser error : chunk is not well balanced
[08:01:48.343]
[08:01:48.343] ^
[08:01:48.344] postgres.sgml:279: parser error : Entity 'contrib' failed to parse
[08:01:48.344] &contrib;
In v7 this is fixed, I believe, (/varlistentry was missing)
And 32-bit build fail [1]:
SELECT * FROM gin_entrypage_items(get_raw_page('test1_y_idx', 1), 'test1_y_idx'::regclass);
-[ RECORD 1 ]--------------
itemoffset | 1
-downlink | (2147483664,1)
+downlink | (2147483660,1)
tids | {"(0,1)"}
keys | y=11
-[ RECORD 2 ]--------------
itemoffset | 2
-downlink | (2147483664,1)
+downlink | (2147483660,1)
tids | {"(0,1)"}
keys | y=111Thanks!
Best regards, Andrey Borodin.
[0] https://github.com/x4m/postgres_g/runs/59444268144
[1] https://cirrus-ci.com/task/6255878661210112
This is because GIN on-disk format is platform-dependent, see [0]https://github.com/postgres/postgres/blob/master/src/backend/access/gin/ginentrypage.c#L91. We
align the offset where to start the Compressed GIN tuples list, that's
where the difference comes from. So, GinSetPostingOffset(itup,
newsize) on line 93 sets offset to 12 | GIN_ITUP_COMPRESSED on 32 bit
and 16 | GIN_ITUP_COMPRESSED on 64-bit. I added alternative regression
output gin_1.out to v7.
PFA v7.
[0]: https://github.com/postgres/postgres/blob/master/src/backend/access/gin/ginentrypage.c#L91
--
Best regards,
Kirill Reshke
Attachments:
v7-0001-GIN-pageinspect-support-for-entry-tree-and-postin.patchapplication/octet-stream; name=v7-0001-GIN-pageinspect-support-for-entry-tree-and-postin.patchDownload+687-7
On Mon, 5 Jan 2026 at 13:39, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
On 4 Jan 2026, at 00:25, Kirill Reshke <reshkekirill@gmail.com> wrote:
PFA v6
Would it be theoretically possible to unite functions for different GIN page types?
e.g. merge gin_entrypage_items + gin_datapage_items -> gin_tree_items? Or is it an awkward API?
For this, I borrowed this design from HASH and BRIN pageinspect
implementation. For them, we have one function-per-page-type. So,
maybe we can have dynamic schema here, but I don't see this as an
improvement to design.
--
Best regards,
Kirill Reshke
On Tue, 6 Jan 2026 at 21:28, Kirill Reshke <reshkekirill@gmail.com> wrote:
On Mon, 5 Jan 2026 at 13:39, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
On 4 Jan 2026, at 00:25, Kirill Reshke <reshkekirill@gmail.com> wrote:
PFA v6
Would it be theoretically possible to unite functions for different GIN page types?
e.g. merge gin_entrypage_items + gin_datapage_items -> gin_tree_items? Or is it an awkward API?For this, I borrowed this design from HASH and BRIN pageinspect
implementation. For them, we have one function-per-page-type. So,
maybe we can have dynamic schema here, but I don't see this as an
improvement to design.--
Best regards,
Kirill Reshke
CF bot did like trailing whitespace in regression output files,
posting v8 with this issue fixed.
--
Best regards,
Kirill Reshke
Attachments:
v8-0001-GIN-pageinspect-support-for-entry-tree-and-postin.patchapplication/octet-stream; name=v8-0001-GIN-pageinspect-support-for-entry-tree-and-postin.patchDownload+687-7
On 06.01.26 17:44, Kirill Reshke wrote:
On Tue, 6 Jan 2026 at 21:28, Kirill Reshke <reshkekirill@gmail.com> wrote:
On Mon, 5 Jan 2026 at 13:39, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
On 4 Jan 2026, at 00:25, Kirill Reshke <reshkekirill@gmail.com> wrote:
PFA v6
Would it be theoretically possible to unite functions for different GIN page types?
e.g. merge gin_entrypage_items + gin_datapage_items -> gin_tree_items? Or is it an awkward API?For this, I borrowed this design from HASH and BRIN pageinspect
implementation. For them, we have one function-per-page-type. So,
maybe we can have dynamic schema here, but I don't see this as an
improvement to design.--
Best regards,
Kirill ReshkeCF bot did like trailing whitespace in regression output files,
posting v8 with this issue fixed.
I noticed that the newly added C code could use a bit of modernization.
See attached patch for some suggestions. I only treated the new
gin_entrypage_items(); the new gin_datapage_items() could use mostly the
same changes. One of the main changes is to push many local variables
to smaller scopes. Also some changes in ereport parentheses style,
palloc style, some whitespace changes. (The whole patch also needs
pgindent treatment.)
Attachments:
0001-some-code-cleanup.patch.nocfbottext/plain; charset=UTF-8; name=0001-some-code-cleanup.patch.nocfbotDownload+44-52
On Wed, 7 Jan 2026 at 21:41, Peter Eisentraut <peter@eisentraut.org> wrote:
On 06.01.26 17:44, Kirill Reshke wrote:
On Tue, 6 Jan 2026 at 21:28, Kirill Reshke <reshkekirill@gmail.com> wrote:
On Mon, 5 Jan 2026 at 13:39, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
On 4 Jan 2026, at 00:25, Kirill Reshke <reshkekirill@gmail.com> wrote:
PFA v6
Would it be theoretically possible to unite functions for different GIN page types?
e.g. merge gin_entrypage_items + gin_datapage_items -> gin_tree_items? Or is it an awkward API?For this, I borrowed this design from HASH and BRIN pageinspect
implementation. For them, we have one function-per-page-type. So,
maybe we can have dynamic schema here, but I don't see this as an
improvement to design.--
Best regards,
Kirill ReshkeCF bot did like trailing whitespace in regression output files,
posting v8 with this issue fixed.I noticed that the newly added C code could use a bit of modernization.
See attached patch for some suggestions. I only treated the new
gin_entrypage_items(); the new gin_datapage_items() could use mostly the
same changes. One of the main changes is to push many local variables
to smaller scopes. Also some changes in ereport parentheses style,
palloc style, some whitespace changes. (The whole patch also needs
pgindent treatment.)
Hi!
Thank you. I included your changes in v9, and also did a similar
cleanup in gin_datapage_items().
For pg_ident run sake, and for general benefit, there is also v9-0001
which does cleanups similar to your changes for already-existing
functions
like gin_leafpage_items etc.
--
Best regards,
Kirill Reshke
On Thu, 8 Jan 2026 at 01:13, Kirill Reshke <reshkekirill@gmail.com> wrote:
On Wed, 7 Jan 2026 at 21:41, Peter Eisentraut <peter@eisentraut.org> wrote:
On 06.01.26 17:44, Kirill Reshke wrote:
On Tue, 6 Jan 2026 at 21:28, Kirill Reshke <reshkekirill@gmail.com> wrote:
On Mon, 5 Jan 2026 at 13:39, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
On 4 Jan 2026, at 00:25, Kirill Reshke <reshkekirill@gmail.com> wrote:
PFA v6
Would it be theoretically possible to unite functions for different GIN page types?
e.g. merge gin_entrypage_items + gin_datapage_items -> gin_tree_items? Or is it an awkward API?For this, I borrowed this design from HASH and BRIN pageinspect
implementation. For them, we have one function-per-page-type. So,
maybe we can have dynamic schema here, but I don't see this as an
improvement to design.--
Best regards,
Kirill ReshkeCF bot did like trailing whitespace in regression output files,
posting v8 with this issue fixed.I noticed that the newly added C code could use a bit of modernization.
See attached patch for some suggestions. I only treated the new
gin_entrypage_items(); the new gin_datapage_items() could use mostly the
same changes. One of the main changes is to push many local variables
to smaller scopes. Also some changes in ereport parentheses style,
palloc style, some whitespace changes. (The whole patch also needs
pgindent treatment.)Hi!
Thank you. I included your changes in v9, and also did a similar
cleanup in gin_datapage_items().
For pg_ident run sake, and for general benefit, there is also v9-0001
which does cleanups similar to your changes for already-existing
functions
like gin_leafpage_items etc.--
Best regards,
Kirill Reshke
PFA v10 with `make check` that passes.
Previous version missed dot at the end of errhint sentence
--
Best regards,
Kirill Reshke
On 8 Jan 2026, at 01:57, Kirill Reshke <reshkekirill@gmail.com> wrote:
PFA v10
"Prelimitary cleaunup" has two typos. Also it seems that you used something that is not pgindent.
Looks like clang-format with default settings.
+ errdetail("Expected special size %d, got %d.",
+ (int) MAXALIGN(sizeof(GinPageOpaqueData)),
+ PageGetSpecialSize(page)));
I PageGetSpecialSize() returns uint16, maybe let's cast it to (int) too?Besides this 2nd patch looks good to me.
Best regards, Andrey Borodin.
On Thu, 8 Jan 2026 at 21:49, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
On 8 Jan 2026, at 01:57, Kirill Reshke <reshkekirill@gmail.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
On Thu, 8 Jan 2026 at 22:11, Kirill Reshke <reshkekirill@gmail.com> wrote:
On Thu, 8 Jan 2026 at 21:49, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
On 8 Jan 2026, at 01:57, Kirill Reshke <reshkekirill@gmail.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
On 8 Jan 2026, at 23:00, Kirill Reshke <reshkekirill@gmail.com> wrote:
v11 with this and commit message polishing
LGTM, I switched CF item to RfC.
Thanks!
Best regards, Andrey Borodin.
On Jan 9, 2026, at 02:00, Kirill Reshke <reshkekirill@gmail.com> wrote:
On Thu, 8 Jan 2026 at 22:11, Kirill Reshke <reshkekirill@gmail.com> wrote:
On Thu, 8 Jan 2026 at 21:49, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
On 8 Jan 2026, at 01:57, Kirill Reshke <reshkekirill@gmail.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 ReshkeOkay, 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@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 rather than leaving to the committer.
Otherwise, code changes of 0001 is straightforward and LGTM.
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.
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/
