On Mon, Mar 07, 2022 at 01:27:40PM +0000, Simon Riggs wrote:
> > +/*
> > + * 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.
Sorry, I somehow missed the "for example", and was thinking that
SubTransGetTopmostTransaction was used in many other places compared to
TransactionIdDidCommit and friends.
> > 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.
I was thinking that having a cache for SubTransGetTopmostTransaction could help
at least to some extent for that problem, sorry if that's not the case.
I'm still curious on how much this simple optimization can help in some
scenarios, even if they're somewhat artificial.
> The patch was posted because TransactionLogFetch() has a cache, yet
> SubTransGetTopmostTransaction() does not, yet the argument should be
> identical in both cases.
I totally agree with that.