amcheck: support for GiST

On 30 Jun 2025, at 16:34, Andrey Borodin <x4mmm@yandex-team.ru> wrote:

Please find attached two new steps for amcheck:
1. A function to verify GiST integrity. This patch is in decent shape, simply rebased from previous year.
2. Support on pg_amcheck's side for this function. This patch did not receive such review attention before. And, perhaps, should be extended to support existing GIN functions.

Here's a version that adds GIN functions to pg_amcheck.
IDK, maybe we should split pg_amcheck part into another thread and add there BRIN too...

Thanks!

Best regards, Andrey Borodin.

Hi, Andrey!

Thank you for working on this! There is a long history of the patch, I
hope it will be committed soon!)

On Fri, Jul 11, 2025 at 3:39 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:

On 30 Jun 2025, at 16:34, Andrey Borodin <x4mmm@yandex-team.ru> wrote:

Please find attached two new steps for amcheck:
1. A function to verify GiST integrity. This patch is in decent shape, simply rebased from previous year.
2. Support on pg_amcheck's side for this function. This patch did not receive such review attention before. And, perhaps, should be extended to support existing GIN functions.

Here's a version that adds GIN functions to pg_amcheck.
IDK, maybe we should split pg_amcheck part into another thread and add there BRIN too...

Speaking of BRIN pg_amcheck, I probably wouldn't merge it with the
gist/gin pg_amceck patchset because that would create a dependency on
the amcheck BRIN support patch, which is not clear when it will be
ready.

There are some points about the patch:

1) There are several typos in verify_gist.c:

downlinks -> downlink (header comment)
discrepencies -> discrepancies
Correctess -> Correctness
hande -> handle
Initaliaze -> Initialize
numbmer -> number
replcaed -> replaced
aquire -> aqcuire

2) Copyright year is 2023 in the patch. Time flies:)

3) There is the same check in btree and while reviewing the patch I
realised it should be added to the BRIN amcheck as well. Probably it
will be needed for GIN someday. What do you think about moving it to
verify_common?

if (IsolationUsesXactSnapshot() && rel->rd_index->indcheckxmin &&
!TransactionIdPrecedes(HeapTupleHeaderGetXmin(rel->rd_indextuple->t_data),
result->snapshot->xmin))

4) Should we check blknum of the new entry before pushing to the
stack? Probably we can check if it's a valid blknum and it's not
outside of the index. This way we can give a more detailed error
message in case we meet the wrong blknum.

in the split detection code:

ptr->blkno = GistPageGetOpaque(page)->rightlink;

and when we add children of an inner page:

ptr->blkno = ItemPointerGetBlockNumber(&(idxtuple->t_tid));

5) There is a macros for 0xffff - 'TUPLE_IS_VALID'. Maybe we can use
it to make the code more readable? Also the error message contains one
extra 'has'.

if (off != 0xffff)
ereport(ERROR,
(errcode(ERRCODE_INDEX_CORRUPTED),
errmsg("index \"%s\" has on page %u offset %u has
item id not pointing to 0xffff, but %hu",
RelationGetRelationName(check_state->rel),
stack->blkno, i, off)));

6) Several points about 'gistFormNormalizedTuple'. I read the previous
thread [1]/messages/by-id/CAAhFRxiHCWe_6AmqGWZqYEkgN_uQG3Jgw0WgPw+0zO3_D-q4DA@mail.gmail.com and it seems there is an unfinished discussion about
normalizing during heapallindexed check. I think normalization needs
some more work here.

6a) There is a TODO
/* pfree(DatumGetPointer(old)); // TODO: this fails. Why? */

6b) AFAICS 'compress' is an optional support function. If opclass
doesn't have a 'compress' function, then 'gistCompressValues' leaves
such attributes as it is. Here we get attdata from the heap scan, and
it could be toasted. That means that these checks can result in false
positives:

gistCompressValues(giststate, r, attdata, isnull, true, compatt);
...

