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