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/15703-c12c5bc0ea34ba26@postgresql.org

1 files changed, 17 insertions, 0 deletions

diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 6e262d1a3ad..22d0fe5ac4f 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)