for (int i = 0; i < r->rd_att->natts; i++)
{
if (VARATT_IS_EXTERNAL(DatumGetPointer(compatt[i])))
ereport(ERROR,
(errcode(ERRCODE_INDEX_CORRUPTED),
....
if (VARATT_IS_COMPRESSED(DatumGetPointer(compatt[i])))
{
if (i < IndexRelationGetNumberOfKeyAttributes(r))
ereport(ERROR,

Also 'VARATT_IS_EXTERNAL' check will always result in a false
positive for toasted include attributes here. Reproducer for it:

DROP TABLE IF EXISTS tbl;
CREATE TABLE tbl(a point, t text);
-- disable compression for 't', but let it to be external
ALTER TABLE tbl ALTER COLUMN t SET STORAGE external ;
INSERT INTO tbl values (point(random(), random()), repeat('a',3000 ));
CREATE INDEX tbl_idx ON tbl using gist (a) include (t);
SELECT gist_index_check('tbl_idx', true);

So I think we need to remove these checks completely.

6c) Current code doesn't apply normalization for existing index tuples
during adding to bloom filter, which can result in false positive,
reproducer:
Here we use plain storage during index build, then during check we
have extended storage, which results in different binary
representation of the same data and we have false positive here.

DROP TABLE IF EXISTS tbl;
CREATE TABLE tbl(a tsvector);
CREATE INDEX tbl_idx ON tbl using gist (a) ;
ALTER TABLE tbl ALTER COLUMN a SET STORAGE plain;
INSERT INTO tbl values ('a' ::tsvector);
ALTER TABLE tbl ALTER COLUMN a SET STORAGE extended ;
SELECT gist_index_check('tbl_idx', true);

6d) In the end of 'gistFormNormalizedTuple' we have

ItemPointerSetOffsetNumber(&(res->t_tid), 0xffff);

I guess it follows gistFormTuple function, but here we use
gistFormNormalizedTuple only for leaf tuples and we override
offsetnumber right after 'gistFormNormalizedTuple' function call, so
looks like we can drop it.

In general I think normalization here can follow the same logic as
for verify_nbtree. We can even reuse 'bt_normalize_tuple' as a
normalization function. It handles all corner cases like short varatt,
differences in compressions etc, that we can have in gist as well. It
contains just a few lines about btree and everything else valid for
gist, so we need to modify it a bit. I think we can move it to
verify_common. Then we need to normalize every existing leaf index
tuple before adding it to the bloom filter. During the probing phase I
think we just can use 'gistFormTuple' to build an index tuple and then
normalize it before probing. What do you think?

Thank you!

[1]: /messages/by-id/CAAhFRxiHCWe_6AmqGWZqYEkgN_uQG3Jgw0WgPw+0zO3_D-q4DA@mail.gmail.com

Best regards,
Arseniy Mukhin

Hi! Thank you for your review.
Im posting new version of 0001 patch of series

On Tue, 22 Jul 2025 at 15:47, Arseniy Mukhin
<arseniy.mukhin.dev@gmail.com> wrote:

Hi, Andrey!

Thank you for working on this! There is a long history of the patch, I
hope it will be committed soon!)

On Fri, Jul 11, 2025 at 3:39 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:

On 30 Jun 2025, at 16:34, Andrey Borodin <x4mmm@yandex-team.ru> wrote:

Please find attached two new steps for amcheck:
1. A function to verify GiST integrity. This patch is in decent shape, simply rebased from previous year.
2. Support on pg_amcheck's side for this function. This patch did not receive such review attention before. And, perhaps, should be extended to support existing GIN functions.

Here's a version that adds GIN functions to pg_amcheck.
IDK, maybe we should split pg_amcheck part into another thread and add there BRIN too...

Speaking of BRIN pg_amcheck, I probably wouldn't merge it with the
gist/gin pg_amceck patchset because that would create a dependency on
the amcheck BRIN support patch, which is not clear when it will be
ready.

There are some points about the patch:

1) There are several typos in verify_gist.c:

downlinks -> downlink (header comment)
discrepencies -> discrepancies
Correctess -> Correctness
hande -> handle
Initaliaze -> Initialize
numbmer -> number
replcaed -> replaced
aquire -> aqcuire

2) Copyright year is 2023 in the patch. Time flies:)

These two are (trivially) fixed.

3) There is the same check in btree and while reviewing the patch I
realised it should be added to the BRIN amcheck as well. Probably it
will be needed for GIN someday. What do you think about moving it to
verify_common?

if (IsolationUsesXactSnapshot() && rel->rd_index->indcheckxmin &&
!TransactionIdPrecedes(HeapTupleHeaderGetXmin(rel->rd_indextuple->t_data),
result->snapshot->xmin))

I think this is a good idea. I'm not sure if we should bother with
refactoring in this series though...

4) Should we check blknum of the new entry before pushing to the
stack? Probably we can check if it's a valid blknum and it's not
outside of the index. This way we can give a more detailed error
message in case we meet the wrong blknum.

in the split detection code:

ptr->blkno = GistPageGetOpaque(page)->rightlink;

and when we add children of an inner page:

ptr->blkno = ItemPointerGetBlockNumber(&(idxtuple->t_tid));

