Simplify code by getting rid of SPI_push, SPI_pop, SPI_restore_connection.

The idea behind SPI_push was to allow transitioning back into an
"unconnected" state when a SPI-using procedure calls unrelated code that
might or might not invoke SPI.  That sounds good, but in practice the only
thing it does for us is to catch cases where a called SPI-using function
forgets to call SPI_connect --- which is a highly improbable failure mode,
since it would be exposed immediately by direct testing of said function.
As against that, we've had multiple bugs induced by forgetting to call
SPI_push/SPI_pop around code that might invoke SPI-using functions; these
are much harder to catch and indeed have gone undetected for years in some
cases.  And we've had to band-aid around some problems of this ilk by
introducing conditional push/pop pairs in some places, which really kind
of defeats the purpose altogether; if we can't draw bright lines between
connected and unconnected code, what's the point?

Hence, get rid of SPI_push[_conditional], SPI_pop[_conditional], and the
underlying state variable _SPI_curid.  It turns out SPI_restore_connection
can go away too, which is a nice side benefit since it was never more than
a kluge.  Provide no-op macros for the deleted functions so as to avoid an
API break for external modules.

A side effect of this removal is that SPI_palloc and allied functions no
longer permit being called when unconnected; they'll throw an error
instead.  The apparent usefulness of the previous behavior was a mirage
as well, because it was depended on by only a few places (which I fixed in
preceding commits), and it posed a risk of allocations being unexpectedly
long-lived if someone forgot a SPI_push call.

Discussion: <20808.1478481403@sss.pgh.pa.us>

6 files changed, 0 insertions, 139 deletions

diff --git a/src/pl/plperl/plperl.c b/src/pl/plperl/plperl.c
index 461986cda3..9a2d0527f8 100644
--- a/src/pl/plperl/plperl.c
+++ b/src/pl/plperl/plperl.c
@@ -3057,12 +3057,6 @@ plperl_spi_exec(char *query, int limit)
 		ReleaseCurrentSubTransaction();
 		MemoryContextSwitchTo(oldcontext);
 		CurrentResourceOwner = oldowner;
