database/postgres
1
0
Fork 0
mirror of https://github.com/postgres/postgres.git synced 2025-06-27 23:21:58 +03:00
Code Activity

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

Browse Source 
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>
This commit is contained in:
Tom Lane
2016-11-08 17:39:45 -05:00
parent 577f0bdd2b
commit 1833f1a1c3
12 changed files with 105 additions and 493 deletions
Download Patch File Download Diff File Expand all files Collapse all files

200
src/backend/executor/spi.c
View File

@ -44,8 +44,7 @@ int SPI_result;
static _SPI_connection *_SPI_stack = NULL;
static _SPI_connection *_SPI_current = NULL;
static int _SPI_stack_depth = 0; /* allocated size of _SPI_stack */
static int _SPI_connected = -1;
static int _SPI_curid = -1;
static int _SPI_connected = -1; /* current stack index */
static Portal SPI_cursor_open_internal(const char *name, SPIPlanPtr plan,
ParamListInfo paramLI, bool read_only);
@ -86,13 +85,7 @@ SPI_connect(void)
{
int newdepth;
/*
* When procedure called by Executor _SPI_curid expected to be equal to
* _SPI_connected
*/
if (_SPI_curid != _SPI_connected)
return SPI_ERROR_CONNECT;
/* Enlarge stack if necessary */
if (_SPI_stack == NULL)
{
if (_SPI_connected != -1 || _SPI_stack_depth != 0)
@ -117,9 +110,7 @@ SPI_connect(void)
}
}
/*
* We're entering procedure where _SPI_curid == _SPI_connected - 1
*/
/* Enter new stack level */
_SPI_connected++;
Assert(_SPI_connected >= 0 && _SPI_connected < _SPI_stack_depth);
@ -178,14 +169,9 @@ SPI_finish(void)
SPI_lastoid = InvalidOid;
SPI_tuptable = NULL;
/*
* After _SPI_begin_call _SPI_connected == _SPI_curid. Now we are closing
* connection to SPI and returning to upper Executor and so _SPI_connected
* must be equal to _SPI_curid.
*/
/* Exit stack level */
_SPI_connected--;
_SPI_curid--;
if (_SPI_connected == -1)
if (_SPI_connected < 0)
_SPI_current = NULL;
else
_SPI_current = &(_SPI_stack[_SPI_connected]);
@ -212,7 +198,7 @@ AtEOXact_SPI(bool isCommit)
_SPI_current = _SPI_stack = NULL;
_SPI_stack_depth = 0;
_SPI_connected = _SPI_curid = -1;
_SPI_connected = -1;
SPI_processed = 0;
SPI_lastoid = InvalidOid;
SPI_tuptable = NULL;
@ -258,8 +244,7 @@ AtEOSubXact_SPI(bool isCommit, SubTransactionId mySubid)
* be already gone.
*/
_SPI_connected--;
_SPI_curid = _SPI_connected;
if (_SPI_connected == -1)
if (_SPI_connected < 0)
_SPI_current = NULL;
else
_SPI_current = &(_SPI_stack[_SPI_connected]);
@ -313,53 +298,6 @@ AtEOSubXact_SPI(bool isCommit, SubTransactionId mySubid)
}
/* Pushes SPI stack to allow recursive SPI calls */
void
SPI_push(void)
{
_SPI_curid++;
}
/* Pops SPI stack to allow recursive SPI calls */
void
SPI_pop(void)
{
_SPI_curid--;
}
/* Conditional push: push only if we're inside a SPI procedure */
bool
SPI_push_conditional(void)
{
bool pushed = (_SPI_curid != _SPI_connected);
if (pushed)
{
_SPI_curid++;
/* We should now be in a state where SPI_connect would succeed */
Assert(_SPI_curid == _SPI_connected);
}
return pushed;
}
/* Conditional pop: pop only if SPI_push_conditional pushed */
void
SPI_pop_conditional(bool pushed)
{
/* We should be in a state where SPI_connect would succeed */
Assert(_SPI_curid == _SPI_connected);
if (pushed)
_SPI_curid--;
}
/* Restore state of SPI stack after aborting a subtransaction */
void
SPI_restore_connection(void)
{
Assert(_SPI_connected >= 0);
_SPI_curid = _SPI_connected - 1;
}
/* Parse, plan, and execute a query string */
int
SPI_execute(const char *src, bool read_only, long tcount)
@ -691,7 +629,7 @@ SPI_freeplan(SPIPlanPtr plan)
HeapTuple
SPI_copytuple(HeapTuple tuple)
{
MemoryContext oldcxt = NULL;
MemoryContext oldcxt;
HeapTuple ctuple;
if (tuple == NULL)
@ -700,17 +638,17 @@ SPI_copytuple(HeapTuple tuple)
return NULL;
}
if (_SPI_curid + 1 == _SPI_connected) /* connected */
if (_SPI_current == NULL)
{
if (_SPI_current != &(_SPI_stack[_SPI_curid + 1]))
elog(ERROR, "SPI stack corrupted");
oldcxt = MemoryContextSwitchTo(_SPI_current->savedcxt);
SPI_result = SPI_ERROR_UNCONNECTED;
return NULL;
}
oldcxt = MemoryContextSwitchTo(_SPI_current->savedcxt);
ctuple = heap_copytuple(tuple);
if (oldcxt)
MemoryContextSwitchTo(oldcxt);
MemoryContextSwitchTo(oldcxt);
return ctuple;
}
@ -718,7 +656,7 @@ SPI_copytuple(HeapTuple tuple)
HeapTupleHeader
SPI_returntuple(HeapTuple tuple, TupleDesc tupdesc)
{
MemoryContext oldcxt = NULL;
MemoryContext oldcxt;
HeapTupleHeader dtup;
if (tuple == NULL || tupdesc == NULL)
@ -727,22 +665,22 @@ SPI_returntuple(HeapTuple tuple, TupleDesc tupdesc)
return NULL;
}
if (_SPI_current == NULL)
{
SPI_result = SPI_ERROR_UNCONNECTED;
return NULL;
}
/* For RECORD results, make sure a typmod has been assigned */
if (tupdesc->tdtypeid == RECORDOID &&
tupdesc->tdtypmod < 0)
assign_record_type_typmod(tupdesc);
if (_SPI_curid + 1 == _SPI_connected) /* connected */
{
if (_SPI_current != &(_SPI_stack[_SPI_curid + 1]))
elog(ERROR, "SPI stack corrupted");
oldcxt = MemoryContextSwitchTo(_SPI_current->savedcxt);
}
oldcxt = MemoryContextSwitchTo(_SPI_current->savedcxt);
dtup = DatumGetHeapTupleHeader(heap_copy_tuple_as_datum(tuple, tupdesc));
if (oldcxt)
MemoryContextSwitchTo(oldcxt);
MemoryContextSwitchTo(oldcxt);
return dtup;
}
@ -751,7 +689,7 @@ HeapTuple
SPI_modifytuple(Relation rel, HeapTuple tuple, int natts, int *attnum,
Datum *Values, const char *Nulls)
{
MemoryContext oldcxt = NULL;
MemoryContext oldcxt;
HeapTuple mtuple;
int numberOfAttributes;
Datum *v;
@ -764,13 +702,16 @@ SPI_modifytuple(Relation rel, HeapTuple tuple, int natts, int *attnum,
return NULL;
}
if (_SPI_curid + 1 == _SPI_connected) /* connected */
if (_SPI_current == NULL)
{
if (_SPI_current != &(_SPI_stack[_SPI_curid + 1]))
elog(ERROR, "SPI stack corrupted");
oldcxt = MemoryContextSwitchTo(_SPI_current->savedcxt);
SPI_result = SPI_ERROR_UNCONNECTED;
return NULL;
}
oldcxt = MemoryContextSwitchTo(_SPI_current->savedcxt);
SPI_result = 0;
numberOfAttributes = rel->rd_att->natts;
v = (Datum *) palloc(numberOfAttributes * sizeof(Datum));
n = (bool *) palloc(numberOfAttributes * sizeof(bool));
@ -810,8 +751,7 @@ SPI_modifytuple(Relation rel, HeapTuple tuple, int natts, int *attnum,
pfree(v);
pfree(n);
if (oldcxt)
MemoryContextSwitchTo(oldcxt);
MemoryContextSwitchTo(oldcxt);
return mtuple;
}
@ -980,22 +920,10 @@ SPI_getnspname(Relation rel)
void *
SPI_palloc(Size size)
{
MemoryContext oldcxt = NULL;
void *pointer;
if (_SPI_current == NULL)
elog(ERROR, "SPI_palloc called while not connected to SPI");
if (_SPI_curid + 1 == _SPI_connected) /* connected */
{
if (_SPI_current != &(_SPI_stack[_SPI_curid + 1]))
elog(ERROR, "SPI stack corrupted");
oldcxt = MemoryContextSwitchTo(_SPI_current->savedcxt);
}
pointer = palloc(size);
if (oldcxt)
MemoryContextSwitchTo(oldcxt);
return pointer;
return MemoryContextAlloc(_SPI_current->savedcxt, size);
}
void *
@ -1015,20 +943,17 @@ SPI_pfree(void *pointer)
Datum
SPI_datumTransfer(Datum value, bool typByVal, int typLen)
{
MemoryContext oldcxt = NULL;
MemoryContext oldcxt;
Datum result;
if (_SPI_curid + 1 == _SPI_connected) /* connected */
{
if (_SPI_current != &(_SPI_stack[_SPI_curid + 1]))
elog(ERROR, "SPI stack corrupted");
oldcxt = MemoryContextSwitchTo(_SPI_current->savedcxt);
}
if (_SPI_current == NULL)
elog(ERROR, "SPI_datumTransfer called while not connected to SPI");
oldcxt = MemoryContextSwitchTo(_SPI_current->savedcxt);
result = datumTransfer(value, typByVal, typLen);
if (oldcxt)
MemoryContextSwitchTo(oldcxt);
MemoryContextSwitchTo(oldcxt);
return result;
}
@ -1050,17 +975,12 @@ SPI_freetuptable(SPITupleTable *tuptable)
return;
/*
* Since this function might be called during error recovery, it seems
* best not to insist that the caller be actively connected. We just
* search the topmost SPI context, connected or not.
* Search only the topmost SPI context for a matching tuple table.
*/
if (_SPI_connected >= 0)
if (_SPI_current != NULL)
{
slist_mutable_iter siter;
if (_SPI_current != &(_SPI_stack[_SPI_connected]))
elog(ERROR, "SPI stack corrupted");
/* find tuptable in active list, then remove it */
slist_foreach_modify(siter, &_SPI_current->tuptables)
{
@ -1168,13 +1088,9 @@ SPI_cursor_open_with_args(const char *name,
/* We needn't copy the plan; SPI_cursor_open_internal will do so */
/* Adjust stack so that SPI_cursor_open_internal doesn't complain */
_SPI_curid--;
result = SPI_cursor_open_internal(name, &plan, paramLI, read_only);
/* And clean up */
_SPI_curid++;
_SPI_end_call(true);
return result;
@ -1723,14 +1639,8 @@ spi_dest_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
MemoryContext oldcxt;
MemoryContext tuptabcxt;
/*
* When called by Executor _SPI_curid expected to be equal to
* _SPI_connected
*/
if (_SPI_curid != _SPI_connected || _SPI_connected < 0)
elog(ERROR, "improper call to spi_dest_startup");
if (_SPI_current != &(_SPI_stack[_SPI_curid]))
elog(ERROR, "SPI stack corrupted");
if (_SPI_current == NULL)
elog(ERROR, "spi_dest_startup called while not connected to SPI");
if (_SPI_current->tuptable != NULL)
elog(ERROR, "improper call to spi_dest_startup");
@ -1775,14 +1685,8 @@ spi_printtup(TupleTableSlot *slot, DestReceiver *self)
SPITupleTable *tuptable;
MemoryContext oldcxt;
/*
* When called by Executor _SPI_curid expected to be equal to
* _SPI_connected
*/
if (_SPI_curid != _SPI_connected || _SPI_connected < 0)
elog(ERROR, "improper call to spi_printtup");
if (_SPI_current != &(_SPI_stack[_SPI_curid]))
elog(ERROR, "SPI stack corrupted");
if (_SPI_current == NULL)
elog(ERROR, "spi_printtup called while not connected to SPI");
tuptable = _SPI_current->tuptable;
if (tuptable == NULL)
@ -2534,11 +2438,8 @@ _SPI_procmem(void)
static int
_SPI_begin_call(bool execmem)
{
if (_SPI_curid + 1 != _SPI_connected)
if (_SPI_current == NULL)
return SPI_ERROR_UNCONNECTED;
_SPI_curid++;
if (_SPI_current != &(_SPI_stack[_SPI_curid]))
elog(ERROR, "SPI stack corrupted");
if (execmem) /* switch to the Executor memory context */
_SPI_execmem();
@ -2554,11 +2455,6 @@ _SPI_begin_call(bool execmem)
static int
_SPI_end_call(bool procmem)
{
/*
* We're returning to procedure where _SPI_curid == _SPI_connected - 1
*/
_SPI_curid--;
if (procmem) /* switch to the procedure memory context */
{
_SPI_procmem();