We indeed need to recheck all bytes in possibly-corrupted indexes,
including downlinks.
But amcheck can be run concurrently with Index insert, which will
change the current index size, so checking is not trivial.
And given it is not checked in already-committed nbtree & GIN amcheck
modules, I suggest not bother with that in this thread.
This can be a separate patch to verify_nbtree.

5) There is a macros for 0xffff - 'TUPLE_IS_VALID'. Maybe we can use
it to make the code more readable? Also the error message contains one
extra 'has'.

if (off != 0xffff)
ereport(ERROR,
(errcode(ERRCODE_INDEX_CORRUPTED),
errmsg("index \"%s\" has on page %u offset %u has
item id not pointing to 0xffff, but %hu",
RelationGetRelationName(check_state->rel),
stack->blkno, i, off)));

Sure, I replaced all usages with TUPLE_IS_VALID.

6) Several points about 'gistFormNormalizedTuple'. I read the previous
thread [1] and it seems there is an unfinished discussion about
normalizing during heapallindexed check. I think normalization needs
some more work here.

6a) There is a TODO
/* pfree(DatumGetPointer(old)); // TODO: this fails. Why? */

6b) AFAICS 'compress' is an optional support function. If opclass
doesn't have a 'compress' function, then 'gistCompressValues' leaves
such attributes as it is. Here we get attdata from the heap scan, and
it could be toasted. That means that these checks can result in false
positives:

gistCompressValues(giststate, r, attdata, isnull, true, compatt);
...

for (int i = 0; i < r->rd_att->natts; i++)
{
if (VARATT_IS_EXTERNAL(DatumGetPointer(compatt[i])))
ereport(ERROR,
(errcode(ERRCODE_INDEX_CORRUPTED),
....
if (VARATT_IS_COMPRESSED(DatumGetPointer(compatt[i])))
{
if (i < IndexRelationGetNumberOfKeyAttributes(r))
ereport(ERROR,

Also 'VARATT_IS_EXTERNAL' check will always result in a false
positive for toasted include attributes here. Reproducer for it:

DROP TABLE IF EXISTS tbl;
CREATE TABLE tbl(a point, t text);
-- disable compression for 't', but let it to be external
ALTER TABLE tbl ALTER COLUMN t SET STORAGE external ;
INSERT INTO tbl values (point(random(), random()), repeat('a',3000 ));
CREATE INDEX tbl_idx ON tbl using gist (a) include (t);
SELECT gist_index_check('tbl_idx', true);

So I think we need to remove these checks completely.

6c) Current code doesn't apply normalization for existing index tuples
during adding to bloom filter, which can result in false positive,
reproducer:
Here we use plain storage during index build, then during check we
have extended storage, which results in different binary
representation of the same data and we have false positive here.

DROP TABLE IF EXISTS tbl;
CREATE TABLE tbl(a tsvector);
CREATE INDEX tbl_idx ON tbl using gist (a) ;
ALTER TABLE tbl ALTER COLUMN a SET STORAGE plain;
INSERT INTO tbl values ('a' ::tsvector);
ALTER TABLE tbl ALTER COLUMN a SET STORAGE extended ;
SELECT gist_index_check('tbl_idx', true);

6d) In the end of 'gistFormNormalizedTuple' we have

ItemPointerSetOffsetNumber(&(res->t_tid), 0xffff);

I guess it follows gistFormTuple function, but here we use
gistFormNormalizedTuple only for leaf tuples and we override
offsetnumber right after 'gistFormNormalizedTuple' function call, so
looks like we can drop it.

In general I think normalization here can follow the same logic as
for verify_nbtree. We can even reuse 'bt_normalize_tuple' as a
normalization function. It handles all corner cases like short varatt,
differences in compressions etc, that we can have in gist as well. It
contains just a few lines about btree and everything else valid for
gist, so we need to modify it a bit. I think we can move it to
verify_common. Then we need to normalize every existing leaf index
tuple before adding it to the bloom filter. During the probing phase I
think we just can use 'gistFormTuple' to build an index tuple and then
normalize it before probing. What do you think?

Thank you!

I did a little refactor in patch one, so we can reuse
bt_normalize_tuple. With these changes, your reproducers do not
complain.
I guess `gistFormNormalizedTuple` is now unneeded and its comment no
longer true. index_form_tuple in gist_tuple_present_callback ensures
all attributes are detoasted, and then we do `amcheck_normalize_tuple`
call.
I will remove gistFormNormalizedTuple function in next iteration if
this approach is OK

[1] /messages/by-id/CAAhFRxiHCWe_6AmqGWZqYEkgN_uQG3Jgw0WgPw+0zO3_D-q4DA@mail.gmail.com

Best regards,
Arseniy Mukhin

--
Best regards,
Kirill Reshke

Hi,

On Wed, Oct 22, 2025 at 9:57 PM Kirill Reshke <reshkekirill@gmail.com> wrote:

Hi! Thank you for your review.

Thank you for the new version!

Im posting new version of 0001 patch of series

On Tue, 22 Jul 2025 at 15:47, Arseniy Mukhin
<arseniy.mukhin.dev@gmail.com> wrote:

Hi, Andrey!

Thank you for working on this! There is a long history of the patch, I
hope it will be committed soon!)

On Fri, Jul 11, 2025 at 3:39 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:

On 30 Jun 2025, at 16:34, Andrey Borodin <x4mmm@yandex-team.ru> wrote:

Please find attached two new steps for amcheck:
1. A function to verify GiST integrity. This patch is in decent shape, simply rebased from previous year.
2. Support on pg_amcheck's side for this function. This patch did not receive such review attention before. And, perhaps, should be extended to support existing GIN functions.

Here's a version that adds GIN functions to pg_amcheck.
IDK, maybe we should split pg_amcheck part into another thread and add there BRIN too...

Speaking of BRIN pg_amcheck, I probably wouldn't merge it with the
gist/gin pg_amceck patchset because that would create a dependency on
the amcheck BRIN support patch, which is not clear when it will be
ready.

There are some points about the patch:

1) There are several typos in verify_gist.c:

downlinks -> downlink (header comment)
discrepencies -> discrepancies
Correctess -> Correctness
hande -> handle
Initaliaze -> Initialize
numbmer -> number
replcaed -> replaced
aquire -> aqcuire

2) Copyright year is 2023 in the patch. Time flies:)

