On Mon, 7 Mar 2022 at 09:49, Julien Rouhaud <rjuju...@gmail.com> wrote: > > Hi, > > On Mon, Jan 17, 2022 at 01:44:02PM +0000, Simon Riggs wrote: > > > > Re-attached, so that the CFapp isn't confused between the multiple > > patches on this thread. > > Thanks a lot for working on this! > > The patch is simple and overall looks good to me. A few comments though: > > > +/* > + * Single-item cache for results of SubTransGetTopmostTransaction. It's > worth having > + * such a cache because we frequently find ourselves repeatedly checking the > + * same XID, for example when scanning a table just after a bulk insert, > + * update, or delete. > + */ > +static TransactionId cachedFetchXid = InvalidTransactionId; > +static TransactionId cachedFetchTopmostXid = InvalidTransactionId; > > The comment is above the 80 chars after > s/TransactionLogFetch/SubTransGetTopmostTransaction/, and I don't think this > comment is valid for subtrans.c.
What aspect makes it invalid? The comment seems exactly applicable to me; Andrey thinks so also. > Also, maybe naming the first variable cachedFetchSubXid would make it a bit > clearer? Sure, that can be done. > It would be nice to see some benchmarks, for both when this change is > enough to avoid a contention (when there's a single long-running overflowed > backend) and when it's not enough. That will also be useful if/when working > on > the "rethink_subtrans" patch. The patch doesn't do anything about the case of when there's a single long-running overflowed backend, nor does it claim that. The patch will speed up calls to SubTransGetTopmostTransaction(), which occur in src/backend/access/heap/heapam.c src/backend/utils/time/snapmgr.c src/backend/storage/lmgr/lmgr.c src/backend/storage/ipc/procarray.c The patch was posted because TransactionLogFetch() has a cache, yet SubTransGetTopmostTransaction() does not, yet the argument should be identical in both cases. -- Simon Riggs http://www.EnterpriseDB.com/
