Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. amcheck: Fix posting tree checks in gin_index_check()

  2. amcheck: Fix parent key check in gin_index_check()

  3. amcheck: Fix checks of entry order for GIN indexes

  4. amcheck: Remove unused GinScanItem->parentlsn field

  5. amcheck: Test gin_index_check on a multicolumn index

  6. Remove incidental md5() function use from test

  7. amcheck: Add a GIN index to the CREATE INDEX CONCURRENTLY tests

  8. amcheck: Add a test with GIN index on JSONB data

  9. amcheck: Fix indentation in verify_gin.c

  10. amcheck: Add gin_index_check() to verify GIN index

  11. amcheck: Move common routines into a separate module

  12. Fix grammar in GIN README

  13. Avoid amcheck inline compression false positives.

  1. Amcheck verification of GiST and GIN

    Andrey Borodin <x4mmm@yandex-team.ru> — 2022-05-30T09:40:06Z

    Hello world!

Few years ago we had a thread with $subj [0]. A year ago Heikki put a lot of effort in improving GIN checks [1] while hunting a GIN bug.
And in view of some releases with a recommendation to reindex anything that fails or lacks amcheck verification, I decided that I want to review the thread.

PFA $subj incorporating all Heikki's improvements and restored GiST checks. Also I've added heapallindexed verification for GiST. I'm sure that we must add it for GIN too. Yet I do not know how to implement it. Maybe just check that every entry generated from heap present in entry tree? Or that every tids is present in the index?

GiST verification does parent check despite taking only AccessShareLock. It's possible because when the key discrepancy is found we acquire parent tuple with lock coupling. I'm sure that this is correct to check keys this way. And I'm almost sure it will not deadlock, because split is doing the same locking.

What do you think?

Best regards, Andrey Borodin.

[0] https://www.postgresql.org/message-id/flat/CAF3eApa07-BajjG8%2BRYx-Dr_cq28ZA0GsZmUQrGu5b2ayRhB5A%40mail.gmail.com
[1] https://www.postgresql.org/message-id/flat/9fdbb584-1e10-6a55-ecc2-9ba8b5dca1cf%40iki.fi#fec2751faf1ca52495b0a61acc0f5532

  2. Re: Amcheck verification of GiST and GIN

    Andrey Borodin <x4mmm@yandex-team.ru> — 2022-06-22T17:40:56Z

    
> On 30 May 2022, at 12:40, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
> 
> What do you think?

Hi Andrey!

Here's a version with better tests. I've made sure that GiST tests actually trigger page reuse after deletion. And enhanced comments in both GiST and GIN test scripts. I hope you'll like it.

GIN heapallindexed is still a no-op check. Looking forward to hear any ideas on what it could be.


Best regards, Andrey Borodin.

  3. Re: Amcheck verification of GiST and GIN

    Nikolay Samokhvalov <samokhvalov@gmail.com> — 2022-06-22T19:27:25Z

    On Wed, Jun 22, 2022 at 11:35 AM Andrey Borodin <x4mmm@yandex-team.ru>
wrote:

> > On 30 May 2022, at 12:40, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
> >
> > What do you think?
>
> Hi Andrey!
>

Hi Andrey!

Since you're talking to yourself, just wanted to support you – this is an
important thing, definitely should be very useful for many projects; I hope
to find time to test it in the next few days.

Thanks for working on it.

  4. Re: Amcheck verification of GiST and GIN

    Andres Freund <andres@anarazel.de> — 2022-06-22T23:29:12Z

    Hi,

I think having amcheck for more indexes is great.

On 2022-06-22 20:40:56 +0300, Andrey Borodin wrote:
>> diff --git a/contrib/amcheck/amcheck.c b/contrib/amcheck/amcheck.c
> new file mode 100644
> index 0000000000..7a222719dd
> --- /dev/null
> +++ b/contrib/amcheck/amcheck.c
> @@ -0,0 +1,187 @@
> +/*-------------------------------------------------------------------------
> + *
> + * amcheck.c
> + *		Utility functions common to all access methods.

This'd likely be easier to read if the reorganization were split into its own
commit.

I'd also split gin / gist support. It's a large enough patch that that imo
makes reviewing easier.


> +void amcheck_lock_relation_and_check(Oid indrelid, IndexCheckableCallback checkable,
> +												IndexDoCheckCallback check, LOCKMODE lockmode, void *state)

Might be worth pgindenting - the void for function definitions (but not for
declarations) is typically on its own line in PG code.


> +static GistCheckState
> +gist_init_heapallindexed(Relation rel)
> +{
> +	int64		total_pages;
> +	int64		total_elems;
> +	uint64		seed;
> +	GistCheckState result;
> +
> +	/*
> +	* Size Bloom filter based on estimated number of tuples in index
> +	*/
> +	total_pages = RelationGetNumberOfBlocks(rel);
> +	total_elems = Max(total_pages * (MaxOffsetNumber / 5),
> +						(int64) rel->rd_rel->reltuples);
> +	/* Generate a random seed to avoid repetition */
> +	seed = pg_prng_uint64(&pg_global_prng_state);
> +	/* Create Bloom filter to fingerprint index */
> +	result.filter = bloom_create(total_elems, maintenance_work_mem, seed);
> +
> +	/*
> +	 * Register our own snapshot
> +	 */
> +	result.snapshot = RegisterSnapshot(GetTransactionSnapshot());

FWIW, comments like this, that just restate exactly what the code does, are
imo not helpful.  Also, there's a trailing space :)


Greetings,

Andres Freund

  5. Re: Amcheck verification of GiST and GIN

    Andrey Borodin <x4mmm@yandex-team.ru> — 2022-06-25T19:10:11Z

    
> On 23 Jun 2022, at 00:27, Nikolay Samokhvalov <samokhvalov@gmail.com> wrote:
> 
> Since you're talking to yourself, just wanted to support you – this is an important thing, definitely should be very useful for many projects; I hope to find time to test it in the next few days. 

Thanks Nikolay!


> On 23 Jun 2022, at 04:29, Andres Freund <andres@anarazel.de> wrote:
Thanks for looking into the patch, Andres!