These two are (trivially) fixed.

3) There is the same check in btree and while reviewing the patch I
realised it should be added to the BRIN amcheck as well. Probably it
will be needed for GIN someday. What do you think about moving it to
verify_common?

if (IsolationUsesXactSnapshot() && rel->rd_index->indcheckxmin &&
!TransactionIdPrecedes(HeapTupleHeaderGetXmin(rel->rd_indextuple->t_data),
result->snapshot->xmin))

I think this is a good idea. I'm not sure if we should bother with
refactoring in this series though...

Great, so maybe we can start a separate thread for this small
refactoring. Some work in this direction has been already done in brin
amcheck thread [0]/messages/by-id/CAE7r3MKUOGJ0v5-b5fYaF6sxKZvr0J-YXHTJf8u8GUr1tTcvNg@mail.gmail.com.

4) Should we check blknum of the new entry before pushing to the
stack? Probably we can check if it's a valid blknum and it's not
outside of the index. This way we can give a more detailed error
message in case we meet the wrong blknum.

in the split detection code:

ptr->blkno = GistPageGetOpaque(page)->rightlink;

and when we add children of an inner page:

ptr->blkno = ItemPointerGetBlockNumber(&(idxtuple->t_tid));

We indeed need to recheck all bytes in possibly-corrupted indexes,
including downlinks.
But amcheck can be run concurrently with Index insert, which will
change the current index size, so checking is not trivial.
And given it is not checked in already-committed nbtree & GIN amcheck
modules, I suggest not bother with that in this thread.
This can be a separate patch to verify_nbtree.

OK.

6) Several points about 'gistFormNormalizedTuple'. I read the previous
thread [1] and it seems there is an unfinished discussion about
normalizing during heapallindexed check. I think normalization needs
some more work here.

6a) There is a TODO
/* pfree(DatumGetPointer(old)); // TODO: this fails. Why? */

6b) AFAICS 'compress' is an optional support function. If opclass
doesn't have a 'compress' function, then 'gistCompressValues' leaves
such attributes as it is. Here we get attdata from the heap scan, and
it could be toasted. That means that these checks can result in false
positives:

gistCompressValues(giststate, r, attdata, isnull, true, compatt);
...

