Fix SPI's handling of errors during transaction commit.

SPI_commit previously left it up to the caller to recover from any error
occurring during commit.  Since that's complicated and requires use of
low-level xact.c facilities, it's not too surprising that no caller got
it right.  Let's move the responsibility for cleanup into spi.c.  Doing
that requires redefining SPI_commit as starting a new transaction, so
that it becomes equivalent to SPI_commit_and_chain except that you get
default transaction characteristics instead of preserving the prior
transaction's characteristics.  We can make this pretty transparent
API-wise by redefining SPI_start_transaction() as a no-op.  Callers
that expect to do something in between might be surprised, but
available evidence is that no callers do so.

Having made that API redefinition, we can fix this mess by having
SPI_commit[_and_chain] trap errors and start a new, clean transaction
before re-throwing the error.  Likewise for SPI_rollback[_and_chain].
Some cleanup is also needed in AtEOXact_SPI, which was nowhere near
smart enough to deal with SPI contexts nested inside a committing
context.

While plperl and pltcl need no changes beyond removing their now-useless
SPI_start_transaction() calls, plpython needs some more work because it
hadn't gotten the memo about catching commit/rollback errors in the
first place.  Such an error resulted in longjmp'ing out of the Python
interpreter, which leaks Python stack entries at present and is reported
to crash Python 3.11 altogether.  Add the missing logic to catch such
errors and convert them into Python exceptions.

We are probably going to have to back-patch this once Python 3.11 ships,
but it's a sufficiently basic change that I'm a bit nervous about doing
so immediately.  Let's let it bake awhile in HEAD first.

Peter Eisentraut and Tom Lane

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

12 files changed, 354 insertions, 46 deletions

diff --git a/src/pl/plperl/expected/plperl_transaction.out b/src/pl/plperl/expected/plperl_transaction.out
index 7ca0ef35fb8..da4283cbced 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 81bb480bc20..edb93ec1c4c 100644
--- a/src/pl/plperl/plperl.c
+++ b/src/pl/plperl/plperl.c
@@ -3986,7 +3986,6 @@ plperl_spi_commit(void)
 	PG_TRY();
 	{
 		SPI_commit();
-		SPI_start_transaction();
 	}
 	PG_CATCH();
 	{
@@ -4013,7 +4012,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 0a607998055..d10c8bee897 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 9674c292505..915139378e6 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 14152993c75..72d1e45a768 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 0365acc95b0..907f89d1535 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 99c1b4f28f6..86d70470a74 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 a5e2e60da7f..98ccd210931 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 33b37e5b7fc..68588d9fb02 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 007204b99ad..f557b791386 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 c5fad05e122..68c9bd1970e 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 c752faf6654..bd759850a70 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;