> On 2022-06-22 20:40:56 +0300, Andrey Borodin wrote:
>>> diff --git a/contrib/amcheck/amcheck.c b/contrib/amcheck/amcheck.c
>> new file mode 100644
>> index 0000000000..7a222719dd
>> --- /dev/null
>> +++ b/contrib/amcheck/amcheck.c
>> @@ -0,0 +1,187 @@
>> +/*-------------------------------------------------------------------------
>> + *
>> + * amcheck.c
>> + *		Utility functions common to all access methods.
> 
> This'd likely be easier to read if the reorganization were split into its own
> commit.
> 
> I'd also split gin / gist support. It's a large enough patch that that imo
> makes reviewing easier.
I will split the patch in 3 steps:
1. extract generic functions to amcheck.c
2. add gist functions
3. add gin functions
But each this step is just adding few independent files + some lines to Makefile.

I'll fix other notes too in the next version.

Thanks!

Best regards, Andrey Borodin.

  6. Re: Amcheck verification of GiST and GIN

    Andrey Borodin <x4mmm@yandex-team.ru> — 2022-07-23T09:40:44Z

    
> On 26 Jun 2022, at 00:10, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
> 
> I will split the patch in 3 steps:
> 1. extract generic functions to amcheck.c
> 2. add gist functions
> 3. add gin functions
> 
> I'll fix other notes too in the next version.


Done. PFA attached patchset.

Thanks!

Best regards, Andrey Borodin.

  7. Re: Amcheck verification of GiST and GIN

    Andrey Borodin <x4mmm@yandex-team.ru> — 2022-08-17T12:28:02Z

    
> On 23 Jul 2022, at 14:40, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
> 
> Done. PFA attached patchset.
> 
> Best regards, Andrey Borodin.
> <v12-0001-Refactor-amcheck-to-extract-common-locking-routi.patch><v12-0002-Add-gist_index_parent_check-function-to-verify-G.patch><v12-0003-Add-gin_index_parent_check-to-verify-GIN-index.patch>

Here's v13. Changes:
1. Fixed passing through downlink in GIN index
2. Fixed GIN tests (one test case was not working)

Thanks to Vitaliy Kukharik for trying this patches.

Best regards, Andrey Borodin.

  8. Re: Amcheck verification of GiST and GIN

    Andres Freund <andres@anarazel.de> — 2022-09-22T15:19:09Z

    Hi,

On 2022-08-17 17:28:02 +0500, Andrey Borodin wrote:
> Here's v13. Changes:
> 1. Fixed passing through downlink in GIN index
> 2. Fixed GIN tests (one test case was not working)
> 
> Thanks to Vitaliy Kukharik for trying this patches.

Due to the merge of the meson based build, this patch needs to be
adjusted. See
https://cirrus-ci.com/build/6637154947301376

The changes should be fairly simple, just mirroring the Makefile ones.

Greetings,

Andres Freund

  9. Re: Amcheck verification of GiST and GIN

    Andres Freund <andres@anarazel.de> — 2022-10-02T07:12:30Z

    Hi,

On 2022-09-22 08:19:09 -0700, Andres Freund wrote:
> Hi,
> 
> On 2022-08-17 17:28:02 +0500, Andrey Borodin wrote:
> > Here's v13. Changes:
> > 1. Fixed passing through downlink in GIN index
> > 2. Fixed GIN tests (one test case was not working)
> > 
> > Thanks to Vitaliy Kukharik for trying this patches.
> 
> Due to the merge of the meson based build, this patch needs to be
> adjusted. See
> https://cirrus-ci.com/build/6637154947301376
> 
> The changes should be fairly simple, just mirroring the Makefile ones.

Here's an updated patch adding meson compat.

I didn't fix the following warnings:

[25/28 3  89%] Compiling C object contrib/amcheck/amcheck.dll.p/amcheck.c.obj
../../home/andres/src/postgresql/contrib/amcheck/amcheck.c: In function ‘amcheck_lock_relation_and_check’:
../../home/andres/src/postgresql/contrib/amcheck/amcheck.c:81:20: warning: implicit declaration of function ‘NewGUCNestLevel’ [-Wimplicit-function-declaration]
   81 |   save_nestlevel = NewGUCNestLevel();
      |                    ^~~~~~~~~~~~~~~
../../home/andres/src/postgresql/contrib/amcheck/amcheck.c:124:2: warning: implicit declaration of function ‘AtEOXact_GUC’; did you mean ‘AtEOXact_SMgr’? [-Wimplicit-function-declaration]
  124 |  AtEOXact_GUC(false, save_nestlevel);
      |  ^~~~~~~~~~~~
      |  AtEOXact_SMgr
[26/28 2  92%] Compiling C object contrib/amcheck/amcheck.dll.p/verify_gin.c.obj
../../home/andres/src/postgresql/contrib/amcheck/verify_gin.c: In function ‘gin_check_parent_keys_consistency’:
../../home/andres/src/postgresql/contrib/amcheck/verify_gin.c:423:8: warning: unused variable ‘heapallindexed’ [-Wunused-variable]
  423 |  bool  heapallindexed = *((bool*)callback_state);
      |        ^~~~~~~~~~~~~~
[28/28 1 100%] Linking target contrib/amcheck/amcheck.dll


Greetings,

Andres Freund

  10. Re: Amcheck verification of GiST and GIN

    Andrey Borodin <amborodin86@gmail.com> — 2022-10-08T22:36:52Z

    On Sun, Oct 2, 2022 at 12:12 AM Andres Freund <andres@anarazel.de> wrote:
>
> Here's an updated patch adding meson compat.

Thank you, Andres! Here's one more rebase (something was adjusted in
amcheck build).
Also I've fixed new warnings except warning about absent
heapallindexed for GIN. It's a TODO.

Thanks!

Best regards, Andrey Borodin.

  11. Re: Amcheck verification of GiST and GIN

    José Arthur Benetasso Villanova <jose.arthur@gmail.com> — 2022-11-25T02:04:37Z

    Hello.

I reviewed this patch and I would like to share some comments.

It compiled with those 2 warnings:

verify_gin.c: In function 'gin_check_parent_keys_consistency':
verify_gin.c:481:38: warning: declaration of 'maxoff' shadows a previous 
local [-Wshadow=compatible-local]
   481 |                         OffsetNumber maxoff = 
PageGetMaxOffsetNumber(page);
       |                                      ^~~~~~
verify_gin.c:453:41: note: shadowed declaration is here
   453 |                                         maxoff;
       |                                         ^~~~~~
verify_gin.c:423:25: warning: unused variable 'heapallindexed' 
[-Wunused-variable]
   423 |         bool            heapallindexed = *((bool*)callback_state);
       |                         ^~~~~~~~~~~~~~


Also, I'm not sure about postgres' headers conventions, inside amcheck.h, 
there is "miscadmin.h" included, and inside verify_gin.c, verify_gist.h 
and verify_nbtree.c both amcheck.h and miscadmin.h are included.

About the documentation, the bt_index_parent_check has comments about the 
ShareLock and "SET client_min_messages = DEBUG1;", and both 
gist_index_parent_check and gin_index_parent_check lack it. verify_gin 
uses DEBUG3, I'm not sure if it is on purpose, but it would be nice to 
document it or put DEBUG1 to be consistent.

I lack enough context to do a deep review on the code, so in this area 
this patch needs more eyes.

I did the following test:

postgres=# create table teste (t text, tv tsvector);
CREATE TABLE
postgres=# insert into teste values ('hello', 'hello'::tsvector);
INSERT 0 1
postgres=# create index teste_tv on teste using gist(tv);
CREATE INDEX
postgres=# select pg_relation_filepath('teste_tv');
  pg_relation_filepath
----------------------
  base/5/16441
(1 row)

postgres=#
\q
$ bin/pg_ctl -D data -l log
waiting for server to shut down.... done
server stopped
$ okteta base/5/16441 # I couldn't figure out the dd syntax to change the 
1FE9 to '0'
$ bin/pg_ctl -D data -l log
waiting for server to start.... done
server started
$ bin/psql -U ze postgres
psql (16devel)
Type "help" for help.

postgres=# SET client_min_messages = DEBUG3;
SET
postgres=# select gist_index_parent_check('teste_tv'::regclass, true);
DEBUG:  verifying that tuples from index "teste_tv" are present in "teste"
ERROR:  heap tuple (0,1) from table "teste" lacks matching index tuple 
within index "teste_tv"
postgres=#

A simple index corruption in gin:

postgres=# CREATE TABLE "gin_check"("Column1" int[]);
CREATE TABLE
postgres=# insert into gin_check values (ARRAY[1]),(ARRAY[2]);
INSERT 0 2
postgres=# CREATE INDEX gin_check_idx on "gin_check" USING GIN("Column1");
CREATE INDEX
postgres=# select pg_relation_filepath('gin_check_idx');
  pg_relation_filepath
----------------------
  base/5/16453
(1 row)

postgres=#
\q
$ bin/pg_ctl -D data -l logfile stop
waiting for server to shut down.... done
server stopped
$ okteta data/base/5/16453 # edited some bits near 3FCC
$ bin/pg_ctl -D data -l logfile start
waiting for server to start.... done
server started
$ bin/psql -U ze postgres
psql (16devel)
Type "help" for help.

postgres=# SET client_min_messages = DEBUG3;
SET
postgres=# SELECT gin_index_parent_check('gin_check_idx', true);
ERROR:  number of items mismatch in GIN entry tuple, 49 in tuple header, 1 
decoded
postgres=#

There are more code paths to follow to check the entire code, and I had a 
hard time to corrupt the indices. Is there any automated code to corrupt 
index to test such code?


--
Jose Arthur Benetasso Villanova

  12. Re: Amcheck verification of GiST and GIN

    Andrey Borodin <amborodin86@gmail.com> — 2022-11-27T21:29:18Z

    Hello!

Thank you for the review!

On Thu, Nov 24, 2022 at 6:04 PM Jose Arthur Benetasso Villanova
<jose.arthur@gmail.com> wrote:
>
> It compiled with those 2 warnings:
>
> verify_gin.c: In function 'gin_check_parent_keys_consistency':
> verify_gin.c:481:38: warning: declaration of 'maxoff' shadows a previous
> local [-Wshadow=compatible-local]
>    481 |                         OffsetNumber maxoff =
> PageGetMaxOffsetNumber(page);
>        |                                      ^~~~~~
> verify_gin.c:453:41: note: shadowed declaration is here
>    453 |                                         maxoff;
>        |                                         ^~~~~~
> verify_gin.c:423:25: warning: unused variable 'heapallindexed'
> [-Wunused-variable]

Fixed.

>    423 |         bool            heapallindexed = *((bool*)callback_state);
>        |                         ^~~~~~~~~~~~~~
>

This one is in progress yet, heapallindexed check is not implemented yet...


>
> Also, I'm not sure about postgres' headers conventions, inside amcheck.h,
> there is "miscadmin.h" included, and inside verify_gin.c, verify_gist.h
> and verify_nbtree.c both amcheck.h and miscadmin.h are included.
Fixed.

>
> About the documentation, the bt_index_parent_check has comments about the
> ShareLock and "SET client_min_messages = DEBUG1;", and both
> gist_index_parent_check and gin_index_parent_check lack it. verify_gin
> uses DEBUG3, I'm not sure if it is on purpose, but it would be nice to
> document it or put DEBUG1 to be consistent.
GiST and GIN verifications do not take ShareLock for parent checks.
B-tree check cannot verify cross-level invariants between levels when
the index is changing.

GiST verification checks only one invariant that can be verified if
page locks acquired the same way as page split does.
GIN does not require ShareLock because it does not check cross-level invariants.

Reporting progress with DEBUG1 is a good idea, I did not know that
this feature exists. I'll implement something similar in following
versions.

> I did the following test:

Cool! Thank you!

>
> There are more code paths to follow to check the entire code, and I had a
> hard time to corrupt the indices. Is there any automated code to corrupt
> index to test such code?
>

Heapam tests do this in an automated way, look into this file
t/001_verify_heapam.pl.
Surely we can write these tests. At least automate what you have just
done in the review. However, committing similar checks is a very
tedious work: something will inevitably turn buildfarm red as a
watermelon.

I hope I'll post a version with DEBUG1 reporting and heapallindexed soon.
PFA current state.
Thank you for looking into this!

Best regards, Andrey Borodin.

  13. Re: Amcheck verification of GiST and GIN

    Andrey Borodin <amborodin86@gmail.com> — 2022-11-28T01:07:40Z

    On Sun, Nov 27, 2022 at 1:29 PM Andrey Borodin <amborodin86@gmail.com> wrote:
>
> GiST verification checks only one invariant that can be verified if
> page locks acquired the same way as page split does.
> GIN does not require ShareLock because it does not check cross-level invariants.
>

I was wrong. GIN check does similar gin_refind_parent() to lock pages
in bottom-up manner and truly verify downlink-child_page invariant.

Here's v17. The only difference is that I added progress reporting to
GiST verification.
I still did not implement heapallindexed for GIN. Existence of pending
lists makes this just too difficult for a weekend coding project :(

Thank you!

Best regards, Andrey Borodin.

  14. Re: Amcheck verification of GiST and GIN

    José Arthur Benetasso Villanova <jose.arthur@gmail.com> — 2022-12-14T12:18:44Z

    On Sun, 27 Nov 2022, Andrey Borodin wrote:

> On Sun, Nov 27, 2022 at 1:29 PM Andrey Borodin <amborodin86@gmail.com> wrote:
>>
> I was wrong. GIN check does similar gin_refind_parent() to lock pages
> in bottom-up manner and truly verify downlink-child_page invariant.

Does this mean that we need the adjustment in docs?

> Here's v17. The only difference is that I added progress reporting to
> GiST verification.
> I still did not implement heapallindexed for GIN. Existence of pending
> lists makes this just too difficult for a weekend coding project :(
>
> Thank you!
>
> Best regards, Andrey Borodin.
>

I'm a bit lost here. I tried your patch again and indeed the 
heapallindexed inside gin_check_parent_keys_consistency has a TODO 
comment, but it's unclear to me if you are going to implement it or if the 
patch "needs review". Right now it's "Waiting on Author".

-- 
Jose Arthur Benetasso Villanova

  15. Re: Amcheck verification of GiST and GIN

    Robert Haas <robertmhaas@gmail.com> — 2022-12-14T17:25:17Z

    On Wed, Dec 14, 2022 at 7:19 AM Jose Arthur Benetasso Villanova
<jose.arthur@gmail.com> wrote:
> I'm a bit lost here. I tried your patch again and indeed the
> heapallindexed inside gin_check_parent_keys_consistency has a TODO
> comment, but it's unclear to me if you are going to implement it or if the
> patch "needs review". Right now it's "Waiting on Author".

FWIW, I don't think there's a hard requirement that every index AM
needs to support the same set of amcheck options. Where it makes sense
and can be done in a reasonably straightforward manner, we should. But
sometimes that may not be the case, and that seems fine, too.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  16. Re: Amcheck verification of GiST and GIN

    Andrey Borodin <amborodin86@gmail.com> — 2023-01-09T04:05:25Z

    Hi Jose, thank you for review and sorry for so long delay to answer.

On Wed, Dec 14, 2022 at 4:19 AM Jose Arthur Benetasso Villanova
<jose.arthur@gmail.com> wrote:
>
>
> On Sun, 27 Nov 2022, Andrey Borodin wrote:
>
> > On Sun, Nov 27, 2022 at 1:29 PM Andrey Borodin <amborodin86@gmail.com> wrote:
> >>
> > I was wrong. GIN check does similar gin_refind_parent() to lock pages
> > in bottom-up manner and truly verify downlink-child_page invariant.
>
> Does this mean that we need the adjustment in docs?
It seems to me that gin_index_parent_check() docs are correct.

>
> > Here's v17. The only difference is that I added progress reporting to
> > GiST verification.
> > I still did not implement heapallindexed for GIN. Existence of pending
> > lists makes this just too difficult for a weekend coding project :(
> >
> > Thank you!
> >
> > Best regards, Andrey Borodin.
> >
>
> I'm a bit lost here. I tried your patch again and indeed the
> heapallindexed inside gin_check_parent_keys_consistency has a TODO
> comment, but it's unclear to me if you are going to implement it or if the
> patch "needs review". Right now it's "Waiting on Author".
>

Please find the attached new version. In this patchset heapallindexed
flag is removed from GIN checks.

Thank you!

Best regards, Andrey Borodin.

  17. Re: Amcheck verification of GiST and GIN

    Andrey Borodin <amborodin86@gmail.com> — 2023-01-09T04:08:05Z

    On Sun, Jan 8, 2023 at 8:05 PM Andrey Borodin <amborodin86@gmail.com> wrote:
>
> Please find the attached new version. In this patchset heapallindexed
> flag is removed from GIN checks.
>
Uh... sorry, git-formatted wrong branch.
Here's the correct version. Double checked.

Best regards, Andrey Borodin.

  18. Re: Amcheck verification of GiST and GIN

    José Arthur Benetasso Villanova <jose.arthur@gmail.com> — 2023-01-13T11:46:45Z

    On Sun, 8 Jan 2023, Andrey Borodin wrote:

> On Sun, Jan 8, 2023 at 8:05 PM Andrey Borodin <amborodin86@gmail.com> wrote:
>>
>> Please find the attached new version. In this patchset heapallindexed
>> flag is removed from GIN checks.
>>
> Uh... sorry, git-formatted wrong branch.
> Here's the correct version. Double checked.
>

Hello again.

I applied the patch without errors / warnings and did the same tests. All 
working as expected.

The only thing that I found is the gin_index_parent_check function in docs 
still references the "gin_index_parent_check(index regclass, 
heapallindexed boolean) returns void"

--
Jose Arthur Benetasso Villanova

  19. Re: Amcheck verification of GiST and GIN

    Andrey Borodin <amborodin86@gmail.com> — 2023-01-14T00:18:23Z

    On Fri, Jan 13, 2023 at 3:46 AM Jose Arthur Benetasso Villanova
<jose.arthur@gmail.com> wrote:
>
> The only thing that I found is the gin_index_parent_check function in docs
> still references the "gin_index_parent_check(index regclass,
> heapallindexed boolean) returns void"
>

Correct! Please find the attached fixed version.

Thank you!

Best regards, Andrey Borodin.

  20. Re: Amcheck verification of GiST and GIN

    José Arthur Benetasso Villanova <jose.arthur@gmail.com> — 2023-01-14T03:34:38Z

    On Fri, 13 Jan 2023, Andrey Borodin wrote:

> On Fri, Jan 13, 2023 at 3:46 AM Jose Arthur Benetasso Villanova
> <jose.arthur@gmail.com> wrote:
>>
>> The only thing that I found is the gin_index_parent_check function in docs
>> still references the "gin_index_parent_check(index regclass,
>> heapallindexed boolean) returns void"
>>
>
> Correct! Please find the attached fixed version.
>
> Thank you!
>
> Best regards, Andrey Borodin.
>

Hello again. I see the change. Thanks

--
Jose Arthur Benetasso Villanova

  21. Re: Amcheck verification of GiST and GIN

    Andrey Borodin <amborodin86@gmail.com> — 2023-01-14T04:14:46Z

    On Fri, Jan 13, 2023 at 7:35 PM Jose Arthur Benetasso Villanova
<jose.arthur@gmail.com> wrote:
>
> Hello again. I see the change. Thanks
>

Thanks! I also found out that there was a CI complaint about amcheck.h
not including some necessary stuff. Here's a version with a fix for
that.

Best regards, Andrey Borodin.

  22. Re: Amcheck verification of GiST and GIN

    Aleksander Alekseev <aleksander@timescale.com> — 2023-01-30T13:38:03Z

    Hi Andrey,

> Thanks! I also found out that there was a CI complaint about amcheck.h
> not including some necessary stuff. Here's a version with a fix for
> that.

Thanks for the updated patchset.

One little nitpick I have is that the tests cover only cases when all
the checks pass successfully. The tests don't show that the checks
will fail if the indexes are corrupted. Usually we check this as well,
see bd807be6 and other amcheck replated patches and commits.

-- 
Best regards,
Aleksander Alekseev

  23. Re: Amcheck verification of GiST and GIN

    Peter Geoghegan <pg@bowt.ie> — 2023-02-02T19:51:45Z

    On Fri, Jan 13, 2023 at 8:15 PM Andrey Borodin <amborodin86@gmail.com> wrote:
> (v21 of patch series)

I can see why the refactoring patch is necessary overall, but I have
some concerns about the details. More specifically:

* PageGetItemIdCareful() doesn't seem like it needs to be moved to
amcheck.c and generalized to work with GIN and GiST.

It seems better to just allow some redundancy, by having static/local
versions of PageGetItemIdCareful() for both GIN and GiST. There are
numerous reasons why that seems better to me. For one thing it's
simpler. For another, the requirements are already a bit different,
and may become more different in the future. I have seriously
considered adding a new PageGetItemCareful() routine to nbtree in the
past (which would work along similar lines when we access
IndexTuples), which would have to be quite different across each index
AM. Maybe this idea of adding a PageGetItemCareful() would totally
supersede the existing PageGetItemIdCareful() function.

But even now, without any of that, the rules for
PageGetItemIdCareful() are already different. For example, with GIN
you cannot have LP_DEAD bits set, so ISTM that you should be checking
for that in its own custom version of PageGetItemIdCareful().

You can just have comments that refer the reader to the original
nbtree version of PageGetItemIdCareful() for a high level overview.

* You have distinct versions of the current btree_index_checkable()
function for both GIN and GiST, which doesn't seem necessary to me --
so this is kind of the opposite of the situation with
PageGetItemIdCareful() IMV.

The only reason to have separate versions of these is to detect when
the wrong index AM is used -- the other 2 checks are 100% common to
all index AMs. Why not just move that one non-generic check out of the
function, to each respective index AM .c file, while keeping the other
2 generic checks in amcheck.c?

Once things are structured this way, it would then make sense to add a
can't-be-LP_DEAD check to the GIN specific version of
PageGetItemIdCareful().

I also have some questions about the verification functionality itself:

* Why haven't you done something like palloc_btree_page() for both
GiST and GIN, and use that for everything?

Obviously this may not be possible in100% of all cases -- even
verify_nbtree.c doesn't manage that. But I see no reason for that
here. Though, in general, it's not exactly clear what's going on with
buffer lock coupling in general.

* Why does gin_refind_parent() buffer lock the parent while the child
buffer lock remains held?

In any case this doesn't really need to have any buffer lock coupling.
Since you're both of the new verification functions you're adding are
"parent" variants, that acquire a ShareLock to block concurrent
modifications and concurrent VACUUM?

* Oh wait, they don't use a ShareLock at all -- they use an
AccessShareLock. This means that there are significant inconsistencies
with the verify_nbtree.c scheme.

I now realize that gist_index_parent_check() and
gin_index_parent_check() are actually much closer to bt_index_check()
than to bt_index_parent_check(). I think that you should stick with
the convention of using the word "parent" whenever we'll need a
ShareLock, and omitting "parent" whenever we will only require an
AccessShareLock. I'm not sure if that means that you should change the
lock strength or change the name of the functions. I am sure that you
should follow the general convention that we have already.

I feel rather pessimistic about our ability to get all the details
right with GIN. Frankly I have serious doubts that GIN itself gets
everything right, which makes our task just about impossible. The GIN
README did gain a "Concurrency" section in 2019, at my behest, but in
general the locking protocols are still chronically under-documented,
and have been revised in various ways as a response to bugs. So at
least in the case of GIN, we really need amcheck coverage, but should
take a very conservative approach.

With GIN I think that we need to make the most modest possible
assumptions about concurrency, by using a ShareLock. Without that, I
think that we can have very little confidence in the verification
checks -- the concurrency rules are just too complicated right now.
Maybe it will be possible in the future, but right now I'd rather not
try that. I find it very difficult to figure out the GIN locking
protocol, even for things that seem like they should be quite
straightforward. This situation would be totally unthinkable in
nbtree, and perhaps with GiST.

* Why does the GIN patch change a comment in contrib/amcheck/amcheck.c?

* There is no pg_amcheck patch here, but I think that there should be,
since that is now the preferred and recommended way to run amcheck in
general.

We could probably do something very similar to what is already there
for nbtree. Maybe it would make sense to change --heapallindexed and
--parent-check so that they call your parent check functions for GiST
and GIN -- though the locking/naming situation must be resolved before
we decide what to do here, for pg_amcheck.

-- 
Peter Geoghegan

  24. Re: Amcheck verification of GiST and GIN

    Peter Geoghegan <pg@bowt.ie> — 2023-02-02T20:15:32Z

    On Thu, Feb 2, 2023 at 11:51 AM Peter Geoghegan <pg@bowt.ie> wrote:
> I also have some questions about the verification functionality itself:

I forgot to include another big concern here:

* Why are there only WARNINGs, never ERRORs here?

It's far more likely that you'll run into problems when running
amcheck this way. I understand that the heapam checks can do that, but
that is both more useful, and less risky. With heapam we're not
traversing a tree structure in logical/keyspace order. I'm not
claiming that this approach is impossible; just that it doesn't seem
even remotely worth it. Indexes are never supposed to be corrupt, but
if they are corrupt the solution always involves a REINDEX. You never
try to recover the data from an index, since it's redundant and less
authoritative, almost by definition (at least in Postgres).

By far the most important piece of information is that an index has
some non-zero amount of corruption. Any amount of corruption is
supposed to be extremely surprising. It's kind of like if you see one
cockroach in your home. The problem is not that you have one cockroach
in your home; the problem is that you simply have cockroaches. We can
all agree that in some abstract sense, fewer cockroaches is better.
But that doesn't seem to have any practical relevance -- it's a purely
theoretical point. It doesn't really affect what you do about the
problem at that point.

Admittedly there is some value in seeing multiple WARNINGs to true
experts that are performing some kind of forensic analysis, but that
doesn't seem worth it to me -- I'm an expert, and I don't think that
I'd do it this way for any reason other than it being more convenient
as a way to get information about a system that I don't have access
to. Even then, I think that I'd probably have serious doubts about
most of the extra information that I'd get, since it might very well
be a downstream consequence of the same basic problem.

-- 
Peter Geoghegan

  25. Re: Amcheck verification of GiST and GIN

    Nikolay Samokhvalov <samokhvalov@gmail.com> — 2023-02-02T20:31:33Z

    On Thu, Feb 2, 2023 at 12:15 PM Peter Geoghegan <pg@bowt.ie> wrote:

> On Thu, Feb 2, 2023 at 11:51 AM Peter Geoghegan <pg@bowt.ie> wrote:
>
...

> Admittedly there is some value in seeing multiple WARNINGs to true
> experts that are performing some kind of forensic analysis, but that
> doesn't seem worth it to me -- I'm an expert, and I don't think that
> I'd do it this way for any reason other than it being more convenient
> as a way to get information about a system that I don't have access
> to. Even then, I think that I'd probably have serious doubts about
> most of the extra information that I'd get, since it might very well
> be a downstream consequence of the same basic problem.
>
...

I understand your thoughts (I think) and agree with them, but at least one
scenario where I do want to see *all* errors is corruption prevention –
running
amcheck in lower environments, not in production, to predict and prevent
issues.
For example, not long ago, Ubuntu 16.04 became EOL (in phases), and people
needed to upgrade, with glibc version change. It was quite good to use
amcheck
on production clones (running on a new OS/glibc) to identify all indexes
that
need to be rebuilt. Being able to see only one of them would be very
inconvenient. Rebuilding all indexes didn't seem a good idea in the case of
large databases.

  26. Re: Amcheck verification of GiST and GIN

    Peter Geoghegan <pg@bowt.ie> — 2023-02-02T20:42:52Z

    On Thu, Feb 2, 2023 at 12:31 PM Nikolay Samokhvalov
<samokhvalov@gmail.com> wrote:
> I understand your thoughts (I think) and agree with them, but at least one
> scenario where I do want to see *all* errors is corruption prevention – running
> amcheck in lower environments, not in production, to predict and prevent issues.
> For example, not long ago, Ubuntu 16.04 became EOL (in phases), and people
> needed to upgrade, with glibc version change. It was quite good to use amcheck
> on production clones (running on a new OS/glibc) to identify all indexes that
> need to be rebuilt. Being able to see only one of them would be very
> inconvenient. Rebuilding all indexes didn't seem a good idea in the case of
> large databases.

I agree that this matters at the level of whole indexes. That is, if
you want to check every index in the database, it is unhelpful if the
whole process stops just because one individual index has corruption.
Any extra information about the index that is corrupt may not be all
that valuable, but information about other indexes remains almost as
valuable.

I think that that problem should be solved at a higher level, in the
program that runs amcheck. Note that pg_amcheck will already do this
for B-Tree indexes. While verify_nbtree.c won't try to limp on with an
index that is known to be corrupt, pg_amcheck will continue with other
indexes.

We should add a "Tip" to the amcheck documentation on 14+ about this.
We should clearly advise users that they should probably just use
pg_amcheck. Using the SQL interface directly should now mostly be
something that only a tiny minority of experts need to do -- and even
the experts won't do it that way unless they have a good reason to.

-- 
Peter Geoghegan

  27. Re: Amcheck verification of GiST and GIN

    Nikolay Samokhvalov <samokhvalov@gmail.com> — 2023-02-02T20:56:47Z

    On Thu, Feb 2, 2023 at 12:43 PM Peter Geoghegan <pg@bowt.ie> wrote:

> I agree that this matters at the level of whole indexes.
>

I already realized my mistake – indeed, having multiple errors for 1 index
doesn't seem to be super practically helpful.


> I think that that problem should be solved at a higher level, in the
> program that runs amcheck. Note that pg_amcheck will already do this
> for B-Tree indexes.
>

That's a great tool, and it's great it supports parallelization, very useful
on large machines.


> We should add a "Tip" to the amcheck documentation on 14+ about this.
> We should clearly advise users that they should probably just use
> pg_amcheck.


and with -j$N, with high $N (unless it's production)

  28. Re: Amcheck verification of GiST and GIN

    Peter Geoghegan <pg@bowt.ie> — 2023-02-02T23:16:32Z

    On Thu, Feb 2, 2023 at 12:56 PM Nikolay Samokhvalov
<samokhvalov@gmail.com> wrote:
> I already realized my mistake – indeed, having multiple errors for 1 index
> doesn't seem to be super practically helpful.

I wouldn't mind supporting it if the cost wasn't too high. But I
believe that it's not a good trade-off.

>> I think that that problem should be solved at a higher level, in the
>> program that runs amcheck. Note that pg_amcheck will already do this
>> for B-Tree indexes.
>
>
> That's a great tool, and it's great it supports parallelization, very useful
> on large machines.

Another big advantage of just using pg_amcheck is that running each
index verification in a standalone query avoids needlessly holding the
same MVCC snapshot across all indexes verified (compared to running
one big SQL query that verifies multiple indexes). As simple as
pg_amcheck's approach is (it's doing nothing that you couldn't
replicate in a shell script), in practice that its standardized
approach probably makes things a lot smoother, especially in terms of
how VACUUM is impacted.

-- 
Peter Geoghegan

  29. Re: Amcheck verification of GiST and GIN

    Peter Geoghegan <pg@bowt.ie> — 2023-02-04T02:49:50Z

    On Thu, Feb 2, 2023 at 12:15 PM Peter Geoghegan <pg@bowt.ie> wrote:
> * Why are there only WARNINGs, never ERRORs here?

Attached revision v22 switches all of the WARNINGs over to ERRORs. It
has also been re-indented, and now uses a non-generic version of
PageGetItemIdCareful() in both verify_gin.c and verify_gist.c.
Obviously this isn't a big set of revisions, but I thought that Andrey
would appreciate it if I posted this much now. I haven't thought much
more about the locking stuff, which is my main concern for now.

Who are the authors of the patch, in full? At some point we'll need to
get the attribution right if this is going to be committed.

I think that it would be good to add some comments explaining the high
level control flow. Is the verification process driven by a
breadth-first search, or a depth-first search, or something else?

I think that we should focus on getting the GiST patch into shape for
commit first, since that seems easier.

-- 
Peter Geoghegan

  30. Re: Amcheck verification of GiST and GIN

    Andrey Borodin <amborodin86@gmail.com> — 2023-02-04T21:37:29Z

    Thank for working on this, Peter!

On Fri, Feb 3, 2023 at 6:50 PM Peter Geoghegan <pg@bowt.ie> wrote:
>
> I think that we should focus on getting the GiST patch into shape for
> commit first, since that seems easier.
>

Here's the next version. I've focused on GiST part in this revision.
Changes:
1. Refactored index_chackable so that is shared between all AMs.
2. Renamed gist_index_parent_check -> gist_index_check
3. Gathered reviewers (in no particular order). I hope I didn't forget
anyone. GIN patch is based on work by Grigory Kryachko, but
essentially rewritten by Heikki. Somewhat cosmetically whacked by me.
4. Extended comments for GistScanItem,
gist_check_parent_keys_consistency() and gist_refind_parent().

I tried adding support of GiST in pg_amcheck, but it is largely
assuming the relation is either heap or B-tree. I hope to do that part
tomorrow or in nearest future.

Here's the current version. Thank you!


Best regards, Andrey Borodin.

  31. Re: Amcheck verification of GiST and GIN

    Andrey Borodin <amborodin86@gmail.com> — 2023-02-06T00:44:53Z

    On Sat, Feb 4, 2023 at 1:37 PM Andrey Borodin <amborodin86@gmail.com> wrote:
>
> I tried adding support of GiST in pg_amcheck, but it is largely
> assuming the relation is either heap or B-tree. I hope to do that part
> tomorrow or in nearest future.
>

Here's v24 == (v23 + a step for pg_amcheck). There's a lot of
shotgun-style changes, but I hope next index types will be easy to add
now.

Adding Mark to cc, just in case.

Thanks!


Best regards, Andrey Borodin.

  32. Re: Amcheck verification of GiST and GIN

    Michael Banck <mbanck@gmx.net> — 2023-02-22T08:51:32Z

    Hi,

On Thu, Feb 02, 2023 at 12:56:47PM -0800, Nikolay Samokhvalov wrote:
> On Thu, Feb 2, 2023 at 12:43 PM Peter Geoghegan <pg@bowt.ie> wrote:
> > I think that that problem should be solved at a higher level, in the
> > program that runs amcheck. Note that pg_amcheck will already do this
> > for B-Tree indexes.
> 
> That's a great tool, and it's great it supports parallelization, very useful
> on large machines.

Right, but unfortunately not an option on managed services. It's clear
that this restriction should not be a general guideline for Postgres
development, but it makes the amcheck extension (that is now shipped
everywhere due to being in-code I believe) somewhat less useful for
use-case of checking your whole database for corruption.


Michael

  33. Re: Amcheck verification of GiST and GIN

    Peter Geoghegan <pg@bowt.ie> — 2023-03-16T23:48:09Z

    On Sun, Feb 5, 2023 at 4:45 PM Andrey Borodin <amborodin86@gmail.com> wrote:
> Here's v24 == (v23 + a step for pg_amcheck). There's a lot of
> shotgun-style changes, but I hope next index types will be easy to add
> now.

Some feedback on the GiST patch:

* You forgot to initialize GistCheckState.heaptuplespresent to 0.

It might be better to allocate GistCheckState dynamically, using
palloc0(). That's future proof. "Simple and obvious" is usually the
most important goal for managing memory in amcheck code. It can be a
little inefficient if that makes it simpler.

* ISTM that gist_index_check() should allow the caller to omit a
"heapallindexed" argument by specifying "DEFAULT FALSE", for
consistency with bt_index_check().

(Actually there are two versions of bt_index_check(), with
overloading, but that's just because of the way that the extension
evolved over time).

* What's the point in having a custom memory context that is never reset?

I believe that gistgetadjusted() will leak memory here, so there is a
need for some kind of high level strategy for managing memory. The
strategy within verify_nbtree.c is to call MemoryContextReset() right
after every loop iteration within bt_check_level_from_leftmost() --
which is pretty much once every call to bt_target_page_check(). That
kind of approach is obviously not going to suffer any memory leaks.

Again, "simple and obvious" is good for memory management in amcheck.

* ISTM that it would be clearer if the per-page code within
gist_check_parent_keys_consistency() was broken out into its own
function -- a little like bt_target_page_check()..

That way the control flow would be easier to understand when looking
at the code at a high level.

* ISTM that gist_refind_parent() should throw an error about
corruption in the event of a parent page somehow becoming a leaf page.

Obviously this is never supposed to happen, and likely never will
happen, even with corruption. But it seems like a good idea to make
the most conservative possible assumption by throwing an error. If it
never happens anyway, then the fact that we handle it with an error
won't matter -- so the error is harmless. If it does happen then we'll
want to hear about it as soon as possible -- so the error is useful.

* I suggest using c99 style variable declarations in loops.

Especially for things like "for (OffsetNumber offset =
FirstOffsetNumber; ... ; ... )".

-- 
Peter Geoghegan

  34. Re: Amcheck verification of GiST and GIN

    Peter Geoghegan <pg@bowt.ie> — 2023-03-17T01:22:42Z

    On Thu, Mar 16, 2023 at 4:48 PM Peter Geoghegan <pg@bowt.ie> wrote:
> Some feedback on the GiST patch:

I see that the Bloom filter that's used to implement heapallindexed
verification fingerprints index tuples that are formed via calls to
gistFormTuple(), without any attempt to normalize-away differences in
TOAST input state. In other words, there is nothing like
verify_nbtree.c's bt_normalize_tuple() function involved in the
fingerprinting process. Why is that safe, though? See the "toast_bug"
test case within contrib/amcheck/sql/check_btree.sql for an example of
how inconsistent TOAST input state confused verify_nbtree.c's
heapallindexed verification (before bugfix commit eba775345d). I'm
concerned about GiST heapallindexed verification being buggy in
exactly the same way, or in some way that is roughly analogous.

I do have some concerns about there being analogous problems that are
unique to GiST, since GiST is an AM that gives opclass authors many
more choices than B-Tree opclass authors have. In particular, I wonder
if heapallindexed verification needs to account for how GiST
compression might end up breaking heapallindexed. I refer to the
"compression" implemented by GiST support routine 3 of GiST opclasses.
The existence of GiST support routine 7, the "same" routine, also
makes me feel a bit squeamish about heapallindexed verification -- the
existence of a "same" routine hints at some confusion about "equality
versus equivalence" issues.

In more general terms: heapallindexed verification works by
fingerprinting index tuples during the index verification stage, and
then performing Bloom filter probes in a separate CREATE INDEX style
heap-matches-index stage (obviously). There must be some justification
for our assumption that there can be no false positive corruption
reports due only to a GiST opclass (either extant or theoretical) that
follows the GiST contract, and yet allows an inconsistency to arise
that isn't really index corruption. This justification won't be easy
to come up with, since the GiST contract was not really designed with
these requirements in mind. But...we should try to come up with
something.

What are the assumptions underlying heapallindexed verification for
GiST? It doesn't have to be provably correct or anything, but it
should at least be empirically falsifiable. Basically, something that
says: "Here are our assumptions, if we were wrong in making these
assumptions then you could tell that we made a mistake because of X,
Y, Z". It's not always clear when something is corrupt. Admittedly I
have much less experience with GiST than other people, which likely
includes you (Andrey). I am likely missing some context around the
evolution of GiST. Possibly I'm making a big deal out of something
without it being unhelpful. Unsure.

Here is an example of the basic definition of correctness being
unclear, in a bad way: Is a HOT chain corrupt when its root
LP_REDIRECT points to an LP_DEAD item, or does that not count as
corruption? I'm pretty sure that the answer is ambiguous even today,
or was ambiguous until recently, at least. Hopefully the
verify_heapam.c HOT chain verification patch will be committed,
providing us with a clear *definition* of HOT chain corruption -- the
definition itself may not be the easy part.

On a totally unrelated note: I wonder if we should be checking that
internal page tuples have 0xffff as their offset number? Seems like
it'd be a cheap enough cross-check.

-- 
Peter Geoghegan

  35. Re: Amcheck verification of GiST and GIN

    Andrey Borodin <amborodin86@gmail.com> — 2023-03-18T03:40:05Z

    Hi Peter,

Thanks for the feedback! I'll work on it during the weekend.

On Thu, Mar 16, 2023 at 6:23 PM Peter Geoghegan <pg@bowt.ie> wrote:
>
> existence of a "same" routine hints at some confusion about "equality
> versus equivalence" issues.

Hmm...yes, actually, GiST deals with floats routinely. And there might
be some sorts of NaNs and Infs that are equal, but not binary
equivalent.
I'll think more about it.

gist_get_adjusted() calls "same" routine, which for type point will
use FPeq(double A, double B). And this might be kind of a corruption
out of the box. Because it's an epsilon-comparison, ε=1.0E-06.
GiST might miss newly inserted data, because the "adjusted" tuple was
"same" if data is in proximity of 0.000001 of any previously indexed
point, but out of known MBRs.
I'll try to reproduce this tomorrow, so far no luck.


Best regards, Andrey Borodin.

  36. Re: Amcheck verification of GiST and GIN

    Andrey Borodin <amborodin86@gmail.com> — 2023-03-19T23:00:14Z

    On Fri, Mar 17, 2023 at 8:40 PM Andrey Borodin <amborodin86@gmail.com> wrote:
>
> On Thu, Mar 16, 2023 at 6:23 PM Peter Geoghegan <pg@bowt.ie> wrote:
> >
> > existence of a "same" routine hints at some confusion about "equality
> > versus equivalence" issues.
>
> Hmm...yes, actually, GiST deals with floats routinely. And there might
> be some sorts of NaNs and Infs that are equal, but not binary
> equivalent.
> I'll think more about it.
>
> gist_get_adjusted() calls "same" routine, which for type point will
> use FPeq(double A, double B). And this might be kind of a corruption
> out of the box. Because it's an epsilon-comparison, ε=1.0E-06.
> GiST might miss newly inserted data, because the "adjusted" tuple was
> "same" if data is in proximity of 0.000001 of any previously indexed
> point, but out of known MBRs.
> I'll try to reproduce this tomorrow, so far no luck.
>
After several attempts to corrupt GiST with this 0.000001 epsilon
adjustment tolerance I think GiST indexing of points is valid.
Because intersection for search purposes is determined with the same epsilon!
So it's kind of odd
postgres=# select point(0.0000001,0)~=point(0,0);
?column?
----------
 t
(1 row)
, yet the index works correctly.



On Thu, Mar 16, 2023 at 4:48 PM Peter Geoghegan <pg@bowt.ie> wrote:
>
> On Sun, Feb 5, 2023 at 4:45 PM Andrey Borodin <amborodin86@gmail.com> wrote:
> > Here's v24 == (v23 + a step for pg_amcheck). There's a lot of
> > shotgun-style changes, but I hope next index types will be easy to add
> > now.
>
> Some feedback on the GiST patch:
>
> * You forgot to initialize GistCheckState.heaptuplespresent to 0.
>
> It might be better to allocate GistCheckState dynamically, using
> palloc0(). That's future proof. "Simple and obvious" is usually the
> most important goal for managing memory in amcheck code. It can be a
> little inefficient if that makes it simpler.
Done.

> * ISTM that gist_index_check() should allow the caller to omit a
> "heapallindexed" argument by specifying "DEFAULT FALSE", for
> consistency with bt_index_check().
Done.

> * What's the point in having a custom memory context that is never reset?
The problem is we traverse index with depth-first scan and must retain
internal tuples for a whole time of the scan.
And gistgetadjusted() will allocate memory only in case of suspicion
of corruption. So, it's kind of an infrequent case.

The context is there only as an overall leak protection mechanism.
Actual memory management is done via pfree() calls.

> Again, "simple and obvious" is good for memory management in amcheck.
Yes, that would be great to come up with some "unit of work" contexts.
Yet, now palloced tuples and scan items have very different lifespans.


> * ISTM that it would be clearer if the per-page code within
> gist_check_parent_keys_consistency() was broken out into its own
> function -- a little like bt_target_page_check()..

I've refactored page logic into gist_check_page().

> * ISTM that gist_refind_parent() should throw an error about
> corruption in the event of a parent page somehow becoming a leaf page.
Done.

> * I suggest using c99 style variable declarations in loops.
Done.


On Thu, Mar 16, 2023 at 6:23 PM Peter Geoghegan <pg@bowt.ie> wrote:
>
> On Thu, Mar 16, 2023 at 4:48 PM Peter Geoghegan <pg@bowt.ie> wrote:
> > Some feedback on the GiST patch:
>
> I see that the Bloom filter that's used to implement heapallindexed
> verification fingerprints index tuples that are formed via calls to
> gistFormTuple(), without any attempt to normalize-away differences in
> TOAST input state. In other words, there is nothing like
> verify_nbtree.c's bt_normalize_tuple() function involved in the
> fingerprinting process. Why is that safe, though? See the "toast_bug"
> test case within contrib/amcheck/sql/check_btree.sql for an example of
> how inconsistent TOAST input state confused verify_nbtree.c's
> heapallindexed verification (before bugfix commit eba775345d). I'm
> concerned about GiST heapallindexed verification being buggy in
> exactly the same way, or in some way that is roughly analogous.
FWIW contrib opclasses, AFAIK, always detoast possibly long datums,
see gbt_var_compress()
https://github.com/postgres/postgres/blob/master/contrib/btree_gist/btree_utils_var.c#L281
But there might be opclasses that do not do so...
Also, there are INCLUDEd attributes. Right now we just put them as-is
to the bloom filter. Does this constitute a TOAST bug as in B-tree?
If so, I think we should use a version of tuple formatting that omits
included attributes...
What do you think?

>
> I do have some concerns about there being analogous problems that are
> unique to GiST, since GiST is an AM that gives opclass authors many
> more choices than B-Tree opclass authors have. In particular, I wonder
> if heapallindexed verification needs to account for how GiST
> compression might end up breaking heapallindexed. I refer to the
> "compression" implemented by GiST support routine 3 of GiST opclasses.
> The existence of GiST support routine 7, the "same" routine, also
> makes me feel a bit squeamish about heapallindexed verification -- the
> existence of a "same" routine hints at some confusion about "equality
> versus equivalence" issues.
>
> In more general terms: heapallindexed verification works by
> fingerprinting index tuples during the index verification stage, and
> then performing Bloom filter probes in a separate CREATE INDEX style
> heap-matches-index stage (obviously). There must be some justification
> for our assumption that there can be no false positive corruption
> reports due only to a GiST opclass (either extant or theoretical) that
> follows the GiST contract, and yet allows an inconsistency to arise
> that isn't really index corruption. This justification won't be easy
> to come up with, since the GiST contract was not really designed with
> these requirements in mind. But...we should try to come up with
> something.
>
> What are the assumptions underlying heapallindexed verification for
> GiST? It doesn't have to be provably correct or anything, but it
> should at least be empirically falsifiable. Basically, something that
> says: "Here are our assumptions, if we were wrong in making these
> assumptions then you could tell that we made a mistake because of X,
> Y, Z". It's not always clear when something is corrupt. Admittedly I
> have much less experience with GiST than other people, which likely
> includes you (Andrey). I am likely missing some context around the
> evolution of GiST. Possibly I'm making a big deal out of something
> without it being unhelpful. Unsure.
>
> Here is an example of the basic definition of correctness being
> unclear, in a bad way: Is a HOT chain corrupt when its root
> LP_REDIRECT points to an LP_DEAD item, or does that not count as
> corruption? I'm pretty sure that the answer is ambiguous even today,
> or was ambiguous until recently, at least. Hopefully the
> verify_heapam.c HOT chain verification patch will be committed,
> providing us with a clear *definition* of HOT chain corruption -- the
> definition itself may not be the easy part.

Rules for compression methods are not described anyware. And I suspect
that it's intentional, to provide more flexibility.
To make heapallindexed check work we need that opclass always returns
the same compression result for the same input datum.
All known to me opclasses (built-in and PostGIS) comply with this requirement.

Yet another behavior might be reasonable. Consider we have a
compression which learns on data. It will observe that some datums are
more frequent and start using shorter version of them.

Compression function actually is not about compression, but kind of a
conversion from heap format to indexable. Many opclasses do not have a
compression function at all.
We can require that the checked opclass would not have a compression
function at all. But GiST is mainly used for PostGIS, and in PostGIS
they use compression to convert complex geometry into a bounding box.

Method "same" is used only for a business of internal tuples, but not
for leaf tuples that we fingerprint in the bloom filter.

We can put requirements for heapallindexed in another way: "the
opclass compression method must be a pure function". It's also a very
strict requirement, disallowing all kinds of detoasting, dictionary
compression etc. And btree_gist opclasses does not comply :) But they
seem to me safe for heapallindexed.

> On a totally unrelated note: I wonder if we should be checking that
> internal page tuples have 0xffff as their offset number? Seems like
> it'd be a cheap enough cross-check.
>

Done.

Thank you!

Best regards, Andrey Borodin.

  37. Re: Amcheck verification of GiST and GIN

    Andrey Borodin <amborodin86@gmail.com> — 2023-03-26T22:17:02Z

    On Sun, Mar 19, 2023 at 4:00 PM Andrey Borodin <amborodin86@gmail.com> wrote:
>
> Also, there are INCLUDEd attributes. Right now we just put them as-is
> to the bloom filter. Does this constitute a TOAST bug as in B-tree?
> If so, I think we should use a version of tuple formatting that omits
> included attributes...
> What do you think?
I've ported the B-tree TOAST test to GiST, and, as expected, it fails.
Finds non-indexed tuple for a fresh valid index.
I've implemented normalization, plz see gistFormNormalizedTuple().
But there are two problems:
1. I could not come up with a proper way to pfree() compressed value
after decompressing. See TODO in gistFormNormalizedTuple().
2. In the index tuples seem to be normalized somewhere. They do not
have to be deformed and normalized. It's not clear to me how this
happened.

Thanks!

Best regards, Andrey Borodin.

  38. Re: Amcheck verification of GiST and GIN

    Peter Geoghegan <pg@bowt.ie> — 2023-03-27T02:34:49Z

    On Sun, Mar 19, 2023 at 4:00 PM Andrey Borodin <amborodin86@gmail.com> wrote:
> After several attempts to corrupt GiST with this 0.000001 epsilon
> adjustment tolerance I think GiST indexing of points is valid.
> Because intersection for search purposes is determined with the same epsilon!
> So it's kind of odd
> postgres=# select point(0.0000001,0)~=point(0,0);
> ?column?
> ----------
>  t
> (1 row)
> , yet the index works correctly.

I think that it's okay, provided that we can assume deterministic
behavior in the code that forms new index tuples. Within nbtree,
operator classes like numeric_ops are supported by heapallindexed
verification, without any requirement for special normalization code
to make it work correctly as a special case. This is true even though
operator classes such as numeric_ops have similar "equality is not
equivalence" issues, which comes up in other areas (e.g., nbtree
deduplication, which must call support routine 4 during a CREATE INDEX
[1]).

The important principle is that amcheck must always be able to produce
a consistent fingerprintable binary output given the same input (the
same heap tuple/Datum array). This must work across all operator
classes that play by the rules for GiST operator classes. We *can*
tolerate some variation here. Well, we really *have* to tolerate a
little of this kind of variation in order to deal with the TOAST input
state thing...but I hope that that's the only complicating factor
here, for GiST (as it is for nbtree). Note that we already rely on the
fact that index_form_tuple() uses palloc0() (not plain palloc) in
verify_nbtree.c, for the obvious reason.

I think that there is a decent chance that it just wouldn't make sense
for an operator class author to ever do something that we need to
worry about. I'm pretty sure that it's just the TOAST thing. But it's
worth thinking about carefully.

[1] https://www.postgresql.org/docs/devel/btree-support-funcs.html
-- 
Peter Geoghegan

  39. Re: Amcheck verification of GiST and GIN

    Alexander Lakhin <exclusion@gmail.com> — 2023-04-06T04:00:01Z

    Hi Andrey,

27.03.2023 01:17, Andrey Borodin wrote:
> I've ported the B-tree TOAST test to GiST, and, as expected, it fails.
> Finds non-indexed tuple for a fresh valid index.

I've tried to use this feature with the latest patch set and discovered that
modified pg_amcheck doesn't find any gist indexes when running without a
schema specification. For example:
CREATE TABLE tbl (id integer, p point);
INSERT INTO tbl VALUES (1, point(1, 1));
CREATE INDEX gist_tbl_idx ON tbl USING gist (p);
CREATE INDEX btree_tbl_idx ON tbl USING btree (id);

pg_amcheck -v -s public
prints:
pg_amcheck: checking index "regression.public.btree_tbl_idx"
pg_amcheck: checking heap table "regression.public.tbl"
pg_amcheck: checking index "regression.public.gist_tbl_idx"

but without "-s public" a message about checking of gist_tbl_idx is absent.

As I can see in the server.log, the queries, that generate relation lists in
these cases, differ in:
... AND ep.pattern_id IS NULL AND c.relam = 2 AND c.relkind IN ('r', 'S', 'm', 't') AND c.relnamespace != 99 ...

... AND ep.pattern_id IS NULL AND c.relam IN (2, 403, 783)AND c.relkind IN ('r', 'S', 'm', 't', 'i') AND ((c.relam = 2 
AND c.relkind IN ('r', 'S', 'm', 't')) OR ((c.relam = 403 OR c.relam = 783) AND c.relkind = 'i')) ...

Best regards,
Alexander

  40. Re: Amcheck verification of GiST and GIN

    vignesh C <vignesh21@gmail.com> — 2024-01-20T02:46:16Z

    On Mon, 27 Mar 2023 at 03:47, Andrey Borodin <amborodin86@gmail.com> wrote:
>
> On Sun, Mar 19, 2023 at 4:00 PM Andrey Borodin <amborodin86@gmail.com> wrote:
> >
> > Also, there are INCLUDEd attributes. Right now we just put them as-is
> > to the bloom filter. Does this constitute a TOAST bug as in B-tree?
> > If so, I think we should use a version of tuple formatting that omits
> > included attributes...
> > What do you think?
> I've ported the B-tree TOAST test to GiST, and, as expected, it fails.
> Finds non-indexed tuple for a fresh valid index.
> I've implemented normalization, plz see gistFormNormalizedTuple().
> But there are two problems:
> 1. I could not come up with a proper way to pfree() compressed value
> after decompressing. See TODO in gistFormNormalizedTuple().
> 2. In the index tuples seem to be normalized somewhere. They do not
> have to be deformed and normalized. It's not clear to me how this
> happened.

I have changed the status of the commitfest entry to "Waiting on
Author" as there was no follow-up on Alexander's queries. Feel free to
address them and change the commitfest entry accordingly.

Regards,
Vignesh

  41. Re: Amcheck verification of GiST and GIN

    Andrey Borodin <x4mmm@yandex-team.ru> — 2024-03-11T06:11:33Z

    
> On 20 Jan 2024, at 07:46, vignesh C <vignesh21@gmail.com> wrote:
> 
> I have changed the status of the commitfest entry to "Waiting on
> Author" as there was no follow-up on Alexander's queries. Feel free to
> address them and change the commitfest entry accordingly.

Thanks Vignesh!

At the moment it’s obvious that this change will not be in 17, but I have plans to continue work on this. So I’ll move this item to July CF.


Best regards, Andrey Borodin.

  42. Re: Amcheck verification of GiST and GIN

    Andrey Borodin <x4mmm@yandex-team.ru> — 2024-07-05T12:27:55Z

    
> On 6 Apr 2023, at 09:00, Alexander Lakhin <exclusion@gmail.com> wrote:
> 
> I've tried to use this feature with the latest patch set and discovered that
> modified pg_amcheck doesn't find any gist indexes when running without a
> schema specification.

Thanks, Alexander! I’ve fixed this problem and rebased on current HEAD.
There’s one more problem in pg_amcheck’s GiST verification. We must check that amcheck is 1.5+ and use GiST verification only in that case…


Best regards, Andrey Borodin.

  43. Re: Amcheck verification of GiST and GIN

    Andrey Borodin <x4mmm@yandex-team.ru> — 2024-07-09T06:36:50Z

    
> On 5 Jul 2024, at 17:27, Andrey M. Borodin <x4mmm@yandex-team.ru> wrote:
> 
> There’s one more problem in pg_amcheck’s GiST verification. We must check that amcheck is 1.5+ and use GiST verification only in that case…

Done. I’ll set the status to “Needs review”.


Best regards, Andrey Borodin.

  44. Re: Amcheck verification of GiST and GIN

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2024-07-10T16:01:40Z

    Hi,

On 7/9/24 08:36, Andrey M. Borodin wrote:
> 
> 
>> On 5 Jul 2024, at 17:27, Andrey M. Borodin <x4mmm@yandex-team.ru> wrote:
>>
>> There’s one more problem in pg_amcheck’s GiST verification. We must
>> check that amcheck is 1.5+ and use GiST verification only in that
>> case …
> 
> Done. I’ll set the status to “Needs review”.
> 

I realized amcheck GIN/GiST support would be useful for testing my
patches adding parallel builds for these index types, so I decided to
take a look at this and do an initial review today.

Attached is a patch series with a extra commits to keep the review
comments and patches adjusting the formatting by pgindent (the patch
seems far enough for this).

Let me quickly go through the review comments:

1) Not sure I like 'amcheck.c' very much, I'd probably go with something
like 'verify_common.c' to match naming of the other files. But it's just
nitpicking and I can live with it.

2) amcheck_lock_relation_and_check seems to be the most important
function, yet there's no comment explaining what it does :-(

3) amcheck_lock_relation_and_check still has a TODO to add the correct
name of the AM

4) Do we actually need amcheck_index_mainfork_expected as a separate
function, or could it be a part of index_checkable?

5) The comment for heaptuplespresent says "debug counter" but that does
not really explain what it's for. (I see verify_nbtree has the same
comment, but maybe let's improve that.)

6) I'd suggest moving the GISTSTATE + blocknum fields to the beginning
of GistCheckState, it seems more natural to start with "generic" fields.

7) I'd adjust the gist_check_parent_keys_consistency comment a bit, to
explain what the function does first, and only then explain how.

8) We seem to be copying PageGetItemIdCareful() around, right? And the
copy in _gist.c still references nbtree - I guess that's not right.

9) Why is the GIN function called gin_index_parent_check() and not
simply gin_index_check() as for the other AMs?

10) The debug in gin_check_posting_tree_parent_keys_consistency triggers
assert when running with client_min_messages='debug5', it seems to be
accessing bogus item pointers.

11) Why does it add pg_amcheck support only for GiST and not GIN?


That's all for now. I'll add this to the stress-testing tests of my
index build patches, and if that triggers more issues I'll report those.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  45. Re: Amcheck verification of GiST and GIN

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2024-07-12T12:16:24Z

    On 7/10/24 18:01, Tomas Vondra wrote:
> ...
>
> That's all for now. I'll add this to the stress-testing tests of my
> index build patches, and if that triggers more issues I'll report those.
> 

As mentioned a couple days ago, I started using this patch to validate
the patches adding parallel builds to GIN and GiST indexes - I scripts
to stress-test the builds, and I added the new amcheck functions as
another validation step.

For GIN indexes it didn't find anything new (in either this or my
patches), aside from the assert crash I already reported.

But for GiST it turned out to be very valuable - it did actually find an
issue in my patches, or rather confirm my hypothesis that the way the
patch generates fake LSN may not be quite right.

In particular, I've observed these two issues:

  ERROR:  heap tuple (13315,38) from table "planet_osm_roads" lacks
          matching index tuple within index "roads_7_1_idx"

  ERROR:  index "roads_7_7_idx" has inconsistent records on page 23723
          offset 113

And those consistency issues are real - I've managed to reproduce issues
with incorrect query results (by comparing the results to an index built
without parallelism).

So that's nice - it shows the value of this patch, and I like it.

One thing I've been wondering about is that currently amcheck (in
general, not just these new GIN/GiST functions) errors out on the first
issue, because it does ereport(ERROR). Which is good enough to decide if
there is some corruption, but a bit inconvenient if you need to assess
how much corruption is there. For example when investigating the issue
in my patch it would have been great to know if there's just one broken
page, or if there are dozens/hundreds/thousands of them.

I'd imagine we could have a flag which says whether to fail on the first
issue, or keep looking at future pages. Essentially, whether to do
ereport(ERROR) or ereport(WARNING). But maybe that's a dead-end, and
once we find the first issue it's futile to inspect the rest of the
index, because it can be garbage. Not sure. In any case, it's not up to
this patch to invent that.

I don't have additional comments, the patch seems to be clean and likely
ready to go. There's a couple committers already involved in this
thread, I wonder if one of them already planned to take care of this?
Peter and Andres, either of you interested in this?

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  46. Re: Amcheck verification of GiST and GIN

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2024-07-12T16:15:03Z

    OK, one mere comment - it seems the 0001 patch has incorrect indentation
in bt_index_check_callback, triggering this:

----------------------------------------------------------------------
verify_nbtree.c: In function ‘bt_index_check_callback’:
verify_nbtree.c:331:25: warning: this ‘if’ clause does not guard...
[-Wmisleading-indentation]
  331 |                         if (indrel->rd_opfamily[i] ==
INTERVAL_BTREE_FAM_OID)
      |                         ^~
In file included from ../../src/include/postgres.h:46,
                 from verify_nbtree.c:24:
../../src/include/utils/elog.h:142:9: note: ...this statement, but the
latter is misleadingly indented as if it were guarded by the ‘if’
  142 |         do { \
      |         ^~
../../src/include/utils/elog.h:164:9: note: in expansion of macro
‘ereport_domain’
  164 |         ereport_domain(elevel, TEXTDOMAIN, __VA_ARGS__)
      |         ^~~~~~~~~~~~~~
verify_nbtree.c:333:33: note: in expansion of macro ‘ereport’
  333 |                                 ereport(ERROR,
      |                                 ^~~~~~~
----------------------------------------------------------------------

This seems to be because the ereport() happens to be indented as if it
was in the "if", but should have been at the "for" level.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  47. Re: Amcheck verification of GiST and GIN

    Tomas Vondra <tomas.vondra@enterprisedb.com> — 2024-07-12T18:16:42Z

    OK, one more issue report. I originally thought it's a bug in my patch
adding parallel builds for GIN indexes, but it turns out it happens even
with serial builds on master ...

If I build any GIN index, and then do gin_index_parent_check() on it, I
get this error:

create index jsonb_hash on messages using gin (msg_headers jsonb_path_ops);

select gin_index_parent_check('jsonb_hash');
ERROR:  index "jsonb_hash" has wrong tuple order, block 43932, offset 328

I did try investigating usinng pageinspect - the page seems to be the
right-most in the tree, judging by rightlink = InvalidBlockNumber:

test=# select gin_page_opaque_info(get_raw_page('jsonb_hash', 43932));
 gin_page_opaque_info
----------------------
 (4294967295,0,{})
(1 row)

But gin_leafpage_items() apparently only works with compressed leaf
pages, so I'm not sure what's in the page. In any case, the index seems
to be working fine, so it seems like a bug in this patch.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  48. Re: Amcheck verification of GiST and GIN

    Andrey Borodin <x4mmm@yandex-team.ru> — 2024-07-14T19:00:19Z

    Hi Tomas!

Thank you so much for your interest in the patchset.

> On 10 Jul 2024, at 19:01, Tomas Vondra <tomas.vondra@enterprisedb.com> wrote:
> 
> I realized amcheck GIN/GiST support would be useful for testing my
> patches adding parallel builds for these index types, so I decided to
> take a look at this and do an initial review today.

Great! Thank you!

> Attached is a patch series with a extra commits to keep the review
> comments and patches adjusting the formatting by pgindent (the patch
> seems far enough for this).

I was hoping to address your review comments this weekend, but unfortunately I could not. I'll do this ASAP, but at least I decided to post answers on questions.

> 
> Let me quickly go through the review comments:
> 
> 1) Not sure I like 'amcheck.c' very much, I'd probably go with something
> like 'verify_common.c' to match naming of the other files. But it's just
> nitpicking and I can live with it.

Any name works for me. We have tens of files ending with "common.c", so I think that's a good way to go.

> 2) amcheck_lock_relation_and_check seems to be the most important
> function, yet there's no comment explaining what it does :-(

Makes sense.

> 3) amcheck_lock_relation_and_check still has a TODO to add the correct
> name of the AM

Yes, I've discovered it during rebase and added TODO.

> 4) Do we actually need amcheck_index_mainfork_expected as a separate
> function, or could it be a part of index_checkable?

It was separate function before refactoring...

> 5) The comment for heaptuplespresent says "debug counter" but that does
> not really explain what it's for. (I see verify_nbtree has the same
> comment, but maybe let's improve that.)

It's there for a DEBUG1 message
ereport(DEBUG1,
                (errmsg_internal("finished verifying presence of " INT64_FORMAT " tuples from table \"%s\" with bitset %.2f%% set",
But the message is gone for GiST. Perhaps, let's restore this message?

> 
> 6) I'd suggest moving the GISTSTATE + blocknum fields to the beginning
> of GistCheckState, it seems more natural to start with "generic" fields.

Makes sense.

> 7) I'd adjust the gist_check_parent_keys_consistency comment a bit, to
> explain what the function does first, and only then explain how.

Makes sense.

> 8) We seem to be copying PageGetItemIdCareful() around, right? And the
> copy in _gist.c still references nbtree - I guess that's not right.

Version differ in two aspects:
1. Size of opaque data may be different. But we can pass it as a parameter.
2. GIN's line pointer verification is slightly more strict.

> 
> 9) Why is the GIN function called gin_index_parent_check() and not
> simply gin_index_check() as for the other AMs?

AFAIR function should be called _parent_ if it takes ShareLock. gin_index_parent_check() does not, so I think we should rename it.

> 10) The debug in gin_check_posting_tree_parent_keys_consistency triggers
> assert when running with client_min_messages='debug5', it seems to be
> accessing bogus item pointers.
> 
> 11) Why does it add pg_amcheck support only for GiST and not GIN?

GiST part is by far more polished. When we were discussing current implementation with Peter G, we decided that we could finish work on GiST, and then proceed to GIN. Main concern is about GIN's locking model.




> On 12 Jul 2024, at 15:16, Tomas Vondra <tomas.vondra@enterprisedb.com> wrote:
> 
> On 7/10/24 18:01, Tomas Vondra wrote:
>> ...
>> 
>> That's all for now. I'll add this to the stress-testing tests of my
>> index build patches, and if that triggers more issues I'll report those.
>> 
> 
> As mentioned a couple days ago, I started using this patch to validate
> the patches adding parallel builds to GIN and GiST indexes - I scripts
> to stress-test the builds, and I added the new amcheck functions as
> another validation step.
> 
> For GIN indexes it didn't find anything new (in either this or my
> patches), aside from the assert crash I already reported.
> 
> But for GiST it turned out to be very valuable - it did actually find an
> issue in my patches, or rather confirm my hypothesis that the way the
> patch generates fake LSN may not be quite right.
> 
> In particular, I've observed these two issues:
> 
>  ERROR:  heap tuple (13315,38) from table "planet_osm_roads" lacks
>          matching index tuple within index "roads_7_1_idx"
> 
>  ERROR:  index "roads_7_7_idx" has inconsistent records on page 23723
>          offset 113
> 
> And those consistency issues are real - I've managed to reproduce issues
> with incorrect query results (by comparing the results to an index built
> without parallelism).
> 
> So that's nice - it shows the value of this patch, and I like it.

That's great!

> One thing I've been wondering about is that currently amcheck (in
> general, not just these new GIN/GiST functions) errors out on the first
> issue, because it does ereport(ERROR). Which is good enough to decide if
> there is some corruption, but a bit inconvenient if you need to assess
> how much corruption is there. For example when investigating the issue
> in my patch it would have been great to know if there's just one broken
> page, or if there are dozens/hundreds/thousands of them.
> 
> I'd imagine we could have a flag which says whether to fail on the first
> issue, or keep looking at future pages. Essentially, whether to do
> ereport(ERROR) or ereport(WARNING). But maybe that's a dead-end, and
> once we find the first issue it's futile to inspect the rest of the
> index, because it can be garbage. Not sure. In any case, it's not up to
> this patch to invent that.

The thing is amcheck tries hard to to do a core dump. It's still possible to crash it with garbage. But if we continue check after encountering first corruption - increase in SegFaults is inevitable.


Thank you! I hope I can get back to code ASAP.


Best regards, Andrey Borodin.

  49. Re: Amcheck verification of GiST and GIN

    Tomas Vondra <tomas@vondra.me> — 2024-08-05T15:05:04Z

    Hi,

I've spent a bit more time looking at the GiST part as part of my
"parallel GiST build" patch nearby, and I think there's some sort of
memory leak.

Consider this:

  create table t (a text);

  insert into t select md5(i::text)
    from generate_series(1,25000000) s(i);

  create index on t using gist (a gist_trgm_ops);

  select gist_index_check('t_a_idx', true);

This creates a ~4GB GiST trigram index, and then checks it. But that
gets killed, because of OOM killer. On my test machine it consumes
~6.5GB of memory before OOM intervenes.

The memory context stats look like this:

  TopPortalContext: 8192 total in 1 blocks; 7680 free (0 chunks); 512 used
    PortalContext: 1024 total in 1 blocks; 616 free (0 chunks); 408
used: <unnamed>
      ExecutorState: 8192 total in 1 blocks; 4024 free (4 chunks); 4168 used
        printtup: 8192 total in 1 blocks; 7952 free (0 chunks); 240 used
        ExprContext: 8192 total in 1 blocks; 7224 free (10 chunks); 968 used
          amcheck context: 3128950872 total in 376 blocks; 219392 free
(1044 chunks); 3128731480 used
            ExecutorState: 8192 total in 1 blocks; 7200 free (0 chunks);
992 used
              ExprContext: 8192 total in 1 blocks; 7952 free (0 chunks);
240 used
            GiST scan context: 22248 total in 2 blocks; 7808 free (8
chunks); 14440 used

This is from before the OOM kill, but it shows there's ~3GB of memory is
the amcheck context.

Seems like a memory leak to me - I didn't look at which place leaks.



regards

-- 
Tomas Vondra

  50. Re: Amcheck verification of GiST and GIN

    Kirill Reshke <reshkekirill@gmail.com> — 2024-10-18T12:42:43Z

    Hi!

On Mon, 5 Aug 2024 at 20:05, Tomas Vondra <tomas@vondra.me> wrote:
>
> Hi,
>
> I've spent a bit more time looking at the GiST part as part of my
> "parallel GiST build" patch nearby, and I think there's some sort of
> memory leak.
>
> Consider this:
>
>   create table t (a text);
>
>   insert into t select md5(i::text)
>     from generate_series(1,25000000) s(i);
>
>   create index on t using gist (a gist_trgm_ops);
>
>   select gist_index_check('t_a_idx', true);
>
> This creates a ~4GB GiST trigram index, and then checks it. But that
> gets killed, because of OOM killer. On my test machine it consumes
> ~6.5GB of memory before OOM intervenes.
>
> The memory context stats look like this:
>
>   TopPortalContext: 8192 total in 1 blocks; 7680 free (0 chunks); 512 used
>     PortalContext: 1024 total in 1 blocks; 616 free (0 chunks); 408
> used: <unnamed>
>       ExecutorState: 8192 total in 1 blocks; 4024 free (4 chunks); 4168 used
>         printtup: 8192 total in 1 blocks; 7952 free (0 chunks); 240 used
>         ExprContext: 8192 total in 1 blocks; 7224 free (10 chunks); 968 used
>           amcheck context: 3128950872 total in 376 blocks; 219392 free
> (1044 chunks); 3128731480 used
>             ExecutorState: 8192 total in 1 blocks; 7200 free (0 chunks);
> 992 used
>               ExprContext: 8192 total in 1 blocks; 7952 free (0 chunks);
> 240 used
>             GiST scan context: 22248 total in 2 blocks; 7808 free (8
> chunks); 14440 used
>
> This is from before the OOM kill, but it shows there's ~3GB of memory is
> the amcheck context.
>
> Seems like a memory leak to me - I didn't look at which place leaks.

+ 1, there is a memory leak.

>
>
> regards
>
> --
> Tomas Vondra
>
>

So, I did some testing, and it seems that the tuple returned by
`gistgetadjusted` inside `gist_check_page` is not being freed.

Trivial fix attached.

-- 
Best regards,
Kirill Reshke

  51. Re: Amcheck verification of GiST and GIN

    Kirill Reshke <reshkekirill@gmail.com> — 2024-11-26T06:50:20Z

    On Mon, 15 Jul 2024 at 00:00, Andrey M. Borodin <x4mmm@yandex-team.ru> wrote:
>
> Hi Tomas!
>
> Thank you so much for your interest in the patchset.
>
> > On 10 Jul 2024, at 19:01, Tomas Vondra <tomas.vondra@enterprisedb.com> wrote:
> >
> > I realized amcheck GIN/GiST support would be useful for testing my
> > patches adding parallel builds for these index types, so I decided to
> > take a look at this and do an initial review today.
>
> Great! Thank you!
>
> > Attached is a patch series with a extra commits to keep the review
> > comments and patches adjusting the formatting by pgindent (the patch
> > seems far enough for this).
>
> I was hoping to address your review comments this weekend, but unfortunately I could not. I'll do this ASAP, but at least I decided to post answers on questions.
>
> >
> > Let me quickly go through the review comments:
> >
> > 1) Not sure I like 'amcheck.c' very much, I'd probably go with something
> > like 'verify_common.c' to match naming of the other files. But it's just
> > nitpicking and I can live with it.
>
> Any name works for me. We have tens of files ending with "common.c", so I think that's a good way to go.
>
> > 2) amcheck_lock_relation_and_check seems to be the most important
> > function, yet there's no comment explaining what it does :-(
>
> Makes sense.
>
> > 3) amcheck_lock_relation_and_check still has a TODO to add the correct
> > name of the AM
>
> Yes, I've discovered it during rebase and added TODO.
>
> > 4) Do we actually need amcheck_index_mainfork_expected as a separate
> > function, or could it be a part of index_checkable?
>
> It was separate function before refactoring...
>
> > 5) The comment for heaptuplespresent says "debug counter" but that does
> > not really explain what it's for. (I see verify_nbtree has the same
> > comment, but maybe let's improve that.)
>
> It's there for a DEBUG1 message
> ereport(DEBUG1,
>                 (errmsg_internal("finished verifying presence of " INT64_FORMAT " tuples from table \"%s\" with bitset %.2f%% set",
> But the message is gone for GiST. Perhaps, let's restore this message?
>
> >
> > 6) I'd suggest moving the GISTSTATE + blocknum fields to the beginning
> > of GistCheckState, it seems more natural to start with "generic" fields.
>
> Makes sense.
>
> > 7) I'd adjust the gist_check_parent_keys_consistency comment a bit, to
> > explain what the function does first, and only then explain how.
>
> Makes sense.
>
> > 8) We seem to be copying PageGetItemIdCareful() around, right? And the
> > copy in _gist.c still references nbtree - I guess that's not right.
>
> Version differ in two aspects:
> 1. Size of opaque data may be different. But we can pass it as a parameter.
> 2. GIN's line pointer verification is slightly more strict.
>
> >
> > 9) Why is the GIN function called gin_index_parent_check() and not
> > simply gin_index_check() as for the other AMs?
>
> AFAIR function should be called _parent_ if it takes ShareLock. gin_index_parent_check() does not, so I think we should rename it.
>
> > 10) The debug in gin_check_posting_tree_parent_keys_consistency triggers
> > assert when running with client_min_messages='debug5', it seems to be
> > accessing bogus item pointers.
> >
> > 11) Why does it add pg_amcheck support only for GiST and not GIN?
>
> GiST part is by far more polished. When we were discussing current implementation with Peter G, we decided that we could finish work on GiST, and then proceed to GIN. Main concern is about GIN's locking model.
>
>
>
>
> > On 12 Jul 2024, at 15:16, Tomas Vondra <tomas.vondra@enterprisedb.com> wrote:
> >
> > On 7/10/24 18:01, Tomas Vondra wrote:
> >> ...
> >>
> >> That's all for now. I'll add this to the stress-testing tests of my
> >> index build patches, and if that triggers more issues I'll report those.
> >>
> >
> > As mentioned a couple days ago, I started using this patch to validate
> > the patches adding parallel builds to GIN and GiST indexes - I scripts
> > to stress-test the builds, and I added the new amcheck functions as
> > another validation step.
> >
> > For GIN indexes it didn't find anything new (in either this or my
> > patches), aside from the assert crash I already reported.
> >
> > But for GiST it turned out to be very valuable - it did actually find an
> > issue in my patches, or rather confirm my hypothesis that the way the
> > patch generates fake LSN may not be quite right.
> >
> > In particular, I've observed these two issues:
> >
> >  ERROR:  heap tuple (13315,38) from table "planet_osm_roads" lacks
> >          matching index tuple within index "roads_7_1_idx"
> >
> >  ERROR:  index "roads_7_7_idx" has inconsistent records on page 23723
> >          offset 113
> >
> > And those consistency issues are real - I've managed to reproduce issues
> > with incorrect query results (by comparing the results to an index built
> > without parallelism).
> >
> > So that's nice - it shows the value of this patch, and I like it.
>
> That's great!
>
> > One thing I've been wondering about is that currently amcheck (in
> > general, not just these new GIN/GiST functions) errors out on the first
> > issue, because it does ereport(ERROR). Which is good enough to decide if
> > there is some corruption, but a bit inconvenient if you need to assess
> > how much corruption is there. For example when investigating the issue
> > in my patch it would have been great to know if there's just one broken
> > page, or if there are dozens/hundreds/thousands of them.
> >
> > I'd imagine we could have a flag which says whether to fail on the first
> > issue, or keep looking at future pages. Essentially, whether to do
> > ereport(ERROR) or ereport(WARNING). But maybe that's a dead-end, and
> > once we find the first issue it's futile to inspect the rest of the
> > index, because it can be garbage. Not sure. In any case, it's not up to
> > this patch to invent that.
>
> The thing is amcheck tries hard to to do a core dump. It's still possible to crash it with garbage. But if we continue check after encountering first corruption - increase in SegFaults is inevitable.
>
>
> Thank you! I hope I can get back to code ASAP.
>
>
> Best regards, Andrey Borodin.
>
Hi!
I did mechanical patch rebase & beautification.

Notice my first patch, i did small refactoring as a separate contribution.

=== review from Tomas fixups

1) 0001-Refactor-amcheck-to-extract-common..

This change was not correct (if stmt now need parenthesis )

> - if (allequalimage && !_bt_allequalimage(indrel, false))
> - {
> - bool has_interval_ops = false;
> -
> - for (int i = 0; i < IndexRelationGetNumberOfKeyAttributes(indrel); i++)
> - if (indrel->rd_opfamily[i] == INTERVAL_BTREE_FAM_OID)
> - has_interval_ops = true;
> - ereport(ERROR,
> + for (int i = 0; i < IndexRelationGetNumberOfKeyAttributes(indrel); i++)
> + if (indrel->rd_opfamily[i] == INTERVAL_BTREE_FAM_OID)
> + has_interval_ops = true;
> + ereport(ERROR,

Applied all tomas review comments. The index relation AM mismatch
error message now looks like this:
```
db1=# select bt_index_check('users_search_idx');
ERROR:  expected "btree" index as targets for verification
DETAIL:  Relation "users_search_idx" is a gin index.
```
I included Tomas to review-by section of this patch (in the commit message).
I also changed the commit message for this patch.

2) 0002-Add-gist_index_check-function-to-verify-G.patch
Did apply Tomas review comments. I left GIST's version of
PageGetItemIdCareful unchanged. Maybe we should have a common check in
verify_common.c, as Tomas was arguing for, but I'm not doing anything
for now, because I don't really understand its purpose. All other
review comments are addressed (i hope), if i'm not missing anything.

I also included my fix for the memory leak mentioned by Tomas.

3) 003-Add gin_index_check() to verify GIN index
The only change is gin_index_parent_check() -> gin_index_check()

4) Applying: Add GiST support to pg_amcheck

Simply rebased & run pgident.

==== tests

make check runs with success


=== problems with gin_index_check

1)
```
reshke@ygp-jammy:~/postgres/contrib/amcheck$ ../../pgbin/bin/psql db1
psql (18devel)
Type "help" for help.

db1=# select gin_index_check('users_search_idx');
ERROR:  index "users_search_idx" has wrong tuple order, block 35868, offset 33
```

For some reason gin_index_check fails on my index. I am 99% there is
no corruption in it. Will try to investigate.

2) this is already discovered by Tomas, but I add my input here:


psql session:
```
db1=# set log_min_messages to debug5;
SET
db1=# select gin_index_check('users_search_idx');

```


gdb session:
```
(gdb) bt
#0  __pthread_kill_implementation (no_tid=0, signo=6,
threadid=140601454760896) at ./nptl/pthread_kill.c:44
#1  __pthread_kill_internal (signo=6, threadid=140601454760896) at
./nptl/pthread_kill.c:78
#2  __GI___pthread_kill (threadid=140601454760896,
signo=signo@entry=6) at ./nptl/pthread_kill.c:89
#3  0x00007fe055af0476 in __GI_raise (sig=sig@entry=6) at
../sysdeps/posix/raise.c:26
#4  0x00007fe055ad67f3 in __GI_abort () at ./stdlib/abort.c:79
#5  0x000055ea82af4ef0 in ExceptionalCondition
(conditionName=conditionName@entry=0x7fe04a87aa35
"ItemPointerIsValid(pointer)",
    fileName=fileName@entry=0x7fe04a87a928
"../../src/include/storage/itemptr.h",
lineNumber=lineNumber@entry=126) at assert.c:66
#6  0x00007fe04a871372 in ItemPointerGetOffsetNumber
(pointer=<optimized out>) at ../../src/include/storage/itemptr.h:126
#7  ItemPointerGetOffsetNumber (pointer=<optimized out>) at
../../src/include/storage/itemptr.h:124
#8  gin_check_posting_tree_parent_keys_consistency
(posting_tree_root=<optimized out>, rel=<optimized out>) at
verify_gin.c:296
#9  gin_check_parent_keys_consistency (rel=rel@entry=0x7fe04a8aa328,
heaprel=heaprel@entry=0x7fe04a8a9db8,
callback_state=callback_state@entry=0x0,
readonly=readonly@entry=false) at verify_gin.c:597
#10 0x00007fe04a87098d in amcheck_lock_relation_and_check
(indrelid=16488, am_id=am_id@entry=2742,
check=check@entry=0x7fe04a870a80 <gin_check_parent_keys_consistency>,
lockmode=lockmode@entry=1,
    state=state@entry=0x0) at verify_common.c:132
#11 0x00007fe04a871e34 in gin_index_check (fcinfo=<optimized out>) at
verify_gin.c:81
#12 0x000055ea827cc275 in ExecInterpExpr (state=0x55ea84903390,
econtext=0x55ea84903138, isnull=<optimized out>) at
execExprInterp.c:770
#13 0x000055ea82804fdc in ExecEvalExprSwitchContext
(isNull=0x7ffeba7fdd37, econtext=0x55ea84903138, state=0x55ea84903390)
at ../../../src/include/executor/executor.h:367
#14 ExecProject (projInfo=0x55ea84903388) at
../../../src/include/executor/executor.h:401
#15 ExecResult (pstate=<optimized out>) at nodeResult.c:135
#16 0x000055ea827d007a in ExecProcNode (node=0x55ea84903028) at
../../../src/include/executor/executor.h:278
#17 ExecutePlan (execute_once=<optimized out>, dest=0x55ea84901940,
direction=<optimized out>, numberTuples=0, sendTuples=<optimized out>,
operation=CMD_SELECT, use_parallel_mode=<optimized out>,
    planstate=0x55ea84903028, estate=0x55ea84902e00) at execMain.c:1655
#18 standard_ExecutorRun (queryDesc=0x55ea8485c1a0,
direction=<optimized out>, count=0, execute_once=<optimized out>) at
execMain.c:362
#19 0x000055ea829ad6df in PortalRunSelect (portal=0x55ea848b1810,
forward=<optimized out>, count=0, dest=<optimized out>) at
pquery.c:924
#20 0x000055ea829aedc1 in PortalRun
(portal=portal@entry=0x55ea848b1810,
count=count@entry=9223372036854775807,
isTopLevel=isTopLevel@entry=true, run_once=run_once@entry=true,
    dest=dest@entry=0x55ea84901940,
altdest=altdest@entry=0x55ea84901940, qc=0x7ffeba7fdfd0) at
pquery.c:768
#21 0x000055ea829aab47 in exec_simple_query
(query_string=0x55ea84831250 "select
gin_index_check('users_search_idx');") at postgres.c:1283
#22 0x000055ea829ac777 in PostgresMain (dbname=<optimized out>,
username=<optimized out>) at postgres.c:4798
#23 0x000055ea829a6a33 in BackendMain (startup_data=<optimized out>,
startup_data_len=<optimized out>) at backend_startup.c:107
#24 0x000055ea8290122f in postmaster_child_launch
(child_type=<optimized out>, child_slot=1,
startup_data=startup_data@entry=0x7ffeba7fe48c "",
startup_data_len=startup_data_len@entry=4,
    client_sock=client_sock@entry=0x7ffeba7fe490) at launch_backend.c:274
#25 0x000055ea82904c3f in BackendStartup (client_sock=0x7ffeba7fe490)
at postmaster.c:3377
#26 ServerLoop () at postmaster.c:1663
#27 0x000055ea8290656b in PostmasterMain (argc=argc@entry=3,
argv=argv@entry=0x55ea8482ab10) at postmaster.c:1361
#28 0x000055ea825ecc0a in main (argc=3, argv=0x55ea8482ab10) at main.c:196
(gdb)
```

We also need to change the default version of the extension to 1.5.
I'm not sure which patch of this series should do that.

====
Overall I think 0001 & 0002 are ready as-is. 0003 is maybe ok.  Other
patches need more review rounds.

-- 
Best regards,
Kirill Reshke

  52. Re: Amcheck verification of GiST and GIN

    Andrey Borodin <x4mmm@yandex-team.ru> — 2024-11-26T07:22:02Z

    
> On 26 Nov 2024, at 11:50, Kirill Reshke <reshkekirill@gmail.com> wrote:
> 
> I did mechanical patch rebase & beautification.

Many thanks! Addressing Tomas' feedback was still one of top items on my todo list. And I'm more than happy that someone advance this patchset.

> Notice my first patch, i did small refactoring as a separate contribution.

It looks like these days such patches are committed via separate threads. Change looks good to me.

> 
> === review from Tomas fixups
> 
> 1) 0001-Refactor-amcheck-to-extract-common..
> 
> This change was not correct (if stmt now need parenthesis )
> 
>> - if (allequalimage && !_bt_allequalimage(indrel, false))
>> - {
>> - bool has_interval_ops = false;
>> -
>> - for (int i = 0; i < IndexRelationGetNumberOfKeyAttributes(indrel); i++)
>> - if (indrel->rd_opfamily[i] == INTERVAL_BTREE_FAM_OID)
>> - has_interval_ops = true;
>> - ereport(ERROR,
>> + for (int i = 0; i < IndexRelationGetNumberOfKeyAttributes(indrel); i++)
>> + if (indrel->rd_opfamily[i] == INTERVAL_BTREE_FAM_OID)
>> + has_interval_ops = true;
>> + ereport(ERROR,
> 
> Applied all tomas review comments.
+1. All changes were correct, but there were some questions from Tomas.

> The index relation AM mismatch
> error message now looks like this:
> ```
> db1=# select bt_index_check('users_search_idx');
> ERROR:  expected "btree" index as targets for verification
> DETAIL:  Relation "users_search_idx" is a gin index.
> ```

Great!

> I included Tomas to review-by section of this patch (in the commit message).
> I also changed the commit message for this patch.
> 
> 2) 0002-Add-gist_index_check-function-to-verify-G.patch
> Did apply Tomas review comments. I left GIST's version of
> PageGetItemIdCareful unchanged. Maybe we should have a common check in
> verify_common.c, as Tomas was arguing for, but I'm not doing anything
> for now, because I don't really understand its purpose. All other
> review comments are addressed (i hope), if i'm not missing anything.
> 
> I also included my fix for the memory leak mentioned by Tomas.

The fix looks correct to me.

> === problems with gin_index_check
> 
> 1)
> ```
> reshke@ygp-jammy:~/postgres/contrib/amcheck$ ../../pgbin/bin/psql db1
> psql (18devel)
> Type "help" for help.
> 
> db1=# select gin_index_check('users_search_idx');
> ERROR:  index "users_search_idx" has wrong tuple order, block 35868, offset 33
> ```

Ughm... are you sure it's not induced by some collation issues? Did you create the index on same VM where test was performed?


> 
> For some reason gin_index_check fails on my index. I am 99% there is
> no corruption in it. Will try to investigate.
> 
> 2) this is already discovered by Tomas, but I add my input here:
> 
> 
> psql session:
> ```
> db1=# set log_min_messages to debug5;
> SET
> db1=# select gin_index_check('users_search_idx');
> 
> ```
> 
> 
> gdb session:
> ```
> (gdb) bt
> #0  __pthread_kill_implementation (no_tid=0, signo=6,
> threadid=140601454760896) at ./nptl/pthread_kill.c:44
> #1  __pthread_kill_internal (signo=6, threadid=140601454760896) at
> ./nptl/pthread_kill.c:78
> #2  __GI___pthread_kill (threadid=140601454760896,
> signo=signo@entry=6) at ./nptl/pthread_kill.c:89
> #3  0x00007fe055af0476 in __GI_raise (sig=sig@entry=6) at
> ../sysdeps/posix/raise.c:26
> #4  0x00007fe055ad67f3 in __GI_abort () at ./stdlib/abort.c:79
> #5  0x000055ea82af4ef0 in ExceptionalCondition
> (conditionName=conditionName@entry=0x7fe04a87aa35
> "ItemPointerIsValid(pointer)",
>    fileName=fileName@entry=0x7fe04a87a928
> "../../src/include/storage/itemptr.h",
> lineNumber=lineNumber@entry=126) at assert.c:66
> #6  0x00007fe04a871372 in ItemPointerGetOffsetNumber
> (pointer=<optimized out>) at ../../src/include/storage/itemptr.h:126
> #7  ItemPointerGetOffsetNumber (pointer=<optimized out>) at
> ../../src/include/storage/itemptr.h:124
> #8  gin_check_posting_tree_parent_keys_consistency
> (posting_tree_root=<optimized out>, rel=<optimized out>) at
> verify_gin.c:296
> #9  gin_check_parent_keys_consistency (rel=rel@entry=0x7fe04a8aa328,
> heaprel=heaprel@entry=0x7fe04a8a9db8,
> callback_state=callback_state@entry=0x0,
> readonly=readonly@entry=false) at verify_gin.c:597
> #10 0x00007fe04a87098d in amcheck_lock_relation_and_check
> (indrelid=16488, am_id=am_id@entry=2742,
> check=check@entry=0x7fe04a870a80 <gin_check_parent_keys_consistency>,
> lockmode=lockmode@entry=1,
>    state=state@entry=0x0) at verify_common.c:132
> #11 0x00007fe04a871e34 in gin_index_check (fcinfo=<optimized out>) at
> verify_gin.c:81
> #12 0x000055ea827cc275 in ExecInterpExpr (state=0x55ea84903390,
> econtext=0x55ea84903138, isnull=<optimized out>) at
> execExprInterp.c:770
> #13 0x000055ea82804fdc in ExecEvalExprSwitchContext
> (isNull=0x7ffeba7fdd37, econtext=0x55ea84903138, state=0x55ea84903390)
> at ../../../src/include/executor/executor.h:367
> #14 ExecProject (projInfo=0x55ea84903388) at
> ../../../src/include/executor/executor.h:401
> #15 ExecResult (pstate=<optimized out>) at nodeResult.c:135
> #16 0x000055ea827d007a in ExecProcNode (node=0x55ea84903028) at
> ../../../src/include/executor/executor.h:278
> #17 ExecutePlan (execute_once=<optimized out>, dest=0x55ea84901940,
> direction=<optimized out>, numberTuples=0, sendTuples=<optimized out>,
> operation=CMD_SELECT, use_parallel_mode=<optimized out>,
>    planstate=0x55ea84903028, estate=0x55ea84902e00) at execMain.c:1655
> #18 standard_ExecutorRun (queryDesc=0x55ea8485c1a0,
> direction=<optimized out>, count=0, execute_once=<optimized out>) at
> execMain.c:362
> #19 0x000055ea829ad6df in PortalRunSelect (portal=0x55ea848b1810,
> forward=<optimized out>, count=0, dest=<optimized out>) at
> pquery.c:924
> #20 0x000055ea829aedc1 in PortalRun
> (portal=portal@entry=0x55ea848b1810,
> count=count@entry=9223372036854775807,
> isTopLevel=isTopLevel@entry=true, run_once=run_once@entry=true,
>    dest=dest@entry=0x55ea84901940,
> altdest=altdest@entry=0x55ea84901940, qc=0x7ffeba7fdfd0) at
> pquery.c:768
> #21 0x000055ea829aab47 in exec_simple_query
> (query_string=0x55ea84831250 "select
> gin_index_check('users_search_idx');") at postgres.c:1283
> #22 0x000055ea829ac777 in PostgresMain (dbname=<optimized out>,
> username=<optimized out>) at postgres.c:4798
> #23 0x000055ea829a6a33 in BackendMain (startup_data=<optimized out>,
> startup_data_len=<optimized out>) at backend_startup.c:107
> #24 0x000055ea8290122f in postmaster_child_launch
> (child_type=<optimized out>, child_slot=1,
> startup_data=startup_data@entry=0x7ffeba7fe48c "",
> startup_data_len=startup_data_len@entry=4,
>    client_sock=client_sock@entry=0x7ffeba7fe490) at launch_backend.c:274
> #25 0x000055ea82904c3f in BackendStartup (client_sock=0x7ffeba7fe490)
> at postmaster.c:3377
> #26 ServerLoop () at postmaster.c:1663
> #27 0x000055ea8290656b in PostmasterMain (argc=argc@entry=3,
> argv=argv@entry=0x55ea8482ab10) at postmaster.c:1361
> #28 0x000055ea825ecc0a in main (argc=3, argv=0x55ea8482ab10) at main.c:196
> (gdb)
> ```
> 
> We also need to change the default version of the extension to 1.5.
> I'm not sure which patch of this series should do that.

+1 

> 
> ====
> Overall I think 0001 & 0002 are ready as-is. 0003 is maybe ok.  Other
> patches need more review rounds.

Yeah, I agree with this analysis.


Best regards, Andrey Borodin.

  53. Re: Amcheck verification of GiST and GIN

    Kirill Reshke <reshkekirill@gmail.com> — 2024-11-26T11:45:03Z

    On Tue, 26 Nov 2024 at 12:22, Andrey M. Borodin <x4mmm@yandex-team.ru> wrote:
>
>
>
> > On 26 Nov 2024, at 11:50, Kirill Reshke <reshkekirill@gmail.com> wrote:
> >
> > I did mechanical patch rebase & beautification.
>
> Many thanks! Addressing Tomas' feedback was still one of top items on my todo list. And I'm more than happy that someone advance this patchset.
>
> > Notice my first patch, i did small refactoring as a separate contribution.
>
> It looks like these days such patches are committed via separate threads. Change looks good to me.
>
> >
> > === review from Tomas fixups
> >
> > 1) 0001-Refactor-amcheck-to-extract-common..
> >
> > This change was not correct (if stmt now need parenthesis )
> >
> >> - if (allequalimage && !_bt_allequalimage(indrel, false))
> >> - {
> >> - bool has_interval_ops = false;
> >> -
> >> - for (int i = 0; i < IndexRelationGetNumberOfKeyAttributes(indrel); i++)
> >> - if (indrel->rd_opfamily[i] == INTERVAL_BTREE_FAM_OID)
> >> - has_interval_ops = true;
> >> - ereport(ERROR,
> >> + for (int i = 0; i < IndexRelationGetNumberOfKeyAttributes(indrel); i++)
> >> + if (indrel->rd_opfamily[i] == INTERVAL_BTREE_FAM_OID)
> >> + has_interval_ops = true;
> >> + ereport(ERROR,
> >
> > Applied all tomas review comments.
> +1. All changes were correct, but there were some questions from Tomas.
>
> > The index relation AM mismatch
> > error message now looks like this:
> > ```
> > db1=# select bt_index_check('users_search_idx');
> > ERROR:  expected "btree" index as targets for verification
> > DETAIL:  Relation "users_search_idx" is a gin index.
> > ```
>
> Great!
>
> > I included Tomas to review-by section of this patch (in the commit message).
> > I also changed the commit message for this patch.
> >
> > 2) 0002-Add-gist_index_check-function-to-verify-G.patch
> > Did apply Tomas review comments. I left GIST's version of
> > PageGetItemIdCareful unchanged. Maybe we should have a common check in
> > verify_common.c, as Tomas was arguing for, but I'm not doing anything
> > for now, because I don't really understand its purpose. All other
> > review comments are addressed (i hope), if i'm not missing anything.
> >
> > I also included my fix for the memory leak mentioned by Tomas.
>
> The fix looks correct to me.
>
> > === problems with gin_index_check
> >
> > 1)
> > ```
> > reshke@ygp-jammy:~/postgres/contrib/amcheck$ ../../pgbin/bin/psql db1
> > psql (18devel)
> > Type "help" for help.
> >
> > db1=# select gin_index_check('users_search_idx');
> > ERROR:  index "users_search_idx" has wrong tuple order, block 35868, offset 33
> > ```
>
> Ughm... are you sure it's not induced by some collation issues? Did you create the index on same VM where test was performed?
>


I have an input for this. I generated a big table with 2 md5 fields
and user binary search to find the exact border where gin_index_check
stops working.

```

db1=# create table users_3084 (like users INCLUDING INDEXES);
CREATE TABLE
db1=# insert into users_3084 select * from users order by first_name limit 3084;
INSERT 0 3084
db1=# select gin_index_check('users_3084_first_name_last_name_idx');
 gin_index_check
-----------------

(1 row)
.....
db1=# create table users_3085 (like users INCLUDING INDEXES);
CREATE TABLE
db1=# insert into users_3085 select * from users order by first_name limit 3085;
INSERT 0 3085
db1=# select gin_index_check('users_3085_first_name_last_name_idx');
ERROR:  index "users_3085_first_name_last_name_idx" has wrong tuple
order, block 589, offset 45
```


DDL:


CREATE TABLE public.users_3085 (
    first_name text,
    last_name text
);
CREATE INDEX users_3085_first_name_idx ON public.users_3085 USING hash
(first_name);
CREATE INDEX users_3085_first_name_last_name_idx ON public.users_3085
USING gin (first_name public.gin_trgm_ops, last_name
public.gin_trgm_ops);


PFA contents of users_3085


> >
> > For some reason gin_index_check fails on my index. I am 99% there is
> > no corruption in it. Will try to investigate.
> >
> > 2) this is already discovered by Tomas, but I add my input here:
> >
> >
> > psql session:
> > ```
> > db1=# set log_min_messages to debug5;
> > SET
> > db1=# select gin_index_check('users_search_idx');
> >
> > ```
> >
> >
> > gdb session:
> > ```
> > (gdb) bt
> > #0  __pthread_kill_implementation (no_tid=0, signo=6,
> > threadid=140601454760896) at ./nptl/pthread_kill.c:44
> > #1  __pthread_kill_internal (signo=6, threadid=140601454760896) at
> > ./nptl/pthread_kill.c:78
> > #2  __GI___pthread_kill (threadid=140601454760896,
> > signo=signo@entry=6) at ./nptl/pthread_kill.c:89
> > #3  0x00007fe055af0476 in __GI_raise (sig=sig@entry=6) at
> > ../sysdeps/posix/raise.c:26
> > #4  0x00007fe055ad67f3 in __GI_abort () at ./stdlib/abort.c:79
> > #5  0x000055ea82af4ef0 in ExceptionalCondition
> > (conditionName=conditionName@entry=0x7fe04a87aa35
> > "ItemPointerIsValid(pointer)",
> >    fileName=fileName@entry=0x7fe04a87a928
> > "../../src/include/storage/itemptr.h",
> > lineNumber=lineNumber@entry=126) at assert.c:66
> > #6  0x00007fe04a871372 in ItemPointerGetOffsetNumber
> > (pointer=<optimized out>) at ../../src/include/storage/itemptr.h:126
> > #7  ItemPointerGetOffsetNumber (pointer=<optimized out>) at
> > ../../src/include/storage/itemptr.h:124
> > #8  gin_check_posting_tree_parent_keys_consistency
> > (posting_tree_root=<optimized out>, rel=<optimized out>) at
> > verify_gin.c:296
> > #9  gin_check_parent_keys_consistency (rel=rel@entry=0x7fe04a8aa328,
> > heaprel=heaprel@entry=0x7fe04a8a9db8,
> > callback_state=callback_state@entry=0x0,
> > readonly=readonly@entry=false) at verify_gin.c:597
> > #10 0x00007fe04a87098d in amcheck_lock_relation_and_check
> > (indrelid=16488, am_id=am_id@entry=2742,
> > check=check@entry=0x7fe04a870a80 <gin_check_parent_keys_consistency>,
> > lockmode=lockmode@entry=1,
> >    state=state@entry=0x0) at verify_common.c:132
> > #11 0x00007fe04a871e34 in gin_index_check (fcinfo=<optimized out>) at
> > verify_gin.c:81
> > #12 0x000055ea827cc275 in ExecInterpExpr (state=0x55ea84903390,
> > econtext=0x55ea84903138, isnull=<optimized out>) at
> > execExprInterp.c:770
> > #13 0x000055ea82804fdc in ExecEvalExprSwitchContext
> > (isNull=0x7ffeba7fdd37, econtext=0x55ea84903138, state=0x55ea84903390)
> > at ../../../src/include/executor/executor.h:367
> > #14 ExecProject (projInfo=0x55ea84903388) at
> > ../../../src/include/executor/executor.h:401
> > #15 ExecResult (pstate=<optimized out>) at nodeResult.c:135
> > #16 0x000055ea827d007a in ExecProcNode (node=0x55ea84903028) at
> > ../../../src/include/executor/executor.h:278
> > #17 ExecutePlan (execute_once=<optimized out>, dest=0x55ea84901940,
> > direction=<optimized out>, numberTuples=0, sendTuples=<optimized out>,
> > operation=CMD_SELECT, use_parallel_mode=<optimized out>,
> >    planstate=0x55ea84903028, estate=0x55ea84902e00) at execMain.c:1655
> > #18 standard_ExecutorRun (queryDesc=0x55ea8485c1a0,
> > direction=<optimized out>, count=0, execute_once=<optimized out>) at
> > execMain.c:362
> > #19 0x000055ea829ad6df in PortalRunSelect (portal=0x55ea848b1810,
> > forward=<optimized out>, count=0, dest=<optimized out>) at
> > pquery.c:924
> > #20 0x000055ea829aedc1 in PortalRun
> > (portal=portal@entry=0x55ea848b1810,
> > count=count@entry=9223372036854775807,
> > isTopLevel=isTopLevel@entry=true, run_once=run_once@entry=true,
> >    dest=dest@entry=0x55ea84901940,
> > altdest=altdest@entry=0x55ea84901940, qc=0x7ffeba7fdfd0) at
> > pquery.c:768
> > #21 0x000055ea829aab47 in exec_simple_query
> > (query_string=0x55ea84831250 "select
> > gin_index_check('users_search_idx');") at postgres.c:1283
> > #22 0x000055ea829ac777 in PostgresMain (dbname=<optimized out>,
> > username=<optimized out>) at postgres.c:4798
> > #23 0x000055ea829a6a33 in BackendMain (startup_data=<optimized out>,
> > startup_data_len=<optimized out>) at backend_startup.c:107
> > #24 0x000055ea8290122f in postmaster_child_launch
> > (child_type=<optimized out>, child_slot=1,
> > startup_data=startup_data@entry=0x7ffeba7fe48c "",
> > startup_data_len=startup_data_len@entry=4,
> >    client_sock=client_sock@entry=0x7ffeba7fe490) at launch_backend.c:274
> > #25 0x000055ea82904c3f in BackendStartup (client_sock=0x7ffeba7fe490)
> > at postmaster.c:3377
> > #26 ServerLoop () at postmaster.c:1663
> > #27 0x000055ea8290656b in PostmasterMain (argc=argc@entry=3,
> > argv=argv@entry=0x55ea8482ab10) at postmaster.c:1361
> > #28 0x000055ea825ecc0a in main (argc=3, argv=0x55ea8482ab10) at main.c:196
> > (gdb)
> > ```
> >
> > We also need to change the default version of the extension to 1.5.
> > I'm not sure which patch of this series should do that.
>
> +1
>
> >
> > ====
> > Overall I think 0001 & 0002 are ready as-is. 0003 is maybe ok.  Other
> > patches need more review rounds.
>
> Yeah, I agree with this analysis.

I now don't: im leaning towards the option to squash 0001-0002 into one patch.

>
> Best regards, Andrey Borodin.



-- 
Best regards,
Kirill Reshke

  54. Re: Amcheck verification of GiST and GIN

    Kirill Reshke <reshkekirill@gmail.com> — 2024-11-29T13:57:42Z

    On Tue, 26 Nov 2024 at 11:50, Kirill Reshke <reshkekirill@gmail.com> wrote:
>
> ====
> Overall I think 0001 & 0002 are ready as-is. 0003 is maybe ok.  Other
> patches need more review rounds.
>
> --
> Best regards,
> Kirill Reshke

=== Patch changes.

Polishing:
1)

> + /* last tuple in layer has no high key */
> + if (i != maxoff && !GinPageGetOpaque(page)->rightlink)
> + {
> + ptr->parenttup = CopyIndexTuple(idxtuple);
> + }
> + else
> + {
> + ptr->parenttup = NULL;
> + }