for (int i = 0; i < r->rd_att->natts; i++)
{
if (VARATT_IS_EXTERNAL(DatumGetPointer(compatt[i])))
ereport(ERROR,
(errcode(ERRCODE_INDEX_CORRUPTED),
....
if (VARATT_IS_COMPRESSED(DatumGetPointer(compatt[i])))
{
if (i < IndexRelationGetNumberOfKeyAttributes(r))
ereport(ERROR,

Also 'VARATT_IS_EXTERNAL' check will always result in a false
positive for toasted include attributes here. Reproducer for it:

DROP TABLE IF EXISTS tbl;
CREATE TABLE tbl(a point, t text);
-- disable compression for 't', but let it to be external
ALTER TABLE tbl ALTER COLUMN t SET STORAGE external ;
INSERT INTO tbl values (point(random(), random()), repeat('a',3000 ));
CREATE INDEX tbl_idx ON tbl using gist (a) include (t);
SELECT gist_index_check('tbl_idx', true);

So I think we need to remove these checks completely.

6c) Current code doesn't apply normalization for existing index tuples
during adding to bloom filter, which can result in false positive,
reproducer:
Here we use plain storage during index build, then during check we
have extended storage, which results in different binary
representation of the same data and we have false positive here.

DROP TABLE IF EXISTS tbl;
CREATE TABLE tbl(a tsvector);
CREATE INDEX tbl_idx ON tbl using gist (a) ;
ALTER TABLE tbl ALTER COLUMN a SET STORAGE plain;
INSERT INTO tbl values ('a' ::tsvector);
ALTER TABLE tbl ALTER COLUMN a SET STORAGE extended ;
SELECT gist_index_check('tbl_idx', true);

6d) In the end of 'gistFormNormalizedTuple' we have

ItemPointerSetOffsetNumber(&(res->t_tid), 0xffff);

I guess it follows gistFormTuple function, but here we use
gistFormNormalizedTuple only for leaf tuples and we override
offsetnumber right after 'gistFormNormalizedTuple' function call, so
looks like we can drop it.

In general I think normalization here can follow the same logic as
for verify_nbtree. We can even reuse 'bt_normalize_tuple' as a
normalization function. It handles all corner cases like short varatt,
differences in compressions etc, that we can have in gist as well. It
contains just a few lines about btree and everything else valid for
gist, so we need to modify it a bit. I think we can move it to
verify_common. Then we need to normalize every existing leaf index
tuple before adding it to the bloom filter. During the probing phase I
think we just can use 'gistFormTuple' to build an index tuple and then
normalize it before probing. What do you think?

Thank you!

I did a little refactor in patch one, so we can reuse
bt_normalize_tuple. With these changes, your reproducers do not
complain.
I guess `gistFormNormalizedTuple` is now unneeded and its comment no
longer true. index_form_tuple in gist_tuple_present_callback ensures
all attributes are detoasted, and then we do `amcheck_normalize_tuple`
call.
I will remove gistFormNormalizedTuple function in next iteration if
this approach is OK

LGTM. Only one point here: I think maybe we need to move the
bt_normalize_tuple comment as well, as now it is more about
amcheck_normalize_tuple than bt_normalize_tuple.

[0]: /messages/by-id/CAE7r3MKUOGJ0v5-b5fYaF6sxKZvr0J-YXHTJf8u8GUr1tTcvNg@mail.gmail.com

Best regards,
Arseniy Mukhin

The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested

Hi,
Together with Sergey we did a review and found a few things that need fixing:

- `contrib/amcheck/amcheck--1.5--1.6.sql:14` - missing space after comma
- `verify_gist.c` should be the second entry in meson.build, not at the end
- Some function arguments like `(GistCheckState * check_state, GistScanItem * stack)` have extra spaces after the `*` - should be `(GistCheckState *check_state, GistScanItem *stack)`
- Missing `#include "access/itup.h"` in `verify_common.h`
- Missing test file `007verify_gist_.pl` (not sure if should be created)
- `contrib/amcheck/sql/check_gist.sql` - missing cleanup statement `DROP TABLE toast_bug;`

Let us know if you need any clarification on these points!

- Miłosz and Sergey

Hi Miłosz and Sergey!

Thanks a lot for reviewing this patch!

On 9 Dec 2025, at 23:54, Miłosz Bieniek <bieniek.milosz@proton.me> wrote:

Hi,
Together with Sergey we did a review and found a few things that need fixing:

- `contrib/amcheck/amcheck--1.5--1.6.sql:14` - missing space after comma

Fixed.

- `verify_gist.c` should be the second entry in meson.build, not at the end

Fixed.

- Some function arguments like `(GistCheckState * check_state, GistScanItem * stack)` have extra spaces after the `*` - should be `(GistCheckState *check_state, GistScanItem *stack)`

Fixed.

- Missing `#include "access/itup.h"` in `verify_common.h`

I do not understand why. Perhaps, optimizing headers would be a good idea. This file is not included by any of files that include verify_common.h.

- Missing test file `007verify_gist_.pl` (not sure if should be created)

I don't think we ever had it for gist.

- `contrib/amcheck/sql/check_gist.sql` - missing cleanup statement `DROP TABLE toast_bug;`

Fixed.

Thanks!

Best regards, Andrey Borodin.

Hello,

On Wed, Oct 22, 2025 at 11:58 AM Kirill Reshke <reshkekirill@gmail.com> wrote:

1) There are several typos in verify_gist.c:

downlinks -> downlink (header comment)
discrepencies -> discrepancies
Correctess -> Correctness
hande -> handle
Initaliaze -> Initialize
numbmer -> number
replcaed -> replaced
aquire -> aqcuire

2) Copyright year is 2023 in the patch. Time flies:)

