Re: Amcheck verification of GiST and GIN

On Mon, Jun 9, 2025 at 7:37 PM Arseniy Mukhin wrote: > > On Mon, Jun 9, 2025 at 6:34 PM Tomas Vondra 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_

Re: Amcheck verification of GiST and GIN

On Mon, Jun 9, 2025 at 6:34 PM Tomas Vondra 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_key

Re: Amcheck verification of GiST and GIN

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_consiste

Re: Amcheck verification of GiST and GIN

On 5/29/25 13:53, Arseniy Mukhin wrote: > On Mon, May 26, 2025 at 7:28 PM Arseniy Mukhin > wrote: >> On Mon, May 26, 2025 at 1:27 PM Tomas Vondra wrote: >>> Also, I've noticed that the TAP test passes even with some (most) of the >>> verify_gin.c changes reverted. See the 0002 patch - this does n

Re: Amcheck verification of GiST and GIN

On Mon, May 26, 2025 at 7:28 PM Arseniy Mukhin wrote: > On Mon, May 26, 2025 at 1:27 PM Tomas Vondra wrote: > > Also, I've noticed that the TAP test passes even with some (most) of the > > verify_gin.c changes reverted. See the 0002 patch - this does not break > > the TAP test. Of course, that do

Re: Amcheck verification of GiST and GIN

Hello Tomas, On Mon, May 26, 2025 at 1:27 PM Tomas Vondra wrote: > > Hello Arseniy, > > I finally got time to look at this more closely, and do some testing. Thank you for looking into this. > Are there any cases when the current code incorrectly reports corruption > for a valid index? So far

Re: Amcheck verification of GiST and GIN

Hello Arseniy, I finally got time to look at this more closely, and do some testing. Are there any cases when the current code incorrectly reports corruption for a valid index? So far I've been unable to find such case. Or am I wrong? It seems to me all the proposed changes are "tightening" the

Re: Amcheck verification of GiST and GIN

On 5/9/25 14:43, Arseniy Mukhin wrote: > Hello, > > Thanks everybody for the patch. > > I noticed there are no tests that GIN check fails if the index is > corrupted, so I thought it would be great to have some. > While writing tests I noticed some issues in the patch (all issues are > for verify

Re: Amcheck verification of GiST and GIN

Hello, Thanks everybody for the patch. I noticed there are no tests that GIN check fails if the index is corrupted, so I thought it would be great to have some. While writing tests I noticed some issues in the patch (all issues are for verify_gin.c) 1) When we add new items to the entry tree sta

Re: Amcheck verification of GiST and GIN

On 3/30/25 06:04, Tom Lane wrote: > Tomas Vondra writes: >> I've pushed all the parts of this patch series, except for the stress >> test - which I think was not meant for commit. >> buildfarm seems happy so far, except for a minor indentation issue >> (forgot to reindent after merging the separat

Re: Amcheck verification of GiST and GIN

Tomas Vondra writes: > I've pushed all the parts of this patch series, except for the stress > test - which I think was not meant for commit. > buildfarm seems happy so far, except for a minor indentation issue > (forgot to reindent after merging the separate fix patch). Not so much here: https:

Re: Amcheck verification of GiST and GIN

On 3/28/25 20:51, Kirill Reshke wrote: > On Fri, 28 Mar 2025 at 21:26, Tomas Vondra wrote: >> >> Here's a polished version of the patches. If you have any >> comments/objections, please speak now. >> -- >> Tomas Vondra > > Hi, no objections, lgtm > I've pushed all the parts of this patch series

Re: Amcheck verification of GiST and GIN

On Fri, 28 Mar 2025 at 21:26, Tomas Vondra wrote: > > Here's a polished version of the patches. If you have any > comments/objections, please speak now. > -- > Tomas Vondra Hi, no objections, lgtm -- Best regards, Kirill Reshke

Re: Amcheck verification of GiST and GIN

