Way back in 2011, commit 57eb009092684e6e1788dd0dae641ccee1668b10
moved AbortTransaction's AtEOXact_Snapshot call to CleanupTransaction
to fix a problem when a ROLLBACK statement was prepared at the
protocol level and executed in a transaction with REPEATABLE READ or
higher isolation. RevalidateCachedQuery would attempt to obtain a
snapshot and end up failing. At the time, it was judged that
plancache.c was behaving correctly and this logic was rejiggered to
make that coding pattern safe. However, commit
ac63dca607e8e22247defbc8fe03b6baa3628c42 subsequently taught
RevalidateCachedQuery not to obtain a snapshot for such commands after
all while fixing an unrelated bug, and there now seems to be no case
in which we obtain a snapshot in an aborted transaction.
I'd like to propose that we upgrade that practice to a formal rule.
We've already taken some steps in this direction; notably, commit
42c80c696e9c8323841180029cc62741c21bd356 added an assertion to the
effect that we never perform catcache lookups outside of a valid,
non-aborted transaction. However, right now, if you made the mistake
of trying to access the database through some means other than a
catcache lookup in an aborted transaction, it might appear to work.
Actually, it would be terribly unsafe, because (1) you might've failed
after inserting a heap tuple and before inserting all the
corresponding index tuples and (2) any random subset of the tuples
inserted by prior commands in your transaction might have been pruned
by other backends after you removed your XID from the ProcArray, while
others would remain visible. In short, such a snapshot is not really
a snapshot at all.
The best way that I've been able to come up with to enforce this rule
after a little bit of thought is to add Assert(IsTransactionState())
to a bunch of functions in snapmgr.c, most notably
GetTransactionSnapshot and GetCatalogSnapshot. The attached patch does
that. It also makes the comments in RevalidateCachedQuery more
explicit about this issue, and it moves the AtEOXact_Snapshot call
back to AbortTransaction, on the theory (or hope?) that it's better to
dispose of resources sooner, especially resources that might look
superficially valid but really are not.
You may (or may not) wonder why I'm poking at this apparently-obscure
topic. The answer is "undo." Without getting into the gory details,
it's better for undo if as much of the cleanup work as possible
happens at AbortTransaction() time and as little as possible is left
until CleanupTransaction(). That seems like a good idea on general
principle too, though, so I'm proposing this as an independent
cleanup.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company