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 builds to GIN and GiST indexes - I scripts to stress-test the builds, and I added the new amcheck functions as another validation step. For GIN indexes it didn't find anything new (in either this or my patches), aside from the assert crash I already reported. But for GiST it turned out to be very valuable - it did actually find an issue in my patches, or rather confirm my hypothesis that the way the patch generates fake LSN may not be quite right. In particular, I've observed these two issues: ERROR: heap tuple (13315,38) from table "planet_osm_roads" lacks matching index tuple within index "roads_7_1_idx" ERROR: index "roads_7_7_idx" has inconsistent records on page 23723 offset 113 And those consistency issues are real - I've managed to reproduce issues with incorrect query results (by comparing the results to an index built without parallelism). So that's nice - it shows the value of this patch, and I like it. One thing I've been wondering about is that currently amcheck (in general, not just these new GIN/GiST functions) errors out on the first issue, because it does ereport(ERROR). Which is good enough to decide if there is some corruption, but a bit inconvenient if you need to assess how much corruption is there. For example when investigating the issue in my patch it would have been great to know if there's just one broken page, or if there are dozens/hundreds/thousands of them. I'd imagine we could have a flag which says whether to fail on the first issue, or keep looking at future pages. Essentially, whether to do ereport(ERROR) or ereport(WARNING). But maybe that's a dead-end, and once we find the first issue it's futile to inspect the rest of the index, because it can be garbage. Not sure. In any case, it's not up to this patch to invent that. I don't have additional comments, the patch seems to be clean and likely ready to go. There's a couple committers already involved in this thread, I wonder if one of them already planned to take care of this? Peter and Andres, either of you interested in this? -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company