Here's a polished version of the patches. If you have any comments/objections, please speak now. I don't plan to push 0006 (the stress test), of course. Changes I did: 1) update / write proper commit messages, hopefully explaining the purpose of each patch well enough 2) update the lists of revi

Re: Amcheck verification of GiST and GIN

On 3/27/25 16:30, Mark Dilger wrote: > > > On Fri, Feb 21, 2025 at 6:29 AM Tomas Vondra > wrote: > > Hi, > > I see this patch didn't move since December :-( I still think these > improvements would be useful, it certainly was very helpful when I was >

Re: Amcheck verification of GiST and GIN

On Fri, Feb 21, 2025 at 6:29 AM Tomas Vondra wrote: > Hi, > > I see this patch didn't move since December :-( I still think these > improvements would be useful, it certainly was very helpful when I was > working on the GIN/GiST parallel builds (the GiST builds stalled, but I > hope to push the G

Re: Amcheck verification of GiST and GIN

On Fri, 28 Feb 2025 at 23:31, Mark Dilger wrote: > The only obvious definition of "wrong" for this is that gin index scans > return different result sets than table scans over the same data. Using your > much smaller reproducible test case, and adding rows like: Yeach, you are 100% right. Act

Re: Amcheck verification of GiST and GIN

> So, we have an entry tree page, where there is tuple on offset 80, > with gin tuple category = 3, and then it goes category 0 again. And > one more such pattern on the same page. > The ginCompareEntries function compares the gin tuples category first. > I do not understand how this would be a val

Re: Amcheck verification of GiST and GIN

On Sat, 22 Feb 2025 at 03:51, Mark Dilger wrote: > > > > > On Feb 21, 2025, at 12:50 PM, Mark Dilger > > wrote: > > > > Could one of the patch authors take a look? > > I turned the TAP test which triggers the error into a regression test that > does likewise, for ease of stepping through the te

Re: Amcheck verification of GiST and GIN

> On Feb 21, 2025, at 12:16 PM, Mark Dilger > wrote: > > 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 suspected the AccessShareLock taken by verify_gin() might be too weak, and upgraded that to ShareRowExclusiveL

Re: Amcheck verification of GiST and GIN

> On Feb 21, 2025, at 9:07 AM, Mark Dilger 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 h

Re: Amcheck verification of GiST and GIN

> On 21 Feb 2025, at 19:29, Tomas Vondra wrote: > > I see this patch didn't move since December :-( I still think these > improvements would be useful, it certainly was very helpful when I was > working on the GIN/GiST parallel builds (the GiST builds stalled, but I > hope to push the GIN patc

Re: Amcheck verification of GiST and GIN

On 2/21/25 18:07, Mark Dilger wrote: > > >> On Feb 21, 2025, at 6:29 AM, Tomas Vondra wrote: >> >> Hi, >> >> I see this patch didn't move since December :-( I still think >> these improvements would be useful, it certainly was very helpful >> when I was working on the GIN/GiST parallel builds

Re: Amcheck verification of GiST and GIN

> On Feb 21, 2025, at 6:29 AM, Tomas Vondra wrote: > > Hi, > > I see this patch didn't move since December :-( I still think these > improvements would be useful, it certainly was very helpful when I was > working on the GIN/GiST parallel builds (the GiST builds stalled, but I > hope to push

Re: Amcheck verification of GiST and GIN

Hi, I see this patch didn't move since December :-( I still think these improvements would be useful, it certainly was very helpful when I was working on the GIN/GiST parallel builds (the GiST builds stalled, but I hope to push the GIN patches soon). So I'd like to get some of this in too. I'm no

Re: Amcheck verification of GiST and GIN

Hi all. I was reviewing nearby thread about parallel index creation for GIN, and spotted this test [0] : create table gin_t (a int[]); insert into gin_t select * from rand_array(3000, 0, 100, 0, 50); create index on gin_t using gin(a); v34 fails on this. The reason is we should never check t

