Fix problems with auto-held portals.

HoldPinnedPortals() did things in the wrong order: it must not mark
a portal autoHeld until it's been successfully held.  Otherwise,
a failure while persisting the portal results in a server crash
because we think the portal is in a good state when it's not.

Also add a check that portal->status is READY before attempting to
hold a pinned portal.  We have such a check before the only other
use of HoldPortal(), so it seems unwise not to check it here.

Lastly, rethink the responsibility for where to call HoldPinnedPortals.
The comment for it imagined that it was optional for any individual PL
to call it or not, but that cannot be the case: if some outer level of
procedure has a pinned portal, failing to persist it when an inner
procedure commits is going to be trouble.  Let's have SPI do it instead
of the individual PLs.  That's not a complete solution, since in theory
a PL might not be using SPI to perform commit/rollback, but such a PL
is going to have to be aware of lots of related requirements anyway.
(This change doesn't cause an API break for any external PLs that might
be calling HoldPinnedPortals per the old regime, because calling it
twice during a commit or rollback sequence won't hurt.)

Per bug #15703 from Julian Schauder.  Back-patch to v11 where this code
came in.

Discussion: https://postgr.es/m/[email protected]

Author: tglsfdc

src/backend/executor/spi.c

@@ -246,6 +246,14 @@ SPI_commit(void)
 				(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;
 
 	/*
@@ -277,6 +285,15 @@ SPI_rollback(void)
 				(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;
 
 	AbortCurrentTransaction();

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;
 		}
 	}
 }

src/pl/plperl/plperl.c

@@ -3966,8 +3966,6 @@ plperl_spi_commit(void)
 
 	PG_TRY();
 	{
-		HoldPinnedPortals();
-
 		SPI_commit();
 		SPI_start_transaction();
 	}
@@ -3993,8 +3991,6 @@ plperl_spi_rollback(void)
 
 	PG_TRY();
 	{
-		HoldPinnedPortals();
-
 		SPI_rollback();
 		SPI_start_transaction();
 	}

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

src/pl/plpgsql/src/pl_exec.c

@@ -4770,8 +4770,6 @@ exec_stmt_close(PLpgSQL_execstate *estate, PLpgSQL_stmt_close *stmt)
 static int
 exec_stmt_commit(PLpgSQL_execstate *estate, PLpgSQL_stmt_commit *stmt)
 {
-	HoldPinnedPortals();
-
 	SPI_commit();
 	SPI_start_transaction();
 
@@ -4789,8 +4787,6 @@ exec_stmt_commit(PLpgSQL_execstate *estate, PLpgSQL_stmt_commit *stmt)
 static int
 exec_stmt_rollback(PLpgSQL_execstate *estate, PLpgSQL_stmt_rollback *stmt)
 {
-	HoldPinnedPortals();
-
 	SPI_rollback();
 	SPI_start_transaction();
 

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 $$

src/pl/plpython/plpy_plpymodule.c

@@ -594,8 +594,6 @@ PLy_commit(PyObject *self, PyObject *args)
 {
 	PLyExecutionContext *exec_ctx = PLy_current_execution_context();
 
-	HoldPinnedPortals();
-
 	SPI_commit();
 	SPI_start_transaction();
 
@@ -610,8 +608,6 @@ PLy_rollback(PyObject *self, PyObject *args)
 {
 	PLyExecutionContext *exec_ctx = PLy_current_execution_context();
 
-	HoldPinnedPortals();
-
 	SPI_rollback();
 	SPI_start_transaction();