From: Tom Lane <tgl@sss.pgh.pa.us>
To: Peter Eisentraut <peter@eisentraut.org>
cc: panso8@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18656: "STABLE" function sometimes does not see changes
In-reply-to: <4082805.1729014675@sss.pgh.pa.us>
References: <18656-cade1780866ef66c@postgresql.org>
 <4082805.1729014675@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Tue, 15 Oct 2024 13:51:15 -0400"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <480024.1729114823.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Wed, 16 Oct 2024 17:40:23 -0400
Message-ID: <480025.1729114823@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/480025.1729114823%40sss.pgh.pa.us>
Precedence: bulk

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 =3D (!(context =3D=3D PROCESS_UTILITY_TOPL=
EVEL || context =3D=3D PROCESS_UTILITY_QUERY_NONATOMIC) || IsTransactionBl=
ock());

IsTransactionBlock() is always true in a subtransaction, so we pass
down atomic =3D 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