Re: Amcheck verification of GiST and GIN

> On 26 Nov 2024, at 11:50, Kirill Reshke wrote: > > I did mechanical patch rebase & beautification. Many thanks! Addressing Tomas' feedback was still one of top items on my todo list. And I'm more than happy that someone advance this patchset. > Notice my first patch, i did small refactori

Re: Amcheck verification of GiST and GIN

Hi! On Mon, 5 Aug 2024 at 20:05, Tomas Vondra wrote: > > Hi, > > I've spent a bit more time looking at the GiST part as part of my > "parallel GiST build" patch nearby, and I think there's some sort of > memory leak. > > Consider this: > > create table t (a text); > > insert into t select md5

Re: Amcheck verification of GiST and GIN

Hi, I've spent a bit more time looking at the GiST part as part of my "parallel GiST build" patch nearby, and I think there's some sort of memory leak. Consider this: create table t (a text); insert into t select md5(i::text) from generate_series(1,2500) s(i); create index on t u

Re: Amcheck verification of GiST and GIN

Hi Tomas! Thank you so much for your interest in the patchset. > On 10 Jul 2024, at 19:01, Tomas Vondra wrote: > > 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 r

Re: Amcheck verification of GiST and GIN

OK, one more issue report. I originally thought it's a bug in my patch adding parallel builds for GIN indexes, but it turns out it happens even with serial builds on master ... If I build any GIN index, and then do gin_index_parent_check() on it, I get this error: create index jsonb_hash on messa

Re: Amcheck verification of GiST and GIN

OK, one mere comment - it seems the 0001 patch has incorrect indentation in bt_index_check_callback, triggering this: -- verify_nbtree.c: In function ‘bt_index_check_callback’: verify_nbtree.c:331:25: warning: this ‘if’ clause doe

Re: Amcheck verification of GiST and GIN

On 7/10/24 18:01, Tomas Vondra wrote: > ... > > 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. > As mentioned a couple days ago, I started using this patch to validate the patches adding parallel build

Re: Amcheck verification of GiST and GIN

> On 20 Jan 2024, at 07:46, vignesh C wrote: > > I have changed the status of the commitfest entry to "Waiting on > Author" as there was no follow-up on Alexander's queries. Feel free to > address them and change the commitfest entry accordingly. Thanks Vignesh! At the moment it’s obvious th

Re: Amcheck verification of GiST and GIN

On Mon, 27 Mar 2023 at 03:47, Andrey Borodin wrote: > > On Sun, Mar 19, 2023 at 4:00 PM Andrey Borodin wrote: > > > > Also, there are INCLUDEd attributes. Right now we just put them as-is > > to the bloom filter. Does this constitute a TOAST bug as in B-tree? > > If so, I think we should use a ve

Re: Amcheck verification of GiST and GIN

Hi Andrey, 27.03.2023 01:17, Andrey Borodin wrote: I've ported the B-tree TOAST test to GiST, and, as expected, it fails. Finds non-indexed tuple for a fresh valid index. I've tried to use this feature with the latest patch set and discovered that modified pg_amcheck doesn't find any gist inde

Re: Amcheck verification of GiST and GIN

On Sun, Mar 19, 2023 at 4:00 PM Andrey Borodin wrote: > After several attempts to corrupt GiST with this 0.01 epsilon > adjustment tolerance I think GiST indexing of points is valid. > Because intersection for search purposes is determined with the same epsilon! > So it's kind of odd > postgre

Re: Amcheck verification of GiST and GIN

Hi Peter, Thanks for the feedback! I'll work on it during the weekend. On Thu, Mar 16, 2023 at 6:23 PM Peter Geoghegan wrote: > > existence of a "same" routine hints at some confusion about "equality > versus equivalence" issues. Hmm...yes, actually, GiST deals with floats routinely. And there

