Re: BUG #15703: Segfault in cancelled CALL-Statements - Mailing list pgsql-bugs

I wrote:
>> I find HoldPinnedPortals suspicious in another way: is it really
>> OK for it to mark *all* pinned portals as auto-held?  What if we're
>> in a nest of procedures run by different PLs (of which only the
>> innermost will have any idea we're committing)?

> After taking another look at this, I'm convinced that it's flat out
> broken.  It cannot be up to individual PLs to decide whether or not
> to call HoldPinnedPortals before issuing a commit.  If a PL that needs
> that calls (a function belonging to) some other PL that thinks it
> doesn't need that, and the second PL issues a commit or rollback,
> we'll have a failure for whatever portal(s) the first PL left pinned.

After further study, it seems like the most practical thing to do here
is to assign the responsibility for calling HoldPinnedPortals to spi.c.

If there are any PLs out there that are trying to implement commit/
rollback in procedures without using SPI, it's going to be on their
heads to include this step --- but there's a lot of other stuff they
are going to need to know if they don't go through SPI, so that doesn't
seem too awful, and in any case this change doesn't make it any more
complicated for them.

Conversely, if there are any PLs out there that are using spi.c and
following the existing rule that they should call HoldPinnedPortals for
themselves, this won't break them, because running HoldPinnedPortals
twice in a row is harmless.

Accordingly I propose the attached patch.  Aside from moving those
calls, adjusting the comments, and fixing the original bug, this
inserts a test on portal->status, which I think is necessary by
analogy to the existing tests in AtCommit_Portals.

            regards, tom lane

diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 6e262d1..22d0fe5 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -241,6 +241,14 @@ _SPI_commit(bool chain)
                 (errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
                  errmsg("cannot commit while a subtransaction is active")));

+    /*
+     * Hold any pinned portals that any PLs might be using.  We have to do
+     * this before changing transaction state, since this will run
+     * user-defined code that might throw an error.
+     */
+    HoldPinnedPortals();
+
+    /* Start the actual commit */
     _SPI_current->internal_xact = true;

     /*
@@ -294,6 +302,15 @@ _SPI_rollback(bool chain)
                 (errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
                  errmsg("cannot roll back while a subtransaction is active")));

+    /*
+     * Hold any pinned portals that any PLs might be using.  We have to do
+     * this before changing transaction state, since this will run
+     * user-defined code that might throw an error, and in any case couldn't
+     * be run in an already-aborted transaction.
+     */
+    HoldPinnedPortals();
+
+    /* Start the actual rollback */
     _SPI_current->internal_xact = true;

     if (chain)
diff --git a/src/backend/utils/mmgr/portalmem.c b/src/backend/utils/mmgr/portalmem.c
index a92b454..334e35b 100644
--- a/src/backend/utils/mmgr/portalmem.c
+++ b/src/backend/utils/mmgr/portalmem.c
@@ -1226,13 +1226,19 @@ ThereAreNoReadyPortals(void)
 /*
  * Hold all pinned portals.
  *
- * A procedural language implementation that uses pinned portals for its
- * internally generated cursors can call this in its COMMIT command to convert
- * those cursors to held cursors, so that they survive the transaction end.
- * We mark those portals as "auto-held" so that exception exit knows to clean
- * them up.  (In normal, non-exception code paths, the PL needs to clean those
- * portals itself, since transaction end won't do it anymore, but that should
- * be normal practice anyway.)
+ * When initiating a COMMIT or ROLLBACK inside a procedure, this must be
+ * called to protect internally-generated cursors from being dropped during
+ * the transaction shutdown.  Currently, SPI calls this automatically; PLs
+ * that initiate COMMIT or ROLLBACK some other way are on the hook to do it
+ * themselves.  (Note that we couldn't do this in, say, AtAbort_Portals
+ * because we need to run user-defined code while persisting a portal.
+ * It's too late to do that once transaction abort has started.)
+ *
+ * We protect such portals by converting them to held cursors.  We mark them
+ * as "auto-held" so that exception exit knows to clean them up.  (In normal,
+ * non-exception code paths, the PL needs to clean such portals itself, since
+ * transaction end won't do it anymore; but that should be normal practice
+ * anyway.)
  */
 void
 HoldPinnedPortals(void)
@@ -1262,8 +1268,12 @@ HoldPinnedPortals(void)
                         (errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
                          errmsg("cannot perform transaction commands inside a cursor loop that is not read-only")));

-            portal->autoHeld = true;
+            /* Verify it's in a suitable state to be held */
+            if (portal->status != PORTAL_READY)
+                elog(ERROR, "pinned portal is not ready to be auto-held");
+
             HoldPortal(portal);
+            portal->autoHeld = true;
         }
     }
 }
diff --git a/src/pl/plperl/plperl.c b/src/pl/plperl/plperl.c
index 31ba2f2..bda9517 100644
--- a/src/pl/plperl/plperl.c
+++ b/src/pl/plperl/plperl.c
@@ -3988,8 +3988,6 @@ plperl_spi_commit(void)

     PG_TRY();
     {
-        HoldPinnedPortals();
-
         SPI_commit();
         SPI_start_transaction();
     }
@@ -4015,8 +4013,6 @@ plperl_spi_rollback(void)

     PG_TRY();
     {
-        HoldPinnedPortals();
-
         SPI_rollback();
         SPI_start_transaction();
     }
