From: Tom Lane <tgl@sss.pgh.pa.us>
To: notegihu@confused.at
cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17199: Calling stored procedure with stable function as
 argument results in wrong result
In-reply-to: <1980152.1632236471@sss.pgh.pa.us>
References: <17199-1ab2561f0d94af92@postgresql.org>
 <1980152.1632236471@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Tue, 21 Sep 2021 11:01:11 -0400"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <2031569.1632239314.0@sss.pgh.pa.us>
Date: Tue, 21 Sep 2021 11:49:56 -0400
Message-ID: <2031644.1632239396@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/2031644.1632239396%40sss.pgh.pa.us>
Precedence: bulk

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

I wrote:
> Ugh.  Looks like I broke this in 84f5c2908, by not thinking about the
> possibility that a CALL's argument expressions would need an up-to-date
> snapshot.

Concretely, the attached seems to take care of it.  I'll refrain
from pushing this till later, though, since v14 is frozen for rc1
for a few hours more.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff;
	name="eval-CALL-arguments-with-fresh-snapshot.patch";
	charset="us-ascii"
Content-ID: <2031569.1632239314.2@sss.pgh.pa.us>
Content-Description: eval-CALL-arguments-with-fresh-snapshot.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/fu=
nctioncmds.c
index 79d875ab10..38e78f7102 100644
--- a/src/backend/commands/functioncmds.c
+++ b/src/backend/commands/functioncmds.c
@@ -73,6 +73,7 @@
 #include "utils/lsyscache.h"
 #include "utils/memutils.h"
 #include "utils/rel.h"
+#include "utils/snapmgr.h"
 #include "utils/syscache.h"
 #include "utils/typcache.h"
 =

@@ -2250,6 +2251,16 @@ ExecuteCallStmt(CallStmt *stmt, ParamListInfo param=
s, bool atomic, DestReceiver
 	estate->es_param_list_info =3D params;
 	econtext =3D CreateExprContext(estate);
 =

+	/*
+	 * If we're called in non-atomic context, we also have to ensure that th=
e
+	 * argument expressions run with an up-to-date snapshot.  Our caller wil=
l
+	 * have provided a current snapshot in atomic contexts, but not in
+	 * non-atomic contexts, because the possibility of a COMMIT/ROLLBACK
+	 * destroying the snapshot makes higher-level management too complicated=
.
+	 */
+	if (!atomic)
+		PushActiveSnapshot(GetTransactionSnapshot());
+
 	i =3D 0;
 	foreach(lc, fexpr->args)
 	{
@@ -2267,20 +2278,23 @@ ExecuteCallStmt(CallStmt *stmt, ParamListInfo para=
ms, bool atomic, DestReceiver
 		i++;
 	}
 =

+	/* Get rid of temporary snapshot for arguments, if we made one */
+	if (!atomic)
+		PopActiveSnapshot();
+
+	/* Here we actually call the procedure */
 	pgstat_init_function_usage(fcinfo, &fcusage);
 	retval =3D FunctionCallInvoke(fcinfo);
 	pgstat_end_function_usage(&fcusage, true);
 =

+	/* Handle the procedure's outputs */
 	if (fexpr->funcresulttype =3D=3D VOIDOID)
 	{
 		/* do nothing */
 	}
 	else if (fexpr->funcresulttype =3D=3D RECORDOID)
 	{
-		/*
-		 * send tuple to client
-		 */
-
+		/* send tuple to client */
 		HeapTupleHeader td;
 		Oid			tupType;
 		int32		tupTypmod;
diff --git a/src/pl/plpgsql/src/expected/plpgsql_transaction.out b/src/pl/=
plpgsql/src/expected/plpgsql_transaction.out
index 57ab0bc0e7..f79f847321 100644
--- a/src/pl/plpgsql/src/expected/plpgsql_transaction.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_transaction.out
@@ -600,6 +600,27 @@ SELECT * FROM test2;
  42
 (1 row)
 =

+-- another snapshot handling case: argument expressions of a CALL need
+-- to be evaluated with an up-to-date snapshot
+CREATE FUNCTION report_count() RETURNS int
+STABLE LANGUAGE sql
+AS $$ SELECT COUNT(*) FROM test2 $$;
+CREATE PROCEDURE transaction_test9b(cnt int)
+LANGUAGE plpgsql
+AS $$
+BEGIN
+  RAISE NOTICE 'count =3D %', cnt;
+END
+$$;
+DO $$
+BEGIN
+    CALL transaction_test9b(report_count());
+    INSERT INTO test2 VALUES(43);
+    CALL transaction_test9b(report_count());
+END
+$$;
+NOTICE:  count =3D 1
+NOTICE:  count =3D 2
 -- Test transaction in procedure with output parameters.  This uses a
 -- different portal strategy and different code paths in pquery.c.
 CREATE PROCEDURE transaction_test10a(INOUT x int)
diff --git a/src/pl/plpgsql/src/sql/plpgsql_transaction.sql b/src/pl/plpgs=
ql/src/sql/plpgsql_transaction.sql
index 8e4783c9a5..888ddccace 100644
--- a/src/pl/plpgsql/src/sql/plpgsql_transaction.sql
+++ b/src/pl/plpgsql/src/sql/plpgsql_transaction.sql
@@ -507,6 +507,29 @@ $$;
 SELECT * FROM test2;
 =

 =

+-- another snapshot handling case: argument expressions of a CALL need
+-- to be evaluated with an up-to-date snapshot
+CREATE FUNCTION report_count() RETURNS int
+STABLE LANGUAGE sql
+AS $$ SELECT COUNT(*) FROM test2 $$;
+
+CREATE PROCEDURE transaction_test9b(cnt int)
+LANGUAGE plpgsql
+AS $$
+BEGIN
+  RAISE NOTICE 'count =3D %', cnt;
+END
+$$;
+
+DO $$
+BEGIN
+    CALL transaction_test9b(report_count());
+    INSERT INTO test2 VALUES(43);
+    CALL transaction_test9b(report_count());
+END
+$$;
+
+
 -- Test transaction in procedure with output parameters.  This uses a
 -- different portal strategy and different code paths in pquery.c.
 CREATE PROCEDURE transaction_test10a(INOUT x int)

------- =_aaaaaaaaaa0--