Re: Amcheck verification of GiST and GIN

On Thu, Mar 16, 2023 at 4:48 PM Peter Geoghegan wrote: > Some feedback on the GiST patch: I see that the Bloom filter that's used to implement heapallindexed verification fingerprints index tuples that are formed via calls to gistFormTuple(), without any attempt to normalize-away differences in T

Re: Amcheck verification of GiST and GIN

On Sun, Feb 5, 2023 at 4:45 PM Andrey Borodin wrote: > Here's v24 == (v23 + a step for pg_amcheck). There's a lot of > shotgun-style changes, but I hope next index types will be easy to add > now. Some feedback on the GiST patch: * You forgot to initialize GistCheckState.heaptuplespresent to 0.

Re: Amcheck verification of GiST and GIN

Hi, On Thu, Feb 02, 2023 at 12:56:47PM -0800, Nikolay Samokhvalov wrote: > On Thu, Feb 2, 2023 at 12:43 PM Peter Geoghegan wrote: > > I think that that problem should be solved at a higher level, in the > > program that runs amcheck. Note that pg_amcheck will already do this > > for B-Tree indexe

Re: Amcheck verification of GiST and GIN

Thank for working on this, Peter! On Fri, Feb 3, 2023 at 6:50 PM Peter Geoghegan wrote: > > I think that we should focus on getting the GiST patch into shape for > commit first, since that seems easier. > Here's the next version. I've focused on GiST part in this revision. Changes: 1. Refactored

Re: Amcheck verification of GiST and GIN

On Thu, Feb 2, 2023 at 12:15 PM Peter Geoghegan wrote: > * Why are there only WARNINGs, never ERRORs here? Attached revision v22 switches all of the WARNINGs over to ERRORs. It has also been re-indented, and now uses a non-generic version of PageGetItemIdCareful() in both verify_gin.c and verify_

Re: Amcheck verification of GiST and GIN

On Thu, Feb 2, 2023 at 12:56 PM Nikolay Samokhvalov wrote: > I already realized my mistake – indeed, having multiple errors for 1 index > doesn't seem to be super practically helpful. I wouldn't mind supporting it if the cost wasn't too high. But I believe that it's not a good trade-off. >> I th

Re: Amcheck verification of GiST and GIN

On Thu, Feb 2, 2023 at 12:43 PM Peter Geoghegan wrote: > I agree that this matters at the level of whole indexes. > I already realized my mistake – indeed, having multiple errors for 1 index doesn't seem to be super practically helpful. > I think that that problem should be solved at a higher

Re: Amcheck verification of GiST and GIN

On Thu, Feb 2, 2023 at 12:31 PM Nikolay Samokhvalov wrote: > I understand your thoughts (I think) and agree with them, but at least one > scenario where I do want to see *all* errors is corruption prevention – > running > amcheck in lower environments, not in production, to predict and prevent >

Re: Amcheck verification of GiST and GIN

On Thu, Feb 2, 2023 at 12:15 PM Peter Geoghegan wrote: > On Thu, Feb 2, 2023 at 11:51 AM Peter Geoghegan wrote: > ... > Admittedly there is some value in seeing multiple WARNINGs to true > experts that are performing some kind of forensic analysis, but that > doesn't seem worth it to me -- I'm

Re: Amcheck verification of GiST and GIN

On Thu, Feb 2, 2023 at 11:51 AM Peter Geoghegan wrote: > I also have some questions about the verification functionality itself: I forgot to include another big concern here: * Why are there only WARNINGs, never ERRORs here? It's far more likely that you'll run into problems when running amchec

Re: Amcheck verification of GiST and GIN

On Fri, Jan 13, 2023 at 8:15 PM Andrey Borodin wrote: > (v21 of patch series) I can see why the refactoring patch is necessary overall, but I have some concerns about the details. More specifically: * PageGetItemIdCareful() doesn't seem like it needs to be moved to amcheck.c and generalized to w

