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 <andres@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