This coding does not align with PostgreSQL style. I removed
parentheses here and in other few places :

```
reshke@ygp-jammy:~/postgres$ git diff contrib/amcheck/verify_gin.c
diff --git a/contrib/amcheck/verify_gin.c b/contrib/amcheck/verify_gin.c
index 47b6e81fbc4..be44cc724f8 100644
--- a/contrib/amcheck/verify_gin.c
+++ b/contrib/amcheck/verify_gin.c
@@ -111,9 +111,7 @@ ginReadTupleWithoutState(IndexTuple itup, int *nitems)
                                         nipd, ndecoded);
                }
                else
-               {
                        ipd = palloc(0);
-               }
        }
        else
        {
@@ -194,7 +192,6 @@
gin_check_posting_tree_parent_keys_consistency(Relation rel,
BlockNumber posting
                        list = GinDataLeafPageGetItems(page, &nlist, minItem);

                        if (nlist > 0)
-                       {
                                snprintf(tidrange_buf, sizeof(tidrange_buf),
                                                 "%d tids (%u, %u) - (%u, %u)",
                                                 nlist,
@@ -202,11 +199,8 @@
gin_check_posting_tree_parent_keys_consistency(Relation rel,
BlockNumber posting

ItemPointerGetOffsetNumberNoCheck(&list[0]),

ItemPointerGetBlockNumberNoCheck(&list[nlist - 1]),

ItemPointerGetOffsetNumberNoCheck(&list[nlist - 1]));
-                       }
                        else
-                       {
                                snprintf(tidrange_buf,
sizeof(tidrange_buf), "0 tids");
-                       }

                        if (stack->parentblk != InvalidBlockNumber)
                        {
@@ -218,11 +212,9 @@
gin_check_posting_tree_parent_keys_consistency(Relation rel,
BlockNumber posting
                                         tidrange_buf);
                        }
                        else
-                       {
                                elog(DEBUG3, "blk %u: root leaf, %s",
                                         stack->blkno,
                                         tidrange_buf);
-                       }

                        if (stack->parentblk != InvalidBlockNumber &&

ItemPointerGetOffsetNumberNoCheck(&stack->parentkey) !=
InvalidOffsetNumber &&
@@ -576,13 +568,9 @@ gin_check_parent_keys_consistency(Relation rel,
                                ptr->depth = stack->depth + 1;
                                /* last tuple in layer has no high key */
                                if (i != maxoff &&
!GinPageGetOpaque(page)->rightlink)
-                               {
                                        ptr->parenttup =
CopyIndexTuple(idxtuple);
-                               }
                                else
-                               {
                                        ptr->parenttup = NULL;
-                               }
                                ptr->parentblk = stack->blkno;
                                ptr->blkno = GinGetDownlink(idxtuple);
                                ptr->parentlsn = lsn;

```