Re: Amcheck verification of GiST and GIN

Hi Andrey, > Thanks! I also found out that there was a CI complaint about amcheck.h > not including some necessary stuff. Here's a version with a fix for > that. Thanks for the updated patchset. One little nitpick I have is that the tests cover only cases when all the checks pass successfully. T

Re: Amcheck verification of GiST and GIN

On Fri, Jan 13, 2023 at 7:35 PM Jose Arthur Benetasso Villanova wrote: > > Hello again. I see the change. Thanks > Thanks! I also found out that there was a CI complaint about amcheck.h not including some necessary stuff. Here's a version with a fix for that. Best regards, Andrey Borodin. v21-

Re: Amcheck verification of GiST and GIN

On Fri, 13 Jan 2023, Andrey Borodin wrote: On Fri, Jan 13, 2023 at 3:46 AM Jose Arthur Benetasso Villanova wrote: The only thing that I found is the gin_index_parent_check function in docs still references the "gin_index_parent_check(index regclass, heapallindexed boolean) returns void"

Re: Amcheck verification of GiST and GIN

On Fri, Jan 13, 2023 at 3:46 AM Jose Arthur Benetasso Villanova wrote: > > The only thing that I found is the gin_index_parent_check function in docs > still references the "gin_index_parent_check(index regclass, > heapallindexed boolean) returns void" > Correct! Please find the attached fixed ve

Re: Amcheck verification of GiST and GIN

On Sun, 8 Jan 2023, Andrey Borodin wrote: On Sun, Jan 8, 2023 at 8:05 PM Andrey Borodin wrote: Please find the attached new version. In this patchset heapallindexed flag is removed from GIN checks. Uh... sorry, git-formatted wrong branch. Here's the correct version. Double checked. Hel

Re: Amcheck verification of GiST and GIN

On Sun, Jan 8, 2023 at 8:05 PM Andrey Borodin wrote: > > Please find the attached new version. In this patchset heapallindexed > flag is removed from GIN checks. > Uh... sorry, git-formatted wrong branch. Here's the correct version. Double checked. Best regards, Andrey Borodin. v19-0003-Add-gin

Re: Amcheck verification of GiST and GIN

Hi Jose, thank you for review and sorry for so long delay to answer. On Wed, Dec 14, 2022 at 4:19 AM Jose Arthur Benetasso Villanova wrote: > > > On Sun, 27 Nov 2022, Andrey Borodin wrote: > > > On Sun, Nov 27, 2022 at 1:29 PM Andrey Borodin > > wrote: > >> > > I was wrong. GIN check does simil

Re: Amcheck verification of GiST and GIN

On Wed, Dec 14, 2022 at 7:19 AM Jose Arthur Benetasso Villanova wrote: > I'm a bit lost here. I tried your patch again and indeed the > heapallindexed inside gin_check_parent_keys_consistency has a TODO > comment, but it's unclear to me if you are going to implement it or if the > patch "needs rev

Re: Amcheck verification of GiST and GIN

On Sun, 27 Nov 2022, Andrey Borodin wrote: On Sun, Nov 27, 2022 at 1:29 PM Andrey Borodin wrote: I was wrong. GIN check does similar gin_refind_parent() to lock pages in bottom-up manner and truly verify downlink-child_page invariant. Does this mean that we need the adjustment in docs?

Re: Amcheck verification of GiST and GIN

On Sun, Nov 27, 2022 at 1:29 PM Andrey Borodin wrote: > > GiST verification checks only one invariant that can be verified if > page locks acquired the same way as page split does. > GIN does not require ShareLock because it does not check cross-level > invariants. > I was wrong. GIN check does

Re: Amcheck verification of GiST and GIN

