From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@lists.postgresql.org
cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
        Victor Yegorov <vyegorov@gmail.com>,
        Pierre Forstmann <pierre.forstmann@gmail.com>
Subject: Re: Unexpected results from CALL and AUTOCOMMIT=off
In-reply-to: <90052.1717442890@sss.pgh.pa.us>
References: <CAGnEboiRe+fG2QxuBO2390F7P8e2MQ6UyBjZSL_w1Cej+E4=Vw@mail.gmail.com> <CAM-sOH80Z=OqYBWgYP=BDGLbxQ72wkC9=tZ-vRKxkmKkhTD7MQ@mail.gmail.com> <CAGnEbojf5Awm862ghvooLku6mm0m4yF60PyHQxCinM8pn01sbw@mail.gmail.com> <90052.1717442890@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Mon, 03 Jun 2024 15:28:10 -0400"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <378270.1717464735.0@sss.pgh.pa.us>
Date: Mon, 03 Jun 2024 21:32:28 -0400
Message-ID: <378291.1717464748@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/378291.1717464748%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <378270.1717464735.1@sss.pgh.pa.us>

[ redirecting to pgsql-hackers ]

I wrote:
> I agree that this looks like a bug, since your example shows that the
> same function works as-expected in an ordinary expression but not in
> a CALL.  The dependency on AUTOCOMMIT (that is, being within an outer
> transaction block) seems even odder.  I've not dug into it yet, but
> I suppose we're passing the wrong snapshot to the CALL arguments.

I poked into this and found that the source of the problem is that
plpgsql's exec_stmt_call passes allow_nonatomic = true even when
it's running in an atomic context.  So we can fix it with basically
a one-line change:

-	options.allow_nonatomic = true;
+	options.allow_nonatomic = !estate->atomic;

I'm worried about whether external callers might've made a comparable
mistake, but I think all we can do is document it a little better.
AFAICS there isn't any good way for spi.c to realize that this mistake
has been made, else we could have it patch up the mistake centrally.
I've not attempted to make those doc updates in the attached draft
patch though, nor have I added a test case yet.

Before realizing that this was the issue, I spent a fair amount of
time on the idea that _SPI_execute_plan() was doing things wrong,
and that led me to notice that its comment about having four modes
of snapshot operation has been falsified in multiple ways.  So this
draft does include fixes for that comment.

Thoughts?

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="fix-call-in-atomic-context-draft.patch";
	charset="us-ascii"
Content-ID: <378270.1717464735.2@sss.pgh.pa.us>
Content-Description: fix-call-in-atomic-context-draft.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index a97a7e3bd4..c93b6c192f 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -2425,12 +2425,17 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecut=
eOptions *options,
 	 * snapshot !=3D InvalidSnapshot, read_only =3D false: use the given sna=
pshot,
 	 * modified by advancing its command ID before each querytree.
 	 *
-	 * snapshot =3D=3D InvalidSnapshot, read_only =3D true: use the entry-ti=
me
-	 * ActiveSnapshot, if any (if there isn't one, we run with no snapshot).
+	 * snapshot =3D=3D InvalidSnapshot, read_only =3D true: do nothing for q=
ueries
+	 * that require no snapshot.  For those that do, ensure that a Portal
+	 * snapshot exists; then use that, or use the entry-time ActiveSnapshot =
if
+	 * that exists and is different.
 	 *
-	 * snapshot =3D=3D InvalidSnapshot, read_only =3D false: take a full new
-	 * snapshot for each user command, and advance its command ID before eac=
h
-	 * querytree within the command.
+	 * snapshot =3D=3D InvalidSnapshot, read_only =3D false: do nothing for =
queries
+	 * that require no snapshot.  For those that do, ensure that a Portal
+	 * snapshot exists; then, in atomic execution (!allow_nonatomic) take a
+	 * full new snapshot for each user command, and advance its command ID
+	 * before each querytree within the command.  In allow_nonatomic mode we
+	 * just use the Portal snapshot unmodified.
 	 *
 	 * In the first two cases, we can just push the snap onto the stack once
 	 * for the whole plan list.
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 6947575b94..54ede9d85e 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -2218,12 +2218,12 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_=
stmt_call *stmt)
 	 * for the plan.  This avoids refcount leakage complaints if the called
 	 * procedure ends the current transaction.
 	 *
-	 * Also, tell SPI to allow non-atomic execution.
+	 * Also, tell SPI to allow non-atomic execution if appropriate.
 	 */
 	memset(&options, 0, sizeof(options));
 	options.params =3D paramLI;
 	options.read_only =3D estate->readonly_func;
-	options.allow_nonatomic =3D true;
+	options.allow_nonatomic =3D !estate->atomic;
 	options.owner =3D estate->procedure_resowner;
 =

 	rc =3D SPI_execute_plan_extended(expr->plan, &options);

------- =_aaaaaaaaaa0--