2)

>+                                               else
>+                                               {
>+                                                       /*
>+                                                        * But now it is properly adjusted - nothing to do
>+                                                        * here.
>+                                                        */
>+                                               }

if (...) ... else {/* comment */} is a strange pattern.


PFA v6.
No other changes from v5 except for mandatory rebase of v5-0005 due to 18954ce.

=== CC changes
I changed Tomas's email to tomas@vondra.me, as @enterprisedb one no
longer exists.

-- 
Best regards,
Kirill Reshke

  55. Re: Amcheck verification of GiST and GIN

    Kirill Reshke <reshkekirill@gmail.com> — 2024-11-29T17:24:37Z

    On Tue, 26 Nov 2024 at 11:50, Kirill Reshke <reshkekirill@gmail.com> wrote:
> === problems with gin_index_check
>
> 1)
> ```
> reshke@ygp-jammy:~/postgres/contrib/amcheck$ ../../pgbin/bin/psql db1
> psql (18devel)
> Type "help" for help.
>
> db1=# select gin_index_check('users_search_idx');
> ERROR:  index "users_search_idx" has wrong tuple order, block 35868, offset 33
> ```
>
> For some reason gin_index_check fails on my index. I am 99% there is
> no corruption in it. Will try to investigate.
>
> 2) this is already discovered by Tomas, but I add my input here:
>
>
> psql session:
> ```
> db1=# set log_min_messages to debug5;
> SET
> db1=# select gin_index_check('users_search_idx');
>
> ```
>
>
> gdb session:
> ```
> (gdb) bt
> #0  __pthread_kill_implementation (no_tid=0, signo=6,
> threadid=140601454760896) at ./nptl/pthread_kill.c:44
> #1  __pthread_kill_internal (signo=6, threadid=140601454760896) at
> ./nptl/pthread_kill.c:78
> #2  __GI___pthread_kill (threadid=140601454760896,
> signo=signo@entry=6) at ./nptl/pthread_kill.c:89
> #3  0x00007fe055af0476 in __GI_raise (sig=sig@entry=6) at
> ../sysdeps/posix/raise.c:26
> #4  0x00007fe055ad67f3 in __GI_abort () at ./stdlib/abort.c:79
> #5  0x000055ea82af4ef0 in ExceptionalCondition
> (conditionName=conditionName@entry=0x7fe04a87aa35
> "ItemPointerIsValid(pointer)",
>     fileName=fileName@entry=0x7fe04a87a928
> "../../src/include/storage/itemptr.h",
> lineNumber=lineNumber@entry=126) at assert.c:66
> #6  0x00007fe04a871372 in ItemPointerGetOffsetNumber
> (pointer=<optimized out>) at ../../src/include/storage/itemptr.h:126
> #7  ItemPointerGetOffsetNumber (pointer=<optimized out>) at
> ../../src/include/storage/itemptr.h:124
> #8  gin_check_posting_tree_parent_keys_consistency
> (posting_tree_root=<optimized out>, rel=<optimized out>) at
> verify_gin.c:296
> #9  gin_check_parent_keys_consistency (rel=rel@entry=0x7fe04a8aa328,
> heaprel=heaprel@entry=0x7fe04a8a9db8,
> callback_state=callback_state@entry=0x0,
> readonly=readonly@entry=false) at verify_gin.c:597
> #10 0x00007fe04a87098d in amcheck_lock_relation_and_check
> (indrelid=16488, am_id=am_id@entry=2742,
> check=check@entry=0x7fe04a870a80 <gin_check_parent_keys_consistency>,
> lockmode=lockmode@entry=1,
>     state=state@entry=0x0) at verify_common.c:132
> #11 0x00007fe04a871e34 in gin_index_check (fcinfo=<optimized out>) at
> verify_gin.c:81
> #12 0x000055ea827cc275 in ExecInterpExpr (state=0x55ea84903390,
> econtext=0x55ea84903138, isnull=<optimized out>) at
> execExprInterp.c:770
> #13 0x000055ea82804fdc in ExecEvalExprSwitchContext
> (isNull=0x7ffeba7fdd37, econtext=0x55ea84903138, state=0x55ea84903390)
> at ../../../src/include/executor/executor.h:367
> #14 ExecProject (projInfo=0x55ea84903388) at
> ../../../src/include/executor/executor.h:401
> #15 ExecResult (pstate=<optimized out>) at nodeResult.c:135
> #16 0x000055ea827d007a in ExecProcNode (node=0x55ea84903028) at
> ../../../src/include/executor/executor.h:278
> #17 ExecutePlan (execute_once=<optimized out>, dest=0x55ea84901940,
> direction=<optimized out>, numberTuples=0, sendTuples=<optimized out>,
> operation=CMD_SELECT, use_parallel_mode=<optimized out>,
>     planstate=0x55ea84903028, estate=0x55ea84902e00) at execMain.c:1655
> #18 standard_ExecutorRun (queryDesc=0x55ea8485c1a0,
> direction=<optimized out>, count=0, execute_once=<optimized out>) at
> execMain.c:362
> #19 0x000055ea829ad6df in PortalRunSelect (portal=0x55ea848b1810,
> forward=<optimized out>, count=0, dest=<optimized out>) at
> pquery.c:924
> #20 0x000055ea829aedc1 in PortalRun
> (portal=portal@entry=0x55ea848b1810,
> count=count@entry=9223372036854775807,
> isTopLevel=isTopLevel@entry=true, run_once=run_once@entry=true,
>     dest=dest@entry=0x55ea84901940,
> altdest=altdest@entry=0x55ea84901940, qc=0x7ffeba7fdfd0) at
> pquery.c:768
> #21 0x000055ea829aab47 in exec_simple_query
> (query_string=0x55ea84831250 "select
> gin_index_check('users_search_idx');") at postgres.c:1283
> #22 0x000055ea829ac777 in PostgresMain (dbname=<optimized out>,
> username=<optimized out>) at postgres.c:4798
> #23 0x000055ea829a6a33 in BackendMain (startup_data=<optimized out>,
> startup_data_len=<optimized out>) at backend_startup.c:107
> #24 0x000055ea8290122f in postmaster_child_launch
> (child_type=<optimized out>, child_slot=1,
> startup_data=startup_data@entry=0x7ffeba7fe48c "",
> startup_data_len=startup_data_len@entry=4,
>     client_sock=client_sock@entry=0x7ffeba7fe490) at launch_backend.c:274
> #25 0x000055ea82904c3f in BackendStartup (client_sock=0x7ffeba7fe490)
> at postmaster.c:3377
> #26 ServerLoop () at postmaster.c:1663
> #27 0x000055ea8290656b in PostmasterMain (argc=argc@entry=3,
> argv=argv@entry=0x55ea8482ab10) at postmaster.c:1361
> #28 0x000055ea825ecc0a in main (argc=3, argv=0x55ea8482ab10) at main.c:196
> (gdb)
> ```
>
> We also need to change the default version of the extension to 1.5.
> I'm not sure which patch of this series should do that.