diff --git a/src/pl/plpgsql/src/expected/plpgsql_transaction.out b/src/pl/plpgsql/src/expected/plpgsql_transaction.out
index ba07453..5f57623 100644
--- a/src/pl/plpgsql/src/expected/plpgsql_transaction.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_transaction.out
@@ -401,6 +401,50 @@ SELECT * FROM test3;
  1
 (1 row)

+-- failure while trying to persist a cursor across a transaction (bug #15703)
+CREATE PROCEDURE cursor_fail_during_commit()
+ LANGUAGE plpgsql
+AS $$
+  DECLARE id int;
+  BEGIN
+    FOR id IN SELECT 1/(x-1000) FROM generate_series(1,1000) x LOOP
+        INSERT INTO test1 VALUES(id);
+        COMMIT;
+    END LOOP;
+  END;
+$$;
+TRUNCATE test1;
+CALL cursor_fail_during_commit();
+ERROR:  division by zero
+CONTEXT:  PL/pgSQL function cursor_fail_during_commit() line 6 at COMMIT
+-- note that error occurs during first COMMIT, hence nothing is in test1
+SELECT count(*) FROM test1;
+ count
+-------
+     0
+(1 row)
+
+CREATE PROCEDURE cursor_fail_during_rollback()
+ LANGUAGE plpgsql
+AS $$
+  DECLARE id int;
+  BEGIN
+    FOR id IN SELECT 1/(x-1000) FROM generate_series(1,1000) x LOOP
+        INSERT INTO test1 VALUES(id);
+        ROLLBACK;
+    END LOOP;
+  END;
+$$;
+TRUNCATE test1;
+CALL cursor_fail_during_rollback();
+ERROR:  division by zero
+CONTEXT:  PL/pgSQL function cursor_fail_during_rollback() line 6 at ROLLBACK
+SELECT count(*) FROM test1;
+ count
+-------
+     0
+(1 row)
+
 -- SET TRANSACTION
 DO LANGUAGE plpgsql $$
 BEGIN
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index f000500..23aed02 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -4791,8 +4791,6 @@ exec_stmt_close(PLpgSQL_execstate *estate, PLpgSQL_stmt_close *stmt)
 static int
 exec_stmt_commit(PLpgSQL_execstate *estate, PLpgSQL_stmt_commit *stmt)
 {
-    HoldPinnedPortals();
-
     if (stmt->chain)
         SPI_commit_and_chain();
     else
@@ -4815,8 +4813,6 @@ exec_stmt_commit(PLpgSQL_execstate *estate, PLpgSQL_stmt_commit *stmt)
 static int
 exec_stmt_rollback(PLpgSQL_execstate *estate, PLpgSQL_stmt_rollback *stmt)
 {
-    HoldPinnedPortals();
-
     if (stmt->chain)
         SPI_rollback_and_chain();
     else
diff --git a/src/pl/plpgsql/src/sql/plpgsql_transaction.sql b/src/pl/plpgsql/src/sql/plpgsql_transaction.sql
index 0c137dd..7575655 100644
--- a/src/pl/plpgsql/src/sql/plpgsql_transaction.sql
+++ b/src/pl/plpgsql/src/sql/plpgsql_transaction.sql
@@ -329,6 +329,44 @@ $$;

 SELECT * FROM test3;

+-- failure while trying to persist a cursor across a transaction (bug #15703)
+CREATE PROCEDURE cursor_fail_during_commit()
+ LANGUAGE plpgsql
+AS $$
+  DECLARE id int;
+  BEGIN
+    FOR id IN SELECT 1/(x-1000) FROM generate_series(1,1000) x LOOP
+        INSERT INTO test1 VALUES(id);
+        COMMIT;
+    END LOOP;
+  END;
+$$;
+
+TRUNCATE test1;
+
+CALL cursor_fail_during_commit();
+
+-- note that error occurs during first COMMIT, hence nothing is in test1
+SELECT count(*) FROM test1;
+
+CREATE PROCEDURE cursor_fail_during_rollback()
+ LANGUAGE plpgsql
+AS $$
+  DECLARE id int;
+  BEGIN
+    FOR id IN SELECT 1/(x-1000) FROM generate_series(1,1000) x LOOP
+        INSERT INTO test1 VALUES(id);
+        ROLLBACK;
+    END LOOP;
+  END;
+$$;
+
+TRUNCATE test1;
+
+CALL cursor_fail_during_rollback();
+
+SELECT count(*) FROM test1;
+

 -- SET TRANSACTION
 DO LANGUAGE plpgsql $$
diff --git a/src/pl/plpython/plpy_plpymodule.c b/src/pl/plpython/plpy_plpymodule.c
index 23e49e4..f40f084 100644
--- a/src/pl/plpython/plpy_plpymodule.c
+++ b/src/pl/plpython/plpy_plpymodule.c
@@ -588,8 +588,6 @@ PLy_commit(PyObject *self, PyObject *args)
 {
     PLyExecutionContext *exec_ctx = PLy_current_execution_context();

-    HoldPinnedPortals();
-
     SPI_commit();
     SPI_start_transaction();

@@ -604,8 +602,6 @@ PLy_rollback(PyObject *self, PyObject *args)
 {
     PLyExecutionContext *exec_ctx = PLy_current_execution_context();

-    HoldPinnedPortals();
-
     SPI_rollback();
     SPI_start_transaction();