Hello! Thank you for the review! On Thu, Nov 24, 2022 at 6:04 PM Jose Arthur Benetasso Villanova wrote: > > It compiled with those 2 warnings: > > verify_gin.c: In function 'gin_check_parent_keys_consistency': > verify_gin.c:481:38: warning: declaration of 'maxoff' shadows a previous > local [-W

Re: Amcheck verification of GiST and GIN

Hello. I reviewed this patch and I would like to share some comments. It compiled with those 2 warnings: verify_gin.c: In function 'gin_check_parent_keys_consistency': verify_gin.c:481:38: warning: declaration of 'maxoff' shadows a previous local [-Wshadow=compatible-local] 481 |

Re: Amcheck verification of GiST and GIN

On Sun, Oct 2, 2022 at 12:12 AM Andres Freund wrote: > > Here's an updated patch adding meson compat. Thank you, Andres! Here's one more rebase (something was adjusted in amcheck build). Also I've fixed new warnings except warning about absent heapallindexed for GIN. It's a TODO. Thanks! Best r

Re: Amcheck verification of GiST and GIN

Hi, On 2022-09-22 08:19:09 -0700, Andres Freund wrote: > Hi, > > On 2022-08-17 17:28:02 +0500, Andrey Borodin wrote: > > Here's v13. Changes: > > 1. Fixed passing through downlink in GIN index > > 2. Fixed GIN tests (one test case was not working) > > > > Thanks to Vitaliy Kukharik for trying th

Re: Amcheck verification of GiST and GIN

Hi, On 2022-08-17 17:28:02 +0500, Andrey Borodin wrote: > Here's v13. Changes: > 1. Fixed passing through downlink in GIN index > 2. Fixed GIN tests (one test case was not working) > > Thanks to Vitaliy Kukharik for trying this patches. Due to the merge of the meson based build, this patch needs

Re: Amcheck verification of GiST and GIN

> On 23 Jul 2022, at 14:40, Andrey Borodin wrote: > > Done. PFA attached patchset. > > Best regards, Andrey Borodin. > Here's v13. Changes: 1. Fixed passing through downlink in GIN index 2. Fixed GIN tests (one test case was not working) Thanks to Vitaliy Kukharik for trying this patches.

Re: Amcheck verification of GiST and GIN

> On 26 Jun 2022, at 00:10, Andrey Borodin wrote: > > I will split the patch in 3 steps: > 1. extract generic functions to amcheck.c > 2. add gist functions > 3. add gin functions > > I'll fix other notes too in the next version. Done. PFA attached patchset. Thanks! Best regards, Andrey Bo

Re: Amcheck verification of GiST and GIN

> On 23 Jun 2022, at 00:27, Nikolay Samokhvalov wrote: > > Since you're talking to yourself, just wanted to support you – this is an > important thing, definitely should be very useful for many projects; I hope > to find time to test it in the next few days. Thanks Nikolay! > On 23 Jun 2

Re: Amcheck verification of GiST and GIN

Hi, I think having amcheck for more indexes is great. On 2022-06-22 20:40:56 +0300, Andrey Borodin wrote: >> diff --git a/contrib/amcheck/amcheck.c b/contrib/amcheck/amcheck.c > new file mode 100644 > index 00..7a222719dd > --- /dev/null > +++ b/contrib/amcheck/amcheck.c > @@ -0,0 +1,187

Re: Amcheck verification of GiST and GIN

On Wed, Jun 22, 2022 at 11:35 AM Andrey Borodin wrote: > > On 30 May 2022, at 12:40, Andrey Borodin wrote: > > > > What do you think? > > Hi Andrey! > Hi Andrey! Since you're talking to yourself, just wanted to support you – this is an important thing, definitely should be very useful for many

Re: Amcheck verification of GiST and GIN

> On 30 May 2022, at 12:40, Andrey Borodin wrote: > > What do you think? Hi Andrey! Here's a version with better tests. I've made sure that GiST tests actually trigger page reuse after deletion. And enhanced comments in both GiST and GIN test scripts. I hope you'll like it. GIN heapallinde