Thread: BUG #17199: Calling stored procedure with stable function as argument results in wrong result

The following bug has been logged on the website:

Bug reference:      17199
Logged by:          Alexander Nawratil
Email address:      notegihu@confused.at
PostgreSQL version: 13.4
Operating system:   Linux
Description:

Hello,

we are facing some inconsistencies in one of our unit tests since the last
upgrade of PostgreSQL to 11.13 and 13.4.
When a function that returns just a row count of a table is marked as STABLE
and is called from a stored procedure as argument, the result of the
function is different than when the function is called beforehand and stored
to a local variable.
It's working fine when the function is marked as VOLATILE, or on PostgreSQL
11.12 / 13.3.

Steps to reproduce:

1. Create an empty table
2. Create a function that is marked as STABLE and returns the row count of
the table (RETURN SELECT COUNT(*) FROM table)
3. Create a procedure that takes an integer parameter and simply outputs it,
e.g. RAISE NOTICE
4. In a single DO block: INSERT a row into the table, then CALL
proc(func()); /* --> output: 0, instead of 1 */
5. Storing the result of func() to a variable x, then CALL proc(x); /* -->
output: 1, as expected */

Example SQL:

CREATE TABLE tbl (col INT4);

CREATE OR REPLACE FUNCTION stable_func()
    RETURNS INT4
    STABLE AS
$code$
BEGIN
    RETURN (SELECT COUNT(1) FROM tbl);
END
$code$ LANGUAGE plpgsql;

CREATE OR REPLACE FUNCTION volatile_func()
    RETURNS INT4
    VOLATILE AS
$code$
BEGIN
    RETURN (SELECT COUNT(1) FROM tbl);
END
$code$ LANGUAGE plpgsql;


CREATE OR REPLACE PROCEDURE call_proc(t TEXT, cnt INT4)
AS
$code$
BEGIN
    RAISE NOTICE '%', (t || ' -- count: ' || cnt::TEXT);
END
$code$ LANGUAGE plpgsql;

DO
$$
DECLARE
    x INT4;
BEGIN
    RAISE NOTICE '%', (VERSION());
    INSERT INTO tbl(col) VALUES (1);

    x := volatile_func();
    CALL call_proc('from_var_volatile_func', x);
    CALL call_proc('inline_volatile_func', volatile_func());

    x := stable_func();
    CALL call_proc('from_var_stable_func', x);
    CALL call_proc('inline_stable_func', stable_func());
END
$$;

I would expect that all calls output the same row count of 1. However, the
actual value differs for the last call since the upgrade.

Output on Postgres 13.3:
PostgreSQL 13.3 (Debian 13.3-1.pgdg100+1) on x86_64-pc-linux-gnu, compiled
by gcc (Debian 8.3.0-6) 8.3.0, 64-bit
from_var_volatile_func -- count: 1
inline_volatile_func -- count: 1
from_var_stable_func -- count: 1
inline_stable_func -- count: 1

Output on Postgres 13.4:
PostgreSQL 13.4 (Debian 13.4-1.pgdg100+1) on x86_64-pc-linux-gnu, compiled
by gcc (Debian 8.3.0-6) 8.3.0, 64-bit
from_var_volatile_func -- count: 1
inline_volatile_func -- count: 1
from_var_stable_func -- count: 1
inline_stable_func -- count: 0

The same change in behavior seems to be introduced in 12.8, not being
present in 12.7. I ran the code above using official Docker Hub images.

Best regards,
Alexander Nawratil

PG Bug reporting form <noreply@postgresql.org> writes:
> When a function that returns just a row count of a table is marked as STABLE
> and is called from a stored procedure as argument, the result of the
> function is different than when the function is called beforehand and stored
> to a local variable.

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.

Will fix, thanks for the report!

            regards, tom lane

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

diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.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 params, bool atomic, DestReceiver
     estate->es_param_list_info = params;
     econtext = CreateExprContext(estate);

+    /*
+     * If we're called in non-atomic context, we also have to ensure that the
+     * argument expressions run with an up-to-date snapshot.  Our caller will
+     * 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 = 0;
     foreach(lc, fexpr->args)
     {
@@ -2267,20 +2278,23 @@ ExecuteCallStmt(CallStmt *stmt, ParamListInfo params, 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 = FunctionCallInvoke(fcinfo);
     pgstat_end_function_usage(&fcusage, true);

+    /* Handle the procedure's outputs */
     if (fexpr->funcresulttype == VOIDOID)
     {
         /* do nothing */
     }
     else if (fexpr->funcresulttype == 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 = %', cnt;
+END
+$$;
+DO $$
+BEGIN
+    CALL transaction_test9b(report_count());
+    INSERT INTO test2 VALUES(43);
+    CALL transaction_test9b(report_count());
+END
+$$;
+NOTICE:  count = 1
+NOTICE:  count = 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/plpgsql/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 = %', 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)