Fix behavior of stable functions called from a CALL's argument list.

If the CALL is within an atomic context (e.g. there's an outer
transaction block), _SPI_execute_plan should acquire a fresh snapshot
to execute any such functions with.  We failed to do that and instead
passed them the Portal snapshot, which had been acquired at the start
of the current SQL command.  This'd lead to seeing stale values of
rows modified since the start of the command.

This is arguably a bug in 84f5c2908: I failed to see that "are we in
non-atomic mode" needs to be defined the same way as it is further
down in _SPI_execute_plan, i.e. check !_SPI_current->atomic not just
options->allow_nonatomic.  Alternatively the blame could be laid on
plpgsql, which is unconditionally passing allow_nonatomic = true
for CALL/DO even when it knows it's in an atomic context.  However,
fixing it in spi.c seems like a better idea since that will also fix
the problem for any extensions that may have copied plpgsql's coding
pattern.

While here, update an obsolete comment about _SPI_execute_plan's
snapshot management.

Per report from Victor Yegorov.  Back-patch to all supported versions.

Discussion: https://postgr.es/m/CAGnEboiRe+fG2QxuBO2390F7P8e2MQ6UyBjZSL_w1Cej+E4=Vw@mail.gmail.com

diff --git a/doc/src/sgml/spi.sgml b/doc/src/sgml/spi.sgml
index bb3778688bdc0ee898073dd21b1d779406d661cb..7d154914b90762c7b7b61aa4310b6d0abaf736b5 100644 (file)
--- a/doc/src/sgml/spi.sgml
+++ b/doc/src/sgml/spi.sgml
@@ -752,7 +752,9 @@ int SPI_execute_extended(const char *<parameter>command</parameter>,
     <listitem>
      <para>
       <literal>true</literal> allows non-atomic execution of CALL and DO
-      statements
+      statements (but this field is ignored unless
+      the <symbol>SPI_OPT_NONATOMIC</symbol> flag was passed
+      to <function>SPI_connect_ext</function>)
      </para>
     </listitem>
    </varlistentry>
@@ -1893,7 +1895,9 @@ int SPI_execute_plan_extended(SPIPlanPtr <parameter>plan</parameter>,
     <listitem>
      <para>
       <literal>true</literal> allows non-atomic execution of CALL and DO
-      statements
+      statements (but this field is ignored unless
+      the <symbol>SPI_OPT_NONATOMIC</symbol> flag was passed
+      to <function>SPI_connect_ext</function>)
      </para>
     </listitem>
    </varlistentry>

diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index a97a7e3bd4e6431216db515d7ff05f7e212b7143..e516c0a67c623aacd4629b1b48656c4fa75ab9dd 100644 (file)
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -2399,6 +2399,7 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecuteOptions *options,
    uint64      my_processed = 0;
    SPITupleTable *my_tuptable = NULL;
    int         res = 0;
+   bool        allow_nonatomic;
    bool        pushed_active_snap = false;
    ResourceOwner plan_owner = options->owner;
    SPICallbackArg spicallbackarg;
@@ -2406,6 +2407,12 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecuteOptions *options,
    CachedPlan *cplan = NULL;
    ListCell   *lc1;
 
+   /*
+    * We allow nonatomic behavior only if options->allow_nonatomic is set
+    * *and* the SPI_OPT_NONATOMIC flag was given when connecting.
+    */
+   allow_nonatomic = options->allow_nonatomic && !_SPI_current->atomic;
+
    /*
     * Setup error traceback support for ereport()
     */
@@ -2425,12 +2432,17 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecuteOptions *options,
     * snapshot != InvalidSnapshot, read_only = false: use the given snapshot,
     * modified by advancing its command ID before each querytree.
     *
-    * snapshot == InvalidSnapshot, read_only = true: use the entry-time
-    * ActiveSnapshot, if any (if there isn't one, we run with no snapshot).
+    * snapshot == InvalidSnapshot, read_only = true: do nothing for queries
+    * 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 == InvalidSnapshot, read_only = false: take a full new
-    * snapshot for each user command, and advance its command ID before each
-    * querytree within the command.
+    * snapshot == InvalidSnapshot, read_only = 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.
@@ -2440,6 +2452,7 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecuteOptions *options,
     */
    if (snapshot != InvalidSnapshot)
    {
+       /* this intentionally tests the options field not the derived value */
        Assert(!options->allow_nonatomic);
        if (options->read_only)
        {
@@ -2585,7 +2598,7 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecuteOptions *options,
             * Skip it when doing non-atomic execution, though (we rely
             * entirely on the Portal snapshot in that case).
             */
-           if (!options->read_only && !options->allow_nonatomic)
+           if (!options->read_only && !allow_nonatomic)
            {
                if (pushed_active_snap)
                    PopActiveSnapshot();
@@ -2685,14 +2698,13 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecuteOptions *options,
                QueryCompletion qc;
 
                /*
-                * If the SPI context is atomic, or we were not told to allow
-                * nonatomic operations, tell ProcessUtility this is an atomic
-                * execution context.
+                * If we're not allowing nonatomic operations, tell
+                * ProcessUtility this is an atomic execution context.
                 */
-               if (_SPI_current->atomic || !options->allow_nonatomic)
-                   context = PROCESS_UTILITY_QUERY;
-               else
+               if (allow_nonatomic)
                    context = PROCESS_UTILITY_QUERY_NONATOMIC;
+               else
+                   context = PROCESS_UTILITY_QUERY;
 
                InitializeQueryCompletion(&qc);
                ProcessUtility(stmt,

diff --git a/src/pl/plpgsql/src/expected/plpgsql_call.out b/src/pl/plpgsql/src/expected/plpgsql_call.out
index 17235fca9120195f42212f3376eec5455c3af254..0a63b1d44ef1655e25412625a123d4061ca05784 100644 (file)
--- a/src/pl/plpgsql/src/expected/plpgsql_call.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_call.out
@@ -564,3 +564,53 @@ NOTICE:  inner_p(44)
  
 (1 row)
 
+-- Check that stable functions in CALL see the correct snapshot
+CREATE TABLE t_test (x int);
+INSERT INTO t_test VALUES (0);
+CREATE FUNCTION f_get_x () RETURNS int
+AS $$
+DECLARE l_result int;
+BEGIN
+  SELECT x INTO l_result FROM t_test;
+  RETURN l_result;
+END
+$$ LANGUAGE plpgsql STABLE;
+CREATE PROCEDURE f_print_x (x int)
+AS $$
+BEGIN
+  RAISE NOTICE 'f_print_x(%)', x;
+END
+$$ LANGUAGE plpgsql;
+-- test in non-atomic context
+DO $$
+BEGIN
+  UPDATE t_test SET x = x + 1;
+  RAISE NOTICE 'f_get_x(%)', f_get_x();
+  CALL f_print_x(f_get_x());
+  UPDATE t_test SET x = x + 1;
+  RAISE NOTICE 'f_get_x(%)', f_get_x();
+  CALL f_print_x(f_get_x());
+  ROLLBACK;
+END
+$$;
+NOTICE:  f_get_x(1)
+NOTICE:  f_print_x(1)
+NOTICE:  f_get_x(2)
+NOTICE:  f_print_x(2)
+-- test in atomic context
+BEGIN;
+DO $$
+BEGIN
+  UPDATE t_test SET x = x + 1;
+  RAISE NOTICE 'f_get_x(%)', f_get_x();
+  CALL f_print_x(f_get_x());
+  UPDATE t_test SET x = x + 1;
+  RAISE NOTICE 'f_get_x(%)', f_get_x();
+  CALL f_print_x(f_get_x());
+END
+$$;
+NOTICE:  f_get_x(1)
+NOTICE:  f_print_x(1)
+NOTICE:  f_get_x(2)
+NOTICE:  f_print_x(2)
+ROLLBACK;

diff --git a/src/pl/plpgsql/src/sql/plpgsql_call.sql b/src/pl/plpgsql/src/sql/plpgsql_call.sql
index 869d021a0754816d1d1877e920a71431df1b5a65..4cbda0382e95776fb3c36bbf567ef9de426a42c7 100644 (file)
--- a/src/pl/plpgsql/src/sql/plpgsql_call.sql
+++ b/src/pl/plpgsql/src/sql/plpgsql_call.sql
@@ -522,3 +522,53 @@ CREATE FUNCTION f(int) RETURNS int AS $$ SELECT $1 + 2 $$ LANGUAGE sql;
 
 CALL outer_p(42);
 SELECT outer_f(42);
+
+-- Check that stable functions in CALL see the correct snapshot
+
+CREATE TABLE t_test (x int);
+INSERT INTO t_test VALUES (0);
+
+CREATE FUNCTION f_get_x () RETURNS int
+AS $$
+DECLARE l_result int;
+BEGIN
+  SELECT x INTO l_result FROM t_test;
+  RETURN l_result;
+END
+$$ LANGUAGE plpgsql STABLE;
+
+CREATE PROCEDURE f_print_x (x int)
+AS $$
+BEGIN
+  RAISE NOTICE 'f_print_x(%)', x;
+END
+$$ LANGUAGE plpgsql;
+
+-- test in non-atomic context
+DO $$
+BEGIN
+  UPDATE t_test SET x = x + 1;
+  RAISE NOTICE 'f_get_x(%)', f_get_x();
+  CALL f_print_x(f_get_x());
+  UPDATE t_test SET x = x + 1;
+  RAISE NOTICE 'f_get_x(%)', f_get_x();
+  CALL f_print_x(f_get_x());
+  ROLLBACK;
+END
+$$;
+
+-- test in atomic context
+BEGIN;
+
+DO $$
+BEGIN
+  UPDATE t_test SET x = x + 1;
+  RAISE NOTICE 'f_get_x(%)', f_get_x();
+  CALL f_print_x(f_get_x());
+  UPDATE t_test SET x = x + 1;
+  RAISE NOTICE 'f_get_x(%)', f_get_x();
+  CALL f_print_x(f_get_x());
+END
+$$;
+
+ROLLBACK;