-
-		/*
-		 * AtEOSubXact_SPI() should not have popped any SPI context, but just
-		 * in case it did, make sure we remain connected.
-		 */
-		SPI_restore_connection();
 	}
 	PG_CATCH();
 	{
@@ -3078,13 +3072,6 @@ plperl_spi_exec(char *query, int limit)
 		MemoryContextSwitchTo(oldcontext);
 		CurrentResourceOwner = oldowner;
 
-		/*
-		 * If AtEOSubXact_SPI() popped any SPI context of the subxact, it will
-		 * have left us in a disconnected state.  We need this hack to return
-		 * to connected state.
-		 */
-		SPI_restore_connection();
-
 		/* Punt the error to Perl */
 		croak_cstr(edata->message);
 
@@ -3296,12 +3283,6 @@ plperl_spi_query(char *query)
 		ReleaseCurrentSubTransaction();
 		MemoryContextSwitchTo(oldcontext);
 		CurrentResourceOwner = oldowner;
-
-		/*
-		 * AtEOSubXact_SPI() should not have popped any SPI context, but just
-		 * in case it did, make sure we remain connected.
-		 */
-		SPI_restore_connection();
 	}
 	PG_CATCH();
 	{
@@ -3317,13 +3298,6 @@ plperl_spi_query(char *query)
 		MemoryContextSwitchTo(oldcontext);
 		CurrentResourceOwner = oldowner;
 
-		/*
-		 * If AtEOSubXact_SPI() popped any SPI context of the subxact, it will
-		 * have left us in a disconnected state.  We need this hack to return
-		 * to connected state.
-		 */
-		SPI_restore_connection();
-
 		/* Punt the error to Perl */
 		croak_cstr(edata->message);
 
@@ -3382,12 +3356,6 @@ plperl_spi_fetchrow(char *cursor)
 		ReleaseCurrentSubTransaction();
 		MemoryContextSwitchTo(oldcontext);
 		CurrentResourceOwner = oldowner;
-
-		/*
-		 * AtEOSubXact_SPI() should not have popped any SPI context, but just
-		 * in case it did, make sure we remain connected.
-		 */
-		SPI_restore_connection();
 	}
 	PG_CATCH();
 	{
@@ -3403,13 +3371,6 @@ plperl_spi_fetchrow(char *cursor)
 		MemoryContextSwitchTo(oldcontext);
 		CurrentResourceOwner = oldowner;
 
-		/*
-		 * If AtEOSubXact_SPI() popped any SPI context of the subxact, it will
-		 * have left us in a disconnected state.  We need this hack to return
-		 * to connected state.
-		 */
-		SPI_restore_connection();
-
 		/* Punt the error to Perl */
 		croak_cstr(edata->message);
 
@@ -3543,12 +3504,6 @@ plperl_spi_prepare(char *query, int argc, SV **argv)
 		ReleaseCurrentSubTransaction();
 		MemoryContextSwitchTo(oldcontext);
 		CurrentResourceOwner = oldowner;
-
-		/*
-		 * AtEOSubXact_SPI() should not have popped any SPI context, but just
-		 * in case it did, make sure we remain connected.
-		 */
-		SPI_restore_connection();
 	}
 	PG_CATCH();
 	{
@@ -3574,13 +3529,6 @@ plperl_spi_prepare(char *query, int argc, SV **argv)
 		MemoryContextSwitchTo(oldcontext);
 		CurrentResourceOwner = oldowner;
 
-		/*
-		 * If AtEOSubXact_SPI() popped any SPI context of the subxact, it will
-		 * have left us in a disconnected state.  We need this hack to return
-		 * to connected state.
-		 */
-		SPI_restore_connection();
-
 		/* Punt the error to Perl */
 		croak_cstr(edata->message);
 
@@ -3694,12 +3642,6 @@ plperl_spi_exec_prepared(char *query, HV *attr, int argc, SV **argv)
 		ReleaseCurrentSubTransaction();
 		MemoryContextSwitchTo(oldcontext);
 		CurrentResourceOwner = oldowner;
-
-		/*
-		 * AtEOSubXact_SPI() should not have popped any SPI context, but just
-		 * in case it did, make sure we remain connected.
-		 */
-		SPI_restore_connection();
 	}
 	PG_CATCH();
 	{
@@ -3715,13 +3657,6 @@ plperl_spi_exec_prepared(char *query, HV *attr, int argc, SV **argv)
 		MemoryContextSwitchTo(oldcontext);
 		CurrentResourceOwner = oldowner;
 
-		/*
-		 * If AtEOSubXact_SPI() popped any SPI context of the subxact, it will
-		 * have left us in a disconnected state.  We need this hack to return
-		 * to connected state.
-		 */
-		SPI_restore_connection();
-
 		/* Punt the error to Perl */
 		croak_cstr(edata->message);
 
@@ -3823,12 +3758,6 @@ plperl_spi_query_prepared(char *query, int argc, SV **argv)
 		ReleaseCurrentSubTransaction();
 		MemoryContextSwitchTo(oldcontext);
 		CurrentResourceOwner = oldowner;
-
-		/*
-		 * AtEOSubXact_SPI() should not have popped any SPI context, but just
-		 * in case it did, make sure we remain connected.
-		 */
-		SPI_restore_connection();
 	}
 	PG_CATCH();
 	{
@@ -3844,13 +3773,6 @@ plperl_spi_query_prepared(char *query, int argc, SV **argv)
 		MemoryContextSwitchTo(oldcontext);
 		CurrentResourceOwner = oldowner;
 
-		/*
-		 * If AtEOSubXact_SPI() popped any SPI context of the subxact, it will
-		 * have left us in a disconnected state.  We need this hack to return
-		 * to connected state.
-		 */
-		SPI_restore_connection();
-
 		/* Punt the error to Perl */
 		croak_cstr(edata->message);
 
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 91e1f8dd3f..77e7440002 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -1337,12 +1337,6 @@ exec_stmt_block(PLpgSQL_execstate *estate, PLpgSQL_stmt_block *block)
 			 * automatically cleaned up during subxact exit.)
 			 */
 			estate->eval_econtext = old_eval_econtext;