These two are (trivially) fixed.

I found a few more typos. Maybe one is left over from Arseniy's
review. Referencing the latest patch files from Andrey, here is what I
see:

in v20251216-0002-Add-gist_index_check-function-to-verify-Gi.patch:

This function traverses GiST with a depth-fisrt search and checks

"fisrt" should be "first".

This traverse takes lock of any page until some discapency found.

"discapency" should be "discrepancy"

To re-check suspicious pair of parent and child tuples it aqcuires

"aqcuires" should be "acquires"

amcheck.sgml:

+ require tuple adjustement) and page graph respects balanced-tree

"adjustement" should be "adjustment"

Also the Makefile ordering is not quite right:

--- a/contrib/amcheck/Makefile
+++ b/contrib/amcheck/Makefile
@@ -4,16 +4,17 @@ MODULE_big    = amcheck
 OBJS = \
    $(WIN32RES) \
    verify_common.o \
+   verify_gist.o \
    verify_gin.o \
    verify_heapam.o \
    verify_nbtree.o

We should put verify_gist.o after verify_gin.o.

Yours,

--
Paul ~{:-)
pj@illuminatedcomputing.com

On Tue, 16 Dec 2025 at 20:24, Paul A Jungwirth
<pj@illuminatedcomputing.com> wrote:

Hello,

On Wed, Oct 22, 2025 at 11:58 AM Kirill Reshke <reshkekirill@gmail.com> wrote:

1) There are several typos in verify_gist.c:

downlinks -> downlink (header comment)
discrepencies -> discrepancies
Correctess -> Correctness
hande -> handle
Initaliaze -> Initialize
numbmer -> number
replcaed -> replaced
aquire -> aqcuire

2) Copyright year is 2023 in the patch. Time flies:)

These two are (trivially) fixed.

I found a few more typos. Maybe one is left over from Arseniy's
review. Referencing the latest patch files from Andrey, here is what I
see:

in v20251216-0002-Add-gist_index_check-function-to-verify-Gi.patch:

This function traverses GiST with a depth-fisrt search and checks

"fisrt" should be "first".

This traverse takes lock of any page until some discapency found.

"discapency" should be "discrepancy"

To re-check suspicious pair of parent and child tuples it aqcuires

"aqcuires" should be "acquires"

amcheck.sgml:

+ require tuple adjustement) and page graph respects balanced-tree

"adjustement" should be "adjustment"

Also the Makefile ordering is not quite right:

--- a/contrib/amcheck/Makefile
+++ b/contrib/amcheck/Makefile
@@ -4,16 +4,17 @@ MODULE_big    = amcheck
OBJS = \
$(WIN32RES) \
verify_common.o \
+   verify_gist.o \
verify_gin.o \
verify_heapam.o \
verify_nbtree.o

We should put verify_gist.o after verify_gin.o.

Yours,

--
Paul ~{:-)
pj@illuminatedcomputing.com

Hi!

Thank you for taking a look. Sending new version which is Andreys's
[0]: + 0003 applied + your review comments addressed + my changes, including:
including:

Commit message:

This function traverses GiST with a depth-first search and checks
that all downlink tuples are included into parent tuple keyspace.
This traverse takes lock of any page until some discrepancy found.
To re-check suspicious pair of parent and child tuples it acquires
locks on both parent and child pages in the same order as page
split does.

" discrepancy found" -> " discrepancy is found"
" re-check suspicious " -> " re-check a suspicious "

I also added you, Arseniy and Miłosz in commit message, in reviewed-by section

+    /* Pointer to a next stack item. */
+    struct GistScanItem *next;
+}            GistScanItem;
+

a next -> the next

+        /*
+         * It's possible that the page was split since we looked at the
+         * parent, so that we didn't missed the downlink of the right sibling
+         * when we scanned the parent.  If so, add the right sibling to the
+         * stack now.
+         */

"didn't miss" not "didn't missed "

