Horton
Menu

Re: Amcheck verification of GiST and GIN

Mark Dilger <mark.dilger@enterprisedb.com>

View thread
From: Mark Dilger <mark.dilger@enterprisedb.com>
To: Tomas Vondra <tomas@vondra.me>
Cc: Kirill Reshke <reshkekirill@gmail.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-02-21T20:16:25Z
Lists: pgsql-hackers

Attachments


> 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.

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.