Both problems resolved with correct maxoff calculation:

```
reshke@ygp-jammy:~/postgres$ git diff contrib/amcheck/verify_gin.c
diff --git a/contrib/amcheck/verify_gin.c b/contrib/amcheck/verify_gin.c
index 39baae40f0c..ddf072d468d 100644
--- a/contrib/amcheck/verify_gin.c
+++ b/contrib/amcheck/verify_gin.c
@@ -412,7 +412,7 @@ gin_check_parent_keys_consistency(Relation rel,
                LockBuffer(buffer, GIN_SHARE);
                page = (Page) BufferGetPage(buffer);
                lsn = BufferGetLSNAtomic(buffer);
-               maxoff = PageGetMaxOffsetNumber(page);
+               maxoff = GinPageGetOpaque(page)->maxoff;

                /* Do basic sanity checks on the page headers */
                check_index_page(rel, buffer, stack->blkno);
```

-- 
Best regards,
Kirill Reshke

  56. Re: Amcheck verification of GiST and GIN

    Kirill Reshke <reshkekirill@gmail.com> — 2024-12-02T07:57:42Z

    On Fri, 29 Nov 2024 at 22:24, Kirill Reshke <reshkekirill@gmail.com> wrote:
ic(buffer);
> -               maxoff = PageGetMaxOffsetNumber(page);
> +               maxoff = GinPageGetOpaque(page)->maxoff;