+            /*
+             * There was a discrepancy between parent and child tuples. We
+             * need to verify it is not a result of concurrent call of
+             * gistplacetopage(). So, lock parent and try to find downlink for
+             * current page. It may be missing due to concurrent page split,
+             * this is OK.

"find a downlink"

also this:

--- a/contrib/amcheck/verify_gist.c
+++ b/contrib/amcheck/verify_gist.c
@@ -583,7 +583,8 @@ gist_refind_parent(Relation rel,
 {
        Buffer          parentbuf;
        Page            parentpage;
-       OffsetNumber parent_maxoff;
+       OffsetNumber parent_maxoff,
+                                               off;
        IndexTuple      result = NULL;

parentbuf = ReadBufferExtended(rel, MAIN_FORKNUM, parentblkno,
RBM_NORMAL,
@@ -605,9 +606,9 @@ gist_refind_parent(Relation rel,
}

        parent_maxoff = PageGetMaxOffsetNumber(parentpage);
-       for (OffsetNumber o = FirstOffsetNumber; o <= parent_maxoff; o
= OffsetNumberNext(o))
+       for (off = FirstOffsetNumber; off <= parent_maxoff; off =
OffsetNumberNext(off))
        {
-               ItemId          p_iid = PageGetItemIdCareful(rel,
parentblkno, parentpage, o);
+               ItemId          p_iid = PageGetItemIdCareful(rel,
parentblkno, parentpage, off);
                IndexTuple      itup = (IndexTuple)
PageGetItem(parentpage, p_iid);

if (ItemPointerGetBlockNumber(&(itup->t_tid)) == childblkno)

--
Best regards,
Kirill Reshke

CF bot was unhappy about the last version due to obvious bug, PFA new
version with fixes.

The problem was "DROP TABLE toast_bug;" missing in expected regression output.

[0]: https://cirrus-ci.com/task/6378051304423424

--
Best regards,
Kirill Reshke

On Thu, 1 Jan 2026 at 17:05, Kirill Reshke <reshkekirill@gmail.com> wrote:

CF bot was unhappy about the last version due to obvious bug, PFA new
version with fixes.

The problem was "DROP TABLE toast_bug;" missing in expected regression output.

[0] https://cirrus-ci.com/task/6378051304423424

--
Best regards,
Kirill Reshke

Attached new version with commit message polishing, and address CF
feedback, which was unhappy due to headercheck

--
Best regards,
Kirill Reshke

On Sat, 10 Jan 2026 at 19:56, Kirill Reshke <reshkekirill@gmail.com> wrote:

On Thu, 1 Jan 2026 at 17:05, Kirill Reshke <reshkekirill@gmail.com> wrote:

CF bot was unhappy about the last version due to obvious bug, PFA new
version with fixes.

The problem was "DROP TABLE toast_bug;" missing in expected regression output.

[0] https://cirrus-ci.com/task/6378051304423424

--
Best regards,
Kirill Reshke

Attached new version with commit message polishing, and address CF
feedback, which was unhappy due to headercheck

After a quick preliminary review, here are some comments.

v2026-01-10-0001
================

1.
I'm pretty sure access/heaptoast.h is not needed by verify_nbtree.c.

v2026-01-10-0002
================

1.
+	if (GistPageGetOpaque(page)->gist_page_id != GIST_PAGE_ID)
+		ereport(ERROR,
+				(errcode(ERRCODE_INDEX_CORRUPTED),
+				 errmsg("index \"%s\" has corrupted page %d",
+						RelationGetRelationName(rel), blockNo)));
+
+	if (GistPageIsDeleted(page))
+	{
+		if (!GistPageIsLeaf(page))
+			ereport(ERROR,
+					(errcode(ERRCODE_INDEX_CORRUPTED),
+					 errmsg("index \"%s\" has deleted internal page %d",
+							RelationGetRelationName(rel), blockNo)));
+		if (PageGetMaxOffsetNumber(page) > InvalidOffsetNumber)
+			ereport(ERROR,
+					(errcode(ERRCODE_INDEX_CORRUPTED),
+					 errmsg("index \"%s\" has deleted page %d with tuples",
+							RelationGetRelationName(rel), blockNo)));

blockNo is unsigned integer, so we should use %u in the format string.

2.
+		ereport(ERROR,
+				(errcode(ERRCODE_INDEX_CORRUPTED),
+				 errmsg("index \"%s\" internal page %d became leaf",
+						RelationGetRelationName(rel), parentblkno)));

The same goes for parentblkno — it should also use %u.

--
Best regards,
Kirill Reshke

[2. text/x-diff; v2026-01-10-0001-Move-normalize-tuple-logic-from-nbtcheck.patch]...

[3. text/x-diff; v2026-01-10-0002-Add-gist_index_check-function-to-verify-.patch]...

--
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.

Hi!
Thanks for your review!

We discussed offline about the patch, and I decided to make a review of it and fixes
up to your comments.

v2026-01-10-0001
================

1.
I'm pretty sure access/heaptoast.h is not needed by verify_nbtree.c.

In fact this include is necessary because of line verify_common.c:236 which
uses TOAST_INDEX_TARGET

v2026-01-10-0002
================

blockNo is unsigned integer, so we should use %u in the format string.

The same goes for parentblkno — it should also use %u.

Yes seems like there was misusage of format symbol.. I attached the v2026-01-12 with
fixed symbol. Thanks!

--
Best regards,
Roman Khapov

Hi, Roman Khapov

On Mon, 12 Jan 2026 at 01:36, Roman Khapov <rkhapov@yandex-team.ru> wrote:

Hi!
Thanks for your review!

We discussed offline about the patch, and I decided to make a review of it and fixes
up to your comments.

Thank you for updating the patches.

v2026-01-10-0001
================

1.
I'm pretty sure access/heaptoast.h is not needed by verify_nbtree.c.

In fact this include is necessary because of line verify_common.c:236 which
uses TOAST_INDEX_TARGET

Yeah, verify_common.c does require the header, but what I meant was that
verify_nbtree.c no longer needs it.

diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c
index c1e24338361..426e23d2960 100644
--- a/contrib/amcheck/verify_nbtree.c
+++ b/contrib/amcheck/verify_nbtree.c
@@ -23,7 +23,6 @@
  */
 #include "postgres.h"

