Hi, On 2020-03-17 23:59:14 +1300, David Rowley wrote: > Nice performance gains.
Thanks. > On Sun, 1 Mar 2020 at 21:36, Andres Freund <and...@anarazel.de> wrote: > 2. I don't quite understand your change in > UpdateSubscriptionRelState(). snap seems unused. Drilling down into > SearchSysCacheCopy2, in SearchCatCacheMiss() the systable_beginscan() > passes a NULL snapshot. > > the whole patch does this. I guess I don't understand why 0002 does this. See the thread at https://postgr.es/m/20200229052459.wzhqnbhrriezg4v2%40alap3.anarazel.de Basically, the way catalog snapshots are handled right now, it's not correct to much without a snapshot held. Any concurrent invalidation can cause the catalog snapshot to be released, which can reset the backend's xmin. Which in turn can allow for pruning etc to remove required data. This is part of this series only because I felt I needed to add stronger asserts to be confident in what's happening. And they started to trigger all over :( - and weren't related to the patchset :(. > 4. Did you consider the location of 'delayChkpt' in PGPROC. Couldn't > you slot it in somewhere it would fit in existing padding? > > 0007 Hm, maybe. I'm not sure what the best thing to do here is - there's some arguments to be made that we should keep the fields moved from PGXACT together on their own cacheline. Compared to some of the other stuff in PGPROC they're still accessed from other backends fairly frequently. > 5. GinPageIsRecyclable() has no comments at all. I know that > ginvacuum.c is not exactly the modal citizen for function header > comments, but likely this patch is no good reason to continue the > trend. Well, I basically just moved the code from the macro of the same name... I'll add something. > 9. I get the idea you don't intend to keep the debug message in > InvisibleToEveryoneTestFullXid(), but if you do, then shouldn't it be > using UINT64_FORMAT? Yea, I don't intend to keep them - they're way too verbose, even for DEBUG*. Note that there's some advantage in the long long cast approach - it's easier to deal with for translations IIRC. > 13. is -> are > > * accessed data is stored contiguously in memory in as few cache lines as Oh? 'data are stored' sounds wrong to me, somehow. Greetings, Andres Freund