This change was not correct at all.
PFA v32.

Little sketch of what's changed:
1) More debug1-debug3 output added.
2) Assertion failure under debug3 (pointed by Tomas Vonda) resolved
again. The fix is not to call ItemPointerGetOffsetNumber on rightmost
tuple, because,
ItemPointerGetOffsetNumber expects a valid pointer and rightmost tuple
pointer is (0, 0)
3) pgindent run


only 0004 was changed since v31. To clarify, gin_index_check still
does not work under some conditions. So, works on some types of
indexes and does not on others.
I think the issue is around the full entry page.

Repro:
```
db1=# create table ttt(t text);
CREATE TABLE
db1=# create index on ttt using gin(t gin_trgm_ops);
CREATE INDEX
db1=# insert into ttt select md5(random()::text) from generate_series(1,50000);
INSERT 0 50000
db1=# set client_min_messages to debug5;
DEBUG:  CommitTransaction(1) name: unnamed; blockState: STARTED;
state: INPROGRESS, xid/subid/cid: 0/1/0
SET
db1=# select gin_index_check('ttt_t_idx');
DEBUG:  StartTransaction(1) name: unnamed; blockState: DEFAULT; state:
INPROGRESS, xid/subid/cid: 0/1/0
DEBUG:  processing entry tree page at blk 1, maxoff: 2
DEBUG:  processing entry tree page at blk 941, maxoff: 229
ERROR:  index "ttt_t_idx" has wrong tuple order on entry tree page,
block 941, offset 229
db1=#
```
I have only observed failures on the last tuple of the entry page. All
other known issues that were on v31 are now fixed (I hope).

-- 
Best regards,
Kirill Reshke

  57. Re: Amcheck verification of GiST and GIN

    Kirill Reshke <reshkekirill@gmail.com> — 2024-12-03T15:04:08Z

    On Mon, 2 Dec 2024 at 12:57, Kirill Reshke <reshkekirill@gmail.com> wrote:
> This change was not correct at all.
> PFA v32.
>
> Repro:
> ```
> db1=# create table ttt(t text);
> CREATE TABLE
> db1=# create index on ttt using gin(t gin_trgm_ops);
> CREATE INDEX
> db1=# insert into ttt select md5(random()::text) from generate_series(1,50000);
> INSERT 0 50000
> db1=# set client_min_messages to debug5;
> DEBUG:  CommitTransaction(1) name: unnamed; blockState: STARTED;
> state: INPROGRESS, xid/subid/cid: 0/1/0
> SET
> db1=# select gin_index_check('ttt_t_idx');
> DEBUG:  StartTransaction(1) name: unnamed; blockState: DEFAULT; state:
> INPROGRESS, xid/subid/cid: 0/1/0
> DEBUG:  processing entry tree page at blk 1, maxoff: 2
> DEBUG:  processing entry tree page at blk 941, maxoff: 229
> ERROR:  index "ttt_t_idx" has wrong tuple order on entry tree page,
> block 941, offset 229
> db1=#
> ```
> I have only observed failures on the last tuple of the entry page. All
> other known issues that were on v31 are now fixed (I hope).

PFA v33.

Main change from v32 is a meson-related fix, and also bug fix that I
reported in a previous message.

I did find this in README.
```
GIN packs keys and downlinks into tuples in a different way.

(P_0, K_1), (P_1, K_2), ... , (P_n, K_{n+1})

P_i is grouped with K_{i+1}.  -Inf key is not needed.

There are couple of additional notes regarding K_{n+1} key.
1) In entry tree rightmost page, a key coupled with P_n doesn't really matter.
Highkey is assumed to be infinity.
2) In posting tree, a key coupled with P_n always doesn't matter.  Highkey for
non-rightmost pages is stored separately and accessed via
GinDataPageGetRightBound().
```
I indeed only observe gin_index_check failures only on the In entry
tree rightmost (non-leaf & non-root) page, on the last tuple
(Highkey). So fix was not to check is:


```
-                       /* (apparently) first block is metadata, skip
order check */
-                       if (i != FirstOffsetNumber && stack->blkno !=
(BlockNumber) 1)
+                       /*
+                        * First block is metadata, skip order check.
Also, never check
+                        * for high key on rightmost page, as this key
is not really
+                        * stored explicitly.
+                        */
+                       if (i != FirstOffsetNumber && stack->blkno !=
GIN_ROOT_BLKNO &&
+                               !(i == maxoff && rightlink ==
InvalidBlockNumber))
                        {
```

I also did a small wording correction for this part of README (this is v33-0006)

-- 
Best regards,
Kirill Reshke

  58. Re: Amcheck verification of GiST and GIN

    Kirill Reshke <reshkekirill@gmail.com> — 2024-12-03T16:04:59Z

    On Tue, 3 Dec 2024 at 20:04, Kirill Reshke <reshkekirill@gmail.com> wrote:
> PFA v33.
CF bot didn't like this version[0], for some reason i attached
v32-0002 as v33-0004.


Hope this version will be built ok.


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

-- 
Best regards,
Kirill Reshke

  59. Re: Amcheck verification of GiST and GIN

    Kirill Reshke <reshkekirill@gmail.com> — 2024-12-16T18:17:10Z

    Hi all.

I was reviewing nearby thread about parallel index creation for GIN,
and spotted this test [0] :

create table gin_t (a int[]);
insert into gin_t select * from rand_array(30000000, 0, 100, 0, 50);
create index on gin_t using gin(a);

v34 fails on this. The reason is we should never check the high key on
the rightmost page in the entry GIN tree explicitly, as this is not
actually stored.

PFA little v35 fix for this. I will update this thread with a full v35
patch set shortly.

[0] https://www.postgresql.org/message-id/87a5gyqnl5.fsf%40163.com


-- 
Best regards,
Kirill Reshke

  60. Re: Amcheck verification of GiST and GIN

    Tomas Vondra <tomas@vondra.me> — 2025-02-21T14:29:02Z

    Hi,

I see this patch didn't move since December :-( I still think these
improvements would be useful, it certainly was very helpful when I was
working on the GIN/GiST parallel builds (the GiST builds stalled, but I
hope to push the GIN patches soon).

So I'd like to get some of this in too. I'm not sure about the GiST
bits, because I know very little about that AM (the parallel builds made
me acutely aware of that).

But I'd like to get the GIN parts in. We're at v34 already, and the
recent changes were mostly cosmetic. Does anyone object to me polishing
and pushing those parts?


regards

-- 
Tomas Vondra

  61. Re: Amcheck verification of GiST and GIN

    Mark Dilger <mark.dilger@enterprisedb.com> — 2025-02-21T17:07:57Z

    
> On Feb 21, 2025, at 6:29 AM, Tomas Vondra <tomas@vondra.me> wrote:
> 
> Hi,
> 
> I see this patch didn't move since December :-( I still think these
> improvements would be useful, it certainly was very helpful when I was
> working on the GIN/GiST parallel builds (the GiST builds stalled, but I
> hope to push the GIN patches soon).
> 
> So I'd like to get some of this in too. I'm not sure about the GiST
> bits, because I know very little about that AM (the parallel builds made
> me acutely aware of that).
> 
> But I'd like to get the GIN parts in. We're at v34 already, and the
> recent changes were mostly cosmetic. Does anyone object to me polishing
> and pushing those parts?

I infer that you intend to make v34-0004, v34-0006, and v35-0001 apply cleanly without the other patches and commit it that way.  If that is correct, be advised that I'm doing a review and will respond back shortly, maybe in a few hours.

—
Mark Dilger
+001 (360) 271-8498
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  62. Re: Amcheck verification of GiST and GIN

    Tomas Vondra <tomas@vondra.me> — 2025-02-21T18:15:09Z

    On 2/21/25 18:07, Mark Dilger wrote:
> 
> 
>> On Feb 21, 2025, at 6:29 AM, Tomas Vondra <tomas@vondra.me> wrote:
>> 
>> Hi,
>> 
>> I see this patch didn't move since December :-( I still think
>> these improvements would be useful, it certainly was very helpful
>> when I was working on the GIN/GiST parallel builds (the GiST
>> builds stalled, but I hope to push the GIN patches soon).
>> 
>> So I'd like to get some of this in too. I'm not sure about the
>> GiST bits, because I know very little about that AM (the parallel
>> builds made me acutely aware of that).
>> 
>> But I'd like to get the GIN parts in. We're at v34 already, and
>> the recent changes were mostly cosmetic. Does anyone object to me
>> polishing and pushing those parts?
> 
> I infer that you intend to make v34-0004, v34-0006, and v35-0001
> apply cleanly without the other patches and commit it that way.  If
> that is correct, be advised that I'm doing a review and will respond
> back shortly, maybe in a few hours.
> 

Something like that. I haven't tried to actually massage the patches
yet, maybe it'd require some of the refactoring patches too.

But OK, I'll wait for the review, no problem. I wouldn't get to this
until Monday anyway.


regards

-- 
Tomas Vondra

  63. Re: Amcheck verification of GiST and GIN

    Andrey Borodin <x4mmm@yandex-team.ru> — 2025-02-21T18:22:28Z

    
> On 21 Feb 2025, at 19:29, Tomas Vondra <tomas@vondra.me> wrote:
> 
> I see this patch didn't move since December :-( I still think these
> improvements would be useful, it certainly was very helpful when I was
> working on the GIN/GiST parallel builds (the GiST builds stalled, but I
> hope to push the GIN patches soon).
> 
> So I'd like to get some of this in too. I'm not sure about the GiST
> bits, because I know very little about that AM (the parallel builds made
> me acutely aware of that).
> 
> But I'd like to get the GIN parts in. We're at v34 already, and the
> recent changes were mostly cosmetic. Does anyone object to me polishing
> and pushing those parts?

Hi Tomas!

Committing verification for any index type would help immensely. Currently we have many separate areas of work that just depend on this common part. GiST and GIN have a code for verification, which is bound together by this patch set. If someone, e.g., wants to work on BRIN - they have to deal with all the patch set.
If we have any second index in amcheck, no matter GiST or GIN, - it's clear how to split the work on other AMs.

Kirill spend a lot of time ironing out various false positives from GIN check. Kirill, what is your opinion about GIN verification? Does it look complete? (in a sense that it will not trigger false alarm, certainly it cannot catch all the type of corruptions)

Thanks!


Best regards, Andrey Borodin.

  64. Re: Amcheck verification of GiST and GIN

    Mark Dilger <mark.dilger@enterprisedb.com> — 2025-02-21T20:16:25Z

    
> On Feb 21, 2025, at 9:07 AM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> 
> I infer that you intend to make v34-0004, v34-0006, and v35-0001 apply cleanly without the other patches and commit it that way.  If that is correct, be advised that I'm doing a review and will respond back shortly, maybe in a few hours.

Ok, here is my review:

v34-0001 looks fine
v34-0002 refactoring is needed by the gin patches, so I kept it in the patchset for review purposes
v34-0004 can mostly be applied without v34-0003, but a few changes are needed to make it apply cleanly.
v34-0006 looks fine
v35-0001 applies cleanly

I find the token quoting and capitalization patterns in sql/check_gin.sql somewhat confusing, but I tried to follow what is already there in extending that test to also check gin indexes over jsonb data using jsonb_path_ops.  I think this is a common enough usage of gin that we should have test coverage for it.

After extending the test a bit, I ran the tests and checked lcov:

	verify_common.c	86.3%
	verify_gin.c		38.4%
	verify_heapam.c	57.2%
	verify_nbtree.c	72.4%

Showing that verify_gin has the least coverage of all.  The main areas lacking coverage have to do with posting list trees and concurrent page splits never being exercised.  My first attempt cover that with a TAP test using pgbench got the number up to 56.8%, but while trying to get that higher, I started getting error reports from verify_gin(), apparently out of function gin_check_parent_keys_consistency():

#   at t/006_gin_concurrency.pl line 137.
#                   'pgbench: error: client 14 script 1 aborted in command 0 query 0: ERROR:  index "ginidx" has wrong tuple order on entry tree page, block 153, offset 8
# pgbench: error: client 0 script 1 aborted in command 0 query 0: ERROR:  index "ginidx" has wrong tuple order on entry tree page, block 153, offset 8
# pgbench: error: client 12 script 1 aborted in command 0 query 0: ERROR:  index "ginidx" has wrong tuple order on entry tree page, block 153, offset 8
# pgbench: error: client 7 script 1 aborted in command 0 query 0: ERROR:  index "ginidx" has wrong tuple order on entry tree page, block 153, offset 8
# pgbench: error: client 1 script 1 aborted in command 0 query 0: ERROR:  index "ginidx" has wrong tuple order on entry tree page, block 153, offset 8

<MORE LINES LIKE THE ABOVE SNIPPED>

The pgbench script is not corrupting anything overtly, so this looks to either be a bug in gin or a bug in the check.  I am including a patchset with the original patches reworked plus the extra test cases.  For completeness, I also added gin indexes to t/002_cic.pl and t/003_cic_2pc.pl.

  65. Re: Amcheck verification of GiST and GIN

    Mark Dilger <mark.dilger@enterprisedb.com> — 2025-02-21T20:50:21Z

    
> On Feb 21, 2025, at 12:16 PM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> 
> The pgbench script is not corrupting anything overtly, so this looks to either be a bug in gin or a bug in the check. 

I suspected the AccessShareLock taken by verify_gin() might be too weak, and upgraded that to ShareRowExclusiveLock so as to prevent the concurrent table modifications (and incidentally other concurrent verify_gin() calls), but to my surprise that didn't fix anything.  Even AccessExclusiveLock doesn't fix it.  So this seems to either be a bug in the checking code complaining about perfectly valid tuple order, or a bug in Gin corrupting its own entry tree page.

On successive runs, (instrumented to print out a bit more info), there doesn't seem to be any obvious pattern in where the corruption occurs.  The offset in the page changes, neither always being at the beginning, nor always at the maxoff; likewise the block where corruption is detected changes from run to run.  I've noticed that the rightlink for the page is always the page's block number plus one, but that might just be that I haven't run enough iterations yet to see counter-examples.

Could one of the patch authors take a look?  I don't have the time to chase this to conclusion just now.  Thanks.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  66. Re: Amcheck verification of GiST and GIN

    Mark Dilger <mark.dilger@enterprisedb.com> — 2025-02-21T22:51:01Z

    
> On Feb 21, 2025, at 12:50 PM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> 
> Could one of the patch authors take a look? 

I turned the TAP test which triggers the error into a regression test that does likewise, for ease of stepping through the test, if anybody should want to do that.  I'm attaching that patch here, but please note that I'm not expecting this to be committed.

  67. Re: Amcheck verification of GiST and GIN

    Kirill Reshke <reshkekirill@gmail.com> — 2025-02-28T06:26:35Z

    On Sat, 22 Feb 2025 at 03:51, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
>
>
>
> > On Feb 21, 2025, at 12:50 PM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> >
> > Could one of the patch authors take a look?
>
> I turned the TAP test which triggers the error into a regression test that does likewise, for ease of stepping through the test, if anybody should want to do that.  I'm attaching that patch here, but please note that I'm not expecting this to be committed.

Hi!
Your efforts are much appreciated!
I used this patch to derive a smaller repro.

> this seems to either be a bug in the checking code complaining about perfectly valid tuple order,

I'm doubtful this is the case. I have added some more logging to
gin_index_check, and here is output after running attached:
```
DEBUG:  processing entry tree page at blk 2, maxoff: 125
....
DEBUG:  comparing for offset 78 category 0
DEBUG:  comparing for offset 79 category 2
DEBUG:  comparing for offset 80 category 3
DEBUG:  comparing for offset 81 category 0
LOG:  index "ginidx" has wrong tuple order on entry tree page, block
2, offset 81, rightlink 4294967295
DEBUG:  comparing for offset 82 category 0
....
DEBUG:  comparing for offset 100 category 0
DEBUG:  comparing for offset 101 category 2
DEBUG:  comparing for offset 102 category 3
DEBUG:  comparing for offset 103 category 0
LOG:  index "ginidx" has wrong tuple order on entry tree page, block
2, offset 103, rightlink 4294967295
DEBUG:  comparing for offset 104 category 0
DEBUG:  comparing for offset 105 category 0
```
So, we have an entry tree page, where there is tuple on offset 80,
with gin tuple category = 3, and then it goes category 0 again. And
one more such pattern on the same page.
The ginCompareEntries function compares the gin tuples category first.
I do not understand how this would be a valid order on the page, given
that
ginCompareEntries used in `ginget.c` logic. . Maybe I'm missing
something vital about GIN.


-- 
Best regards,
Kirill Reshke

  68. Re: Amcheck verification of GiST and GIN

    Mark Dilger <mark.dilger@enterprisedb.com> — 2025-02-28T18:31:29Z

    > So, we have an entry tree page, where there is tuple on offset 80,
> with gin tuple category = 3, and then it goes category 0 again. And
> one more such pattern on the same page.
> The ginCompareEntries function compares the gin tuples category first.
> I do not understand how this would be a valid order on the page, given
> that
> ginCompareEntries used in `ginget.c` logic. . Maybe I'm missing
> something vital about GIN.
>

The only obvious definition of "wrong" for this is that gin index scans
return different result sets than table scans over the same data.  Using
your much smaller reproducible test case, and adding rows like:

SELECT COUNT(*) FROM tbl WHERE j @>
'"1129BBCABFFAACA9VGVKipnwohaccc9TSIMTOQKHmcGYVeFE_PWKLHmpyj60137672qugtsstugg"'::jsonb;
SELECT COUNT(*) FROM tbl WHERE j @> '{"": "r", "hji4124": "",
"HTJP_DAptxn6": 9}'::jsonb;
SELECT COUNT(*) FROM tbl WHERE j @> '[]'::jsonb;
SELECT COUNT(*) FROM tbl WHERE j @> NULL::jsonb;
SELECT COUNT(*) FROM tbl WHERE j @> '{"": -6, "__": [""], "YMb":
-22}'::jsonb;
SELECT COUNT(*) FROM tbl WHERE j @> '{"853": -60, "pjx": "",
"TGLUG_jxmrggv": null}'::jsonb;
SELECT COUNT(*) FROM tbl WHERE j @>
'"D3BDA069074174vx48A37IVHWVXLUP9382542ypsl1465pixtryzCBgrkkhrvCC_BDDFatkyXHLIe"'::jsonb;
SELECT COUNT(*) FROM tbl WHERE j @> '{"F18s": {"": -84194}, "ececab2":
[""]}'::jsonb;

The results are the same with or without the index.  Can you find any
examples where they differ?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  69. Re: Amcheck verification of GiST and GIN

    Kirill Reshke <reshkekirill@gmail.com> — 2025-03-01T14:55:31Z

    On Fri, 28 Feb 2025 at 23:31, Mark Dilger <mark.dilger@enterprisedb.com> wrote:

> The only obvious definition of "wrong" for this is that gin index scans return different result sets than table scans over the same data.  Using your much smaller reproducible test case, and adding rows like:

Yeach, you are 100% right. Actually, along this thread, we have not
spotted any GIN bugs yet, only GIN amcheck bugs.

This turns out to be also an GIN amcheck bug:

```
DEBUG:  comparing for offset 79  category 2  key attnum 1
DEBUG:  comparing for offset 80  category 3  key attnum 1
DEBUG:  comparing for offset 81  category 0  key attnum 2
LOG:  index "ginidx" has wrong tuple order on entry tree page, block
2, offset 81, rightlink 4294967295
DEBUG:  comparing for offset 82  category 0  key attnum 2
....
DEBUG:  comparing for offset 100  category 0  key attnum 2
DEBUG:  comparing for offset 101  category 2  key attnum 2
DEBUG:  comparing for offset 102  category 3  key attnum 2
DEBUG:  comparing for offset 103  category 0  key attnum 3
LOG:  index "ginidx" has wrong tuple order on entry tree page, block
2, offset 103, rightlink 4294967295
DEBUG:  comparing for offset 104  category 0  key attnum 3
DEBUG:  comparing for offset 105  category 0  key attnum 3
```
Turns out we compare page entries for different attributes in
gin_check_parent_keys_consistency.

Trivial fix attached (see v37-0004). I now simply compare current and
prev attribute numbers. This revolves issue discovered by
`v0-0001-Add-a-reproducible-test-case-for-verify_gin-error.patch.no_apply`.
However, the stress test seems to still not pass. On my pc, it never
ens, all processes are in
DELETE waiting/UPDATE waiting state. I will take another look tomorrow.



p.s. I am just about to send this message, while i discovered we now
miss v34-0003-Add-gist_index_check-function-to-verify-GiST-ind.patch &
v34-0005-Add-GiST-support-to-pg_amcheck.patch from this patch series
;(

-- 
Best regards,
Kirill Reshke

  70. Re: Amcheck verification of GiST and GIN

    Mark Dilger <mark.dilger@enterprisedb.com> — 2025-03-27T15:30:35Z

    On Fri, Feb 21, 2025 at 6:29 AM Tomas Vondra <tomas@vondra.me> wrote:

> Hi,
>
> I see this patch didn't move since December :-( I still think these
> improvements would be useful, it certainly was very helpful when I was
> working on the GIN/GiST parallel builds (the GiST builds stalled, but I
> hope to push the GIN patches soon).
>
> So I'd like to get some of this in too. I'm not sure about the GiST
> bits, because I know very little about that AM (the parallel builds made
> me acutely aware of that).
>
> But I'd like to get the GIN parts in. We're at v34 already, and the
> recent changes were mostly cosmetic. Does anyone object to me polishing
> and pushing those parts?
>

Kirill may have addressed my concerns in the latest version.  I have not
had time for another review.  Tomas, would you still like to review and
push this patch?  I have no objection.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  71. Re: Amcheck verification of GiST and GIN

    Tomas Vondra <tomas@vondra.me> — 2025-03-27T16:14:00Z

    
On 3/27/25 16:30, Mark Dilger wrote:
> 
> 
> On Fri, Feb 21, 2025 at 6:29 AM Tomas Vondra <tomas@vondra.me
> <mailto:tomas@vondra.me>> wrote:
> 
>     Hi,
> 
>     I see this patch didn't move since December :-( I still think these
>     improvements would be useful, it certainly was very helpful when I was
>     working on the GIN/GiST parallel builds (the GiST builds stalled, but I
>     hope to push the GIN patches soon).
> 
>     So I'd like to get some of this in too. I'm not sure about the GiST
>     bits, because I know very little about that AM (the parallel builds made
>     me acutely aware of that).
> 
>     But I'd like to get the GIN parts in. We're at v34 already, and the
>     recent changes were mostly cosmetic. Does anyone object to me polishing
>     and pushing those parts?
> 
> 
> Kirill may have addressed my concerns in the latest version.  I have not
> had time for another review.  Tomas, would you still like to review and
> push this patch?  I have no objection.
> 

Thanks for reminding me. I think the patches are in good share, but I'll
take a look once more, and I hope to get it committed.


regards

-- 
Tomas Vondra

  72. Re: Amcheck verification of GiST and GIN

    Tomas Vondra <tomas@vondra.me> — 2025-03-28T16:26:34Z

    Here's a polished version of the patches. If you have any
comments/objections, please speak now. I don't plan to push 0006 (the
stress test), of course.

Changes I did:

1) update / write proper commit messages, hopefully explaining the
purpose of each patch well enough

2) update the lists of reviewers/authors (would appreciate someone
checking - it's hard to keep track for a thread that runs for years, and
it may not be quite clear what qualifies as a review)

3) squash the fix patch into the right patch, moved the README fix to be
the first patch (doesn't really matter)

4) minor cleanups in the main patches (0002 and 0003), mostly adding the
structs to typedefs.list and tweaking a couple comments

5) I've adjusted names of the memory contexts, because having both with
"amcheck context" seemed confusing, especially as it's in caller-callee
functions. So now it's

- amcheck consistency check context
- posting tree check context


regards

-- 
Tomas Vondra

  73. Re: Amcheck verification of GiST and GIN

    Kirill Reshke <reshkekirill@gmail.com> — 2025-03-28T19:51:41Z

    On Fri, 28 Mar 2025 at 21:26, Tomas Vondra <tomas@vondra.me> wrote:
>
> Here's a polished version of the patches. If you have any
> comments/objections, please speak now.
> --
> Tomas Vondra

Hi, no objections, lgtm


