Обсуждение: 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
Вложения
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
Вложения
> 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]. Other AM's seems to defend from each other: if (!IS_INDEX(rel) || !IS_BTREE(rel)) or if (!IS_GIST(indexRel)). I don't seesuch 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() hasno 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
seesuch 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()
hasno 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
Вложения
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
Вложения
> 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
Вложения
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
Вложения
> 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]:
[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]:
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=111
>
>
> Thanks!
>
>
> 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]. 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
Вложения
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
Вложения
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 Reshke > > CF 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.)
Вложения
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 Reshke > > > > CF 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 Reshke > > > > > > CF 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 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.cformatting 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
thanleaving 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
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
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().
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().
*/
```
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/
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
Вложения
On Fri, 09 Jan 2026 at 21:39, Kirill Reshke <reshkekirill@gmail.com> wrote:
> 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 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().
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
>
> [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.
On Sat, 10 Jan 2026 at 07:20, Japin Li <japinli@hotmail.com> wrote:
> >
> > PFA v12.
> >
>
> Just a few small nitpicks on v12:
Hi!
Thank you for your review.
> 1.
> + /* Open the relation */
> + indexRel = index_open(indexRelid, AccessShareLock);
>
> "relation" is technically correct, but "index" feels more precise here.
I have updated this comment, thanks
> 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.
Fixed.
> 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 */
I have rephrased this comment.
> 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.
>
No, this function returns SET OF RECORD. So, `return (Datum) 0;` is
not exactly VOID or NULL, it is rather the end of the output marker.
You can also see `
gist_page_items`
Your comments refer to v12-0002. For the record, did you review 0001,
if yes, do you think it is good? I have included you as a reviewer to
v12-0002 commit message.
--
Best regards,
Kirill Reshke
Вложения
On Sat, 10 Jan 2026 at 12:41, Kirill Reshke <reshkekirill@gmail.com> wrote:
> On Sat, 10 Jan 2026 at 07:20, Japin Li <japinli@hotmail.com> wrote:
>> >
>> > PFA v12.
>> >
>>
>> Just a few small nitpicks on v12:
>
>
> Hi!
> Thank you for your review.
>
>
>> 1.
>> + /* Open the relation */
>> + indexRel = index_open(indexRelid, AccessShareLock);
>>
>> "relation" is technically correct, but "index" feels more precise here.
>
> I have updated this comment, thanks
>
>> 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.
>
> Fixed.
>
>> 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 */
>
> I have rephrased this comment.
>
Thanks for updating the patches.
>> 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.
>>
>
> No, this function returns SET OF RECORD. So, `return (Datum) 0;` is
> not exactly VOID or NULL, it is rather the end of the output marker.
> You can also see `
> gist_page_items`
Thanks for pointing that out!
>
> Your comments refer to v12-0002. For the record, did you review 0001,
> if yes, do you think it is good? I have included you as a reviewer to
> v12-0002 commit message.
>
Yeah, patch 0001 looks good to me.
I noticed that the IS_INDEX macro is currently defined in btreefunc.c,
hashfuncs.c, and now also in ginfuncs.c. Would it be possible to move it
to a shared header file so all these modules can include it from one place?
--
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.
On Sat, 10 Jan 2026 at 17:15, Japin Li <japinli@hotmail.com> wrote: > > Your comments refer to v12-0002. For the record, did you review 0001, > > if yes, do you think it is good? I have included you as a reviewer to > > v12-0002 commit message. > > > > Yeah, patch 0001 looks good to me. Thank you. I have updated 0001 commit message. > > I noticed that the IS_INDEX macro is currently defined in btreefunc.c, > hashfuncs.c, and now also in ginfuncs.c. Would it be possible to move it > to a shared header file so all these modules can include it from one place? > This is something I had thought about also. I do find this idea good, but 0002 patch is already big, and I don't want to overload it. So, v14-0004 with this change attached. -- Best regards, Kirill Reshke
Вложения
> On 10 Jan 2026, at 19:17, Kirill Reshke <reshkekirill@gmail.com> wrote: > > So, > v14-0004 with this change attached. Did you mean 0003? The change makes sense, but I'd note that macro is always used like this: if (!IS_INDEX(indexRel) || !IS_HASH(indexRel)) If we are refactoring this, maybe put IS_INDEX inside of corresponding IS_BTREE(),IS_GIN() and IS_HASH()? Best regards, Andrey Borodin.
On Sun, 11 Jan 2026 at 21:43, Andrey Borodin <x4mmm@yandex-team.ru> wrote: >> On 10 Jan 2026, at 19:17, Kirill Reshke <reshkekirill@gmail.com> wrote: >> >> So, >> v14-0004 with this change attached. > > Did you mean 0003? > > The change makes sense, but I'd note that macro is always used like this: > > if (!IS_INDEX(indexRel) || !IS_HASH(indexRel)) > > If we are refactoring this, maybe put IS_INDEX inside of corresponding IS_BTREE(),IS_GIN() and IS_HASH()? > +1 Attach the v15 patch. v15-0001 and v15-0002 unchanged and v15-0003 changed as above. -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd.
Вложения
On Wed, 14 Jan 2026 at 07:19, Japin Li <japinli@hotmail.com> wrote: > > On Sun, 11 Jan 2026 at 21:43, Andrey Borodin <x4mmm@yandex-team.ru> wrote: > >> On 10 Jan 2026, at 19:17, Kirill Reshke <reshkekirill@gmail.com> wrote: > >> > >> So, > >> v14-0004 with this change attached. > > > > Did you mean 0003? > > > > The change makes sense, but I'd note that macro is always used like this: > > > > if (!IS_INDEX(indexRel) || !IS_HASH(indexRel)) > > > > If we are refactoring this, maybe put IS_INDEX inside of corresponding IS_BTREE(),IS_GIN() and IS_HASH()? > > > > +1 > > Attach the v15 patch. > v15-0001 and v15-0002 unchanged and v15-0003 changed as above. > > -- > Regards, > Japin Li > ChengDu WenWu Information Technology Co., Ltd. > Hi! Thank you 🙏 for your effort. 0003 looks good to me -- Best regards, Kirill Reshke
> Hi! Thank you 🙏 for your effort. 0003 looks good to me
>
> --
> Best regards,
> Kirill Reshke
Hi! Thanks for the patch!
I'v been reviewing your patch, and noticed there are some code logic
that handles NULL values, but tests doesn't cover this scenarios.
So, I added simple line at contrib/pageinspect/sql/gin.sql:
INSERT INTO test1 VALUES (1, ARRAY[11, 111], ARRAY['a', 'b', 'c']);
+INSERT INTO test1 VALUES (1, ARRAY[NULL, 222], ARRAY['d', NULL]);
CREATE INDEX test1_y_idx ON test1 USING gin (y) WITH (fastupdate = off);
And got unexpected result...
As far as I understand, we shouldn't get error for the whole
gin_entrypage_items executions when there are NULL-values columns at index key, but the
output contains only the next error:
SELECT * FROM gin_entrypage_items(get_raw_page('test1_y_idx', 1), 'test1_y_idx'::regclass);
--[ RECORD 1 ]--------------
-itemoffset | 1
-downlink | (2147483664,1)
-tids | {"(0,1)"}
-keys | y=11
--[ RECORD 2 ]--------------
-itemoffset | 2
-downlink | (2147483664,1)
-tids | {"(0,1)"}
-keys | y=111
-
+ERROR: invalid gin entry page tuple at offset 4
Is the NULL values handle correct?
--
Best regards,
Roman Khapov
On Wed, 14 Jan 2026 at 15:26, Roman Khapov <rkhapov@yandex-team.ru> wrote:
>> Hi! Thank you 🙏 for your effort. 0003 looks good to me
>>
>> --
>> Best regards,
>> Kirill Reshke
>
> Hi! Thanks for the patch!
>
> I'v been reviewing your patch, and noticed there are some code logic
> that handles NULL values, but tests doesn't cover this scenarios.
>
> So, I added simple line at contrib/pageinspect/sql/gin.sql:
>
> INSERT INTO test1 VALUES (1, ARRAY[11, 111], ARRAY['a', 'b', 'c']);
> +INSERT INTO test1 VALUES (1, ARRAY[NULL, 222], ARRAY['d', NULL]);
> CREATE INDEX test1_y_idx ON test1 USING gin (y) WITH (fastupdate = off);
>
> And got unexpected result...
> As far as I understand, we shouldn't get error for the whole
> gin_entrypage_items executions when there are NULL-values columns at index key, but the
> output contains only the next error:
>
> SELECT * FROM gin_entrypage_items(get_raw_page('test1_y_idx', 1), 'test1_y_idx'::regclass);
> --[ RECORD 1 ]--------------
> -itemoffset | 1
> -downlink | (2147483664,1)
> -tids | {"(0,1)"}
> -keys | y=11
> --[ RECORD 2 ]--------------
> -itemoffset | 2
> -downlink | (2147483664,1)
> -tids | {"(0,1)"}
> -keys | y=111
> -
> +ERROR: invalid gin entry page tuple at offset 4
>
> Is the NULL values handle correct?
>
Nice catch!
I agree — the NULL handling seems wrong.
Here's a quick patch to fix it. What do you think?
--
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.
Вложения
On Wed, 14 Jan 2026 at 17:17, Japin Li <japinli@hotmail.com> wrote:
>
> On Wed, 14 Jan 2026 at 15:26, Roman Khapov <rkhapov@yandex-team.ru> wrote:
> >> Hi! Thank you 🙏 for your effort. 0003 looks good to me
> >>
> >> --
> >> Best regards,
> >> Kirill Reshke
> >
> > Hi! Thanks for the patch!
> >
> > I'v been reviewing your patch, and noticed there are some code logic
> > that handles NULL values, but tests doesn't cover this scenarios.
> >
> > So, I added simple line at contrib/pageinspect/sql/gin.sql:
> >
> > INSERT INTO test1 VALUES (1, ARRAY[11, 111], ARRAY['a', 'b', 'c']);
> > +INSERT INTO test1 VALUES (1, ARRAY[NULL, 222], ARRAY['d', NULL]);
> > CREATE INDEX test1_y_idx ON test1 USING gin (y) WITH (fastupdate = off);
> >
> > And got unexpected result...
> > As far as I understand, we shouldn't get error for the whole
> > gin_entrypage_items executions when there are NULL-values columns at index key, but the
> > output contains only the next error:
> >
> > SELECT * FROM gin_entrypage_items(get_raw_page('test1_y_idx', 1), 'test1_y_idx'::regclass);
> > --[ RECORD 1 ]--------------
> > -itemoffset | 1
> > -downlink | (2147483664,1)
> > -tids | {"(0,1)"}
> > -keys | y=11
> > --[ RECORD 2 ]--------------
> > -itemoffset | 2
> > -downlink | (2147483664,1)
> > -tids | {"(0,1)"}
> > -keys | y=111
> > -
> > +ERROR: invalid gin entry page tuple at offset 4
> >
> > Is the NULL values handle correct?
> >
>
> Nice catch!
>
> I agree — the NULL handling seems wrong.
> Here's a quick patch to fix it. What do you think?
>
> --
> Regards,
> Japin Li
> ChengDu WenWu Information Technology Co., Ltd.
>
Yes, simply removing elog here is correct. See also
gintuple_get_attrnum - this is where is copied this part from.
Will you post a new patch version with this applied?
--
Best regards,
Kirill Reshke
> I agree — the NULL handling seems wrong. > Here's a quick patch to fix it. What do you think? LGTM, but maybe the test should be expanded with NULL for the two-column indexes? I mean to add NULLs at zero column too. Also, about the tests but not related to NULLs... Seems like we need to run those tests on 32bit system, the test output should be different? -- Best regards, Roman Khapov
On Wed, 14 Jan 2026 at 17:44, Roman Khapov <rkhapov@yandex-team.ru> wrote: > > > > I agree — the NULL handling seems wrong. > > Here's a quick patch to fix it. What do you think? > > LGTM, but maybe the test should be expanded with NULL for the two-column > indexes? I mean to add NULLs at zero column too. > > Also, about the tests but not related to NULLs... > Seems like we need to run those tests on 32bit system, the test output should be different? Yes, gin_1.out is for 32-bit output. Adding new tuples in relation will result in binary layout changes, so we need to update gin.out for 64 bit and gin_1.out for 32 bit -- Best regards, Kirill Reshke
On Wed, 14 Jan 2026 at 17:44, Roman Khapov <rkhapov@yandex-team.ru> wrote: >> I agree — the NULL handling seems wrong. >> Here's a quick patch to fix it. What do you think? > > LGTM, but maybe the test should be expanded with NULL for the two-column > indexes? I mean to add NULLs at zero column too. > Sorry, I’m not sure I understand. Are you suggesting we add one (or both) of these test cases? INSERT INTO test1 VALUES (2, NULL, NULL); INSERT INTO test1 VALUES (2, ARRAY[NULL, 222], ARRAY['d', NULL]); > Also, about the tests but not related to NULLs... > Seems like we need to run those tests on 32bit system, the test output should be different? > Agree. -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd.
On Wed, 14 Jan 2026 at 19:51, Japin Li <japinli@hotmail.com> wrote: > > On Wed, 14 Jan 2026 at 17:44, Roman Khapov <rkhapov@yandex-team.ru> wrote: > >> I agree — the NULL handling seems wrong. > >> Here's a quick patch to fix it. What do you think? > > > > LGTM, but maybe the test should be expanded with NULL for the two-column > > indexes? I mean to add NULLs at zero column too. > > > > Sorry, I’m not sure I understand. > > Are you suggesting we add one (or both) of these test cases? > > INSERT INTO test1 VALUES (2, NULL, NULL); > INSERT INTO test1 VALUES (2, ARRAY[NULL, 222], ARRAY['d', NULL]); > Yes, like this, to have gin_entrypage_items to output y=NULL and z=NULL for y_z index -- Best regards, Kirill Reshke
Hi, Kirill On Wed, 14 Jan 2026 at 17:46, Kirill Reshke <reshkekirill@gmail.com> wrote: > On Wed, 14 Jan 2026 at 17:44, Roman Khapov <rkhapov@yandex-team.ru> wrote: >> >> >> > I agree — the NULL handling seems wrong. >> > Here's a quick patch to fix it. What do you think? >> >> LGTM, but maybe the test should be expanded with NULL for the two-column >> indexes? I mean to add NULLs at zero column too. >> >> Also, about the tests but not related to NULLs... >> Seems like we need to run those tests on 32bit system, the test output should be different? > > Yes, gin_1.out is for 32-bit output. Adding new tuples in relation > will result in binary layout changes, so we need to update gin.out for > 64 bit and gin_1.out for 32 bit > I only have a 64-bit system to work with. I'm unsure how to produce the correct gin_1.out file for 32-bit architecture while running on 64-bit. Any advice or recommended workflow would be greatly appreciated! -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd.
On Wed, 14 Jan 2026 at 20:03, Kirill Reshke <reshkekirill@gmail.com> wrote: > On Wed, 14 Jan 2026 at 19:51, Japin Li <japinli@hotmail.com> wrote: >> >> On Wed, 14 Jan 2026 at 17:44, Roman Khapov <rkhapov@yandex-team.ru> wrote: >> >> I agree — the NULL handling seems wrong. >> >> Here's a quick patch to fix it. What do you think? >> > >> > LGTM, but maybe the test should be expanded with NULL for the two-column >> > indexes? I mean to add NULLs at zero column too. >> > >> >> Sorry, I’m not sure I understand. >> >> Are you suggesting we add one (or both) of these test cases? >> >> INSERT INTO test1 VALUES (2, NULL, NULL); >> INSERT INTO test1 VALUES (2, ARRAY[NULL, 222], ARRAY['d', NULL]); >> > > Yes, like this, to have gin_entrypage_items to output y=NULL and > z=NULL for y_z index > Thanks for the suggestion! I think one of those test cases is sufficient — I'll include it in the next version. -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd.
> Sorry, I’m not sure I understand. Oh, my mail app replaced 'z' with 'zero', sorry.. > INSERT INTO test1 VALUES (2, ARRAY[NULL, 222], ARRAY['d', NULL]); Yes, I meant that scenario, when z contains NULL. -- Best regards, Roman Khapov
On Wed, 14 Jan 2026 at 21:12, Roman Khapov <rkhapov@yandex-team.ru> wrote: >> Sorry, I’m not sure I understand. > > Oh, my mail app replaced 'z' with 'zero', sorry.. > >> INSERT INTO test1 VALUES (2, ARRAY[NULL, 222], ARRAY['d', NULL]); > > Yes, I meant that scenario, when z contains NULL. > I've added the test case in v16-0002. PFA. For v16-0003, I've kept it here as well for now. However, it may become unnecessary once [0] is committed. [0] https://www.postgresql.org/message-id/MEAPR01MB3031A889D4B3F610E9D2A3AFB68FA@MEAPR01MB3031.ausprd01.prod.outlook.com -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd.
Вложения
> On 14 Jan 2026, at 20:06, Japin Li <japinli@hotmail.com> wrote: > > I only have a 64-bit system to work with. I've tried v16 on 32-bit Ubuntu VM. If anyone want to toy with it, here's 3.2Gb disk https://storage.yandexcloud.net/x4m/ubuntu.qcow2 To run it you can just do something in line with x4mmm@x4mmm-osx Downloads % qemu-system-i386 \ -hda ubuntu.qcow2 \ -m 2G \ -display cocoa \ -boot d -smp 8 -netdev user,id=mynet0,hostfwd=tcp::2222-:22 -device e1000,netdev=mynet0 And ssh into VM ssh ubuntu@localhost -p 2222 password ubuntu ..so, v16 patch passes tests via gin_1.out Thanks! Best regards, Andrey Borodin.
On Thu, 15 Jan 2026 at 13:20, Andrey Borodin <x4mmm@yandex-team.ru> wrote: >> On 14 Jan 2026, at 20:06, Japin Li <japinli@hotmail.com> wrote: >> >> I only have a 64-bit system to work with. > > I've tried v16 on 32-bit Ubuntu VM. > > If anyone want to toy with it, here's 3.2Gb disk > https://storage.yandexcloud.net/x4m/ubuntu.qcow2 > > To run it you can just do something in line with > x4mmm@x4mmm-osx Downloads % qemu-system-i386 \ > -hda ubuntu.qcow2 \ > -m 2G \ > -display cocoa \ > -boot d -smp 8 -netdev user,id=mynet0,hostfwd=tcp::2222-:22 -device e1000,netdev=mynet0 > > And ssh into VM > ssh ubuntu@localhost -p 2222 > password ubuntu > > ..so, v16 patch passes tests via gin_1.out > > Thanks! > Thanks! After some searching, I found it can be achieved by installing the 32-bit packages on a 64-bit machine and compiling with: CFLAGS='-m32' CPPFLAGS='-m32' LDFLAGS='-m32' > > Best regards, Andrey Borodin. -- Regards, Japin Li ChengDu WenWu Information Technology Co., Ltd.