-
-			/*
-			 * AtEOSubXact_SPI() should not have popped any SPI context, but
-			 * just in case it did, make sure we remain connected.
-			 */
-			SPI_restore_connection();
 		}
 		PG_CATCH();
 		{
@@ -1385,13 +1379,6 @@ exec_stmt_block(PLpgSQL_execstate *estate, PLpgSQL_stmt_block *block)
 			estate->eval_econtext = old_eval_econtext;
 
 			/*
-			 * If AtEOSubXact_SPI() popped any SPI context of the subxact, it
-			 * will have left us in a disconnected state.  We need this hack
-			 * to return to connected state.
-			 */
-			SPI_restore_connection();
-
-			/*
 			 * Must clean up the econtext too.  However, any tuple table made
 			 * in the subxact will have been thrown away by SPI during subxact
 			 * abort, so we don't need to (and mustn't try to) free the
@@ -5587,8 +5574,6 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
 	 * Without this, stable functions within the expression would fail to see
 	 * updates made so far by our own function.
 	 */
-	SPI_push();
-
 	oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
 	if (!estate->readonly_func)
 	{
@@ -5636,8 +5621,6 @@ exec_eval_simple_expr(PLpgSQL_execstate *estate,
 
 	MemoryContextSwitchTo(oldcontext);
 
-	SPI_pop();
-
 	/*
 	 * Now we can release our refcount on the cached plan.
 	 */
@@ -6281,8 +6264,6 @@ exec_cast_value(PLpgSQL_execstate *estate,
 			ExprContext *econtext = estate->eval_econtext;
 			MemoryContext oldcontext;
 
-			SPI_push();
-
 			oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
 
 			econtext->caseValue_datum = value;
@@ -6296,8 +6277,6 @@ exec_cast_value(PLpgSQL_execstate *estate,
 			cast_entry->cast_in_use = false;
 
 			MemoryContextSwitchTo(oldcontext);
-
-			SPI_pop();
 		}
 	}
 
diff --git a/src/pl/plpython/plpy_exec.c b/src/pl/plpython/plpy_exec.c
index fa5b25a5fa..697a0e1cc0 100644
--- a/src/pl/plpython/plpy_exec.c
+++ b/src/pl/plpython/plpy_exec.c
@@ -1103,8 +1103,6 @@ PLy_abort_open_subtransactions(int save_subxact_level)
 
 		RollbackAndReleaseCurrentSubTransaction();
 
-		SPI_restore_connection();
-
 		subtransactiondata = (PLySubtransactionData *) linitial(explicit_subtransactions);
 		explicit_subtransactions = list_delete_first(explicit_subtransactions);
 
diff --git a/src/pl/plpython/plpy_spi.c b/src/pl/plpython/plpy_spi.c
index b082d017ea..07ab6a087e 100644
--- a/src/pl/plpython/plpy_spi.c
+++ b/src/pl/plpython/plpy_spi.c
@@ -516,12 +516,6 @@ PLy_spi_subtransaction_commit(MemoryContext oldcontext, ResourceOwner oldowner)
 	ReleaseCurrentSubTransaction();
 	MemoryContextSwitchTo(oldcontext);
 	CurrentResourceOwner = oldowner;
-
-	/*
-	 * AtEOSubXact_SPI() should not have popped any SPI context, but just in
-	 * case it did, make sure we remain connected.
-	 */
-	SPI_restore_connection();
 }
 
 void
@@ -541,13 +535,6 @@ PLy_spi_subtransaction_abort(MemoryContext oldcontext, ResourceOwner oldowner)
 	MemoryContextSwitchTo(oldcontext);
 	CurrentResourceOwner = oldowner;
 
-	/*
-	 * If AtEOSubXact_SPI() popped any SPI context of the subxact, it will
-	 * have left us in a disconnected state.  We need this hack to return to
-	 * connected state.
-	 */
-	SPI_restore_connection();
-
 	/* Look up the correct exception */
 	entry = hash_search(PLy_spi_exceptions, &(edata->sqlerrcode),
 						HASH_FIND, NULL);
diff --git a/src/pl/plpython/plpy_subxactobject.c b/src/pl/plpython/plpy_subxactobject.c
index 81fb3a3a4a..9f1caa87d9 100644
--- a/src/pl/plpython/plpy_subxactobject.c
+++ b/src/pl/plpython/plpy_subxactobject.c
@@ -7,7 +7,6 @@
 #include "postgres.h"
 
 #include "access/xact.h"
-#include "executor/spi.h"
 #include "utils/memutils.h"
 
 #include "plpython.h"
@@ -213,12 +212,6 @@ PLy_subtransaction_exit(PyObject *self, PyObject *args)
 	CurrentResourceOwner = subxactdata->oldowner;
 	pfree(subxactdata);
 
-	/*
-	 * AtEOSubXact_SPI() should not have popped any SPI context, but just in
-	 * case it did, make sure we remain connected.
-	 */
-	SPI_restore_connection();
-
 	Py_INCREF(Py_None);
 	return Py_None;
 }
diff --git a/src/pl/tcl/pltcl.c b/src/pl/tcl/pltcl.c
index 20809102ef..b0d9e419bb 100644
--- a/src/pl/tcl/pltcl.c
+++ b/src/pl/tcl/pltcl.c
@@ -2182,11 +2182,9 @@ pltcl_returnnext(ClientData cdata, Tcl_Interp *interp,
 		{
 			HeapTuple	tuple;
 
-			SPI_push();
 			tuple = pltcl_build_tuple_result(interp, rowObjv, rowObjc,
 											 call_state);
 			tuplestore_puttuple(call_state->tuple_store, tuple);
-			SPI_pop();
 		}
 	}
 	else
@@ -2249,12 +2247,6 @@ pltcl_subtrans_commit(MemoryContext oldcontext, ResourceOwner oldowner)
 	ReleaseCurrentSubTransaction();
 	MemoryContextSwitchTo(oldcontext);
 	CurrentResourceOwner = oldowner;
-
-	/*
-	 * AtEOSubXact_SPI() should not have popped any SPI context, but just in
-	 * case it did, make sure we remain connected.
-	 */
-	SPI_restore_connection();
 }
 
 static void
@@ -2273,13 +2265,6 @@ pltcl_subtrans_abort(Tcl_Interp *interp,
 	MemoryContextSwitchTo(oldcontext);
 	CurrentResourceOwner = oldowner;
 
-	/*
-	 * If AtEOSubXact_SPI() popped any SPI context of the subxact, it will
-	 * have left us in a disconnected state.  We need this hack to return to
-	 * connected state.
-	 */
-	SPI_restore_connection();
-
 	/* Pass the error data to Tcl */
 	pltcl_construct_errorCode(interp, edata);
 	UTF_BEGIN;
@@ -3029,9 +3014,6 @@ pltcl_build_tuple_argument(HeapTuple tuple, TupleDesc tupdesc)
  * mess, there's no way to prevent the datatype input functions it calls
  * from leaking.  Run it in a short-lived context, unless we're about to
  * exit the procedure anyway.
- *
- * Also, caller is responsible for doing SPI_push/SPI_pop if calling from
- * inside SPI environment.
  **********************************************************************/
 static HeapTuple
 pltcl_build_tuple_result(Tcl_Interp *interp, Tcl_Obj **kvObjv, int kvObjc,