Thread: Re: fix crash with Python 3.11

Re: fix crash with Python 3.11

From

Tom Lane

Date:

17 January 2022, 01:53:44

Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> The way plpy.commit() and plpy.rollback() handle errors is not ideal. 
> They end up just longjmping back to the main loop, without telling the 
> Python interpreter about it.  This hasn't been a problem so far, 
> apparently, but with Python 3.11-to-be, this appears to cause corruption 
> in the state of the Python interpreter.  This is readily reproducible 
> and will cause crashes in the plpython_transaction test.
> The fix is that we need to catch the PostgreSQL error and turn it into a 
> Python exception, like we do for other places where plpy.* methods call 
> into PostgreSQL internals.

I agree that the existing code is broken and works only accidentally,
but I fear this patch does little to improve matters.  In particular,
it returns control to Python without having done anything to clean
up the now-invalid state of the transaction system.  (That is,
there's nothing analogous to PLy_spi_subtransaction_abort's
RollbackAndReleaseCurrentSubTransaction call in these new PG_CATCH
blocks).  The existing test cases apparently fail to trip over that
because Python just throws the exception again, but what if someone
writes a plpython function that catches the exception and then tries
to perform new SPI actions?

I think a possible fix is:

1. Before entering the PG_TRY block, check for active subtransaction(s)
and immediately throw a Python error if there is one.  (This corresponds
to the existing errors "cannot commit while a subtransaction is active"
and "cannot roll back while a subtransaction is active".  The point is
to reduce the number of system states we have to worry about below.)

2. In the PG_CATCH block, after collecting the error data do
    AbortOutOfAnyTransaction();
    StartTransactionCommand();
which gets us into a good state with no active subtransactions.

I'm not sure that those two are the best choices of xact.c
entry points, but there's precedent for that in autovacuum.c
among other places.  (autovacuum seems to think that blocking
interrupts is a good idea too.)

            regards, tom lane

Re: fix crash with Python 3.11

From

Peter Eisentraut

Date:

25 January 2022, 17:21:15

On 16.01.22 23:53, Tom Lane wrote:
> I think a possible fix is:
> 
> 1. Before entering the PG_TRY block, check for active subtransaction(s)
> and immediately throw a Python error if there is one.  (This corresponds
> to the existing errors "cannot commit while a subtransaction is active"
> and "cannot roll back while a subtransaction is active".  The point is
> to reduce the number of system states we have to worry about below.)
> 
> 2. In the PG_CATCH block, after collecting the error data do
>     AbortOutOfAnyTransaction();
>     StartTransactionCommand();
> which gets us into a good state with no active subtransactions.
> 
> I'm not sure that those two are the best choices of xact.c
> entry points, but there's precedent for that in autovacuum.c
> among other places.

AFAICT, AbortOutOfAnyTransaction() also aborts subtransactions, so why 
do you suggest the separate handling of subtransactions?

Re: fix crash with Python 3.11

From

Tom Lane

Date:

25 January 2022, 18:54:44

Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> On 16.01.22 23:53, Tom Lane wrote:
>> I think a possible fix is:
>> 
>> 1. Before entering the PG_TRY block, check for active subtransaction(s)
>> and immediately throw a Python error if there is one.  (This corresponds
>> to the existing errors "cannot commit while a subtransaction is active"
>> and "cannot roll back while a subtransaction is active".  The point is
>> to reduce the number of system states we have to worry about below.)

> AFAICT, AbortOutOfAnyTransaction() also aborts subtransactions, so why 
> do you suggest the separate handling of subtransactions?

We don't want these operations to be able to cancel subtransactions,
do we?  The existing errors certainly suggest not.

            regards, tom lane

Re: fix crash with Python 3.11

From

Peter Eisentraut

Date:

01 February 2022, 17:24:53

On 25.01.22 16:54, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
>> On 16.01.22 23:53, Tom Lane wrote:
>>> I think a possible fix is:
>>>
>>> 1. Before entering the PG_TRY block, check for active subtransaction(s)
>>> and immediately throw a Python error if there is one.  (This corresponds
>>> to the existing errors "cannot commit while a subtransaction is active"
>>> and "cannot roll back while a subtransaction is active".  The point is
>>> to reduce the number of system states we have to worry about below.)
> 
>> AFAICT, AbortOutOfAnyTransaction() also aborts subtransactions, so why
>> do you suggest the separate handling of subtransactions?
> 
> We don't want these operations to be able to cancel subtransactions,
> do we?  The existing errors certainly suggest not.

I've been struggling to make progress on this.  First, throwing the 
Python exception suggested in #1 above would require a significant 
amount of new code.  (We have code to create an exception out of 
ErrorData, but no code to make one up from nothing.)  This would need 
further thought on how to arrange the code sensibly.  Second, calling 
AbortOutOfAnyTransaction() appears to clean up so much stuff that even 
the data needed for error handling in PL/Python is wiped out.  The 
symptoms are various crashes on pointers now valued 0x7f7f7f...  You fix 
one, you get the next one.  So more analysis would be required there, too.

