On Mon, 7 Mar 2022 at 09:49, Julien Rouhaud <rjuju123@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/
