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>

1 files changed, 0 insertions, 13 deletions

diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index c96a86500ad..884cdab7024 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -53,7 +53,6 @@
 #include "access/transam.h"
 #include "catalog/namespace.h"
 #include "executor/executor.h"
-#include "executor/spi.h"
 #include "miscadmin.h"
 #include "nodes/nodeFuncs.h"
 #include "optimizer/cost.h"
@@ -878,7 +877,6 @@ BuildCachedPlan(CachedPlanSource *plansource, List *qlist,
 	CachedPlan *plan;
 	List	   *plist;
 	bool		snapshot_set;
-	bool		spi_pushed;
 	bool		is_transient;
 	MemoryContext plan_context;
 	MemoryContext oldcxt = CurrentMemoryContext;
@@ -927,21 +925,10 @@ BuildCachedPlan(CachedPlanSource *plansource, List *qlist,
 	}
 
 	/*
-	 * The planner may try to call SPI-using functions, which causes a problem
-	 * if we're already inside one.  Rather than expect all SPI-using code to
-	 * do SPI_push whenever a replan could happen, it seems best to take care
-	 * of the case here.
-	 */
-	spi_pushed = SPI_push_conditional();
-
-	/*
 	 * Generate the plan.
 	 */
 	plist = pg_plan_queries(qlist, plansource->cursor_options, boundParams);
 
-	/* Clean up SPI state */
-	SPI_pop_conditional(spi_pushed);
-
 	/* Release snapshot if we got one */
 	if (snapshot_set)
 		PopActiveSnapshot();