Re: fix crash with Python 3.11 - Mailing list pgsql-hackers

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();
     {