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

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;