I wrote:
> After some study I propose that the correct fix is that
> _SPI_execute_plan should act as though we're inside an atomic
> context if IsSubTransaction(). We are inside an atomic context
> so far as _SPI_commit and _SPI_rollback are concerned: they
> will not allow you to COMMIT/ROLLBACK. So it's not very clear
> why _SPI_execute_plan should think differently.
I dug into this further and identified exactly where it's going off
the rails. As things stand, _SPI_execute_plan thinks it can operate
non-atomically, so it doesn't push a snapshot and it passes
PROCESS_UTILITY_QUERY_NONATOMIC to ProcessUtility. But in
standard_ProcessUtility we find
bool isAtomicContext = (!(context == PROCESS_UTILITY_TOPLEVEL || context == PROCESS_UTILITY_QUERY_NONATOMIC)
||IsTransactionBlock());
IsTransactionBlock() is always true in a subtransaction, so we pass
down atomic = true to ExecuteCallStmt, which then thinks it doesn't
need to push a snapshot either. End result is that the stable
function runs with the Portal snapshot and sees stale data.
So my patch fixes it by ensuring that _SPI_execute_plan will think
it's atomic in the same cases that standard_ProcessUtility will force
that. It's a little concerning though that standard_ProcessUtility is
arriving at its result through a completely different bit of
reasoning. There's an argument to be made that the above-quoted bit
of logic needs revision.
Nonetheless, it seems correct to me that the different parts of spi.c
share identical rules for what is a nonatomic execution environment.
I'm also fairly leery of touching standard_ProcessUtility's logic in a
back-patch --- that seems way too likely to have unforeseen side
effects on unrelated parts of the system. So I went ahead and pushed
what I had (after a bit more work on the comments). We can think
about whether to revise the standard_ProcessUtility logic at leisure.
regards, tom lane
