Horton
Menu

Re: Amcheck verification of GiST and GIN

Tomas Vondra <tomas.vondra@enterprisedb.com>

View thread
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
To: "Andrey M. Borodin" <x4mmm@yandex-team.ru>, Alexander Lakhin <exclusion@gmail.com>
Cc: Andrey Borodin <amborodin86@gmail.com>, Peter Geoghegan <pg@bowt.ie>, Mark Dilger <mark.dilger@enterprisedb.com>, Jose Arthur Benetasso Villanova <jose.arthur@gmail.com>, Andres Freund <andres@anarazel.de>, Nikolay Samokhvalov <samokhvalov@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2024-07-10T16:01:40Z
Lists: pgsql-hackers

Attachments

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

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.