-- 
Best regards,
Kirill Reshke

  74. Re: Amcheck verification of GiST and GIN

    Tomas Vondra <tomas@vondra.me> — 2025-03-29T15:54:24Z

    On 3/28/25 20:51, Kirill Reshke wrote:
> On Fri, 28 Mar 2025 at 21:26, Tomas Vondra <tomas@vondra.me> wrote:
>>
>> Here's a polished version of the patches. If you have any
>> comments/objections, please speak now.
>> --
>> Tomas Vondra
> 
> Hi, no objections, lgtm
> 

I've pushed all the parts of this patch series, except for the stress
test - which I think was not meant for commit.

buildfarm seems happy so far, except for a minor indentation issue
(forgot to reindent after merging the separate fix patch).

Marked as committed in the CFA - that's not entirely correct, because
the original patch series also included amcheck support for GiST, but
that was not committed. I suggest we open a new CF entry if that gets
resubmitted for PG19 (I hope that will be the case).



Thanks for the patches, reviews, etc.!


-- 
Tomas Vondra

  75. Re: Amcheck verification of GiST and GIN

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-03-30T04:04:52Z

    Tomas Vondra <tomas@vondra.me> writes:
> I've pushed all the parts of this patch series, except for the stress
> test - which I think was not meant for commit.
> buildfarm seems happy so far, except for a minor indentation issue
> (forgot to reindent after merging the separate fix patch).

Not so much here:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=gecko&dt=2025-03-29%2020%3A25%3A23

Need to avoid depending on md5(), or it'll fail on machines with
FIPS mode enabled.  See for example 95b856de2.

			regards, tom lane

  76. Re: Amcheck verification of GiST and GIN

    Tomas Vondra <tomas@vondra.me> — 2025-03-30T12:14:09Z

    On 3/30/25 06:04, Tom Lane wrote:
> Tomas Vondra <tomas@vondra.me> writes:
>> I've pushed all the parts of this patch series, except for the stress
>> test - which I think was not meant for commit.
>> buildfarm seems happy so far, except for a minor indentation issue
>> (forgot to reindent after merging the separate fix patch).
> 
> Not so much here:
> 
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=gecko&dt=2025-03-29%2020%3A25%3A23
> 
> Need to avoid depending on md5(), or it'll fail on machines with
> FIPS mode enabled.  See for example 95b856de2.
> 

Ah, I forgot about that. Fixed.


regards

-- 
Tomas Vondra

  77. Re: Amcheck verification of GiST and GIN

    Arseniy Mukhin <arseniy.mukhin.dev@gmail.com> — 2025-05-09T12:43:46Z

    Hello,

Thanks everybody for the patch.

I noticed there are no tests that GIN check fails if the index is
corrupted, so I thought it would be great to have some.
While writing tests I noticed some issues in the patch (all issues are
for verify_gin.c)

1) When we add new items to the entry tree stack, ptr->parenttup is always null
because GinPageGetOpaque(page)->rightlink is never NULL.

             /* last tuple in layer has no high key */
                if (i != maxoff && !GinPageGetOpaque(page)->rightlink)
                    ptr->parenttup = CopyIndexTuple(idxtuple);
                else
                    ptr->parenttup = NULL;

Right way to check if entry doesn't have right neighbour is

            GinPageGetOpaque(page)->rightlink == InvalidBlockNumber

But even if we fix it, the condition would not do what the comment
says. If we want to have NULL as parenttup
only for the last tuple of the btree layer, the right check would be:

                if (i == maxoff && rightlink == InvalidBlockNumber)
                    ptr->parenttup = NULL;
             else
                    ptr->parenttup = CopyIndexTuple(idxtuple);

2) Check don't use attnum in comparisons, but for multicolumn indexes
attnum defines order. When we compare max entry page key
with parent key we ignore attnum. It means we occasionally can try to
compare keys of different columns.
While checking order within the same page we skip checking order for
tuples with different attnums now,
but it can be checked. Fix is easy: using ginCompareAttEntries()
instead of ginCompareEntries().


3) Here a code of the split detection

       if (rightlink != InvalidBlockNumber &&
             ginCompareEntries(&state, attnum, page_max_key,
                           page_max_key_category, parent_key,
                           parent_key_category) > 0)
          {
             /* split page detected, install right link to the stack */

Condition seems not right, because the child page max item key never
can be bigger then parent key.
It can be equal to the parentkey, and it means that there was no split
and the parent key that we cached in the stack is still
relevant. Or it could be less then cached parent key and it means that
split took place and old max item key moved to the
right neighbour and current page max item key should be less then
cached parent key. So I think we should replace > with <.

4) Here is the code for checking the order within the entry page.

           /*
           * First block is metadata, skip order check. Also, never check
           * for high key on rightmost page, as this key is not really
           * stored explicitly.
           *
           * Also make sure to not compare entries for different attnums,
           * which may be stored on the same page.
           */
          if (i != FirstOffsetNumber && attnum == prev_attnum &&
stack->blkno != GIN_ROOT_BLKNO &&
             !(i == maxoff && rightlink == InvalidBlockNumber))
          {
             prev_key = gintuple_get_key(&state, prev_tuple,
&prev_key_category);
             if (ginCompareEntries(&state, attnum, prev_key,
                              prev_key_category, current_key,
                              current_key_category) >= 0)

We skip checking the order for the root page, it's not clear why.
Probably there is some mess with the meta page, because
comment says "First block is metadata, skip order check". So I think
we can remove

                    stack->blkno != GIN_ROOT_BLKNO

5) The same place as 4). We skip checking the order for the high key
on the rightmost page, as this key is not really stored explicitly,
but for leaf pages all keys are stored explicitly, so we can check the
order for the last item of the leaf page too.
So I think we can change the condition to this:

            !(i == maxoff && rightlink == InvalidBlockNumber &&
!GinPageIsLeaf(page))

6) In posting tree parent key check part:

              /*
              * Check if this tuple is consistent with the downlink in the
              * parent.
              */
             if (stack->parentblk != InvalidBlockNumber && i == maxoff &&
                ItemPointerCompare(&stack->parentkey, &posting_item->key) < 0)
                ereport(ERROR,
                      ...
Here we don't check if stack->parentkey is valid, so sometimes we
compare invalid parentkey (because we can have
valid parentblk and invalid parentkey the same time). Invalid
parentkey is always bigger, so the code never triggers
ereport, but it doesn't look right. so probably we can rewrite it this way:

                if (i == maxoff && ItemPointerIsValid(&stack->parentkey) &&
                ItemPointerCompare(&stack->parentkey, &posting_item->key) < 0)

7) When producing stack entries for posting tree check, we set parent
key like this:

             /*
              * Set rightmost parent key to invalid item pointer. Its value
              * is 'Infinity' and not explicitly stored.
              */
             if (rightlink == InvalidBlockNumber)
                ItemPointerSetInvalid(&ptr->parentkey);
             else
                ptr->parentkey = posting_item->key;

We set invalid parent key for all items of the rightmost page. But
it's the only rightmost item that doesn't have an explicit
parentkey (actually the comment says exactly this, but the code does a
different thing). All others have an explicit parent
key and we can set it. So fix can look like this:

                if (rightlink == InvalidBlockNumber && i == maxoff)
                ItemPointerSetInvalid(&ptr->parentkey);
             else
                ptr->parentkey = posting_item->key;

But for (rightlink == InvalidBlockNumber && i == maxoff)
posting_item->key is always (0,0) (we check it a little bit earlier),
so I think we can simplify it:

                ptr->parentkey = posting_item->key;

8) In the function gin_refind_parent() the code

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

triggers Assert(ItemPointerIsValid(pointer)) within the
ItemPointerGetBlockNumber(), because itup->t_tid could be invalid.
AFAIS GIN code uses a special method GinGetDownlink(itup) that avoids
this Assert. So we can use it here too.

Please find the attached patch with fixes for issues above. Also there
are added regression test for multicolumn index,
and several tap tests with some basic corrupted index cases. I'm not
sure if it's the right way to write such tests and would
be glad to hear any feedback, especially about
invalid_entry_columns_order_test() where it seems important to
preserve
byte ordering. Also all tests expect standard page size 8192 now.


Also there are several points that I think also worth addressing:

9) Field 'parentlsn' is set but never actually used for any check. Or
I missed something.

10) README says "Vacuum never deletes tuples or pages from the entry
tree." But check assumes that it's possible to have
deleted leaf page with 0 entries.

    if (GinPageIsDeleted(page))
    {
       if (!GinPageIsLeaf(page))
          ereport(ERROR,
                (errcode(ERRCODE_INDEX_CORRUPTED),
                 errmsg("index \"%s\" has deleted internal page %u",
                      RelationGetRelationName(rel), blockNo)));
       if (PageGetMaxOffsetNumber(page) > InvalidOffsetNumber)
          ereport(ERROR,
                (errcode(ERRCODE_INDEX_CORRUPTED),
                 errmsg("index \"%s\" has deleted page %u with tuples",
                      RelationGetRelationName(rel), blockNo)));
    }
11) When we compare entry tree max page key with parent key:

             if (ginCompareAttEntries(&state, attnum, current_key,
                              current_key_category, parent_key_attnum,
                                      parent_key, parent_key_category) > 0)
             {
                /*
                 * 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.
                 */
                pfree(stack->parenttup);
                stack->parenttup = gin_refind_parent(rel, stack->parentblk,
                                            stack->blkno, strategy);

I think we can remove gin_refind_parent() and do ereport right away here.
The same logic as with 3). AFAIK it's impossible to have a child item
with a key that is higher than the cached parent key.
Parent key bounds what keys we can insert into the child page, so it
seems there is no way how they can appear there.


Best regards,
Arseniy Mukhin

  78. Re: Amcheck verification of GiST and GIN

    Tomas Vondra <tomas@vondra.me> — 2025-05-09T14:22:44Z

    On 5/9/25 14:43, Arseniy Mukhin wrote:
> Hello,
> 
> Thanks everybody for the patch.
> 
> I noticed there are no tests that GIN check fails if the index is
> corrupted, so I thought it would be great to have some.
> While writing tests I noticed some issues in the patch (all issues are
> for verify_gin.c)
> 
> 1) When we add new items to the entry tree stack, ptr->parenttup is always null
> because GinPageGetOpaque(page)->rightlink is never NULL.
> 
>              /* last tuple in layer has no high key */
>                 if (i != maxoff && !GinPageGetOpaque(page)->rightlink)
>                     ptr->parenttup = CopyIndexTuple(idxtuple);
>                 else
>                     ptr->parenttup = NULL;
> 
> Right way to check if entry doesn't have right neighbour is
> 
>             GinPageGetOpaque(page)->rightlink == InvalidBlockNumber
> 
> But even if we fix it, the condition would not do what the comment
> says. If we want to have NULL as parenttup
> only for the last tuple of the btree layer, the right check would be:
> 
>                 if (i == maxoff && rightlink == InvalidBlockNumber)
>                     ptr->parenttup = NULL;
>              else
>                     ptr->parenttup = CopyIndexTuple(idxtuple);
> 
> 2) Check don't use attnum in comparisons, but for multicolumn indexes
> attnum defines order. When we compare max entry page key
> with parent key we ignore attnum. It means we occasionally can try to
> compare keys of different columns.
> While checking order within the same page we skip checking order for
> tuples with different attnums now,
> but it can be checked. Fix is easy: using ginCompareAttEntries()
> instead of ginCompareEntries().
> 
> 
> 3) Here a code of the split detection
> 
>        if (rightlink != InvalidBlockNumber &&
>              ginCompareEntries(&state, attnum, page_max_key,
>                            page_max_key_category, parent_key,
>                            parent_key_category) > 0)
>           {
>              /* split page detected, install right link to the stack */
> 
> Condition seems not right, because the child page max item key never
> can be bigger then parent key.
> It can be equal to the parentkey, and it means that there was no split
> and the parent key that we cached in the stack is still
> relevant. Or it could be less then cached parent key and it means that
> split took place and old max item key moved to the
> right neighbour and current page max item key should be less then
> cached parent key. So I think we should replace > with <.
> 
> 4) Here is the code for checking the order within the entry page.
> 
>            /*
>            * First block is metadata, skip order check. Also, never check
>            * for high key on rightmost page, as this key is not really
>            * stored explicitly.
>            *
>            * Also make sure to not compare entries for different attnums,
>            * which may be stored on the same page.
>            */
>           if (i != FirstOffsetNumber && attnum == prev_attnum &&
> stack->blkno != GIN_ROOT_BLKNO &&
>              !(i == maxoff && rightlink == InvalidBlockNumber))
>           {
>              prev_key = gintuple_get_key(&state, prev_tuple,
> &prev_key_category);
>              if (ginCompareEntries(&state, attnum, prev_key,
>                               prev_key_category, current_key,
>                               current_key_category) >= 0)
> 
> We skip checking the order for the root page, it's not clear why.
> Probably there is some mess with the meta page, because
> comment says "First block is metadata, skip order check". So I think
> we can remove
> 
>                     stack->blkno != GIN_ROOT_BLKNO
> 
> 5) The same place as 4). We skip checking the order for the high key
> on the rightmost page, as this key is not really stored explicitly,
> but for leaf pages all keys are stored explicitly, so we can check the
> order for the last item of the leaf page too.
> So I think we can change the condition to this:
> 
>             !(i == maxoff && rightlink == InvalidBlockNumber &&
> !GinPageIsLeaf(page))
> 
> 6) In posting tree parent key check part:
> 
>               /*
>               * Check if this tuple is consistent with the downlink in the
>               * parent.
>               */
>              if (stack->parentblk != InvalidBlockNumber && i == maxoff &&
>                 ItemPointerCompare(&stack->parentkey, &posting_item->key) < 0)
>                 ereport(ERROR,
>                       ...
> Here we don't check if stack->parentkey is valid, so sometimes we
> compare invalid parentkey (because we can have
> valid parentblk and invalid parentkey the same time). Invalid
> parentkey is always bigger, so the code never triggers
> ereport, but it doesn't look right. so probably we can rewrite it this way:
> 
>                 if (i == maxoff && ItemPointerIsValid(&stack->parentkey) &&
>                 ItemPointerCompare(&stack->parentkey, &posting_item->key) < 0)
> 
> 7) When producing stack entries for posting tree check, we set parent
> key like this:
> 
>              /*
>               * Set rightmost parent key to invalid item pointer. Its value
>               * is 'Infinity' and not explicitly stored.
>               */
>              if (rightlink == InvalidBlockNumber)
>                 ItemPointerSetInvalid(&ptr->parentkey);
>              else
>                 ptr->parentkey = posting_item->key;
> 
> We set invalid parent key for all items of the rightmost page. But
> it's the only rightmost item that doesn't have an explicit
> parentkey (actually the comment says exactly this, but the code does a
> different thing). All others have an explicit parent
> key and we can set it. So fix can look like this:
> 
>                 if (rightlink == InvalidBlockNumber && i == maxoff)
>                 ItemPointerSetInvalid(&ptr->parentkey);
>              else
>                 ptr->parentkey = posting_item->key;
> 
> But for (rightlink == InvalidBlockNumber && i == maxoff)
> posting_item->key is always (0,0) (we check it a little bit earlier),
> so I think we can simplify it:
> 
>                 ptr->parentkey = posting_item->key;
> 
> 8) In the function gin_refind_parent() the code
> 
>                 if (ItemPointerGetBlockNumber(&(itup->t_tid)) == childblkno)
> 
> triggers Assert(ItemPointerIsValid(pointer)) within the
> ItemPointerGetBlockNumber(), because itup->t_tid could be invalid.
> AFAIS GIN code uses a special method GinGetDownlink(itup) that avoids
> this Assert. So we can use it here too.
> 
> Please find the attached patch with fixes for issues above. Also there
> are added regression test for multicolumn index,
> and several tap tests with some basic corrupted index cases. I'm not
> sure if it's the right way to write such tests and would
> be glad to hear any feedback, especially about
> invalid_entry_columns_order_test() where it seems important to
> preserve
> byte ordering. Also all tests expect standard page size 8192 now.
> 
> 
> Also there are several points that I think also worth addressing:
> 
> 9) Field 'parentlsn' is set but never actually used for any check. Or
> I missed something.
> 
> 10) README says "Vacuum never deletes tuples or pages from the entry
> tree." But check assumes that it's possible to have
> deleted leaf page with 0 entries.
> 
>     if (GinPageIsDeleted(page))
>     {
>        if (!GinPageIsLeaf(page))
>           ereport(ERROR,
>                 (errcode(ERRCODE_INDEX_CORRUPTED),
>                  errmsg("index \"%s\" has deleted internal page %u",
>                       RelationGetRelationName(rel), blockNo)));
>        if (PageGetMaxOffsetNumber(page) > InvalidOffsetNumber)
>           ereport(ERROR,
>                 (errcode(ERRCODE_INDEX_CORRUPTED),
>                  errmsg("index \"%s\" has deleted page %u with tuples",
>                       RelationGetRelationName(rel), blockNo)));
>     }
> 11) When we compare entry tree max page key with parent key:
> 
>              if (ginCompareAttEntries(&state, attnum, current_key,
>                               current_key_category, parent_key_attnum,
>                                       parent_key, parent_key_category) > 0)
>              {
>                 /*
>                  * 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.
>                  */
>                 pfree(stack->parenttup);
>                 stack->parenttup = gin_refind_parent(rel, stack->parentblk,
>                                             stack->blkno, strategy);
> 
> I think we can remove gin_refind_parent() and do ereport right away here.
> The same logic as with 3). AFAIK it's impossible to have a child item
> with a key that is higher than the cached parent key.
> Parent key bounds what keys we can insert into the child page, so it
> seems there is no way how they can appear there.
> 

These look like good points. I've added it to open items so that we
don't forget about this, I won't have time to look at this until after
pgconf.dev.


thanks

-- 
Tomas Vondra

  79. Re: Amcheck verification of GiST and GIN

    Tomas Vondra <tomas@vondra.me> — 2025-05-26T10:27:45Z

    Hello Arseniy,

I finally got time to look at this more closely, and do some testing.

Are there any cases when the current code incorrectly reports corruption
for a valid index? So far I've been unable to find such case. Or am I wrong?

It seems to me all the proposed changes are "tightening" the checks, in
the sense that we might have missed certain types of issues before. This
is supported by the fact that the new TAP test fails on master, i.e.
master does not report the corruption the TAP introduces.

(The TAP test is great, it would have been great to add something like
this in the original commit.)

Also, I've noticed that the TAP test passes even with some (most) of the
verify_gin.c changes reverted. See the 0002 patch - this does not break
the TAP test. Of course, that does not prove the changes are wrong and
I'm not claiming that. But can we improve the TAP test to trigger this
too? To show the current code (in master) misses this?

Grigory, Andrey, Heikki, any opinions on the tweaks?


regards

-- 
Tomas Vondra

  80. Re: Amcheck verification of GiST and GIN

    Arseniy Mukhin <arseniy.mukhin.dev@gmail.com> — 2025-05-26T16:28:47Z

    Hello Tomas,

On Mon, May 26, 2025 at 1:27 PM Tomas Vondra <tomas@vondra.me> wrote:

>
> Hello Arseniy,
>
> I finally got time to look at this more closely, and do some testing.

Thank you for looking into this.

> Are there any cases when the current code incorrectly reports corruption
> for a valid index? So far I've been unable to find such case. Or am I wrong?

I think you are right, I'm not aware of such cases either.

> It seems to me all the proposed changes are "tightening" the checks, in
> the sense that we might have missed certain types of issues before. This
> is supported by the fact that the new TAP test fails on master, i.e.
> master does not report the corruption the TAP introduces.

I would say points 4, 5, 7 - yes, they are about tightening checks.

I think point 1 is more about fixing the existing code. In the current code,
parent_key is always NULL for the entry tree, so a bunch of code
(related to checking consistency between parents and children) is unreachable.

Then if you apply changes of the 1st point and parent_key comparison code starts
working, you will need changes of the 2nd point. The current code
ignores attribute
numbers in parent_key check, which can lead to comparing keys of
different columns.
I see one scenario where it can happen: let's say we have a 2 column
index. The first
attribute type is "int", the second attribute type is "text". In the
multicolumn gin
index tuple has two parts: attno and key value. Let's write it as
(attno, key). While
traversing the entry tree the current code caches parent keys with child blkno.
Let's say it cached (2, "a") parent key. It means that there was a
time when the child
page's high key was (2, "a"). But when the child page check actually
starts, it's possible
that as a result of parallel splits, the child page now contains keys
of the first
attribute only, for example (1, 1), (1, 5), (1, 10). So if we ignore
the attribute
number here, we will end up comparing 10 with "a". Hope the example is
not too confusing.

The 3rd point is about the code that never runs. As I understood it is
supposed that the check detects
splits so we can check more index pages, but If I'm not wrong it
doesn't work now.

The 6th point is about comparison with invalid pointer. I thought that
it's probably
not right to compare it with invalid pointer, but now I'm not sure.

> (The TAP test is great, it would have been great to add something like
> this in the original commit.)

Great, thank you for the feedback.

> Also, I've noticed that the TAP test passes even with some (most) of the
> verify_gin.c changes reverted. See the 0002 patch - this does not break
> the TAP test. Of course, that does not prove the changes are wrong and
> I'm not claiming that. But can we improve the TAP test to trigger this
> too? To show the current code (in master) misses this?

Yes, changes in the undo patch is about posting tree check part (6, 7 points)
and I haven't written tests for it, because to break posting tree you need to
manipulate with tids which is not as easy as replace "aaaa" with "cccc" as tests
for entry tree do. Probably it would be much easier to use page api to
corrupt some
posting tree pages, but I don't know, is it impossible in TAP tests?

  81. Re: Amcheck verification of GiST and GIN

    Arseniy Mukhin <arseniy.mukhin.dev@gmail.com> — 2025-05-29T11:53:32Z

    On Mon, May 26, 2025 at 7:28 PM Arseniy Mukhin
<arseniy.mukhin.dev@gmail.com> wrote:
> On Mon, May 26, 2025 at 1:27 PM Tomas Vondra <tomas@vondra.me> wrote:
> > Also, I've noticed that the TAP test passes even with some (most) of the
> > verify_gin.c changes reverted. See the 0002 patch - this does not break
> > the TAP test. Of course, that does not prove the changes are wrong and
> > I'm not claiming that. But can we improve the TAP test to trigger this
> > too? To show the current code (in master) misses this?
>
> Yes, changes in the undo patch is about posting tree check part (6, 7 points)
> and I haven't written tests for it, because to break posting tree you need to
> manipulate with tids which is not as easy as replace "aaaa" with "cccc" as tests
> for entry tree do. Probably it would be much easier to use page api to
> corrupt some
> posting tree pages, but I don't know, is it impossible in TAP tests?

I added the test for the posting tree parent_key check. Now applying
'undo patch' results in
a test failure.
Also I realized that the test 'invalid_entry_columns_order_test' will
fail on big endian machines,
because varlena len encoding is different for little endian and big
endian, so I changed the test a little bit.
Now the test doesn't use varlena len byte in regex.
I also remove the blksize hardcode and start getting it from the
cluster configuration. But anyway some tests
will fail with not standard block size (probably all tests where tree
growth is expected).


Best regards,
Arseniy Mukhin

  82. Re: Amcheck verification of GiST and GIN

    Tomas Vondra <tomas@vondra.me> — 2025-06-08T22:14:58Z

    On 5/29/25 13:53, Arseniy Mukhin wrote:
> On Mon, May 26, 2025 at 7:28 PM Arseniy Mukhin
> <arseniy.mukhin.dev@gmail.com> wrote:
>> On Mon, May 26, 2025 at 1:27 PM Tomas Vondra <tomas@vondra.me> wrote:
>>> Also, I've noticed that the TAP test passes even with some (most) of the
>>> verify_gin.c changes reverted. See the 0002 patch - this does not break
>>> the TAP test. Of course, that does not prove the changes are wrong and
>>> I'm not claiming that. But can we improve the TAP test to trigger this
>>> too? To show the current code (in master) misses this?
>>
>> Yes, changes in the undo patch is about posting tree check part (6, 7 points)
>> and I haven't written tests for it, because to break posting tree you need to
>> manipulate with tids which is not as easy as replace "aaaa" with "cccc" as tests
>> for entry tree do. Probably it would be much easier to use page api to
>> corrupt some
>> posting tree pages, but I don't know, is it impossible in TAP tests?
> 
> I added the test for the posting tree parent_key check. Now applying
> 'undo patch' results in a test failure.

Great, thank you.

I noticed git-am complaining about a couple whitespace issues in the
test, mostly about mixing spaces/tabs. The v4 fixes them (in a separate
part, but should be merged into 0001). It's a detail, but might be good
to try git-am on patches ;-)

