Fix assert in nested SQL procedure call

When executing CALL in PL/pgSQL, we need to set a snapshot before
invoking the to-be-called procedure.  Otherwise, the to-be-called
procedure might end up running without a snapshot.  For LANGUAGE SQL
procedures, this would result in an assertion failure.  (For most other
languages, this is usually not a problem, because those use SPI and SPI
sets snapshots in most cases.)  Setting the snapshot restores the
behavior of how CALL worked when it was handled as a generic SQL
statement in PL/pgSQL (exec_stmt_execsql()).

This change revealed another problem:  In SPI_commit(), we popped the
active snapshot before committing the transaction, to avoid "snapshot %p
still active" errors.  However, there is no particular reason why only
at most one snapshot should be on the stack.  So change this to pop all
active snapshots instead of only one.

diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 22dd55c37838bce1371cf254d8a79b02347fddc5..5756365c8f312c4a3a0d3efe7a7f13ea0b12006b 100644 (file)
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -228,8 +228,13 @@ SPI_commit(void)
 
        _SPI_current->internal_xact = true;
 
-       if (ActiveSnapshotSet())
+       /*
+        * Before committing, pop all active snapshots to avoid error about
+        * "snapshot %p still active".
+        */
+       while (ActiveSnapshotSet())
                PopActiveSnapshot();
+
        CommitTransactionCommand();
        MemoryContextSwitchTo(oldcontext);
 

diff --git a/src/pl/plpgsql/src/expected/plpgsql_transaction.out b/src/pl/plpgsql/src/expected/plpgsql_transaction.out
index 7f008ac57e92f4da66d36ac0957bb1f4b413676d..274b2c6f1709d90bd643fcfe9ef72266812bbcce 100644 (file)
--- a/src/pl/plpgsql/src/expected/plpgsql_transaction.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_transaction.out
@@ -432,6 +432,25 @@ END;
 $$;
 ERROR:  EXECUTE of transaction commands is not implemented
 CONTEXT:  PL/pgSQL function inline_code_block line 3 at EXECUTE
+-- snapshot handling test
+TRUNCATE test2;
+CREATE PROCEDURE transaction_test9()
+LANGUAGE SQL
+AS $$
+INSERT INTO test2 VALUES (42);
+$$;
+DO LANGUAGE plpgsql $$
+BEGIN
+  ROLLBACK;
+  CALL transaction_test9();
+END
+$$;
+SELECT * FROM test2;
+ x  
+----
+ 42
+(1 row)
+
 DROP TABLE test1;
 DROP TABLE test2;
 DROP TABLE test3;

diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 66ecf5eb559da27daef2de57a45bec75955aa1cf..e39f7357bd54fb24dcb67b796237595cce38437a 100644 (file)
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -2075,6 +2075,7 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
        ParamListInfo paramLI;
        LocalTransactionId before_lxid;
        LocalTransactionId after_lxid;
+       bool            pushed_active_snap = false;
        int                     rc;
 
        if (expr->plan == NULL)
@@ -2090,6 +2091,7 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
                /*
                 * The procedure call could end transactions, which would upset the
                 * snapshot management in SPI_execute*, so don't let it do it.
+                * Instead, we set the snapshots ourselves below.
                 */
                expr->plan->no_snapshots = true;
        }
@@ -2098,6 +2100,16 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
 
        before_lxid = MyProc->lxid;
 
+       /*
+        * Set snapshot only for non-read-only procedures, similar to SPI
+        * behavior.
+        */
+       if (!estate->readonly_func)
+       {
+               PushActiveSnapshot(GetTransactionSnapshot());
+               pushed_active_snap = true;
+       }
+
        PG_TRY();
        {
                rc = SPI_execute_plan_with_paramlist(expr->plan, paramLI,
@@ -2126,12 +2138,22 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
                elog(ERROR, "SPI_execute_plan_with_paramlist failed executing query \"%s\": %s",
                         expr->query, SPI_result_code_string(rc));
 
-       /*
-        * If we are in a new transaction after the call, we need to reset some
-        * internal state.
-        */
-       if (before_lxid != after_lxid)
+       if (before_lxid == after_lxid)
+       {
+               /*
+                * If we are still in the same transaction after the call, pop the
+                * snapshot that we might have pushed.  (If it's a new transaction,
+                * then all the snapshots are gone already.)
+                */
+               if (pushed_active_snap)
+                       PopActiveSnapshot();
+       }
+       else
        {
+               /*
+                * If we are in a new transaction after the call, we need to reset
+                * some internal state.
+                */
                estate->simple_eval_estate = NULL;
                plpgsql_create_econtext(estate);
        }

diff --git a/src/pl/plpgsql/src/sql/plpgsql_transaction.sql b/src/pl/plpgsql/src/sql/plpgsql_transaction.sql
index eddc518bb6aaa21083b183644f3a8ea3cf30b9bd..1624aed6eca27578f8ab0f0baa7746511d553878 100644 (file)
--- a/src/pl/plpgsql/src/sql/plpgsql_transaction.sql
+++ b/src/pl/plpgsql/src/sql/plpgsql_transaction.sql
@@ -354,6 +354,26 @@ BEGIN
 END;
 $$;
 
+
+-- snapshot handling test
+TRUNCATE test2;
+
+CREATE PROCEDURE transaction_test9()
+LANGUAGE SQL
+AS $$
+INSERT INTO test2 VALUES (42);
+$$;
+
+DO LANGUAGE plpgsql $$
+BEGIN
+  ROLLBACK;
+  CALL transaction_test9();
+END
+$$;
+
+SELECT * FROM test2;
+
+
 DROP TABLE test1;
 DROP TABLE test2;
 DROP TABLE test3;