One way to way to address the first problem is to not handle the "cannot 
commit while a subtransaction is active" and similar cases manually but 
let _SPI_commit() throw the error and then check in PL/Python for the 
error code ERRCODE_INVALID_TRANSACTION_TERMINATION.  (The code in 
_SPI_commit() is there after all explicitly for PLs, so if we're not 
using it, then we're doing it wrong.)  And then only call 
AbortOutOfAnyTransaction() (or whatever) if it's a different code, which 
would mean something in the actual committing went wrong.

But in any case, for either implementation it seems then you'd get 
different error behavior from .commit and .rollback calls depending on 
the specific error, which seems weird.

Altogether, maybe the response to

 > The existing test cases apparently fail to trip over that
 > because Python just throws the exception again, but what if someone
 > writes a plpython function that catches the exception and then tries
 > to perform new SPI actions?

perhaps should be, don't do that then?

Re: fix crash with Python 3.11

From

Tom Lane

Date:

08 February 2022, 01:38:01

Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> I've been struggling to make progress on this.  First, throwing the
> Python exception suggested in #1 above would require a significant
> amount of new code.  (We have code to create an exception out of
> ErrorData, but no code to make one up from nothing.)  This would need
> further thought on how to arrange the code sensibly.  Second, calling
> AbortOutOfAnyTransaction() appears to clean up so much stuff that even
> the data needed for error handling in PL/Python is wiped out.

Yeah, it's messy.

> One way to way to address the first problem is to not handle the "cannot
> commit while a subtransaction is active" and similar cases manually but
> let _SPI_commit() throw the error and then check in PL/Python for the
> error code ERRCODE_INVALID_TRANSACTION_TERMINATION.  (The code in
> _SPI_commit() is there after all explicitly for PLs, so if we're not
> using it, then we're doing it wrong.)

TBH, I've thought from day one that _SPI_commit and friends are unusably
simplistic, precisely because they throw all this error-recovery
complexity onto the caller, and provide no tools to handle it without
dropping down to a lower level of abstraction.

> But in any case, for either implementation it seems then you'd get
> different error behavior from .commit and .rollback calls depending on
> the specific error, which seems weird.

Well, I think we *have to* do that.  The INVALID_TRANSACTION_TERMINATION
errors mean that we're in some kind of atomic execution context that
we mustn't destroy.  Other errors mean that the commit failed, and that
has to be cleaned up somehow.

(Note: there is also an INVALID_TRANSACTION_TERMINATION error in
HoldPinnedPortals, which is now seeming like a mistake to me; we should
change that to some other errcode, perhaps.  There are no others
besides _SPI_commit/_SPI_rollback.)

> Altogether, maybe the response to

>>> The existing test cases apparently fail to trip over that
>>> because Python just throws the exception again, but what if someone
>>> writes a plpython function that catches the exception and then tries
>>> to perform new SPI actions?

> perhaps should be, don't do that then?

That seems far south of acceptable to me.  I experimented with the
attached script, which in HEAD just results in aborting the Python
script -- not good, but at least the general state of the session
is okay.  With your patch, I get

INFO:  sqlstate: 23503
INFO:  after exception
WARNING:  buffer refcount leak: [8760] (rel=base/16384/38401, blockNum=0, flags=0x93800000, refcount=1 1)
WARNING:  relcache reference leak: relation "testfk" not closed
WARNING:  relcache reference leak: relation "testpk" not closed
WARNING:  TupleDesc reference leak: TupleDesc 0x7f3a12e0de80 (38403,-1) still referenced
WARNING:  snapshot 0x1cba150 still active
DO
 f1
----
  0
  1
(2 rows)

So aside from all the internal problems, we've actually managed to commit
an invalid database state.  I do not find that OK.

I think that maybe we could get somewhere by having _SPI_commit/rollback
work like this:

1. Throw the INVALID_TRANSACTION_TERMINATION errors if appropriate.
The caller can catch those and proceed if desired, knowing that the
transaction system is in the same state it was.

2. Use a PG_TRY block to do the commit or rollback.  On error,
roll back (knowing that we are not in a subtransaction) and then
re-throw the error.

If the caller catches an error other than INVALID_TRANSACTION_TERMINATION,
it can proceed, but it's still on the hook to do SPI_start_transaction
before it attempts any new database access (just as it would be if
there had not been an error).

We could provide a simplified API in which SPI_start_transaction is
automatically included for either success or failure, so that the caller
doesn't have the delayed-cleanup responsibility.  I'm not actually sure
that there is any situation where that couldn't be done, but putting it
into the existing functions would be a API break (... unless we turn
SPI_start_transaction into a no-op, which might be OK?)  Basically this'd
be making the behavior of SPI_commit_and_chain the norm, except you'd
have the option of reverting to default transaction settings instead
of the previous ones.

So basically where we'd end up is that plpython would do about what
you show in your patch, but then there's additional work needed in
spi.c so that we're not leaving the rest of the system in a bad state.

            regards, tom lane

create extension if not exists plpython3u;

drop table if exists testpk, testfk;

create table testpk (id int primary key);

create table testfk(f1 int references testpk deferrable initially deferred);

DO
LANGUAGE plpython3u
$$
  plpy.execute("INSERT INTO testfk VALUES (0)")
  try:
    plpy.commit()
  except Exception as e:
    plpy.info('sqlstate: %s' % (e.sqlstate))
  plpy.info('after exception')
  plpy.execute("INSERT INTO testpk VALUES (1)")
  plpy.execute("INSERT INTO testfk VALUES (1)")
$$;

table testfk;

Re: fix crash with Python 3.11

From

Tom Lane

Date:

23 February 2022, 22:31:22

I wrote:
> We could provide a simplified API in which SPI_start_transaction is
> automatically included for either success or failure, so that the caller
> doesn't have the delayed-cleanup responsibility.  I'm not actually sure
> that there is any situation where that couldn't be done, but putting it
> into the existing functions would be a API break (... unless we turn
> SPI_start_transaction into a no-op, which might be OK?)  Basically this'd
> be making the behavior of SPI_commit_and_chain the norm, except you'd
> have the option of reverting to default transaction settings instead
> of the previous ones.
> So basically where we'd end up is that plpython would do about what
> you show in your patch, but then there's additional work needed in
> spi.c so that we're not leaving the rest of the system in a bad state.

Here's a draft patch that works that way.  I haven't tested it against
Python 3.11, but it handles the case I showed upthread (now incorporated
as a regression test), as well as Alexander's repeat-till-stack-overflow
problem.  The one thing I found that I wasn't expecting is that
AtEOXact_SPI is quite a few bricks shy of a load: it doesn't handle
cases where some atomic contexts are atop an internal_xact one.  That
happens in the new test case because the error is thrown from RI
triggers that themselves use SPI.

This is draft mainly in that

* I didn't touch spi.sgml yet.

* It might be a good idea to add parallel test cases for the other PLs.

* I'm not satisfied with using static storage for
SaveTransactionCharacteristics/RestoreTransactionCharacteristics.
It'd be better for them to use a struct allocated locally in the caller.
That would be a simple enough change, but also an API break; on the
other hand, I really doubt anything outside core code is calling those.
Anyone object to changing them?

I'm not sure how to proceed with this.  It's kind of a large change
to be putting into back branches, but our hands will be forced once
Python 3.11 is out.  Maybe put it in HEAD now, and plan to back-patch
in a few months?

            regards, tom lane

diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index c93f90de9b..7971050746 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -156,7 +156,8 @@ SPI_connect_ext(int options)
      * XXX It could be better to use PortalContext as the parent context in
      * all cases, but we may not be inside a portal (consider deferred-trigger
      * execution).  Perhaps CurTransactionContext could be an option?  For now
-     * it doesn't matter because we clean up explicitly in AtEOSubXact_SPI().
+     * it doesn't matter because we clean up explicitly in AtEOSubXact_SPI();
+     * but see also AtEOXact_SPI().
      */
     _SPI_current->procCxt = AllocSetContextCreate(_SPI_current->atomic ? TopTransactionContext : PortalContext,
                                                   "SPI Proc",
@@ -214,13 +215,13 @@ SPI_finish(void)
     return SPI_OK_FINISH;
 }

+/*
+ * SPI_start_transaction is a no-op, kept for backwards compatibility.
+ * SPI callers are *always* inside a transaction.
+ */
 void
 SPI_start_transaction(void)
 {
-    MemoryContext oldcontext = CurrentMemoryContext;
-
-    StartTransactionCommand();
-    MemoryContextSwitchTo(oldcontext);
 }

 static void
@@ -228,6 +229,12 @@ _SPI_commit(bool chain)
 {
     MemoryContext oldcontext = CurrentMemoryContext;

+    /*
+     * Complain if we are in a context that doesn't permit transaction
+     * termination.  (Note: here and _SPI_rollback should be the only places
+     * that throw ERRCODE_INVALID_TRANSACTION_TERMINATION, so that callers can
+     * test for that with security that they know what happened.)
+     */
     if (_SPI_current->atomic)
         ereport(ERROR,
                 (errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
@@ -240,40 +247,74 @@ _SPI_commit(bool chain)
      * top-level transaction in such a block violates that idea.  A future PL
      * implementation might have different ideas about this, in which case
      * this restriction would have to be refined or the check possibly be
-     * moved out of SPI into the PLs.
+     * moved out of SPI into the PLs.  Note however that the code below relies
+     * on not being within a subtransaction.
      */
     if (IsSubTransaction())
         ereport(ERROR,
                 (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();
+    /* XXX this ain't re-entrant enough for my taste */
+    if (chain)
+        SaveTransactionCharacteristics();

-    /* Start the actual commit */
-    _SPI_current->internal_xact = true;
+    /* Catch any error occurring during the COMMIT */
+    PG_TRY();
+    {
+        /* Protect current SPI stack entry against deletion */
+        _SPI_current->internal_xact = true;

-    /* Release snapshots associated with portals */
-    ForgetPortalSnapshots();
+        /*
+         * 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();

-    if (chain)
-        SaveTransactionCharacteristics();
+        /* Release snapshots associated with portals */
+        ForgetPortalSnapshots();

-    CommitTransactionCommand();
+        /* Do the deed */
+        CommitTransactionCommand();

-    if (chain)
-    {
+        /* Immediately start a new transaction */
         StartTransactionCommand();
-        RestoreTransactionCharacteristics();
+        if (chain)
+            RestoreTransactionCharacteristics();
+
+        MemoryContextSwitchTo(oldcontext);
+
+        _SPI_current->internal_xact = false;
     }
+    PG_CATCH();
+    {
+        ErrorData  *edata;

-    MemoryContextSwitchTo(oldcontext);
+        /* Save error info in caller's context */
+        MemoryContextSwitchTo(oldcontext);
+        edata = CopyErrorData();
+        FlushErrorState();

-    _SPI_current->internal_xact = false;
+        /*
+         * Abort the failed transaction.  If this fails too, we'll just
+         * propagate the error out ... there's not that much we can do.
+         */
+        AbortCurrentTransaction();
+
+        /* ... and start a new one */
+        StartTransactionCommand();
+        if (chain)
+            RestoreTransactionCharacteristics();
+
+        MemoryContextSwitchTo(oldcontext);
+
+        _SPI_current->internal_xact = false;
+
+        /* Now that we've cleaned up the transaction, re-throw the error */
+        ReThrowError(edata);
+    }
+    PG_END_TRY();
 }

 void
@@ -293,6 +334,7 @@ _SPI_rollback(bool chain)
 {
     MemoryContext oldcontext = CurrentMemoryContext;

+    /* see under SPI_commit() */
     if (_SPI_current->atomic)
         ereport(ERROR,
                 (errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
@@ -304,34 +346,68 @@ _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();
+    /* XXX this ain't re-entrant enough for my taste */
+    if (chain)
+        SaveTransactionCharacteristics();

-    /* Start the actual rollback */
-    _SPI_current->internal_xact = true;
+    /* Catch any error occurring during the ROLLBACK */
+    PG_TRY();
+    {
+        /* Protect current SPI stack entry against deletion */
+        _SPI_current->internal_xact = true;

-    /* Release snapshots associated with portals */
-    ForgetPortalSnapshots();
+        /*
+         * 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();

-    if (chain)
-        SaveTransactionCharacteristics();
+        /* Release snapshots associated with portals */
+        ForgetPortalSnapshots();

-    AbortCurrentTransaction();
+        /* Do the deed */
+        AbortCurrentTransaction();

-    if (chain)
-    {
+        /* Immediately start a new transaction */
         StartTransactionCommand();
-        RestoreTransactionCharacteristics();
+        if (chain)
+            RestoreTransactionCharacteristics();
+
+        MemoryContextSwitchTo(oldcontext);
+
+        _SPI_current->internal_xact = false;
     }
+    PG_CATCH();
+    {
+        ErrorData  *edata;

-    MemoryContextSwitchTo(oldcontext);
+        /* Save error info in caller's context */
+        MemoryContextSwitchTo(oldcontext);
+        edata = CopyErrorData();
+        FlushErrorState();

-    _SPI_current->internal_xact = false;
+        /*
+         * Try again to abort the failed transaction.  If this fails too,
+         * we'll just propagate the error out ... there's not that much we can
+         * do.
+         */
+        AbortCurrentTransaction();
+
+        /* ... and start a new one */
+        StartTransactionCommand();
+        if (chain)
+            RestoreTransactionCharacteristics();
+
+        MemoryContextSwitchTo(oldcontext);
+
+        _SPI_current->internal_xact = false;
+
+        /* Now that we've cleaned up the transaction, re-throw the error */
+        ReThrowError(edata);
+    }
+    PG_END_TRY();
 }

 void
@@ -346,38 +422,55 @@ SPI_rollback_and_chain(void)
     _SPI_rollback(true);
 }

-/*
- * Clean up SPI state.  Called on transaction end (of non-SPI-internal
- * transactions) and when returning to the main loop on error.
- */
-void
-SPICleanup(void)
-{
-    _SPI_current = NULL;
-    _SPI_connected = -1;
-    /* Reset API global variables, too */
-    SPI_processed = 0;
-    SPI_tuptable = NULL;
-    SPI_result = 0;
-}
-
 /*
  * Clean up SPI state at transaction commit or abort.
  */
 void
 AtEOXact_SPI(bool isCommit)
 {
-    /* Do nothing if the transaction end was initiated by SPI. */
-    if (_SPI_current && _SPI_current->internal_xact)
-        return;
+    bool        found = false;

-    if (isCommit && _SPI_connected != -1)
+    /*
+     * Pop stack entries, stopping if we find one marked internal_xact (that
+     * one belongs to the caller of SPI_commit or SPI_abort).
+     */
+    while (_SPI_connected >= 0)
+    {
+        _SPI_connection *connection = &(_SPI_stack[_SPI_connected]);
+
+        if (connection->internal_xact)
+            break;
+
+        found = true;
+
+        /*
+         * We need not release the procedure's memory contexts explicitly, as
+         * they'll go away automatically when their parent context does; see
+         * notes in SPI_connect_ext.
+         */
+
+        /*
+         * Restore outer global variables and pop the stack entry.  Unlike
+         * SPI_finish(), we don't risk switching to memory contexts that might
+         * be already gone.
+         */
+        SPI_processed = connection->outer_processed;
+        SPI_tuptable = connection->outer_tuptable;
+        SPI_result = connection->outer_result;
+
+        _SPI_connected--;
+        if (_SPI_connected < 0)
+            _SPI_current = NULL;
+        else
+            _SPI_current = &(_SPI_stack[_SPI_connected]);
+    }
+
+    /* We should only find entries to pop during an ABORT. */
+    if (found && isCommit)
         ereport(WARNING,
                 (errcode(ERRCODE_WARNING),
                  errmsg("transaction left non-empty SPI stack"),
                  errhint("Check for missing \"SPI_finish\" calls.")));
-
-    SPICleanup();
 }

 /*
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 3c7d08209f..34c13a1113 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -43,7 +43,6 @@
 #include "commands/async.h"
 #include "commands/prepare.h"
 #include "common/pg_prng.h"
-#include "executor/spi.h"
 #include "jit/jit.h"
 #include "libpq/libpq.h"
 #include "libpq/pqformat.h"
@@ -4263,7 +4262,6 @@ PostgresMain(const char *dbname, const char *username)
             WalSndErrorCleanup();

         PortalErrorCleanup();
-        SPICleanup();

         /*
          * We can't release replication slots inside AbortTransaction() as we
diff --git a/src/backend/utils/mmgr/portalmem.c b/src/backend/utils/mmgr/portalmem.c
index 7885344164..29777b3667 100644
--- a/src/backend/utils/mmgr/portalmem.c
+++ b/src/backend/utils/mmgr/portalmem.c
@@ -1272,7 +1272,7 @@ HoldPinnedPortals(void)
              */
             if (portal->strategy != PORTAL_ONE_SELECT)
                 ereport(ERROR,
-                        (errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
+                        (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
                          errmsg("cannot perform transaction commands inside a cursor loop that is not read-only")));

             /* Verify it's in a suitable state to be held */
diff --git a/src/include/executor/spi.h b/src/include/executor/spi.h
index e20e7df780..6ec3851444 100644
--- a/src/include/executor/spi.h
+++ b/src/include/executor/spi.h
@@ -205,7 +205,6 @@ extern void SPI_commit_and_chain(void);
 extern void SPI_rollback(void);
 extern void SPI_rollback_and_chain(void);

-extern void SPICleanup(void);
 extern void AtEOXact_SPI(bool isCommit);
 extern void AtEOSubXact_SPI(bool isCommit, SubTransactionId mySubid);
 extern bool SPI_inside_nonatomic_context(void);
diff --git a/src/pl/plperl/plperl.c b/src/pl/plperl/plperl.c
index b5879c2947..916b4db0b0 100644
--- a/src/pl/plperl/plperl.c
+++ b/src/pl/plperl/plperl.c
@@ -3961,7 +3961,6 @@ plperl_spi_commit(void)
     PG_TRY();
     {
         SPI_commit();
-        SPI_start_transaction();
     }
     PG_CATCH();
     {
@@ -3986,7 +3985,6 @@ plperl_spi_rollback(void)
     PG_TRY();
     {
         SPI_rollback();
-        SPI_start_transaction();
     }
     PG_CATCH();
     {
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 70c4a75295..87994532dd 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -4907,10 +4907,7 @@ exec_stmt_commit(PLpgSQL_execstate *estate, PLpgSQL_stmt_commit *stmt)
     if (stmt->chain)
         SPI_commit_and_chain();
     else
-    {
         SPI_commit();
-        SPI_start_transaction();
-    }

     /*
      * We need to build new simple-expression infrastructure, since the old
@@ -4934,10 +4931,7 @@ exec_stmt_rollback(PLpgSQL_execstate *estate, PLpgSQL_stmt_rollback *stmt)
     if (stmt->chain)
         SPI_rollback_and_chain();
     else
-    {
         SPI_rollback();
-        SPI_start_transaction();
-    }

     /*
      * We need to build new simple-expression infrastructure, since the old
diff --git a/src/pl/plpython/expected/plpython_transaction.out b/src/pl/plpython/expected/plpython_transaction.out
index 14152993c7..8ae02862d9 100644
--- a/src/pl/plpython/expected/plpython_transaction.out
+++ b/src/pl/plpython/expected/plpython_transaction.out
@@ -55,8 +55,11 @@ for i in range(0, 10):
 return 1
 $$;
 SELECT transaction_test2();
-ERROR:  invalid transaction termination
-CONTEXT:  PL/Python function "transaction_test2"
+ERROR:  spiexceptions.InvalidTransactionTermination: invalid transaction termination
+CONTEXT:  Traceback (most recent call last):
+  PL/Python function "transaction_test2", line 5, in <module>
+    plpy.commit()
+PL/Python function "transaction_test2"
 SELECT * FROM test1;
  a | b
 ---+---
@@ -70,7 +73,7 @@ plpy.execute("CALL transaction_test1()")
 return 1
 $$;
 SELECT transaction_test3();
-ERROR:  spiexceptions.InvalidTransactionTermination: invalid transaction termination
+ERROR:  spiexceptions.InvalidTransactionTermination: spiexceptions.InvalidTransactionTermination: invalid transaction
termination
 CONTEXT:  Traceback (most recent call last):
   PL/Python function "transaction_test3", line 2, in <module>
     plpy.execute("CALL transaction_test1()")
@@ -88,7 +91,7 @@ plpy.execute("DO LANGUAGE plpythonu $x$ plpy.commit() $x$")
 return 1
 $$;
 SELECT transaction_test4();
-ERROR:  spiexceptions.InvalidTransactionTermination: invalid transaction termination
+ERROR:  spiexceptions.InvalidTransactionTermination: spiexceptions.InvalidTransactionTermination: invalid transaction
termination
 CONTEXT:  Traceback (most recent call last):
   PL/Python function "transaction_test4", line 2, in <module>
     plpy.execute("DO LANGUAGE plpythonu $x$ plpy.commit() $x$")
@@ -100,8 +103,11 @@ s.enter()
 plpy.commit()
 $$;
 WARNING:  forcibly aborting a subtransaction that has not been exited
-ERROR:  cannot commit while a subtransaction is active
-CONTEXT:  PL/Python anonymous code block
+ERROR:  spiexceptions.InvalidTransactionTermination: cannot commit while a subtransaction is active
+CONTEXT:  Traceback (most recent call last):
+  PL/Python anonymous code block, line 4, in <module>
+    plpy.commit()
+PL/Python anonymous code block
 -- commit inside cursor loop
 CREATE TABLE test2 (x int);
 INSERT INTO test2 VALUES (0), (1), (2), (3), (4);
@@ -191,5 +197,32 @@ SELECT * FROM pg_cursors;
 ------+-----------+-------------+-----------+---------------+---------------
 (0 rows)

+-- check handling of an error during a procedure's COMMIT
+CREATE TABLE testpk (id int PRIMARY KEY);
+CREATE TABLE testfk(f1 int REFERENCES testpk DEFERRABLE INITIALLY DEFERRED);
+DO LANGUAGE plpythonu $$
+# this insert will fail during commit:
+plpy.execute("INSERT INTO testfk VALUES (0)")
+try:
+    plpy.commit()
+except Exception as e:
+    plpy.info('sqlstate: %s' % (e.sqlstate))
+# these inserts should work:
+plpy.execute("INSERT INTO testpk VALUES (1)")
+plpy.execute("INSERT INTO testfk VALUES (1)")
+$$;
+INFO:  sqlstate: 23503
+SELECT * FROM testpk;
+ id
+----
+  1
+(1 row)
+
+SELECT * FROM testfk;
+ f1
+----
+  1
+(1 row)
+
 DROP TABLE test1;
 DROP TABLE test2;
diff --git a/src/pl/plpython/plpy_plpymodule.c b/src/pl/plpython/plpy_plpymodule.c
index 0365acc95b..907f89d153 100644
--- a/src/pl/plpython/plpy_plpymodule.c
+++ b/src/pl/plpython/plpy_plpymodule.c
@@ -40,8 +40,6 @@ static PyObject *PLy_fatal(PyObject *self, PyObject *args, PyObject *kw);
 static PyObject *PLy_quote_literal(PyObject *self, PyObject *args);
 static PyObject *PLy_quote_nullable(PyObject *self, PyObject *args);
 static PyObject *PLy_quote_ident(PyObject *self, PyObject *args);
-static PyObject *PLy_commit(PyObject *self, PyObject *args);
-static PyObject *PLy_rollback(PyObject *self, PyObject *args);


 /* A list of all known exceptions, generated from backend/utils/errcodes.txt */
@@ -577,31 +575,3 @@ PLy_output(volatile int level, PyObject *self, PyObject *args, PyObject *kw)
      */
     Py_RETURN_NONE;
 }
-
-static PyObject *
-PLy_commit(PyObject *self, PyObject *args)
-{
-    PLyExecutionContext *exec_ctx = PLy_current_execution_context();
-
-    SPI_commit();
-    SPI_start_transaction();
-
-    /* was cleared at transaction end, reset pointer */
-    exec_ctx->scratch_ctx = NULL;
-
-    Py_RETURN_NONE;
-}
-
-static PyObject *
-PLy_rollback(PyObject *self, PyObject *args)
-{
-    PLyExecutionContext *exec_ctx = PLy_current_execution_context();
-
-    SPI_rollback();
-    SPI_start_transaction();
-
-    /* was cleared at transaction end, reset pointer */
-    exec_ctx->scratch_ctx = NULL;
-
-    Py_RETURN_NONE;
-}
diff --git a/src/pl/plpython/plpy_spi.c b/src/pl/plpython/plpy_spi.c
index 99c1b4f28f..86d70470a7 100644
--- a/src/pl/plpython/plpy_spi.c
+++ b/src/pl/plpython/plpy_spi.c
@@ -456,6 +456,100 @@ PLy_spi_execute_fetch_result(SPITupleTable *tuptable, uint64 rows, int status)
     return (PyObject *) result;
 }

+PyObject *
+PLy_commit(PyObject *self, PyObject *args)
+{
+    MemoryContext oldcontext = CurrentMemoryContext;
+    PLyExecutionContext *exec_ctx = PLy_current_execution_context();
+
+    PG_TRY();
+    {
+        SPI_commit();
+
+        /* was cleared at transaction end, reset pointer */
+        exec_ctx->scratch_ctx = NULL;
+    }
+    PG_CATCH();
+    {
+        ErrorData  *edata;
+        PLyExceptionEntry *entry;
+        PyObject   *exc;
+
+        /* Save error info */
+        MemoryContextSwitchTo(oldcontext);
+        edata = CopyErrorData();
+        FlushErrorState();
+
+        /* was cleared at transaction end, reset pointer */
+        exec_ctx->scratch_ctx = NULL;
+
+        /* Look up the correct exception */
+        entry = hash_search(PLy_spi_exceptions, &(edata->sqlerrcode),
+                            HASH_FIND, NULL);
+
+        /*
+         * This could be a custom error code, if that's the case fallback to
+         * SPIError
+         */
+        exc = entry ? entry->exc : PLy_exc_spi_error;
+        /* Make Python raise the exception */
+        PLy_spi_exception_set(exc, edata);
+        FreeErrorData(edata);
+
+        return NULL;
+    }
+    PG_END_TRY();
+
+    Py_RETURN_NONE;
+}
+
+PyObject *
+PLy_rollback(PyObject *self, PyObject *args)
+{
+    MemoryContext oldcontext = CurrentMemoryContext;
+    PLyExecutionContext *exec_ctx = PLy_current_execution_context();
+
+    PG_TRY();
+    {
+        SPI_rollback();
+
+        /* was cleared at transaction end, reset pointer */
+        exec_ctx->scratch_ctx = NULL;
+    }
+    PG_CATCH();
+    {
+        ErrorData  *edata;
+        PLyExceptionEntry *entry;
+        PyObject   *exc;
+
+        /* Save error info */
+        MemoryContextSwitchTo(oldcontext);
+        edata = CopyErrorData();
+        FlushErrorState();
+
+        /* was cleared at transaction end, reset pointer */
+        exec_ctx->scratch_ctx = NULL;
+
+        /* Look up the correct exception */
+        entry = hash_search(PLy_spi_exceptions, &(edata->sqlerrcode),
+                            HASH_FIND, NULL);
+
+        /*
+         * This could be a custom error code, if that's the case fallback to
+         * SPIError
+         */
+        exc = entry ? entry->exc : PLy_exc_spi_error;
+        /* Make Python raise the exception */
+        PLy_spi_exception_set(exc, edata);
+        FreeErrorData(edata);
+
+        return NULL;
+    }
+    PG_END_TRY();
+
+    Py_RETURN_NONE;
+}
+
 /*
  * Utilities for running SPI functions in subtransactions.
  *
diff --git a/src/pl/plpython/plpy_spi.h b/src/pl/plpython/plpy_spi.h
index a5e2e60da7..98ccd21093 100644
--- a/src/pl/plpython/plpy_spi.h
+++ b/src/pl/plpython/plpy_spi.h
@@ -12,6 +12,9 @@ extern PyObject *PLy_spi_prepare(PyObject *self, PyObject *args);
 extern PyObject *PLy_spi_execute(PyObject *self, PyObject *args);
 extern PyObject *PLy_spi_execute_plan(PyObject *ob, PyObject *list, long limit);

+extern PyObject *PLy_commit(PyObject *self, PyObject *args);
+extern PyObject *PLy_rollback(PyObject *self, PyObject *args);
+
 typedef struct PLyExceptionEntry
 {
     int            sqlstate;        /* hash key, must be first */
diff --git a/src/pl/plpython/sql/plpython_transaction.sql b/src/pl/plpython/sql/plpython_transaction.sql
index 33b37e5b7f..9ba95d8ee3 100644
--- a/src/pl/plpython/sql/plpython_transaction.sql
+++ b/src/pl/plpython/sql/plpython_transaction.sql
@@ -148,5 +148,25 @@ SELECT * FROM test1;
 SELECT * FROM pg_cursors;


+-- check handling of an error during a procedure's COMMIT
+CREATE TABLE testpk (id int PRIMARY KEY);
+CREATE TABLE testfk(f1 int REFERENCES testpk DEFERRABLE INITIALLY DEFERRED);
+
+DO LANGUAGE plpythonu $$
+# this insert will fail during commit:
+plpy.execute("INSERT INTO testfk VALUES (0)")
+try:
+    plpy.commit()
+except Exception as e:
+    plpy.info('sqlstate: %s' % (e.sqlstate))
+# these inserts should work:
+plpy.execute("INSERT INTO testpk VALUES (1)")
+plpy.execute("INSERT INTO testfk VALUES (1)")
+$$;
+
+SELECT * FROM testpk;
+SELECT * FROM testfk;
+
+
 DROP TABLE test1;
 DROP TABLE test2;
diff --git a/src/pl/tcl/pltcl.c b/src/pl/tcl/pltcl.c
index ab759833db..34cf4e32be 100644
--- a/src/pl/tcl/pltcl.c
+++ b/src/pl/tcl/pltcl.c
@@ -2931,7 +2931,6 @@ pltcl_commit(ClientData cdata, Tcl_Interp *interp,
     PG_TRY();
     {
         SPI_commit();
-        SPI_start_transaction();
     }
     PG_CATCH();
     {
@@ -2971,7 +2970,6 @@ pltcl_rollback(ClientData cdata, Tcl_Interp *interp,
     PG_TRY();
     {
         SPI_rollback();
-        SPI_start_transaction();
     }
     PG_CATCH();
     {

Re: fix crash with Python 3.11

From

Tom Lane

Date:

24 February 2022, 21:17:38

I wrote:
> * It might be a good idea to add parallel test cases for the other PLs.

As I suspected, plperl and pltcl show exactly the same problems
when trapping a failure at commit, reinforcing my opinion that this
is a SPI bug that needs to be fixed in SPI.  (plpgsql is not subject
to this problem, because its only mechanism for trapping errors is
a BEGIN block, ie a subtransaction, so it won't get to the interesting
part.)  They do have logic to catch the thrown error, though, so no
additional fix is needed in either once we fix the core code.

v2 attached adds those test cases.

            regards, tom lane

diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index c93f90de9b..7971050746 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -156,7 +156,8 @@ SPI_connect_ext(int options)
      * XXX It could be better to use PortalContext as the parent context in
      * all cases, but we may not be inside a portal (consider deferred-trigger
      * execution).  Perhaps CurTransactionContext could be an option?  For now
-     * it doesn't matter because we clean up explicitly in AtEOSubXact_SPI().
+     * it doesn't matter because we clean up explicitly in AtEOSubXact_SPI();
+     * but see also AtEOXact_SPI().
      */
     _SPI_current->procCxt = AllocSetContextCreate(_SPI_current->atomic ? TopTransactionContext : PortalContext,
                                                   "SPI Proc",
@@ -214,13 +215,13 @@ SPI_finish(void)
     return SPI_OK_FINISH;
 }

+/*
+ * SPI_start_transaction is a no-op, kept for backwards compatibility.
+ * SPI callers are *always* inside a transaction.
+ */
 void
 SPI_start_transaction(void)
 {
-    MemoryContext oldcontext = CurrentMemoryContext;
-
-    StartTransactionCommand();
-    MemoryContextSwitchTo(oldcontext);
 }

 static void
@@ -228,6 +229,12 @@ _SPI_commit(bool chain)
 {
     MemoryContext oldcontext = CurrentMemoryContext;

+    /*
+     * Complain if we are in a context that doesn't permit transaction
+     * termination.  (Note: here and _SPI_rollback should be the only places
+     * that throw ERRCODE_INVALID_TRANSACTION_TERMINATION, so that callers can
+     * test for that with security that they know what happened.)
+     */
     if (_SPI_current->atomic)
         ereport(ERROR,
                 (errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
@@ -240,40 +247,74 @@ _SPI_commit(bool chain)
      * top-level transaction in such a block violates that idea.  A future PL
      * implementation might have different ideas about this, in which case
      * this restriction would have to be refined or the check possibly be
-     * moved out of SPI into the PLs.
+     * moved out of SPI into the PLs.  Note however that the code below relies
+     * on not being within a subtransaction.
      */
     if (IsSubTransaction())
         ereport(ERROR,
                 (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();
+    /* XXX this ain't re-entrant enough for my taste */
+    if (chain)
+        SaveTransactionCharacteristics();

-    /* Start the actual commit */
-    _SPI_current->internal_xact = true;
+    /* Catch any error occurring during the COMMIT */
+    PG_TRY();
+    {
+        /* Protect current SPI stack entry against deletion */
+        _SPI_current->internal_xact = true;

-    /* Release snapshots associated with portals */
-    ForgetPortalSnapshots();
+        /*
+         * 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();

-    if (chain)
-        SaveTransactionCharacteristics();
+        /* Release snapshots associated with portals */
+        ForgetPortalSnapshots();

-    CommitTransactionCommand();
+        /* Do the deed */
+        CommitTransactionCommand();

-    if (chain)
-    {
+        /* Immediately start a new transaction */
         StartTransactionCommand();
-        RestoreTransactionCharacteristics();
+        if (chain)
+            RestoreTransactionCharacteristics();
+
+        MemoryContextSwitchTo(oldcontext);
+
+        _SPI_current->internal_xact = false;
     }
+    PG_CATCH();
+    {
+        ErrorData  *edata;

-    MemoryContextSwitchTo(oldcontext);
+        /* Save error info in caller's context */
+        MemoryContextSwitchTo(oldcontext);
+        edata = CopyErrorData();
+        FlushErrorState();

-    _SPI_current->internal_xact = false;
+        /*
+         * Abort the failed transaction.  If this fails too, we'll just
+         * propagate the error out ... there's not that much we can do.
+         */
+        AbortCurrentTransaction();
+
+        /* ... and start a new one */
+        StartTransactionCommand();
+        if (chain)
+            RestoreTransactionCharacteristics();
+
+        MemoryContextSwitchTo(oldcontext);
+
+        _SPI_current->internal_xact = false;
+
+        /* Now that we've cleaned up the transaction, re-throw the error */
+        ReThrowError(edata);
+    }
+    PG_END_TRY();
 }

 void
@@ -293,6 +334,7 @@ _SPI_rollback(bool chain)
 {
     MemoryContext oldcontext = CurrentMemoryContext;

+    /* see under SPI_commit() */
     if (_SPI_current->atomic)
         ereport(ERROR,
                 (errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
@@ -304,34 +346,68 @@ _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();
+    /* XXX this ain't re-entrant enough for my taste */
+    if (chain)
+        SaveTransactionCharacteristics();

-    /* Start the actual rollback */
-    _SPI_current->internal_xact = true;
+    /* Catch any error occurring during the ROLLBACK */
+    PG_TRY();
+    {
+        /* Protect current SPI stack entry against deletion */
+        _SPI_current->internal_xact = true;

-    /* Release snapshots associated with portals */
-    ForgetPortalSnapshots();
+        /*
+         * 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();

-    if (chain)
-        SaveTransactionCharacteristics();
+        /* Release snapshots associated with portals */
+        ForgetPortalSnapshots();

-    AbortCurrentTransaction();
+        /* Do the deed */
+        AbortCurrentTransaction();

-    if (chain)
-    {
+        /* Immediately start a new transaction */
         StartTransactionCommand();
-        RestoreTransactionCharacteristics();
+        if (chain)
+            RestoreTransactionCharacteristics();
+
+        MemoryContextSwitchTo(oldcontext);
+
+        _SPI_current->internal_xact = false;
     }
+    PG_CATCH();
+    {
+        ErrorData  *edata;

-    MemoryContextSwitchTo(oldcontext);
+        /* Save error info in caller's context */
+        MemoryContextSwitchTo(oldcontext);
+        edata = CopyErrorData();
+        FlushErrorState();

-    _SPI_current->internal_xact = false;
+        /*
+         * Try again to abort the failed transaction.  If this fails too,
+         * we'll just propagate the error out ... there's not that much we can
+         * do.
+         */
+        AbortCurrentTransaction();
+
+        /* ... and start a new one */
+        StartTransactionCommand();
+        if (chain)
+            RestoreTransactionCharacteristics();
+
+        MemoryContextSwitchTo(oldcontext);
+
+        _SPI_current->internal_xact = false;
+
+        /* Now that we've cleaned up the transaction, re-throw the error */
+        ReThrowError(edata);
+    }
+    PG_END_TRY();
 }

 void
@@ -346,38 +422,55 @@ SPI_rollback_and_chain(void)
     _SPI_rollback(true);
 }

-/*
- * Clean up SPI state.  Called on transaction end (of non-SPI-internal
- * transactions) and when returning to the main loop on error.
- */
-void
-SPICleanup(void)
-{
-    _SPI_current = NULL;
-    _SPI_connected = -1;
-    /* Reset API global variables, too */
-    SPI_processed = 0;
-    SPI_tuptable = NULL;
-    SPI_result = 0;
-}
-
 /*
  * Clean up SPI state at transaction commit or abort.
  */
 void
 AtEOXact_SPI(bool isCommit)
 {
-    /* Do nothing if the transaction end was initiated by SPI. */
-    if (_SPI_current && _SPI_current->internal_xact)
-        return;
+    bool        found = false;

-    if (isCommit && _SPI_connected != -1)
+    /*
+     * Pop stack entries, stopping if we find one marked internal_xact (that
+     * one belongs to the caller of SPI_commit or SPI_abort).
+     */
+    while (_SPI_connected >= 0)
+    {
+        _SPI_connection *connection = &(_SPI_stack[_SPI_connected]);
+
+        if (connection->internal_xact)
+            break;
+
+        found = true;
+
+        /*
+         * We need not release the procedure's memory contexts explicitly, as
+         * they'll go away automatically when their parent context does; see
+         * notes in SPI_connect_ext.
+         */
+
+        /*
+         * Restore outer global variables and pop the stack entry.  Unlike
+         * SPI_finish(), we don't risk switching to memory contexts that might
+         * be already gone.
+         */
+        SPI_processed = connection->outer_processed;
+        SPI_tuptable = connection->outer_tuptable;
+        SPI_result = connection->outer_result;
+
+        _SPI_connected--;
+        if (_SPI_connected < 0)
+            _SPI_current = NULL;
+        else
+            _SPI_current = &(_SPI_stack[_SPI_connected]);
+    }
+
+    /* We should only find entries to pop during an ABORT. */
+    if (found && isCommit)
         ereport(WARNING,
                 (errcode(ERRCODE_WARNING),
                  errmsg("transaction left non-empty SPI stack"),
                  errhint("Check for missing \"SPI_finish\" calls.")));
-
-    SPICleanup();
 }

 /*
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 3c7d08209f..34c13a1113 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -43,7 +43,6 @@
 #include "commands/async.h"
 #include "commands/prepare.h"
 #include "common/pg_prng.h"
-#include "executor/spi.h"
 #include "jit/jit.h"
 #include "libpq/libpq.h"
 #include "libpq/pqformat.h"
@@ -4263,7 +4262,6 @@ PostgresMain(const char *dbname, const char *username)
             WalSndErrorCleanup();

         PortalErrorCleanup();
-        SPICleanup();

         /*
          * We can't release replication slots inside AbortTransaction() as we
diff --git a/src/backend/utils/mmgr/portalmem.c b/src/backend/utils/mmgr/portalmem.c
index 21ad87c024..afc03682d9 100644
--- a/src/backend/utils/mmgr/portalmem.c
+++ b/src/backend/utils/mmgr/portalmem.c
@@ -1261,7 +1261,7 @@ HoldPinnedPortals(void)
              */
             if (portal->strategy != PORTAL_ONE_SELECT)
                 ereport(ERROR,
-                        (errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
+                        (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
                          errmsg("cannot perform transaction commands inside a cursor loop that is not read-only")));

             /* Verify it's in a suitable state to be held */
diff --git a/src/include/executor/spi.h b/src/include/executor/spi.h
index e20e7df780..6ec3851444 100644
--- a/src/include/executor/spi.h
+++ b/src/include/executor/spi.h
@@ -205,7 +205,6 @@ extern void SPI_commit_and_chain(void);
 extern void SPI_rollback(void);
 extern void SPI_rollback_and_chain(void);

-extern void SPICleanup(void);
 extern void AtEOXact_SPI(bool isCommit);
 extern void AtEOSubXact_SPI(bool isCommit, SubTransactionId mySubid);
 extern bool SPI_inside_nonatomic_context(void);
diff --git a/src/pl/plperl/expected/plperl_transaction.out b/src/pl/plperl/expected/plperl_transaction.out
index 7ca0ef35fb..da4283cbce 100644
--- a/src/pl/plperl/expected/plperl_transaction.out
+++ b/src/pl/plperl/expected/plperl_transaction.out
@@ -192,5 +192,53 @@ SELECT * FROM pg_cursors;
 ------+-----------+-------------+-----------+---------------+---------------
 (0 rows)

+-- check handling of an error during COMMIT
+CREATE TABLE testpk (id int PRIMARY KEY);
+CREATE TABLE testfk(f1 int REFERENCES testpk DEFERRABLE INITIALLY DEFERRED);
+DO LANGUAGE plperl $$
+# this insert will fail during commit:
+spi_exec_query("INSERT INTO testfk VALUES (0)");
+spi_commit();
+elog(WARNING, 'should not get here');
+$$;
+ERROR:  insert or update on table "testfk" violates foreign key constraint "testfk_f1_fkey" at line 4.
+CONTEXT:  PL/Perl anonymous code block
+SELECT * FROM testpk;
+ id
+----
+(0 rows)
+
+SELECT * FROM testfk;
+ f1
+----
+(0 rows)
+
+DO LANGUAGE plperl $$
+# this insert will fail during commit:
+spi_exec_query("INSERT INTO testfk VALUES (0)");
+eval {
+    spi_commit();
+};
+if ($@) {
+    elog(INFO, $@);
+}
+# these inserts should work:
+spi_exec_query("INSERT INTO testpk VALUES (1)");
+spi_exec_query("INSERT INTO testfk VALUES (1)");
+$$;
+INFO:  insert or update on table "testfk" violates foreign key constraint "testfk_f1_fkey" at line 5.
+
+SELECT * FROM testpk;
+ id
+----
+  1
+(1 row)
+
+SELECT * FROM testfk;
+ f1
+----
+  1
+(1 row)
+
 DROP TABLE test1;
 DROP TABLE test2;
diff --git a/src/pl/plperl/plperl.c b/src/pl/plperl/plperl.c
index 81d9c46e00..76ee997b43 100644
--- a/src/pl/plperl/plperl.c
+++ b/src/pl/plperl/plperl.c
@@ -3964,7 +3964,6 @@ plperl_spi_commit(void)
     PG_TRY();
     {
         SPI_commit();
-        SPI_start_transaction();
     }
     PG_CATCH();
     {
@@ -3989,7 +3988,6 @@ plperl_spi_rollback(void)
     PG_TRY();
     {
         SPI_rollback();
-        SPI_start_transaction();
     }
     PG_CATCH();
     {
diff --git a/src/pl/plperl/sql/plperl_transaction.sql b/src/pl/plperl/sql/plperl_transaction.sql
index 0a60799805..d10c8bee89 100644
--- a/src/pl/plperl/sql/plperl_transaction.sql
+++ b/src/pl/plperl/sql/plperl_transaction.sql
@@ -159,5 +159,37 @@ SELECT * FROM test1;
 SELECT * FROM pg_cursors;


+-- check handling of an error during COMMIT
+CREATE TABLE testpk (id int PRIMARY KEY);
+CREATE TABLE testfk(f1 int REFERENCES testpk DEFERRABLE INITIALLY DEFERRED);
+
+DO LANGUAGE plperl $$
+# this insert will fail during commit:
+spi_exec_query("INSERT INTO testfk VALUES (0)");
+spi_commit();
+elog(WARNING, 'should not get here');
+$$;
+
+SELECT * FROM testpk;
+SELECT * FROM testfk;
+
+DO LANGUAGE plperl $$
+# this insert will fail during commit:
+spi_exec_query("INSERT INTO testfk VALUES (0)");
+eval {
+    spi_commit();
+};
+if ($@) {
+    elog(INFO, $@);
+}
+# these inserts should work:
+spi_exec_query("INSERT INTO testpk VALUES (1)");
+spi_exec_query("INSERT INTO testfk VALUES (1)");
+$$;
+
+SELECT * FROM testpk;
+SELECT * FROM testfk;
+
+
 DROP TABLE test1;
 DROP TABLE test2;
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 9674c29250..915139378e 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -4916,10 +4916,7 @@ exec_stmt_commit(PLpgSQL_execstate *estate, PLpgSQL_stmt_commit *stmt)
     if (stmt->chain)
         SPI_commit_and_chain();
     else
-    {
         SPI_commit();
-        SPI_start_transaction();
-    }

     /*
      * We need to build new simple-expression infrastructure, since the old
@@ -4943,10 +4940,7 @@ exec_stmt_rollback(PLpgSQL_execstate *estate, PLpgSQL_stmt_rollback *stmt)
     if (stmt->chain)
         SPI_rollback_and_chain();
     else
-    {
         SPI_rollback();
-        SPI_start_transaction();
-    }

     /*
      * We need to build new simple-expression infrastructure, since the old
diff --git a/src/pl/plpython/expected/plpython_transaction.out b/src/pl/plpython/expected/plpython_transaction.out
index 14152993c7..72d1e45a76 100644
--- a/src/pl/plpython/expected/plpython_transaction.out
+++ b/src/pl/plpython/expected/plpython_transaction.out
@@ -55,8 +55,11 @@ for i in range(0, 10):
 return 1
 $$;
 SELECT transaction_test2();
-ERROR:  invalid transaction termination
-CONTEXT:  PL/Python function "transaction_test2"
+ERROR:  spiexceptions.InvalidTransactionTermination: invalid transaction termination
+CONTEXT:  Traceback (most recent call last):
+  PL/Python function "transaction_test2", line 5, in <module>
+    plpy.commit()
+PL/Python function "transaction_test2"
 SELECT * FROM test1;
  a | b
 ---+---
@@ -70,7 +73,7 @@ plpy.execute("CALL transaction_test1()")
 return 1
 $$;
 SELECT transaction_test3();
-ERROR:  spiexceptions.InvalidTransactionTermination: invalid transaction termination
+ERROR:  spiexceptions.InvalidTransactionTermination: spiexceptions.InvalidTransactionTermination: invalid transaction
termination
 CONTEXT:  Traceback (most recent call last):
   PL/Python function "transaction_test3", line 2, in <module>
     plpy.execute("CALL transaction_test1()")
@@ -88,7 +91,7 @@ plpy.execute("DO LANGUAGE plpythonu $x$ plpy.commit() $x$")
 return 1
 $$;
 SELECT transaction_test4();
-ERROR:  spiexceptions.InvalidTransactionTermination: invalid transaction termination
+ERROR:  spiexceptions.InvalidTransactionTermination: spiexceptions.InvalidTransactionTermination: invalid transaction
termination
 CONTEXT:  Traceback (most recent call last):
   PL/Python function "transaction_test4", line 2, in <module>
     plpy.execute("DO LANGUAGE plpythonu $x$ plpy.commit() $x$")
@@ -100,8 +103,11 @@ s.enter()
 plpy.commit()
 $$;
 WARNING:  forcibly aborting a subtransaction that has not been exited
-ERROR:  cannot commit while a subtransaction is active
-CONTEXT:  PL/Python anonymous code block
+ERROR:  spiexceptions.InvalidTransactionTermination: cannot commit while a subtransaction is active
+CONTEXT:  Traceback (most recent call last):
+  PL/Python anonymous code block, line 4, in <module>
+    plpy.commit()
+PL/Python anonymous code block
 -- commit inside cursor loop
 CREATE TABLE test2 (x int);
 INSERT INTO test2 VALUES (0), (1), (2), (3), (4);
@@ -191,5 +197,54 @@ SELECT * FROM pg_cursors;
 ------+-----------+-------------+-----------+---------------+---------------
 (0 rows)

+-- check handling of an error during COMMIT
+CREATE TABLE testpk (id int PRIMARY KEY);
+CREATE TABLE testfk(f1 int REFERENCES testpk DEFERRABLE INITIALLY DEFERRED);
+DO LANGUAGE plpythonu $$
+# this insert will fail during commit:
+plpy.execute("INSERT INTO testfk VALUES (0)")
+plpy.commit()
+plpy.warning('should not get here')
+$$;
+ERROR:  spiexceptions.ForeignKeyViolation: insert or update on table "testfk" violates foreign key constraint
"testfk_f1_fkey"
+DETAIL:  Key (f1)=(0) is not present in table "testpk".
+CONTEXT:  Traceback (most recent call last):
+  PL/Python anonymous code block, line 4, in <module>
+    plpy.commit()
+PL/Python anonymous code block
+SELECT * FROM testpk;
+ id
+----
+(0 rows)
+
+SELECT * FROM testfk;
+ f1
+----
+(0 rows)
+
+DO LANGUAGE plpythonu $$
+# this insert will fail during commit:
+plpy.execute("INSERT INTO testfk VALUES (0)")
+try:
+    plpy.commit()
+except Exception as e:
+    plpy.info('sqlstate: %s' % (e.sqlstate))
+# these inserts should work:
+plpy.execute("INSERT INTO testpk VALUES (1)")
+plpy.execute("INSERT INTO testfk VALUES (1)")
+$$;
+INFO:  sqlstate: 23503
+SELECT * FROM testpk;
+ id
+----
+  1
+(1 row)
+
+SELECT * FROM testfk;
+ f1
+----
+  1
+(1 row)
+
 DROP TABLE test1;
 DROP TABLE test2;
diff --git a/src/pl/plpython/plpy_plpymodule.c b/src/pl/plpython/plpy_plpymodule.c
index 0365acc95b..907f89d153 100644
--- a/src/pl/plpython/plpy_plpymodule.c
+++ b/src/pl/plpython/plpy_plpymodule.c
@@ -40,8 +40,6 @@ static PyObject *PLy_fatal(PyObject *self, PyObject *args, PyObject *kw);
 static PyObject *PLy_quote_literal(PyObject *self, PyObject *args);
 static PyObject *PLy_quote_nullable(PyObject *self, PyObject *args);
 static PyObject *PLy_quote_ident(PyObject *self, PyObject *args);
-static PyObject *PLy_commit(PyObject *self, PyObject *args);
-static PyObject *PLy_rollback(PyObject *self, PyObject *args);


 /* A list of all known exceptions, generated from backend/utils/errcodes.txt */
@@ -577,31 +575,3 @@ PLy_output(volatile int level, PyObject *self, PyObject *args, PyObject *kw)
      */
     Py_RETURN_NONE;
 }
-
-static PyObject *
-PLy_commit(PyObject *self, PyObject *args)
-{
-    PLyExecutionContext *exec_ctx = PLy_current_execution_context();
-
-    SPI_commit();
-    SPI_start_transaction();
-
-    /* was cleared at transaction end, reset pointer */
-    exec_ctx->scratch_ctx = NULL;
-
-    Py_RETURN_NONE;
-}
-
-static PyObject *
-PLy_rollback(PyObject *self, PyObject *args)
-{
-    PLyExecutionContext *exec_ctx = PLy_current_execution_context();
-
-    SPI_rollback();
-    SPI_start_transaction();
-
-    /* was cleared at transaction end, reset pointer */
-    exec_ctx->scratch_ctx = NULL;
-
-    Py_RETURN_NONE;
-}
diff --git a/src/pl/plpython/plpy_spi.c b/src/pl/plpython/plpy_spi.c
index 99c1b4f28f..86d70470a7 100644
--- a/src/pl/plpython/plpy_spi.c
+++ b/src/pl/plpython/plpy_spi.c
@@ -456,6 +456,100 @@ PLy_spi_execute_fetch_result(SPITupleTable *tuptable, uint64 rows, int status)
     return (PyObject *) result;
 }

+PyObject *
+PLy_commit(PyObject *self, PyObject *args)
+{
+    MemoryContext oldcontext = CurrentMemoryContext;
+    PLyExecutionContext *exec_ctx = PLy_current_execution_context();
+
+    PG_TRY();
+    {
+        SPI_commit();
+
+        /* was cleared at transaction end, reset pointer */
+        exec_ctx->scratch_ctx = NULL;
+    }
+    PG_CATCH();
+    {
+        ErrorData  *edata;
+        PLyExceptionEntry *entry;
+        PyObject   *exc;
+
+        /* Save error info */
+        MemoryContextSwitchTo(oldcontext);
+        edata = CopyErrorData();
+        FlushErrorState();
+
+        /* was cleared at transaction end, reset pointer */
+        exec_ctx->scratch_ctx = NULL;
+
+        /* Look up the correct exception */
+        entry = hash_search(PLy_spi_exceptions, &(edata->sqlerrcode),
+                            HASH_FIND, NULL);
+
+        /*
+         * This could be a custom error code, if that's the case fallback to
+         * SPIError
+         */
+        exc = entry ? entry->exc : PLy_exc_spi_error;
+        /* Make Python raise the exception */
+        PLy_spi_exception_set(exc, edata);
+        FreeErrorData(edata);
+
+        return NULL;
+    }
+    PG_END_TRY();
+
+    Py_RETURN_NONE;
+}
+
+PyObject *
+PLy_rollback(PyObject *self, PyObject *args)
+{
+    MemoryContext oldcontext = CurrentMemoryContext;
+    PLyExecutionContext *exec_ctx = PLy_current_execution_context();
+
+    PG_TRY();
+    {
+        SPI_rollback();
+
+        /* was cleared at transaction end, reset pointer */
+        exec_ctx->scratch_ctx = NULL;
+    }
+    PG_CATCH();
+    {
+        ErrorData  *edata;
+        PLyExceptionEntry *entry;
+        PyObject   *exc;
+
+        /* Save error info */
+        MemoryContextSwitchTo(oldcontext);
+        edata = CopyErrorData();
+        FlushErrorState();
+
+        /* was cleared at transaction end, reset pointer */
+        exec_ctx->scratch_ctx = NULL;
+
+        /* Look up the correct exception */
+        entry = hash_search(PLy_spi_exceptions, &(edata->sqlerrcode),
+                            HASH_FIND, NULL);
+
+        /*
+         * This could be a custom error code, if that's the case fallback to
+         * SPIError
+         */
+        exc = entry ? entry->exc : PLy_exc_spi_error;
+        /* Make Python raise the exception */
+        PLy_spi_exception_set(exc, edata);
+        FreeErrorData(edata);
+
+        return NULL;
+    }
+    PG_END_TRY();
+
+    Py_RETURN_NONE;
+}
+
 /*
  * Utilities for running SPI functions in subtransactions.
  *
diff --git a/src/pl/plpython/plpy_spi.h b/src/pl/plpython/plpy_spi.h
index a5e2e60da7..98ccd21093 100644
--- a/src/pl/plpython/plpy_spi.h
+++ b/src/pl/plpython/plpy_spi.h
@@ -12,6 +12,9 @@ extern PyObject *PLy_spi_prepare(PyObject *self, PyObject *args);
 extern PyObject *PLy_spi_execute(PyObject *self, PyObject *args);
 extern PyObject *PLy_spi_execute_plan(PyObject *ob, PyObject *list, long limit);

+extern PyObject *PLy_commit(PyObject *self, PyObject *args);
+extern PyObject *PLy_rollback(PyObject *self, PyObject *args);
+
 typedef struct PLyExceptionEntry
 {
     int            sqlstate;        /* hash key, must be first */
diff --git a/src/pl/plpython/sql/plpython_transaction.sql b/src/pl/plpython/sql/plpython_transaction.sql
index 33b37e5b7f..68588d9fb0 100644
--- a/src/pl/plpython/sql/plpython_transaction.sql
+++ b/src/pl/plpython/sql/plpython_transaction.sql
@@ -148,5 +148,35 @@ SELECT * FROM test1;
 SELECT * FROM pg_cursors;


+-- check handling of an error during COMMIT
+CREATE TABLE testpk (id int PRIMARY KEY);
+CREATE TABLE testfk(f1 int REFERENCES testpk DEFERRABLE INITIALLY DEFERRED);
+
+DO LANGUAGE plpythonu $$
+# this insert will fail during commit:
+plpy.execute("INSERT INTO testfk VALUES (0)")
+plpy.commit()
+plpy.warning('should not get here')
+$$;
+
+SELECT * FROM testpk;
+SELECT * FROM testfk;
+
+DO LANGUAGE plpythonu $$
+# this insert will fail during commit:
+plpy.execute("INSERT INTO testfk VALUES (0)")
+try:
+    plpy.commit()
+except Exception as e:
+    plpy.info('sqlstate: %s' % (e.sqlstate))
+# these inserts should work:
+plpy.execute("INSERT INTO testpk VALUES (1)")
+plpy.execute("INSERT INTO testfk VALUES (1)")
+$$;
+
+SELECT * FROM testpk;
+SELECT * FROM testfk;
+
+
 DROP TABLE test1;
 DROP TABLE test2;
diff --git a/src/pl/tcl/expected/pltcl_transaction.out b/src/pl/tcl/expected/pltcl_transaction.out
index 007204b99a..f557b79138 100644
--- a/src/pl/tcl/expected/pltcl_transaction.out
+++ b/src/pl/tcl/expected/pltcl_transaction.out
@@ -96,5 +96,54 @@ SELECT * FROM test1;
 ---+---
 (0 rows)

+-- check handling of an error during COMMIT
+CREATE TABLE testpk (id int PRIMARY KEY);
+CREATE TABLE testfk(f1 int REFERENCES testpk DEFERRABLE INITIALLY DEFERRED);
+CREATE PROCEDURE transaction_testfk()
+LANGUAGE pltcl
+AS $$
+# this insert will fail during commit:
+spi_exec "INSERT INTO testfk VALUES (0)"
+commit
+elog WARNING "should not get here"
+$$;
+CALL transaction_testfk();
+ERROR:  insert or update on table "testfk" violates foreign key constraint "testfk_f1_fkey"
+SELECT * FROM testpk;
+ id
+----
+(0 rows)
+
+SELECT * FROM testfk;
+ f1
+----
+(0 rows)
+
+CREATE OR REPLACE PROCEDURE transaction_testfk()
+LANGUAGE pltcl
+AS $$
+# this insert will fail during commit:
+spi_exec "INSERT INTO testfk VALUES (0)"
+if [catch {commit} msg] {
+    elog INFO $msg
+}
+# these inserts should work:
+spi_exec "INSERT INTO testpk VALUES (1)"
+spi_exec "INSERT INTO testfk VALUES (1)"
+$$;
+CALL transaction_testfk();
+INFO:  insert or update on table "testfk" violates foreign key constraint "testfk_f1_fkey"
+SELECT * FROM testpk;
+ id
+----
+  1
+(1 row)
+
+SELECT * FROM testfk;
+ f1
+----
+  1
+(1 row)
+
 DROP TABLE test1;
 DROP TABLE test2;
diff --git a/src/pl/tcl/pltcl.c b/src/pl/tcl/pltcl.c
index c5fad05e12..68c9bd1970 100644
--- a/src/pl/tcl/pltcl.c
+++ b/src/pl/tcl/pltcl.c
@@ -2935,7 +2935,6 @@ pltcl_commit(ClientData cdata, Tcl_Interp *interp,
     PG_TRY();
     {
         SPI_commit();
-        SPI_start_transaction();
     }
     PG_CATCH();
     {
@@ -2975,7 +2974,6 @@ pltcl_rollback(ClientData cdata, Tcl_Interp *interp,
     PG_TRY();
     {
         SPI_rollback();
-        SPI_start_transaction();
     }
     PG_CATCH();
     {
diff --git a/src/pl/tcl/sql/pltcl_transaction.sql b/src/pl/tcl/sql/pltcl_transaction.sql
index c752faf665..bd759850a7 100644
--- a/src/pl/tcl/sql/pltcl_transaction.sql
+++ b/src/pl/tcl/sql/pltcl_transaction.sql
@@ -94,5 +94,42 @@ CALL transaction_test4b();
 SELECT * FROM test1;


+-- check handling of an error during COMMIT
+CREATE TABLE testpk (id int PRIMARY KEY);
+CREATE TABLE testfk(f1 int REFERENCES testpk DEFERRABLE INITIALLY DEFERRED);
+
+CREATE PROCEDURE transaction_testfk()
+LANGUAGE pltcl
+AS $$
+# this insert will fail during commit:
+spi_exec "INSERT INTO testfk VALUES (0)"
+commit
+elog WARNING "should not get here"
+$$;
+
+CALL transaction_testfk();
+
+SELECT * FROM testpk;
+SELECT * FROM testfk;
+
+CREATE OR REPLACE PROCEDURE transaction_testfk()
+LANGUAGE pltcl
+AS $$
+# this insert will fail during commit:
+spi_exec "INSERT INTO testfk VALUES (0)"
+if [catch {commit} msg] {
+    elog INFO $msg
+}
+# these inserts should work:
+spi_exec "INSERT INTO testpk VALUES (1)"
+spi_exec "INSERT INTO testfk VALUES (1)"
+$$;
+
+CALL transaction_testfk();
+
+SELECT * FROM testpk;
+SELECT * FROM testfk;
+
+
 DROP TABLE test1;
 DROP TABLE test2;

Re: fix crash with Python 3.11

From

Tom Lane

Date:

24 February 2022, 22:34:05

I wrote:
> * I'm not satisfied with using static storage for
> SaveTransactionCharacteristics/RestoreTransactionCharacteristics.

Looking closer at this, I was not too amused to discover that of the three
existing SaveTransactionCharacteristics calls, two already conflict with
each other: _SPI_commit calls SaveTransactionCharacteristics and then
calls CommitTransactionCommand, which again calls
SaveTransactionCharacteristics, overwriting the static storage.
I *think* there's no live bug there, because the state probably wouldn't
have changed in between; but considering we run arbitrary user-defined
code between those two points I sure wouldn't bet on it.

0001 attached is the same code patch as before, but now with spi.sgml
updates; 0002 changes the API for Save/RestoreTransactionCharacteristics.
If anyone's really worried about backpatching 0002, we could perhaps
get away with doing that only in HEAD.

I found in 0002 that I had to make CommitTransactionCommand call
SaveTransactionCharacteristics unconditionally, else I got warnings
about possibly-uninitialized local variables.  It's cheap enough
that I'm not too fussed about that.  I don't get any warnings from
the similar code in spi.c, probably because those functions can't
be inlined there.

            regards, tom lane

diff --git a/doc/src/sgml/spi.sgml b/doc/src/sgml/spi.sgml
index d710e2d0df..7581661fc4 100644
--- a/doc/src/sgml/spi.sgml
+++ b/doc/src/sgml/spi.sgml
@@ -99,10 +99,9 @@ int SPI_connect_ext(int <parameter>options</parameter>)
      <listitem>
       <para>
        Sets the SPI connection to be <firstterm>nonatomic</firstterm>, which
-       means that transaction control calls <function>SPI_commit</function>,
-       <function>SPI_rollback</function>, and
-       <function>SPI_start_transaction</function> are allowed.  Otherwise,
-       calling these functions will result in an immediate error.
+       means that transaction control calls (<function>SPI_commit</function>,
+       <function>SPI_rollback</function>) are allowed.  Otherwise,
+       calling those functions will result in an immediate error.
       </para>
      </listitem>
     </varlistentry>
@@ -5040,15 +5039,17 @@ void SPI_commit_and_chain(void)
   <para>
    <function>SPI_commit</function> commits the current transaction.  It is
    approximately equivalent to running the SQL
-   command <command>COMMIT</command>.  After a transaction is committed, a new
-   transaction has to be started
-   using <function>SPI_start_transaction</function> before further database
-   actions can be executed.
+   command <command>COMMIT</command>.  After the transaction is committed, a
+   new transaction is automatically started using default transaction
+   characteristics, so that the caller can continue using SPI facilities.
+   If there is a failure during commit, the current transaction is instead
+   rolled back and a new transaction is started, after which the error is
+   thrown in the usual way.
   </para>

   <para>
-   <function>SPI_commit_and_chain</function> is the same, but a new
-   transaction is immediately started with the same transaction
+   <function>SPI_commit_and_chain</function> is the same, but the new
+   transaction is started with the same transaction
    characteristics as the just finished one, like with the SQL command
    <command>COMMIT AND CHAIN</command>.
   </para>
@@ -5093,14 +5094,13 @@ void SPI_rollback_and_chain(void)
   <para>
    <function>SPI_rollback</function> rolls back the current transaction.  It
    is approximately equivalent to running the SQL
-   command <command>ROLLBACK</command>.  After a transaction is rolled back, a
-   new transaction has to be started
-   using <function>SPI_start_transaction</function> before further database
-   actions can be executed.
+   command <command>ROLLBACK</command>.  After the transaction is rolled back,
+   a new transaction is automatically started using default transaction
+   characteristics, so that the caller can continue using SPI facilities.
   </para>
   <para>
-   <function>SPI_rollback_and_chain</function> is the same, but a new
-   transaction is immediately started with the same transaction
+   <function>SPI_rollback_and_chain</function> is the same, but the new
+   transaction is started with the same transaction
    characteristics as the just finished one, like with the SQL command
    <command>ROLLBACK AND CHAIN</command>.
   </para>
@@ -5124,7 +5124,7 @@ void SPI_rollback_and_chain(void)

  <refnamediv>
   <refname>SPI_start_transaction</refname>
-  <refpurpose>start a new transaction</refpurpose>
+  <refpurpose>obsolete function</refpurpose>
  </refnamediv>

  <refsynopsisdiv>
@@ -5137,17 +5137,12 @@ void SPI_start_transaction(void)
   <title>Description</title>

   <para>
-   <function>SPI_start_transaction</function> starts a new transaction.  It
-   can only be called after <function>SPI_commit</function>
-   or <function>SPI_rollback</function>, as there is no transaction active at
-   that point.  Normally, when an SPI-using procedure is called, there is already a
-   transaction active, so attempting to start another one before closing out
-   the current one will result in an error.
-  </para>
-
-  <para>
-   This function can only be executed if the SPI connection has been set as
-   nonatomic in the call to <function>SPI_connect_ext</function>.
+   <function>SPI_start_transaction</function> does nothing, and exists
+   only for code compatibility with
+   earlier <productname>PostgreSQL</productname> releases.  It used to
+   be required after calling <function>SPI_commit</function>
+   or <function>SPI_rollback</function>, but now those functions start
+   a new transaction automatically.
   </para>
  </refsect1>
 </refentry>
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index c93f90de9b..7971050746 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -156,7 +156,8 @@ SPI_connect_ext(int options)
      * XXX It could be better to use PortalContext as the parent context in
      * all cases, but we may not be inside a portal (consider deferred-trigger
      * execution).  Perhaps CurTransactionContext could be an option?  For now
-     * it doesn't matter because we clean up explicitly in AtEOSubXact_SPI().
+     * it doesn't matter because we clean up explicitly in AtEOSubXact_SPI();
+     * but see also AtEOXact_SPI().
      */
     _SPI_current->procCxt = AllocSetContextCreate(_SPI_current->atomic ? TopTransactionContext : PortalContext,
                                                   "SPI Proc",
@@ -214,13 +215,13 @@ SPI_finish(void)
     return SPI_OK_FINISH;
 }

+/*
+ * SPI_start_transaction is a no-op, kept for backwards compatibility.
+ * SPI callers are *always* inside a transaction.
+ */
 void
 SPI_start_transaction(void)
 {
-    MemoryContext oldcontext = CurrentMemoryContext;
-
-    StartTransactionCommand();
-    MemoryContextSwitchTo(oldcontext);
 }

 static void
@@ -228,6 +229,12 @@ _SPI_commit(bool chain)
 {
     MemoryContext oldcontext = CurrentMemoryContext;

+    /*
+     * Complain if we are in a context that doesn't permit transaction
+     * termination.  (Note: here and _SPI_rollback should be the only places
+     * that throw ERRCODE_INVALID_TRANSACTION_TERMINATION, so that callers can
+     * test for that with security that they know what happened.)
+     */
     if (_SPI_current->atomic)
         ereport(ERROR,
                 (errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
@@ -240,40 +247,74 @@ _SPI_commit(bool chain)
      * top-level transaction in such a block violates that idea.  A future PL
      * implementation might have different ideas about this, in which case
      * this restriction would have to be refined or the check possibly be
-     * moved out of SPI into the PLs.
+     * moved out of SPI into the PLs.  Note however that the code below relies
+     * on not being within a subtransaction.
      */
     if (IsSubTransaction())
         ereport(ERROR,
                 (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();
+    /* XXX this ain't re-entrant enough for my taste */
+    if (chain)
+        SaveTransactionCharacteristics();

-    /* Start the actual commit */
-    _SPI_current->internal_xact = true;
+    /* Catch any error occurring during the COMMIT */
+    PG_TRY();
+    {
+        /* Protect current SPI stack entry against deletion */
+        _SPI_current->internal_xact = true;

-    /* Release snapshots associated with portals */
-    ForgetPortalSnapshots();
+        /*
+         * 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();

-    if (chain)
-        SaveTransactionCharacteristics();
+        /* Release snapshots associated with portals */
+        ForgetPortalSnapshots();

-    CommitTransactionCommand();
+        /* Do the deed */
+        CommitTransactionCommand();

-    if (chain)
-    {
+        /* Immediately start a new transaction */
         StartTransactionCommand();
-        RestoreTransactionCharacteristics();
+        if (chain)
+            RestoreTransactionCharacteristics();
+
+        MemoryContextSwitchTo(oldcontext);
+
+        _SPI_current->internal_xact = false;
     }
+    PG_CATCH();
+    {
+        ErrorData  *edata;

-    MemoryContextSwitchTo(oldcontext);
+        /* Save error info in caller's context */
+        MemoryContextSwitchTo(oldcontext);
+        edata = CopyErrorData();
+        FlushErrorState();

-    _SPI_current->internal_xact = false;
+        /*
+         * Abort the failed transaction.  If this fails too, we'll just
+         * propagate the error out ... there's not that much we can do.
+         */
+        AbortCurrentTransaction();
+
+        /* ... and start a new one */
+        StartTransactionCommand();
+        if (chain)
+            RestoreTransactionCharacteristics();
+
+        MemoryContextSwitchTo(oldcontext);
+
+        _SPI_current->internal_xact = false;
+
+        /* Now that we've cleaned up the transaction, re-throw the error */
+        ReThrowError(edata);
+    }
+    PG_END_TRY();
 }

 void
@@ -293,6 +334,7 @@ _SPI_rollback(bool chain)
 {
     MemoryContext oldcontext = CurrentMemoryContext;

+    /* see under SPI_commit() */
     if (_SPI_current->atomic)
         ereport(ERROR,
                 (errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
@@ -304,34 +346,68 @@ _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();
+    /* XXX this ain't re-entrant enough for my taste */
+    if (chain)
+        SaveTransactionCharacteristics();

-    /* Start the actual rollback */
-    _SPI_current->internal_xact = true;
+    /* Catch any error occurring during the ROLLBACK */
+    PG_TRY();
+    {
+        /* Protect current SPI stack entry against deletion */
+        _SPI_current->internal_xact = true;

-    /* Release snapshots associated with portals */
-    ForgetPortalSnapshots();
+        /*
+         * 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();

-    if (chain)
-        SaveTransactionCharacteristics();
+        /* Release snapshots associated with portals */
+        ForgetPortalSnapshots();

-    AbortCurrentTransaction();
+        /* Do the deed */
+        AbortCurrentTransaction();

-    if (chain)
-    {
+        /* Immediately start a new transaction */
         StartTransactionCommand();
-        RestoreTransactionCharacteristics();
+        if (chain)
+            RestoreTransactionCharacteristics();
+
+        MemoryContextSwitchTo(oldcontext);
+
+        _SPI_current->internal_xact = false;
     }
+    PG_CATCH();
+    {
+        ErrorData  *edata;

-    MemoryContextSwitchTo(oldcontext);
+        /* Save error info in caller's context */
+        MemoryContextSwitchTo(oldcontext);
+        edata = CopyErrorData();
+        FlushErrorState();

-    _SPI_current->internal_xact = false;
+        /*
+         * Try again to abort the failed transaction.  If this fails too,
+         * we'll just propagate the error out ... there's not that much we can
+         * do.
+         */
+        AbortCurrentTransaction();
+
+        /* ... and start a new one */
+        StartTransactionCommand();
+        if (chain)
+            RestoreTransactionCharacteristics();
+
+        MemoryContextSwitchTo(oldcontext);
+
+        _SPI_current->internal_xact = false;
+
+        /* Now that we've cleaned up the transaction, re-throw the error */
+        ReThrowError(edata);
+    }
+    PG_END_TRY();
 }

 void
@@ -346,38 +422,55 @@ SPI_rollback_and_chain(void)
     _SPI_rollback(true);
 }

-/*
- * Clean up SPI state.  Called on transaction end (of non-SPI-internal
- * transactions) and when returning to the main loop on error.
- */
-void
-SPICleanup(void)
-{
-    _SPI_current = NULL;
-    _SPI_connected = -1;
-    /* Reset API global variables, too */
-    SPI_processed = 0;
-    SPI_tuptable = NULL;
-    SPI_result = 0;
-}
-
 /*
  * Clean up SPI state at transaction commit or abort.
  */
 void
 AtEOXact_SPI(bool isCommit)
 {
-    /* Do nothing if the transaction end was initiated by SPI. */
-    if (_SPI_current && _SPI_current->internal_xact)
-        return;
+    bool        found = false;

-    if (isCommit && _SPI_connected != -1)
+    /*
+     * Pop stack entries, stopping if we find one marked internal_xact (that
+     * one belongs to the caller of SPI_commit or SPI_abort).
+     */
+    while (_SPI_connected >= 0)
+    {
+        _SPI_connection *connection = &(_SPI_stack[_SPI_connected]);
+
+        if (connection->internal_xact)
+            break;
+
+        found = true;
+
+        /*
+         * We need not release the procedure's memory contexts explicitly, as
+         * they'll go away automatically when their parent context does; see
+         * notes in SPI_connect_ext.
+         */
+
+        /*
+         * Restore outer global variables and pop the stack entry.  Unlike
+         * SPI_finish(), we don't risk switching to memory contexts that might
+         * be already gone.
+         */
+        SPI_processed = connection->outer_processed;
+        SPI_tuptable = connection->outer_tuptable;
+        SPI_result = connection->outer_result;
+
+        _SPI_connected--;
+        if (_SPI_connected < 0)
+            _SPI_current = NULL;
+        else
+            _SPI_current = &(_SPI_stack[_SPI_connected]);
+    }
+
+    /* We should only find entries to pop during an ABORT. */
+    if (found && isCommit)
         ereport(WARNING,
                 (errcode(ERRCODE_WARNING),
                  errmsg("transaction left non-empty SPI stack"),
                  errhint("Check for missing \"SPI_finish\" calls.")));
-
-    SPICleanup();
 }

 /*
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 3c7d08209f..34c13a1113 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -43,7 +43,6 @@
 #include "commands/async.h"
 #include "commands/prepare.h"
 #include "common/pg_prng.h"
-#include "executor/spi.h"
 #include "jit/jit.h"
 #include "libpq/libpq.h"
 #include "libpq/pqformat.h"
@@ -4263,7 +4262,6 @@ PostgresMain(const char *dbname, const char *username)
             WalSndErrorCleanup();

         PortalErrorCleanup();
-        SPICleanup();

         /*
          * We can't release replication slots inside AbortTransaction() as we
diff --git a/src/backend/utils/mmgr/portalmem.c b/src/backend/utils/mmgr/portalmem.c
index 21ad87c024..afc03682d9 100644
--- a/src/backend/utils/mmgr/portalmem.c
+++ b/src/backend/utils/mmgr/portalmem.c
@@ -1261,7 +1261,7 @@ HoldPinnedPortals(void)
              */
             if (portal->strategy != PORTAL_ONE_SELECT)
                 ereport(ERROR,
-                        (errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
+                        (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
                          errmsg("cannot perform transaction commands inside a cursor loop that is not read-only")));

             /* Verify it's in a suitable state to be held */
diff --git a/src/include/executor/spi.h b/src/include/executor/spi.h
index e20e7df780..6ec3851444 100644
--- a/src/include/executor/spi.h
+++ b/src/include/executor/spi.h
@@ -205,7 +205,6 @@ extern void SPI_commit_and_chain(void);
 extern void SPI_rollback(void);
 extern void SPI_rollback_and_chain(void);

-extern void SPICleanup(void);
 extern void AtEOXact_SPI(bool isCommit);
 extern void AtEOSubXact_SPI(bool isCommit, SubTransactionId mySubid);
 extern bool SPI_inside_nonatomic_context(void);
diff --git a/src/pl/plperl/expected/plperl_transaction.out b/src/pl/plperl/expected/plperl_transaction.out
index 7ca0ef35fb..da4283cbce 100644
--- a/src/pl/plperl/expected/plperl_transaction.out
+++ b/src/pl/plperl/expected/plperl_transaction.out
@@ -192,5 +192,53 @@ SELECT * FROM pg_cursors;
 ------+-----------+-------------+-----------+---------------+---------------
 (0 rows)

+-- check handling of an error during COMMIT
+CREATE TABLE testpk (id int PRIMARY KEY);
+CREATE TABLE testfk(f1 int REFERENCES testpk DEFERRABLE INITIALLY DEFERRED);
+DO LANGUAGE plperl $$
+# this insert will fail during commit:
+spi_exec_query("INSERT INTO testfk VALUES (0)");
+spi_commit();
+elog(WARNING, 'should not get here');
+$$;
+ERROR:  insert or update on table "testfk" violates foreign key constraint "testfk_f1_fkey" at line 4.
+CONTEXT:  PL/Perl anonymous code block
+SELECT * FROM testpk;
+ id
+----
+(0 rows)
+
+SELECT * FROM testfk;
+ f1
+----
+(0 rows)
+
+DO LANGUAGE plperl $$
+# this insert will fail during commit:
+spi_exec_query("INSERT INTO testfk VALUES (0)");
+eval {
+    spi_commit();
+};
+if ($@) {
+    elog(INFO, $@);
+}
+# these inserts should work:
+spi_exec_query("INSERT INTO testpk VALUES (1)");
+spi_exec_query("INSERT INTO testfk VALUES (1)");
+$$;
+INFO:  insert or update on table "testfk" violates foreign key constraint "testfk_f1_fkey" at line 5.
+
+SELECT * FROM testpk;
+ id
+----
+  1
+(1 row)
+
+SELECT * FROM testfk;
+ f1
+----
+  1
+(1 row)
+
 DROP TABLE test1;
 DROP TABLE test2;
diff --git a/src/pl/plperl/plperl.c b/src/pl/plperl/plperl.c
index 81d9c46e00..76ee997b43 100644
--- a/src/pl/plperl/plperl.c
+++ b/src/pl/plperl/plperl.c
@@ -3964,7 +3964,6 @@ plperl_spi_commit(void)
     PG_TRY();
     {
         SPI_commit();
-        SPI_start_transaction();
     }
     PG_CATCH();
     {
@@ -3989,7 +3988,6 @@ plperl_spi_rollback(void)
     PG_TRY();
     {
         SPI_rollback();
-        SPI_start_transaction();
     }
     PG_CATCH();
     {
diff --git a/src/pl/plperl/sql/plperl_transaction.sql b/src/pl/plperl/sql/plperl_transaction.sql
index 0a60799805..d10c8bee89 100644
--- a/src/pl/plperl/sql/plperl_transaction.sql
+++ b/src/pl/plperl/sql/plperl_transaction.sql
@@ -159,5 +159,37 @@ SELECT * FROM test1;
 SELECT * FROM pg_cursors;


+-- check handling of an error during COMMIT
+CREATE TABLE testpk (id int PRIMARY KEY);
+CREATE TABLE testfk(f1 int REFERENCES testpk DEFERRABLE INITIALLY DEFERRED);
+
+DO LANGUAGE plperl $$
+# this insert will fail during commit:
+spi_exec_query("INSERT INTO testfk VALUES (0)");
+spi_commit();
+elog(WARNING, 'should not get here');
+$$;
+
+SELECT * FROM testpk;
+SELECT * FROM testfk;
+
+DO LANGUAGE plperl $$
+# this insert will fail during commit:
+spi_exec_query("INSERT INTO testfk VALUES (0)");
+eval {
+    spi_commit();
+};
+if ($@) {
+    elog(INFO, $@);
+}
+# these inserts should work:
+spi_exec_query("INSERT INTO testpk VALUES (1)");
+spi_exec_query("INSERT INTO testfk VALUES (1)");
+$$;
+
+SELECT * FROM testpk;
+SELECT * FROM testfk;
+
+
 DROP TABLE test1;
 DROP TABLE test2;
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 9674c29250..915139378e 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -4916,10 +4916,7 @@ exec_stmt_commit(PLpgSQL_execstate *estate, PLpgSQL_stmt_commit *stmt)
     if (stmt->chain)
         SPI_commit_and_chain();
     else
-    {
         SPI_commit();
-        SPI_start_transaction();
-    }

     /*
      * We need to build new simple-expression infrastructure, since the old
@@ -4943,10 +4940,7 @@ exec_stmt_rollback(PLpgSQL_execstate *estate, PLpgSQL_stmt_rollback *stmt)
     if (stmt->chain)
         SPI_rollback_and_chain();
     else
-    {
         SPI_rollback();
-        SPI_start_transaction();
-    }

     /*
      * We need to build new simple-expression infrastructure, since the old
diff --git a/src/pl/plpython/expected/plpython_transaction.out b/src/pl/plpython/expected/plpython_transaction.out
index 14152993c7..72d1e45a76 100644
--- a/src/pl/plpython/expected/plpython_transaction.out
+++ b/src/pl/plpython/expected/plpython_transaction.out
@@ -55,8 +55,11 @@ for i in range(0, 10):
 return 1
 $$;
 SELECT transaction_test2();
-ERROR:  invalid transaction termination
-CONTEXT:  PL/Python function "transaction_test2"
+ERROR:  spiexceptions.InvalidTransactionTermination: invalid transaction termination
+CONTEXT:  Traceback (most recent call last):
+  PL/Python function "transaction_test2", line 5, in <module>
+    plpy.commit()
+PL/Python function "transaction_test2"
 SELECT * FROM test1;
  a | b
 ---+---
@@ -70,7 +73,7 @@ plpy.execute("CALL transaction_test1()")
 return 1
 $$;
 SELECT transaction_test3();
-ERROR:  spiexceptions.InvalidTransactionTermination: invalid transaction termination
+ERROR:  spiexceptions.InvalidTransactionTermination: spiexceptions.InvalidTransactionTermination: invalid transaction
termination
 CONTEXT:  Traceback (most recent call last):
   PL/Python function "transaction_test3", line 2, in <module>
     plpy.execute("CALL transaction_test1()")
@@ -88,7 +91,7 @@ plpy.execute("DO LANGUAGE plpythonu $x$ plpy.commit() $x$")
 return 1
 $$;
 SELECT transaction_test4();
-ERROR:  spiexceptions.InvalidTransactionTermination: invalid transaction termination
+ERROR:  spiexceptions.InvalidTransactionTermination: spiexceptions.InvalidTransactionTermination: invalid transaction
termination
 CONTEXT:  Traceback (most recent call last):
   PL/Python function "transaction_test4", line 2, in <module>
     plpy.execute("DO LANGUAGE plpythonu $x$ plpy.commit() $x$")
@@ -100,8 +103,11 @@ s.enter()
 plpy.commit()
 $$;
 WARNING:  forcibly aborting a subtransaction that has not been exited
-ERROR:  cannot commit while a subtransaction is active
-CONTEXT:  PL/Python anonymous code block
+ERROR:  spiexceptions.InvalidTransactionTermination: cannot commit while a subtransaction is active
+CONTEXT:  Traceback (most recent call last):
+  PL/Python anonymous code block, line 4, in <module>
+    plpy.commit()
+PL/Python anonymous code block
 -- commit inside cursor loop
 CREATE TABLE test2 (x int);
 INSERT INTO test2 VALUES (0), (1), (2), (3), (4);
@@ -191,5 +197,54 @@ SELECT * FROM pg_cursors;
 ------+-----------+-------------+-----------+---------------+---------------
 (0 rows)

+-- check handling of an error during COMMIT
+CREATE TABLE testpk (id int PRIMARY KEY);
+CREATE TABLE testfk(f1 int REFERENCES testpk DEFERRABLE INITIALLY DEFERRED);
+DO LANGUAGE plpythonu $$
+# this insert will fail during commit:
+plpy.execute("INSERT INTO testfk VALUES (0)")
+plpy.commit()
+plpy.warning('should not get here')
+$$;
+ERROR:  spiexceptions.ForeignKeyViolation: insert or update on table "testfk" violates foreign key constraint
"testfk_f1_fkey"
+DETAIL:  Key (f1)=(0) is not present in table "testpk".
+CONTEXT:  Traceback (most recent call last):
+  PL/Python anonymous code block, line 4, in <module>
+    plpy.commit()
+PL/Python anonymous code block
+SELECT * FROM testpk;
+ id
+----
+(0 rows)
+
+SELECT * FROM testfk;
+ f1
+----
+(0 rows)
+
+DO LANGUAGE plpythonu $$
+# this insert will fail during commit:
+plpy.execute("INSERT INTO testfk VALUES (0)")
+try:
+    plpy.commit()
+except Exception as e:
+    plpy.info('sqlstate: %s' % (e.sqlstate))
+# these inserts should work:
+plpy.execute("INSERT INTO testpk VALUES (1)")
+plpy.execute("INSERT INTO testfk VALUES (1)")
+$$;
+INFO:  sqlstate: 23503
+SELECT * FROM testpk;
+ id
+----
+  1
+(1 row)
+
+SELECT * FROM testfk;
+ f1
+----
+  1
+(1 row)
+
 DROP TABLE test1;
 DROP TABLE test2;
diff --git a/src/pl/plpython/plpy_plpymodule.c b/src/pl/plpython/plpy_plpymodule.c
index 0365acc95b..907f89d153 100644
--- a/src/pl/plpython/plpy_plpymodule.c
+++ b/src/pl/plpython/plpy_plpymodule.c
@@ -40,8 +40,6 @@ static PyObject *PLy_fatal(PyObject *self, PyObject *args, PyObject *kw);
 static PyObject *PLy_quote_literal(PyObject *self, PyObject *args);
 static PyObject *PLy_quote_nullable(PyObject *self, PyObject *args);
 static PyObject *PLy_quote_ident(PyObject *self, PyObject *args);
-static PyObject *PLy_commit(PyObject *self, PyObject *args);
-static PyObject *PLy_rollback(PyObject *self, PyObject *args);


 /* A list of all known exceptions, generated from backend/utils/errcodes.txt */
@@ -577,31 +575,3 @@ PLy_output(volatile int level, PyObject *self, PyObject *args, PyObject *kw)
      */
     Py_RETURN_NONE;
 }
-
-static PyObject *
-PLy_commit(PyObject *self, PyObject *args)
-{
-    PLyExecutionContext *exec_ctx = PLy_current_execution_context();
-
-    SPI_commit();
-    SPI_start_transaction();
-
-    /* was cleared at transaction end, reset pointer */
-    exec_ctx->scratch_ctx = NULL;
-
-    Py_RETURN_NONE;
-}
-
-static PyObject *
-PLy_rollback(PyObject *self, PyObject *args)
-{
-    PLyExecutionContext *exec_ctx = PLy_current_execution_context();
-
-    SPI_rollback();
-    SPI_start_transaction();
-
-    /* was cleared at transaction end, reset pointer */
-    exec_ctx->scratch_ctx = NULL;
-
-    Py_RETURN_NONE;
-}
diff --git a/src/pl/plpython/plpy_spi.c b/src/pl/plpython/plpy_spi.c
index 99c1b4f28f..86d70470a7 100644
--- a/src/pl/plpython/plpy_spi.c
+++ b/src/pl/plpython/plpy_spi.c
@@ -456,6 +456,100 @@ PLy_spi_execute_fetch_result(SPITupleTable *tuptable, uint64 rows, int status)
     return (PyObject *) result;
 }

+PyObject *
+PLy_commit(PyObject *self, PyObject *args)
+{
+    MemoryContext oldcontext = CurrentMemoryContext;
+    PLyExecutionContext *exec_ctx = PLy_current_execution_context();
+
+    PG_TRY();
+    {
+        SPI_commit();
+
+        /* was cleared at transaction end, reset pointer */
+        exec_ctx->scratch_ctx = NULL;
+    }
+    PG_CATCH();
+    {
+        ErrorData  *edata;
+        PLyExceptionEntry *entry;
+        PyObject   *exc;
+
+        /* Save error info */
+        MemoryContextSwitchTo(oldcontext);
+        edata = CopyErrorData();
+        FlushErrorState();
+
+        /* was cleared at transaction end, reset pointer */
+        exec_ctx->scratch_ctx = NULL;
+
+        /* Look up the correct exception */
+        entry = hash_search(PLy_spi_exceptions, &(edata->sqlerrcode),
+                            HASH_FIND, NULL);
+
+        /*
+         * This could be a custom error code, if that's the case fallback to
+         * SPIError
+         */
+        exc = entry ? entry->exc : PLy_exc_spi_error;
+        /* Make Python raise the exception */
+        PLy_spi_exception_set(exc, edata);
+        FreeErrorData(edata);
+
+        return NULL;
+    }
+    PG_END_TRY();
+
+    Py_RETURN_NONE;
+}
+
+PyObject *
+PLy_rollback(PyObject *self, PyObject *args)
+{
+    MemoryContext oldcontext = CurrentMemoryContext;
+    PLyExecutionContext *exec_ctx = PLy_current_execution_context();
+
+    PG_TRY();
+    {
+        SPI_rollback();
+
+        /* was cleared at transaction end, reset pointer */
+        exec_ctx->scratch_ctx = NULL;
+    }
+    PG_CATCH();
+    {
+        ErrorData  *edata;
+        PLyExceptionEntry *entry;
+        PyObject   *exc;
+
+        /* Save error info */
+        MemoryContextSwitchTo(oldcontext);
+        edata = CopyErrorData();
+        FlushErrorState();
+
+        /* was cleared at transaction end, reset pointer */
+        exec_ctx->scratch_ctx = NULL;
+
+        /* Look up the correct exception */
+        entry = hash_search(PLy_spi_exceptions, &(edata->sqlerrcode),
+                            HASH_FIND, NULL);
+
+        /*
+         * This could be a custom error code, if that's the case fallback to
+         * SPIError
+         */
+        exc = entry ? entry->exc : PLy_exc_spi_error;
+        /* Make Python raise the exception */
+        PLy_spi_exception_set(exc, edata);
+        FreeErrorData(edata);
+
+        return NULL;
+    }
+    PG_END_TRY();
+
+    Py_RETURN_NONE;
+}
+
 /*
  * Utilities for running SPI functions in subtransactions.
  *
diff --git a/src/pl/plpython/plpy_spi.h b/src/pl/plpython/plpy_spi.h
index a5e2e60da7..98ccd21093 100644
--- a/src/pl/plpython/plpy_spi.h
+++ b/src/pl/plpython/plpy_spi.h
@@ -12,6 +12,9 @@ extern PyObject *PLy_spi_prepare(PyObject *self, PyObject *args);
 extern PyObject *PLy_spi_execute(PyObject *self, PyObject *args);
 extern PyObject *PLy_spi_execute_plan(PyObject *ob, PyObject *list, long limit);

+extern PyObject *PLy_commit(PyObject *self, PyObject *args);
+extern PyObject *PLy_rollback(PyObject *self, PyObject *args);
+
 typedef struct PLyExceptionEntry
 {
     int            sqlstate;        /* hash key, must be first */
diff --git a/src/pl/plpython/sql/plpython_transaction.sql b/src/pl/plpython/sql/plpython_transaction.sql
index 33b37e5b7f..68588d9fb0 100644
--- a/src/pl/plpython/sql/plpython_transaction.sql
+++ b/src/pl/plpython/sql/plpython_transaction.sql
@@ -148,5 +148,35 @@ SELECT * FROM test1;
 SELECT * FROM pg_cursors;


+-- check handling of an error during COMMIT
+CREATE TABLE testpk (id int PRIMARY KEY);
+CREATE TABLE testfk(f1 int REFERENCES testpk DEFERRABLE INITIALLY DEFERRED);
+
+DO LANGUAGE plpythonu $$
+# this insert will fail during commit:
+plpy.execute("INSERT INTO testfk VALUES (0)")
+plpy.commit()
+plpy.warning('should not get here')
+$$;
+
+SELECT * FROM testpk;
+SELECT * FROM testfk;
+
+DO LANGUAGE plpythonu $$
+# this insert will fail during commit:
+plpy.execute("INSERT INTO testfk VALUES (0)")
+try:
+    plpy.commit()
+except Exception as e:
+    plpy.info('sqlstate: %s' % (e.sqlstate))
+# these inserts should work:
+plpy.execute("INSERT INTO testpk VALUES (1)")
+plpy.execute("INSERT INTO testfk VALUES (1)")
+$$;
+
+SELECT * FROM testpk;
+SELECT * FROM testfk;
+
+
 DROP TABLE test1;
 DROP TABLE test2;
diff --git a/src/pl/tcl/expected/pltcl_transaction.out b/src/pl/tcl/expected/pltcl_transaction.out
index 007204b99a..f557b79138 100644
--- a/src/pl/tcl/expected/pltcl_transaction.out
+++ b/src/pl/tcl/expected/pltcl_transaction.out
@@ -96,5 +96,54 @@ SELECT * FROM test1;
 ---+---
 (0 rows)

+-- check handling of an error during COMMIT
+CREATE TABLE testpk (id int PRIMARY KEY);
+CREATE TABLE testfk(f1 int REFERENCES testpk DEFERRABLE INITIALLY DEFERRED);
+CREATE PROCEDURE transaction_testfk()
+LANGUAGE pltcl
+AS $$
+# this insert will fail during commit:
+spi_exec "INSERT INTO testfk VALUES (0)"
+commit
+elog WARNING "should not get here"
+$$;
+CALL transaction_testfk();
+ERROR:  insert or update on table "testfk" violates foreign key constraint "testfk_f1_fkey"
+SELECT * FROM testpk;
+ id
+----
+(0 rows)
+
+SELECT * FROM testfk;
+ f1
+----
+(0 rows)
+
+CREATE OR REPLACE PROCEDURE transaction_testfk()
+LANGUAGE pltcl
+AS $$
+# this insert will fail during commit:
+spi_exec "INSERT INTO testfk VALUES (0)"
+if [catch {commit} msg] {
+    elog INFO $msg
+}
+# these inserts should work:
+spi_exec "INSERT INTO testpk VALUES (1)"
+spi_exec "INSERT INTO testfk VALUES (1)"
+$$;
+CALL transaction_testfk();
+INFO:  insert or update on table "testfk" violates foreign key constraint "testfk_f1_fkey"
+SELECT * FROM testpk;
+ id
+----
+  1
+(1 row)
+
+SELECT * FROM testfk;
+ f1
+----
+  1
+(1 row)
+
 DROP TABLE test1;
 DROP TABLE test2;
diff --git a/src/pl/tcl/pltcl.c b/src/pl/tcl/pltcl.c
index c5fad05e12..68c9bd1970 100644
--- a/src/pl/tcl/pltcl.c
+++ b/src/pl/tcl/pltcl.c
@@ -2935,7 +2935,6 @@ pltcl_commit(ClientData cdata, Tcl_Interp *interp,
     PG_TRY();
     {
         SPI_commit();
-        SPI_start_transaction();
     }
     PG_CATCH();
     {
@@ -2975,7 +2974,6 @@ pltcl_rollback(ClientData cdata, Tcl_Interp *interp,
     PG_TRY();
     {
         SPI_rollback();
-        SPI_start_transaction();
     }
     PG_CATCH();
     {
diff --git a/src/pl/tcl/sql/pltcl_transaction.sql b/src/pl/tcl/sql/pltcl_transaction.sql
index c752faf665..bd759850a7 100644
--- a/src/pl/tcl/sql/pltcl_transaction.sql
+++ b/src/pl/tcl/sql/pltcl_transaction.sql
@@ -94,5 +94,42 @@ CALL transaction_test4b();
 SELECT * FROM test1;


+-- check handling of an error during COMMIT
+CREATE TABLE testpk (id int PRIMARY KEY);
+CREATE TABLE testfk(f1 int REFERENCES testpk DEFERRABLE INITIALLY DEFERRED);
+
+CREATE PROCEDURE transaction_testfk()
+LANGUAGE pltcl
+AS $$
+# this insert will fail during commit:
+spi_exec "INSERT INTO testfk VALUES (0)"
+commit
+elog WARNING "should not get here"
+$$;
+
+CALL transaction_testfk();
+
+SELECT * FROM testpk;
+SELECT * FROM testfk;
+
+CREATE OR REPLACE PROCEDURE transaction_testfk()
+LANGUAGE pltcl
+AS $$
+# this insert will fail during commit:
+spi_exec "INSERT INTO testfk VALUES (0)"
+if [catch {commit} msg] {
+    elog INFO $msg
+}
+# these inserts should work:
+spi_exec "INSERT INTO testpk VALUES (1)"
+spi_exec "INSERT INTO testfk VALUES (1)"
+$$;
+
+CALL transaction_testfk();
+
+SELECT * FROM testpk;
+SELECT * FROM testfk;
+
+
 DROP TABLE test1;
 DROP TABLE test2;
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index bb1f106946..adf763a8ea 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -2983,24 +2983,20 @@ StartTransactionCommand(void)
  * GUC system resets the characteristics at transaction end, so for example
  * just skipping the reset in StartTransaction() won't work.)
  */
-static int    save_XactIsoLevel;
-static bool save_XactReadOnly;
-static bool save_XactDeferrable;
-
 void
-SaveTransactionCharacteristics(void)
+SaveTransactionCharacteristics(SavedTransactionCharacteristics *s)
 {
-    save_XactIsoLevel = XactIsoLevel;
-    save_XactReadOnly = XactReadOnly;
-    save_XactDeferrable = XactDeferrable;
+    s->save_XactIsoLevel = XactIsoLevel;
+    s->save_XactReadOnly = XactReadOnly;
+    s->save_XactDeferrable = XactDeferrable;
 }

 void
-RestoreTransactionCharacteristics(void)
+RestoreTransactionCharacteristics(const SavedTransactionCharacteristics *s)
 {
-    XactIsoLevel = save_XactIsoLevel;
-    XactReadOnly = save_XactReadOnly;
-    XactDeferrable = save_XactDeferrable;
+    XactIsoLevel = s->save_XactIsoLevel;
+    XactReadOnly = s->save_XactReadOnly;
+    XactDeferrable = s->save_XactDeferrable;
 }


@@ -3011,9 +3007,9 @@ void
 CommitTransactionCommand(void)
 {
     TransactionState s = CurrentTransactionState;
+    SavedTransactionCharacteristics savetc;

-    if (s->chain)
-        SaveTransactionCharacteristics();
+    SaveTransactionCharacteristics(&savetc);

     switch (s->blockState)
     {
@@ -3071,7 +3067,7 @@ CommitTransactionCommand(void)
                 StartTransaction();
                 s->blockState = TBLOCK_INPROGRESS;
                 s->chain = false;
-                RestoreTransactionCharacteristics();
+                RestoreTransactionCharacteristics(&savetc);
             }
             break;

@@ -3097,7 +3093,7 @@ CommitTransactionCommand(void)
                 StartTransaction();
                 s->blockState = TBLOCK_INPROGRESS;
                 s->chain = false;
-                RestoreTransactionCharacteristics();
+                RestoreTransactionCharacteristics(&savetc);
             }
             break;

@@ -3115,7 +3111,7 @@ CommitTransactionCommand(void)
                 StartTransaction();
                 s->blockState = TBLOCK_INPROGRESS;
                 s->chain = false;
-                RestoreTransactionCharacteristics();
+                RestoreTransactionCharacteristics(&savetc);
             }
             break;

@@ -3182,7 +3178,7 @@ CommitTransactionCommand(void)
                     StartTransaction();
                     s->blockState = TBLOCK_INPROGRESS;
                     s->chain = false;
-                    RestoreTransactionCharacteristics();
+                    RestoreTransactionCharacteristics(&savetc);
                 }
             }
             else if (s->blockState == TBLOCK_PREPARE)
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 7971050746..5b353cb93a 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -228,6 +228,7 @@ static void
 _SPI_commit(bool chain)
 {
     MemoryContext oldcontext = CurrentMemoryContext;
+    SavedTransactionCharacteristics savetc;

     /*
      * Complain if we are in a context that doesn't permit transaction
@@ -255,9 +256,8 @@ _SPI_commit(bool chain)
                 (errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
                  errmsg("cannot commit while a subtransaction is active")));

-    /* XXX this ain't re-entrant enough for my taste */
     if (chain)
-        SaveTransactionCharacteristics();
+        SaveTransactionCharacteristics(&savetc);

     /* Catch any error occurring during the COMMIT */
     PG_TRY();
@@ -281,7 +281,7 @@ _SPI_commit(bool chain)
         /* Immediately start a new transaction */
         StartTransactionCommand();
         if (chain)
-            RestoreTransactionCharacteristics();
+            RestoreTransactionCharacteristics(&savetc);

         MemoryContextSwitchTo(oldcontext);

@@ -305,7 +305,7 @@ _SPI_commit(bool chain)
         /* ... and start a new one */
         StartTransactionCommand();
         if (chain)
-            RestoreTransactionCharacteristics();
+            RestoreTransactionCharacteristics(&savetc);

         MemoryContextSwitchTo(oldcontext);

@@ -333,6 +333,7 @@ static void
 _SPI_rollback(bool chain)
 {
     MemoryContext oldcontext = CurrentMemoryContext;
+    SavedTransactionCharacteristics savetc;

     /* see under SPI_commit() */
     if (_SPI_current->atomic)
@@ -346,9 +347,8 @@ _SPI_rollback(bool chain)
                 (errcode(ERRCODE_INVALID_TRANSACTION_TERMINATION),
                  errmsg("cannot roll back while a subtransaction is active")));

-    /* XXX this ain't re-entrant enough for my taste */
     if (chain)
-        SaveTransactionCharacteristics();
+        SaveTransactionCharacteristics(&savetc);

     /* Catch any error occurring during the ROLLBACK */
     PG_TRY();
@@ -373,7 +373,7 @@ _SPI_rollback(bool chain)
         /* Immediately start a new transaction */
         StartTransactionCommand();
         if (chain)
-            RestoreTransactionCharacteristics();
+            RestoreTransactionCharacteristics(&savetc);

         MemoryContextSwitchTo(oldcontext);

@@ -398,7 +398,7 @@ _SPI_rollback(bool chain)
         /* ... and start a new one */
         StartTransactionCommand();
         if (chain)
-            RestoreTransactionCharacteristics();
+            RestoreTransactionCharacteristics(&savetc);

         MemoryContextSwitchTo(oldcontext);

diff --git a/src/include/access/xact.h b/src/include/access/xact.h
index 17a6fa4abd..062cc7e17d 100644
--- a/src/include/access/xact.h
+++ b/src/include/access/xact.h
@@ -135,6 +135,14 @@ typedef enum
 typedef void (*SubXactCallback) (SubXactEvent event, SubTransactionId mySubid,
                                  SubTransactionId parentSubid, void *arg);

+/* Data structure for Save/RestoreTransactionCharacteristics */
+typedef struct SavedTransactionCharacteristics
+{
+    int            save_XactIsoLevel;
+    bool        save_XactReadOnly;
+    bool        save_XactDeferrable;
+} SavedTransactionCharacteristics;
+

 /* ----------------
  *        transaction-related XLOG entries
@@ -399,8 +407,8 @@ extern bool TransactionIdIsCurrentTransactionId(TransactionId xid);
 extern void CommandCounterIncrement(void);
 extern void ForceSyncCommit(void);
 extern void StartTransactionCommand(void);
-extern void SaveTransactionCharacteristics(void);
-extern void RestoreTransactionCharacteristics(void);
+extern void SaveTransactionCharacteristics(SavedTransactionCharacteristics *s);
+extern void RestoreTransactionCharacteristics(const SavedTransactionCharacteristics *s);
 extern void CommitTransactionCommand(void);
 extern void AbortCurrentTransaction(void);
 extern void BeginTransactionBlock(void);

Re: fix crash with Python 3.11

From

Tom Lane

Date:

21 June 2022, 19:33:31

Is it time yet to back-patch 2e517818f ("Fix SPI's handling of errors
during transaction commit")?  We know we're going to have to do it
before Python 3.11 ships, and it's been stable in HEAD for 3.5 months
now.  Also, the Fedora guys absorbed the patch a couple weeks ago [1]
because they're already using 3.11 in rawhide, and I've not heard
complaints from that direction.

My inclination at this point is to not back-patch the second change
12d768e70 ("Don't use static storage for SaveTransactionCharacteristics").
It's not clear that the benefit would be worth even a small risk of
somebody being unhappy about the API break.

            regards, tom lane

[1] https://www.postgresql.org/message-id/flat/CA%2BHKMWMY_e2otmTJDjKUAvC8Urh4rzSWOPZ%3DfszU5brkBP97ng%40mail.gmail.com

Re: fix crash with Python 3.11

From

Markus Wanner

Date:

23 June 2022, 10:41:00

On 6/21/22 18:33, Tom Lane wrote:
> My inclination at this point is to not back-patch the second change
> 12d768e70 ("Don't use static storage for SaveTransactionCharacteristics").
> It's not clear that the benefit would be worth even a small risk of
> somebody being unhappy about the API break.

Actually, the backport of 2e517818f ("Fix SPI's handling of errors") 
already broke the API for code using SPICleanup, as that function had 
been removed. Granted, it's not documented, but still exported.

I propose to re-introduce a no-op placeholder similar to what we have 
for SPI_start_transaction, somewhat like the attached patch.

Regards

Markus

Attachment

add-spicleanup-placeholder.diff

Re: fix crash with Python 3.11

From

Tom Lane

Date:

23 June 2022, 16:34:57

Markus Wanner <markus.wanner@enterprisedb.com> writes:
> Actually, the backport of 2e517818f ("Fix SPI's handling of errors") 
> already broke the API for code using SPICleanup, as that function had 
> been removed. Granted, it's not documented, but still exported.

Under what circumstances would it be OK for outside code to call
SPICleanup?

            regards, tom lane

Re: fix crash with Python 3.11

From

Markus Wanner

Date:

23 June 2022, 22:57:07

On 6/23/22 15:34, Tom Lane wrote:
> Under what circumstances would it be OK for outside code to call
> SPICleanup?

For the same reasons previous Postgres versions called SPICleanup: from 
a sigsetjmp handler that duplicates most of what Postgres does in such a 
situation.

However, I think that's the wrong question to ask for a stable branch. 
Postgres did export this function in previous versions. Removing it 
altogether constitutes an API change and makes extensions that link to 
it fail to even load, which is a bad way to fail after a patch version 
upgrade. Even if its original use was not sound in the first place.

Ofc my proposed patch is not meant for master, only for stable branches.

Best Regards

Markus

Re: fix crash with Python 3.11

From

Tom Lane

Date:

24 June 2022, 01:54:43

Markus Wanner <markus.wanner@enterprisedb.com> writes:
> On 6/23/22 15:34, Tom Lane wrote:
>> Under what circumstances would it be OK for outside code to call
>> SPICleanup?

> For the same reasons previous Postgres versions called SPICleanup: from 
> a sigsetjmp handler that duplicates most of what Postgres does in such a 
> situation.

Does such code exist?  I don't see any other calls in Debian code search,
and I find it hard to believe that anyone would think such a thing is
maintainable.

            regards, tom lane

Re: fix crash with Python 3.11

From

Markus Wanner

Date:

24 June 2022, 15:04:50

On 6/24/22 00:54, Tom Lane wrote:
> Does such code exist?  I don't see any other calls in Debian code search,
> and I find it hard to believe that anyone would think such a thing is
> maintainable.

Such a thing does exist within PGLogical and BDR, yes.

Thanks for your concern about maintainability. So far, that part was not 
posing any trouble. Looking at e.g. postgres.c, the sigsetjmp handler 
there didn't change all that much in recent years. Much of the code 
there is from around 2004 written by you.

However, that shouldn't be your concern at all. Postgres refusing to 
start after a minor upgrade probably should, especially when it's due to 
an API change in a stable branch.

Regards

Markus

Re: fix crash with Python 3.11

From

Peter Eisentraut

Date:

18 July 2022, 22:09:54

On 23.06.22 09:41, Markus Wanner wrote:
> 
> On 6/21/22 18:33, Tom Lane wrote:
>> My inclination at this point is to not back-patch the second change
>> 12d768e70 ("Don't use static storage for 
>> SaveTransactionCharacteristics").
>> It's not clear that the benefit would be worth even a small risk of
>> somebody being unhappy about the API break.
> 
> Actually, the backport of 2e517818f ("Fix SPI's handling of errors") 
> already broke the API for code using SPICleanup, as that function had 
> been removed. Granted, it's not documented, but still exported.
> 
> I propose to re-introduce a no-op placeholder similar to what we have 
> for SPI_start_transaction, somewhat like the attached patch.

I have applied your patch to branches 11 through 14.