Re: Amcheck verification of GiST and GIN

From: Mark Dilger <mark(dot)dilger(at)enterprisedb(dot)com>
To: Tomas Vondra <tomas(at)vondra(dot)me>
Cc: Kirill Reshke <reshkekirill(at)gmail(dot)com>, "Andrey M(dot) Borodin" <x4mmm(at)yandex-team(dot)ru>, Alexander Lakhin <exclusion(at)gmail(dot)com>, Andrey Borodin <amborodin86(at)gmail(dot)com>, Peter Geoghegan <pg(at)bowt(dot)ie>, Jose Arthur Benetasso Villanova <jose(dot)arthur(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Nikolay Samokhvalov <samokhvalov(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Amcheck verification of GiST and GIN
Date: 2025-02-21 20:16:25
Message-ID: BC221A56-977C-418E-A1B8-9EFC881D80C5@enterprisedb.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> On Feb 21, 2025, at 9:07 AM, Mark Dilger <mark(dot)dilger(at)enterprisedb(dot)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.

Attachment Content-Type Size
v36-0001-A-tiny-nitpicky-tweak-to-beautify-the-Amcheck-in.patch application/octet-stream 964 bytes
v36-0002-Refactor-amcheck-internals-to-isolate-common-loc.patch application/octet-stream 21.4 KB
v36-0003-Add-gin_index_check-to-verify-GIN-index.patch application/octet-stream 33.6 KB
v36-0004-Fix-wording-in-GIN-README.patch application/octet-stream 1.2 KB
v36-0005-Fix-for-gin_index_check.patch application/octet-stream 4.3 KB
v36-0006-Add-gin-index-checking-test-for-jsonb-data.patch application/octet-stream 2.1 KB
v36-0007-Add-gin-to-the-create-index-concurrently-tap-tes.patch application/octet-stream 5.8 KB
v36-0008-Stress-test-verify_gin-using-pgbench.patch application/octet-stream 9.6 KB
unknown_filename text/plain 98 bytes

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2025-02-21 20:23:00 Re: Statistics Import and Export
Previous Message Sami Imseih 2025-02-21 19:54:17 Re: Psql meta-command conninfo+