On 7/22/24 11:00, Andrey M. Borodin wrote: > > >> On 21 Jul 2024, at 23:42, Tomas Vondra <tomas.von...@enterprisedb.com> wrote: >> >>> >>> 1. Do I get it right that is_parallel argument for gistGetFakeLSN() >>> is only needed for assertion? And this assertion can be ensured just >>> by inspecting code. Is it really necessary? >> >> Yes, in the patch it's only used for an assert. But it's actually >> incorrect - just as I speculated in my initial message (in the section >> about gistGetFakeLSN), it sometimes fails to detect a page split. I >> noticed that while testing the patch adding GiST to amcheck, and I >> reported that in that thread [1]. But I apparently forgot to post an >> updated version of this patch :-( > > Oops, I just though that it's a version with solved FakeLSN problem. > >> >> I'll post a new version tomorrow, but in short it needs to update the >> fake LSN even if (lastlsn != currlsn) if is_parallel=true. It's a bit >> annoying this means we generate a new fake LSN on every call, and I'm >> not sure that's actually necessary. But I've been unable to come up with >> a better condition when to generate a new LSN. > > Why don't we just use an atomic counter withtin shared build state? >
I don't understand how would that solve the problem, can you elaborate? Which of the values are you suggesting should be replaced with the shared counter? lastlsn? >> >> [1] >> https://www.postgresql.org/message-id/79622955-6d1a-4439-b358-ec2b6a9e7cbf%40enterprisedb.com > Yes, I'll be back in that thread soon. I'm still on vacation and it's hard to > get continuous uninterrupted time here. You did a great review, and I want to > address all issues there wholistically. Thank you! > >>> 2. gistBuildParallelCallback() updates indtuplesSize, but it seems to be >>> not used anywhere. AFAIK it's only needed to buffered build. 3. I >>> think we need a test that reliably triggers parallel and serial >>> builds. >>> >> >> Yeah, it's possible the variable is unused. Agreed on the testing. >> >>> As far as I know, there's a well known trick to build better GiST >>> over PostGIS data: randomize input. I think parallel scan is just >>> what is needed, it will shuffle tuples enough... >>> >> >> I'm not sure I understand this comment. What do you mean by "better >> GiST" or what does that mean for this patch? > > I think parallel build indexes will have faster IndexScans. > That's interesting. I haven't thought about measuring stuff like that (and it hasn't occurred to me it might have this benefit, or why would that be the case). regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company