-#include "access/heaptoast.h"
#include "access/htup_details.h"
#include "access/nbtree.h"
#include "access/table.h"

--
Regards,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.

Yeah, verify_common.c does require the header, but what I meant was that
verify_nbtree.c no longer needs it.

Oh, thanks, now I see..

Updated the patches.

--
Best regards,
Roman Khapov

On Sat, Jan 31, 2026 at 2:17 PM Roman Khapov <rkhapov@yandex-team.ru> wrote:

Yeah, verify_common.c does require the header, but what I meant was that
verify_nbtree.c no longer needs it.

Oh, thanks, now I see..

Updated the patches.

I additionally tested the latest version (v2026-01-12-v2) -- and all looked
good to me.

testing performed:

1. Applied cleanly; built with --enable-cassert, make -C contrib/amcheck
check — all tests pass

2. Tested for 9 core GiST opclasses, on larger indexes (10M records), with
both heapallindexed=false and heapallindexed=true
(10M records, old macbook m1):

Opclass heapallindexed=false heapallindexed=true
-------------- -------------------- -------------------
point_ops 3.9s 5.9s
box_ops 6.3s 9.3s
circle_ops 5.5s 10.6s
poly_ops 5.8s 13.7s
inet_ops 0.6s 5.6s
range_ops 3.2s 6.9s
multirange_ops 2.4s 10.8s
tsvector_ops 3.8s 13.7s
tsquery_ops 0.3s 4.9s

3. Similarly, just to double-check, for extensions that use GiST:
- contrib modules tested: btree_gist, cube, hstore, ltree, pg_trgm, seg
(tested a single opclass for each; so 8 out of 30+ opclasses)
- skipped: intarray because it was annoyingly slow to build (looks like
O(n²)?)

4. Verified corruption detection works. Reproduction steps:

Setup (with data checksums disabled)

create table t as select point(random()*1000, random()*1000) c from
generate_series(1,50000);
create index t_idx on t using gist(c);
select pg_relation_filepath('t_idx'); -- e.g., base/5/16398

Stop server, corrupt index, start:

pg_ctl stop -D $PGDATA
python3 -c "
f = open('$PGDATA/base/5/16398', 'r+b')
f.seek(8192 + 24) # page 1, line pointer area
f.write(b'\xDE\xAD\xBE\xEF' * 8)
f.close()
"
pg_ctl start -D $PGDATA

And then we see an error as expected:

select gist_index_check('t_idx', false);
-- ERROR: invalid line pointer storage in index "t_idx"
-- DETAIL: Index tid=(1,20) lp_off=0, lp_len=0 lp_flags=0.

*****

Side note (general amcheck observation, not specific to this patch): the
existing amcheck TAP tests for heapam include corruption injection tests
(see 001_verify_heapam.pl). Similar tests for index checkers would be
valuable for amcheck -- but I think it's a broader effort beyond this
particular patch's scope.

The patch looks solid.

Nik