Hi,
On 2022-04-07 14:36:35 +0900, Michael Paquier wrote:
> On Mon, Mar 07, 2022 at 10:17:41PM +0800, Julien Rouhaud wrote:
> > On Mon, Mar 07, 2022 at 01:27:40PM +0000, Simon Riggs wrote:
> >> 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.
>
> Agreed as well. That's worth doing in isolation and that will save
> some lookups of pg_subtrans anyway while being simple. As mentioned
> upthread, this needed an indentation, and the renaming of
> cachedFetchXid to cachedFetchSubXid looks adapted. So.. Applied all
> those things.
As is, this strikes me as dangerous. At the very least this ought to be
structured so it can have assertions verifying that the cache contents are
correct.
It's far from obvious that it is correct to me, fwiw. Potential issues:
1) The result of SubTransGetTopmostTransaction() can change between subsequent
calls. If TransactionXmin advances, the TransactionXmin cutoff can change
the result. It might be unreachable or harmless, but it's not obvious that
it is, and there's zero comments explaining why it is obvious.
2) xid wraparound. There's nothing forcing SubTransGetTopmostTransaction() to
be called regularly, so even if a backend isn't idle, the cache could just
get more and more outdated until hitting wraparound
To me it also seems odd that we cache in SubTransGetTopmostTransaction(), but
not in SubTransGetParent(). I think it's at least as common to end up with
subtrans access via TransactionIdDidCommit(), which calls SubTransGetParent()
rather than SubTransGetTopmostTransaction()? Why is
SubTransGetTopmostTransaction() the correct layer for caching?
I tried to find a benchmark result for this patch upthread, without
success. Has there been validation this helps with anything?
Greetings,
Andres Freund