> Also I realized that the test 'invalid_entry_columns_order_test' will
> fail on big endian machines,
> because varlena len encoding is different for little endian and big
> endian, so I changed the test a little bit.
> Now the test doesn't use varlena len byte in regex.

I think it'd make sense to split this into smaller patches, each fixing
a different issue. Not one patch for each of the 11 items in your
original message, that would be an overkill ...

I propose to split it like this, into three parts, each addressing a
particular type of mistake:

1) gin_check_posting_tree_parent_keys_consistency

2) gin_check_parent_keys_consistency / att comparisons

3) gin_check_parent_keys_consistency / setting ptr->parenttup (at the end)

Does this make sense to you? If yes, can you split the patch series like
this, including a commit message for each part, explaining the fix? We'd
need the commit message even with a single patch, ofc.


> I also remove the blksize hardcode and start getting it from the
> cluster configuration. But anyway some tests
> will fail with not standard block size (probably all tests where tree
> growth is expected).
> 

I think that's fine. AFAIK we don't expect tests to be 100% stable with
other block sizes. It shouldn't crash / segfault, ofc, but some tests
may be sensitive to this.


BTW I hoped to get this fix pushed this week, but that didn't happen and
I'll be away most of next week :-( Let's try to get this sorted so that
I can push it on June 16 or so.


regards

-- 
Tomas Vondra

  83. Re: Amcheck verification of GiST and GIN

    Tomas Vondra <tomas@vondra.me> — 2025-06-09T15:34:14Z

    On 6/9/25 00:14, Tomas Vondra wrote:
> ...
>
> I propose to split it like this, into three parts, each addressing a
> particular type of mistake:
> 
> 1) gin_check_posting_tree_parent_keys_consistency
> 
> 2) gin_check_parent_keys_consistency / att comparisons
> 
> 3) gin_check_parent_keys_consistency / setting ptr->parenttup (at the end)
> 
> Does this make sense to you? If yes, can you split the patch series like
> this, including a commit message for each part, explaining the fix? We'd
> need the commit message even with a single patch, ofc.
> 
The attached v5 patch splits it along these lines, except that the extra
0001 part merely adds a multicolumn index into the regression test. The
0002-0004 parts are ordered to match the TAP test, i.e. it adds tests.

I've copied the points from the report to the commit messages, but this
needs cleanup/rephrasing, to make it readable. Could you look into
that?Of course, if you think the patches should be split differently,
feel free to move stuff.

And as I said before - if you feel the issues are too intertwined and
can't be split like this (or it just doesn't make sense), please speak
up. We can commit that as a single patch. It still needs the commit
message, though.

regards

-- 
Tomas Vondra

  84. Re: Amcheck verification of GiST and GIN

    Arseniy Mukhin <arseniy.mukhin.dev@gmail.com> — 2025-06-09T16:37:39Z

    On Mon, Jun 9, 2025 at 6:34 PM Tomas Vondra <tomas@vondra.me> wrote:
>
> On 6/9/25 00:14, Tomas Vondra wrote:
> > ...
> >
> > I propose to split it like this, into three parts, each addressing a
> > particular type of mistake:
> >
> > 1) gin_check_posting_tree_parent_keys_consistency
> >
> > 2) gin_check_parent_keys_consistency / att comparisons
> >
> > 3) gin_check_parent_keys_consistency / setting ptr->parenttup (at the end)
> >
> > Does this make sense to you? If yes, can you split the patch series like
> > this, including a commit message for each part, explaining the fix? We'd
> > need the commit message even with a single patch, ofc.
> >
> The attached v5 patch splits it along these lines, except that the extra
> 0001 part merely adds a multicolumn index into the regression test. The
> 0002-0004 parts are ordered to match the TAP test, i.e. it adds tests.

Great, thank you.

> I've copied the points from the report to the commit messages, but this
> needs cleanup/rephrasing, to make it readable. Could you look into
> that?Of course, if you think the patches should be split differently,
> feel free to move stuff.

Yes, sure, I will do it ASAP.

> And as I said before - if you feel the issues are too intertwined and
> can't be split like this (or it just doesn't make sense), please speak
> up. We can commit that as a single patch. It still needs the commit
> message, though.

The way it splitted seems reasonable to me. Intertwined issues are
grouped together, and patches are more or less independent.

Also the test for 'posting tree parent_key check' that was added last
started failing locally. Don't know what changed, but I rewrote it
so now it relies on child blkno, which is stable (I hope), instead of
concrete TID. Will include it in the new patchset.


Best regards,
Arseniy Mukhin

  85. Re: Amcheck verification of GiST and GIN

    Arseniy Mukhin <arseniy.mukhin.dev@gmail.com> — 2025-06-10T08:18:42Z

    On Mon, Jun 9, 2025 at 7:37 PM Arseniy Mukhin
<arseniy.mukhin.dev@gmail.com> wrote:
>
> On Mon, Jun 9, 2025 at 6:34 PM Tomas Vondra <tomas@vondra.me> wrote:
> >
> > On 6/9/25 00:14, Tomas Vondra wrote:
> > > ...
> > >
> > > I propose to split it like this, into three parts, each addressing a
> > > particular type of mistake:
> > >
> > > 1) gin_check_posting_tree_parent_keys_consistency
> > >
> > > 2) gin_check_parent_keys_consistency / att comparisons
> > >
> > > 3) gin_check_parent_keys_consistency / setting ptr->parenttup (at the end)
> > >
> > > Does this make sense to you? If yes, can you split the patch series like
> > > this, including a commit message for each part, explaining the fix? We'd
> > > need the commit message even with a single patch, ofc.
> > >
> > The attached v5 patch splits it along these lines, except that the extra
> > 0001 part merely adds a multicolumn index into the regression test. The
> > 0002-0004 parts are ordered to match the TAP test, i.e. it adds tests.
>
> Great, thank you.
>
> > I've copied the points from the report to the commit messages, but this
> > needs cleanup/rephrasing, to make it readable. Could you look into
> > that?Of course, if you think the patches should be split differently,
> > feel free to move stuff.
>
> Yes, sure, I will do it ASAP.
>

Please find a new version in attachments. There are formatted commit
messages and some cosmetic changes in the tests. Please let me know if
anything needs to be changed. Also FWIW points 9th, 10th and 11th from
the report [1] were not addressed in the fixes. I'm not sure about
10th and 11th, but 9th seems like a no-brainer, so I added a patch
deleting an unused field 'parentlsn'. I tried git-am with patches and
it's ok with it. Thank you for the advice, added git-am step in my
patch preparation routine.

> ...
> Also the test for 'posting tree parent_key check' that was added last
> started failing locally. Don't know what changed, but I rewrote it
> so now it relies on child blkno, which is stable (I hope), instead of
> concrete TID. Will include it in the new patchset.
>

Also changed the regex pattern for this failing test, hope it is more
robust now.


[1] https://postgr.es/m/CAE7r3MJ611B9TE=YqBBncewp7-k64VWs+sjk7XF6fJUX77uFBA@mail.gmail.com


Best regards,
Arseniy Mukhin

  86. Re: Amcheck verification of GiST and GIN

    Andrey Borodin <x4mmm@yandex-team.ru> — 2025-06-15T13:24:04Z

    Hi Arseniy!

Thanks for finding these problems.
I had several attempts to wrap my head around original patch with fixes, but when it was broken into several steps it finally became easier for me.
Here are some thought about patches.



> On 10 Jun 2025, at 13:18, Arseniy Mukhin <arseniy.mukhin.dev@gmail.com> wrote:
> <0001-amcheck-Add-gin_index_check-on-a-multicolumn-index.patch>

The test seems harmless and nice to have. I understand that this test is needed to extend coverage.
Perhaps, we could verify that some code is actually triggered. Personally, I would be happy if we could some add injection points with notices at tested branches. But, AFAIK, it's too much of a burden to have injection points in contrib extensions. We had very similar problem with sort patch in btree_gist and eventually gave up. elog(DEBUG) was not a good solution too, because it was unstable.
See 'gin-finish-incomplete-split' or 'hash-aggregate-enter-spill-mode' for reference.


> On 10 Jun 2025, at 13:18, Arseniy Mukhin <arseniy.mukhin.dev@gmail.com> wrote:
> <0002-patch-1-gin_check_parent_keys_consistency.patch>

Well, we inherited ginCompareEntries() from the very first patch version from 2020. I can't really say anything about differences here, but your proposed change seems correct.

Kirill excluded rightmost keys in v33 and that was kind of a fix. Kirill, do you remember if was particular problem of internal pages? Is it safe to enable tuple order check for rightmost tuples on leaf pages?

You wrote this comment:
+			/*
+			 * First block is metadata, skip order check. Also, never check
+			 * for high key on rightmost page, as this key is not really
+			 * stored explicitly.
+			 */

I agree that exclusion (stack->blkno != GIN_ROOT_BLKNO) make no sense. It was with us from the original version from 2020. As I understand some checks on root page will be used in test invalid_entry_columns_order_test.


Having some TAP tests sounds like a very good idea.

I'm a bit surprised by excluding some letters from random_string(), but perhaps it's fine for this test.

Somewhere here:
+		INSERT INTO $relname (a) VALUES (('{' || 'pppppppppp' || random_string(1870) ||'}')::text[]);
I'd like to have a comment explaining number 1870. And, probably, you expect exactly 2 tuples on root page, right?

Are we 100% certain that 'rrrrrrrrr' will always be on root page?

I do not see much value in having variables $relname and $indexname. I'd just substitute its usages with literals. But I'm not sure, maybe this structure will be used in your tests later...

In this function
+sub string_replace_block
I'd suggest a little bit of comments. Also, perhaps, fsync of files, but 001_verify_heapam.pl does not do fsync. So, maybe it's OK here too.

Also, I have a wild idea. Maybe add an assert that block size if 8192 and just exit otherwise?

And, maybe instead of gin_clean_pending_list() you can just create an index with fastupdate=off.

> On 10 Jun 2025, at 13:18, Arseniy Mukhin <arseniy.mukhin.dev@gmail.com> wrote:
> <0003-patch-2-gin_check_parent_keys_consistency.patch>

The patch seems correct to me.
Except this
+	my $blkno = 5;  # leaf
in test reads scary. Will it be stable on buildfarm?


> On 10 Jun 2025, at 13:18, Arseniy Mukhin <arseniy.mukhin.dev@gmail.com> wrote:
> <0004-patch-3-gin_check_posting_tree_parent_keys_consisten.patch>

I generally agree with direction of this patch.
But please also check the approach of PageGetItemIdCareful() in verify_nbtree.c. It goes extra mile to avoid coredump in case of bogus ItemId. Should we do something like that here too?


> On 10 Jun 2025, at 13:18, Arseniy Mukhin <arseniy.mukhin.dev@gmail.com> wrote:
> 
> <0005-patch-4-remove-unused-parentlsn.patch>

LGTM.


> On 9 May 2025, at 17:43, Arseniy Mukhin <arseniy.mukhin.dev@gmail.com> wrote:
> 
> 10) README says "Vacuum never deletes tuples or pages from the entry
> tree." But check assumes that it's possible to have
> deleted leaf page with 0 entries.
> 
>    if (GinPageIsDeleted(page))
>    {
>       if (!GinPageIsLeaf(page))
>          ereport(ERROR,
>                (errcode(ERRCODE_INDEX_CORRUPTED),
>                 errmsg("index \"%s\" has deleted internal page %u",
>                      RelationGetRelationName(rel), blockNo)));
>       if (PageGetMaxOffsetNumber(page) > InvalidOffsetNumber)
>          ereport(ERROR,
>                (errcode(ERRCODE_INDEX_CORRUPTED),
>                 errmsg("index \"%s\" has deleted page %u with tuples",
>                      RelationGetRelationName(rel), blockNo)));
>    }

To enforce such an invariant we must be sure that GIN never deleted entry pages in older versions. I do not have enough knowledge of the history for this.

> 11) When we compare entry tree max page key with parent key:
> 
>             if (ginCompareAttEntries(&state, attnum, current_key,
>                              current_key_category, parent_key_attnum,
>                                      parent_key, parent_key_category) > 0)
>             {
>                /*
>                 * 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.
>                 */
>                pfree(stack->parenttup);
>                stack->parenttup = gin_refind_parent(rel, stack->parentblk,
>                                            stack->blkno, strategy);
> 
> I think we can remove gin_refind_parent() and do ereport right away here.
> The same logic as with 3). AFAIK it's impossible to have a child item
> with a key that is higher than the cached parent key.
> Parent key bounds what keys we can insert into the child page, so it
> seems there is no way how they can appear there.
> 

This logic was copied from GiST check. In GiST "Area of responsibility" of internal tuple can be extended in any direction. That's why we need to lock parent page.
If in GIN internal tuple keyspace is never extended - it's OK to avoid gin_refind_parent().
But reasoning about GIN concurrency is rather difficult. Unfortunately, we do not have such checks in B-tree verification without ShareLock. Either way we could peep some idea from there.

Thank you!


Best regards, Andrey Borodin.

  87. Re: Amcheck verification of GiST and GIN

    Arseniy Mukhin <arseniy.mukhin.dev@gmail.com> — 2025-06-15T22:25:40Z

    On Sun, Jun 15, 2025 at 4:24 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
>
>
> Hi Arseniy!
>
> Thanks for finding these problems.
> I had several attempts to wrap my head around original patch with fixes, but when it was broken into several steps it finally became easier for me.
> Here are some thought about patches.
>

Hi Andrey! Thank you for the review.

>
> > On 10 Jun 2025, at 13:18, Arseniy Mukhin <arseniy.mukhin.dev@gmail.com> wrote:
> > <0001-amcheck-Add-gin_index_check-on-a-multicolumn-index.patch>
>
> The test seems harmless and nice to have. I understand that this test is needed to extend coverage.
> Perhaps, we could verify that some code is actually triggered. Personally, I would be happy if we could some add injection points with notices at tested branches. But, AFAIK, it's too much of a burden to have injection points in contrib extensions. We had very similar problem with sort patch in btree_gist and eventually gave up. elog(DEBUG) was not a good solution too, because it was unstable.
> See 'gin-finish-incomplete-split' or 'hash-aggregate-enter-spill-mode' for reference.

I'm not familiar with injections points much, but I think I got the
idea, sounds interesting. Thank you for the references.

>
> Having some TAP tests sounds like a very good idea.
>
> I'm a bit surprised by excluding some letters from random_string(), but perhaps it's fine for this test.
>

Yeah, there is no reason why we can't use vowels here, so I will add
them so that it doesn't look like there is any point in their absence.

> Somewhere here:
> +               INSERT INTO $relname (a) VALUES (('{' || 'pppppppppp' || random_string(1870) ||'}')::text[]);
> I'd like to have a comment explaining number 1870. And, probably, you expect exactly 2 tuples on root page, right?
>

The idea behind "random_string(1870)" was to get split as fast as
possible, but tuples with size > 2kb are toasted, so we have to use
something about 2k here. I think I took 1870 from some other place
where it was necessary, but here we can round it to 1900. So I'll
replace 1870 with 1900 and add a comment about the size. Also gonna
add some comments about datasets in some tests to make it more clear.

> Are we 100% certain that 'rrrrrrrrr' will always be on root page?

I'm not 100% sure. AFAIK the split algorithm is deterministic and the
idea was that if we use very long tuples, then all other factors will
be too small to influence what key we will see on the root page.

> I do not see much value in having variables $relname and $indexname. I'd just substitute its usages with literals. But I'm not sure, maybe this structure will be used in your tests later...

I added variables just because we use index name and table name
several times, but I don't mind getting rid of them.

>
> In this function
> +sub string_replace_block
> I'd suggest a little bit of comments. Also, perhaps, fsync of files, but 001_verify_heapam.pl does not do fsync. So, maybe it's OK here too.
>

Will add a comment here.

> Also, I have a wild idea. Maybe add an assert that block size if 8192 and just exit otherwise?

I like the idea. I thought maybe it would be great to have some
function that every TAP test can use if it needs a certain block size?

> And, maybe instead of gin_clean_pending_list() you can just create an index with fastupdate=off.

Yeah, I think we can do it even simpler if we move index creation to
the end as regression tests do.

>
> > On 10 Jun 2025, at 13:18, Arseniy Mukhin <arseniy.mukhin.dev@gmail.com> wrote:
> > <0003-patch-2-gin_check_parent_keys_consistency.patch>
>
> The patch seems correct to me.
> Except this
> +       my $blkno = 5;  # leaf
> in test reads scary. Will it be stable on buildfarm?
>

Not sure, but I thought that blkno should be more or less the same everywhere.

>
> > On 9 May 2025, at 17:43, Arseniy Mukhin <arseniy.mukhin.dev@gmail.com> wrote:
> >
> > 10) README says "Vacuum never deletes tuples or pages from the entry
> > tree." But check assumes that it's possible to have
> > deleted leaf page with 0 entries.
> >
> >    if (GinPageIsDeleted(page))
> >    {
> >       if (!GinPageIsLeaf(page))
> >          ereport(ERROR,
> >                (errcode(ERRCODE_INDEX_CORRUPTED),
> >                 errmsg("index \"%s\" has deleted internal page %u",
> >                      RelationGetRelationName(rel), blockNo)));
> >       if (PageGetMaxOffsetNumber(page) > InvalidOffsetNumber)
> >          ereport(ERROR,
> >                (errcode(ERRCODE_INDEX_CORRUPTED),
> >                 errmsg("index \"%s\" has deleted page %u with tuples",
> >                      RelationGetRelationName(rel), blockNo)));
> >    }
>
> To enforce such an invariant we must be sure that GIN never deleted entry pages in older versions. I do not have enough knowledge of the history for this.

Agree, good point.

>
> > 11) When we compare entry tree max page key with parent key:
> >
> >             if (ginCompareAttEntries(&state, attnum, current_key,
> >                              current_key_category, parent_key_attnum,
> >                                      parent_key, parent_key_category) > 0)
> >             {
> >                /*
> >                 * 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.
> >                 */
> >                pfree(stack->parenttup);
> >                stack->parenttup = gin_refind_parent(rel, stack->parentblk,
> >                                            stack->blkno, strategy);
> >
> > I think we can remove gin_refind_parent() and do ereport right away here.
> > The same logic as with 3). AFAIK it's impossible to have a child item
> > with a key that is higher than the cached parent key.
> > Parent key bounds what keys we can insert into the child page, so it
> > seems there is no way how they can appear there.
> >
>
> This logic was copied from GiST check. In GiST "Area of responsibility" of internal tuple can be extended in any direction. That's why we need to lock parent page.
> If in GIN internal tuple keyspace is never extended - it's OK to avoid gin_refind_parent().
> But reasoning about GIN concurrency is rather difficult. Unfortunately, we do not have such checks in B-tree verification without ShareLock. Either way we could peep some idea from there.
>

Got it.


Here is the new version. I fixed some points that Andrey mentioned.
All of them in the TAP test. Several comments were added, filler size
1870 changed to 1900. Also I added vowels to the replace function and
moved index creation after the data filling. Thank you!


Best regards,
Arseniy Mukhin

  88. Re: Amcheck verification of GiST and GIN

    Tomas Vondra <tomas@vondra.me> — 2025-06-16T15:58:34Z

    Thanks.

I went through the patches, polished the commit messages and did some
minor tweaks in patch 0002 (to make the variable names a bit more
consistent, and reduce the scope a little bit). I left it as a separate
patch to make the changes clearer, but it should be merged into 0002.

Please read through the commit messages, and let me know if I got some
of the details wrong (or not clear enough). Otherwise I plan to start
pushing this soon (~tomorrow).


regards

-- 
Tomas Vondra

  89. Re: Amcheck verification of GiST and GIN

    Arseniy Mukhin <arseniy.mukhin.dev@gmail.com> — 2025-06-16T19:09:25Z

    On Mon, Jun 16, 2025 at 6:58 PM Tomas Vondra <tomas@vondra.me> wrote:
>
> Thanks.
>
> I went through the patches, polished the commit messages and did some
> minor tweaks in patch 0002 (to make the variable names a bit more
> consistent, and reduce the scope a little bit). I left it as a separate
> patch to make the changes clearer, but it should be merged into 0002.
>
> Please read through the commit messages, and let me know if I got some
> of the details wrong (or not clear enough). Otherwise I plan to start
> pushing this soon (~tomorrow).

LGTM.
Noticed a few typos in messages:
in v8-0002-amcheck-Fix-checks-of-entry-order-for-GIN-indexes.patch
   - parent key is creator
   - as the core incorrectly expected
and 'Arseniy Mikhin' in some patches.


Best regards,
Arseniy Mukhin

  90. Re: Amcheck verification of GiST and GIN

    Tomas Vondra <tomas@vondra.me> — 2025-06-16T19:59:53Z

    On 6/16/25 21:09, Arseniy Mukhin wrote:
> On Mon, Jun 16, 2025 at 6:58 PM Tomas Vondra <tomas@vondra.me> wrote:
>>
>> Thanks.
>>
>> I went through the patches, polished the commit messages and did some
>> minor tweaks in patch 0002 (to make the variable names a bit more
>> consistent, and reduce the scope a little bit). I left it as a separate
>> patch to make the changes clearer, but it should be merged into 0002.
>>
>> Please read through the commit messages, and let me know if I got some
>> of the details wrong (or not clear enough). Otherwise I plan to start
>> pushing this soon (~tomorrow).
> 
> LGTM.
> Noticed a few typos in messages:
> in v8-0002-amcheck-Fix-checks-of-entry-order-for-GIN-indexes.patch
>    - parent key is creator
>    - as the core incorrectly expected
> and 'Arseniy Mikhin' in some patches.
> 

Thanks for noticing those typos, especially the one in the name.


regards

-- 
Tomas Vondra

  91. Re: Amcheck verification of GiST and GIN

    Thom Brown <thom@linux.com> — 2025-06-17T14:19:41Z

    On Mon, 16 Jun 2025 at 21:00, Tomas Vondra <tomas@vondra.me> wrote:
>
> On 6/16/25 21:09, Arseniy Mukhin wrote:
> > On Mon, Jun 16, 2025 at 6:58 PM Tomas Vondra <tomas@vondra.me> wrote:
> >>
> >> Thanks.
> >>
> >> I went through the patches, polished the commit messages and did some
> >> minor tweaks in patch 0002 (to make the variable names a bit more
> >> consistent, and reduce the scope a little bit). I left it as a separate
> >> patch to make the changes clearer, but it should be merged into 0002.
> >>
> >> Please read through the commit messages, and let me know if I got some
> >> of the details wrong (or not clear enough). Otherwise I plan to start
> >> pushing this soon (~tomorrow).
> >
> > LGTM.
> > Noticed a few typos in messages:
> > in v8-0002-amcheck-Fix-checks-of-entry-order-for-GIN-indexes.patch
> >    - parent key is creator
> >    - as the core incorrectly expected
> > and 'Arseniy Mikhin' in some patches.
> >
>
> Thanks for noticing those typos, especially the one in the name.

Do today's commits clear this from the PostgreSQL 18 Open Items list?

Thom

  92. Re: Amcheck verification of GiST and GIN

    Tomas Vondra <tomas@vondra.me> — 2025-06-17T14:26:17Z

    On 6/17/25 16:19, Thom Brown wrote:
> On Mon, 16 Jun 2025 at 21:00, Tomas Vondra <tomas@vondra.me> wrote:
>>
>> On 6/16/25 21:09, Arseniy Mukhin wrote:
>>> On Mon, Jun 16, 2025 at 6:58 PM Tomas Vondra <tomas@vondra.me> wrote:
>>>>
>>>> Thanks.
>>>>
>>>> I went through the patches, polished the commit messages and did some
>>>> minor tweaks in patch 0002 (to make the variable names a bit more
>>>> consistent, and reduce the scope a little bit). I left it as a separate
>>>> patch to make the changes clearer, but it should be merged into 0002.
>>>>
>>>> Please read through the commit messages, and let me know if I got some
>>>> of the details wrong (or not clear enough). Otherwise I plan to start
>>>> pushing this soon (~tomorrow).
>>>
>>> LGTM.
>>> Noticed a few typos in messages:
>>> in v8-0002-amcheck-Fix-checks-of-entry-order-for-GIN-indexes.patch
>>>    - parent key is creator
>>>    - as the core incorrectly expected
>>> and 'Arseniy Mikhin' in some patches.
>>>
>>
>> Thanks for noticing those typos, especially the one in the name.
> 
> Do today's commits clear this from the PostgreSQL 18 Open Items list?
> 

That's the intent, yes. There's one remaining commit.


-- 
Tomas Vondra