On Fri, May 27, 2022 at 08:55:02AM -0700, Andres Freund wrote:
> On 2022-05-27 15:44:39 +0530, Amit Kapila wrote:
>> Won't in theory the similar cache in transam.c is also prone to
>> similar behavior?
TransactionIdDidCommit() and TransactionIdDidAbort() are used in much
more code paths for visibility purposes, contrary to the subtrans.c
ones.
> It's not quite the same risk, because there we are likely to actually hit the
> cache regularly. Whereas quite normal workloads might not hit this cache for
> days on end.
Yeah. In short, this mostly depends on the use of savepoints and the
number of XIDs issued until PGPROC_MAX_CACHED_SUBXIDS is reached, and
a single cache entry in this code path would reduce the pressure on
the SLRU lookups depending on the number of queries issued, for
example. One thing I know of that likes to abuse of savepoints and
could cause overflows to make this easier to hit is the ODBC driver
coupled with short queries in long transactions, where its internals
enforce the use of a savepoint each time a query is issued by an
application (pretty much what the benchmark at the top of the thread
does). In this case, even the single cache approach would not help
much because I recall that we finish with one savepoint per query to
be able to rollback to any previous state within a given transaction
(as the ODBC APIs allow).
Doing a caching within SubTransGetParent() would be more interesting,
for sure, though the invalidation to clean the cache and to make that
robust enough may prove tricky.
It took me some time to come back to this thread. The change has now
been reverted.
--
Michael
