Re: Amcheck verification of GiST and GIN

Hi,

On 7/9/24 08:36, Andrey M. Borodin wrote:
>
>
>> On 5 Jul 2024, at 17:27, Andrey M. Borodin <x4mmm(at)yandex-team(dot)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