Horton
Menu

Re: Amcheck verification of GiST and GIN

Arseniy Mukhin <arseniy.mukhin.dev@gmail.com>

View thread
From: Arseniy Mukhin <arseniy.mukhin.dev@gmail.com>
To: Tomas Vondra <tomas@vondra.me>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Kirill Reshke <reshkekirill@gmail.com>, Mark Dilger <mark.dilger@enterprisedb.com>, "Andrey M. Borodin" <x4mmm@yandex-team.ru>, Alexander Lakhin <exclusion@gmail.com>, Andrey Borodin <amborodin86@gmail.com>, Peter Geoghegan <pg@bowt.ie>, 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: 2025-06-10T08:18:42Z
Lists: pgsql-hackers

Attachments

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

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.