diff --git a/doc/src/sgml/spi.sgml b/doc/src/sgml/spi.sgml index 4de6a2512d..079a4571fb 100644 --- a/doc/src/sgml/spi.sgml +++ b/doc/src/sgml/spi.sgml @@ -3934,8 +3934,8 @@ void SPI_freetuptable(SPITupleTable * tuptable) SPI_freetuptable frees a row set created by a prior SPI command execution function, such as - SPI_execute. Therefore, this function is usually called - with the global variable SPI_tupletable as + SPI_execute. Therefore, this function is often called + with the global variable SPI_tuptable as argument. @@ -3944,6 +3944,16 @@ void SPI_freetuptable(SPITupleTable * tuptable) multiple commands and does not want to keep the results of earlier commands around until it ends. Note that any unfreed row sets will be freed anyway at SPI_finish. + Also, if a subtransaction is started and then aborted within execution + of a SPI procedure, SPI automatically frees any row sets created while + the subtransaction was running. + + + + Beginning in PostgreSQL 9.3, + SPI_freetuptable contains guard logic to protect + against duplicate deletion requests for the same row set. In previous + releases, duplicate deletions would lead to crashes. @@ -3955,7 +3965,7 @@ void SPI_freetuptable(SPITupleTable * tuptable) SPITupleTable * tuptable - pointer to row set to free + pointer to row set to free, or NULL to do nothing diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c index f30b2cd933..d91a663e31 100644 --- a/src/backend/executor/spi.c +++ b/src/backend/executor/spi.c @@ -126,6 +126,7 @@ SPI_connect(void) _SPI_current->processed = 0; _SPI_current->lastoid = InvalidOid; _SPI_current->tuptable = NULL; + slist_init(&_SPI_current->tuptables); _SPI_current->procCxt = NULL; /* in case we fail to create 'em */ _SPI_current->execCxt = NULL; _SPI_current->connectSubid = GetCurrentSubTransactionId(); @@ -166,7 +167,7 @@ SPI_finish(void) /* Restore memory context as it was before procedure call */ MemoryContextSwitchTo(_SPI_current->savedcxt); - /* Release memory used in procedure call */ + /* Release memory used in procedure call (including tuptables) */ MemoryContextDelete(_SPI_current->execCxt); _SPI_current->execCxt = NULL; MemoryContextDelete(_SPI_current->procCxt); @@ -282,11 +283,35 @@ AtEOSubXact_SPI(bool isCommit, SubTransactionId mySubid) */ if (_SPI_current && !isCommit) { + slist_mutable_iter siter; + /* free Executor memory the same as _SPI_end_call would do */ MemoryContextResetAndDeleteChildren(_SPI_current->execCxt); - /* throw away any partially created tuple-table */ - SPI_freetuptable(_SPI_current->tuptable); - _SPI_current->tuptable = NULL; + + /* throw away any tuple tables created within current subxact */ + slist_foreach_modify(siter, &_SPI_current->tuptables) + { + SPITupleTable *tuptable; + + tuptable = slist_container(SPITupleTable, next, siter.cur); + if (tuptable->subid >= mySubid) + { + /* + * If we used SPI_freetuptable() here, its internal search of + * the tuptables list would make this operation O(N^2). + * Instead, just free the tuptable manually. This should + * match what SPI_freetuptable() does. + */ + slist_delete_current(&siter); + if (tuptable == _SPI_current->tuptable) + _SPI_current->tuptable = NULL; + if (tuptable == SPI_tuptable) + SPI_tuptable = NULL; + MemoryContextDelete(tuptable->tuptabcxt); + } + } + /* in particular we should have gotten rid of any in-progress table */ + Assert(_SPI_current->tuptable == NULL); } } @@ -1021,8 +1046,59 @@ SPI_freetuple(HeapTuple tuple) void SPI_freetuptable(SPITupleTable *tuptable) { - if (tuptable != NULL) - MemoryContextDelete(tuptable->tuptabcxt); + bool found = false; + + /* ignore call if NULL pointer */ + if (tuptable == NULL) + 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. + */ + if (_SPI_connected >= 0) + { + 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) + { + SPITupleTable *tt; + + tt = slist_container(SPITupleTable, next, siter.cur); + if (tt == tuptable) + { + slist_delete_current(&siter); + found = true; + break; + } + } + } + + /* + * Refuse the deletion if we didn't find it in the topmost SPI context. + * This is primarily a guard against double deletion, but might prevent + * other errors as well. Since the worst consequence of not deleting a + * tuptable would be a transient memory leak, this is just a WARNING. + */ + if (!found) + { + elog(WARNING, "attempt to delete invalid SPITupleTable %p", tuptable); + return; + } + + /* for safety, reset global variables that might point at tuptable */ + if (tuptable == _SPI_current->tuptable) + _SPI_current->tuptable = NULL; + if (tuptable == SPI_tuptable) + SPI_tuptable = NULL; + + /* release all memory belonging to tuptable */ + MemoryContextDelete(tuptable->tuptabcxt); } @@ -1656,6 +1732,8 @@ spi_dest_startup(DestReceiver *self, int operation, TupleDesc typeinfo) if (_SPI_current->tuptable != NULL) elog(ERROR, "improper call to spi_dest_startup"); + /* We create the tuple table context as a child of procCxt */ + oldcxt = _SPI_procmem(); /* switch to procedure memory context */ tuptabcxt = AllocSetContextCreate(CurrentMemoryContext, @@ -1666,8 +1744,18 @@ spi_dest_startup(DestReceiver *self, int operation, TupleDesc typeinfo) MemoryContextSwitchTo(tuptabcxt); _SPI_current->tuptable = tuptable = (SPITupleTable *) - palloc(sizeof(SPITupleTable)); + palloc0(sizeof(SPITupleTable)); tuptable->tuptabcxt = tuptabcxt; + tuptable->subid = GetCurrentSubTransactionId(); + + /* + * The tuptable is now valid enough to be freed by AtEOSubXact_SPI, so put + * it onto the SPI context's tuptables list. This will ensure it's not + * leaked even in the unlikely event the following few lines fail. + */ + slist_push_head(&_SPI_current->tuptables, &tuptable->next); + + /* set up initial allocations */ tuptable->alloced = tuptable->free = 128; tuptable->vals = (HeapTuple *) palloc(tuptable->alloced * sizeof(HeapTuple)); tuptable->tupdesc = CreateTupleDescCopy(typeinfo); diff --git a/src/include/executor/spi.h b/src/include/executor/spi.h index d4f1272cd8..81310e377f 100644 --- a/src/include/executor/spi.h +++ b/src/include/executor/spi.h @@ -13,6 +13,7 @@ #ifndef SPI_H #define SPI_H +#include "lib/ilist.h" #include "nodes/parsenodes.h" #include "utils/portal.h" @@ -24,6 +25,8 @@ typedef struct SPITupleTable uint32 free; /* # of free vals */ TupleDesc tupdesc; /* tuple descriptor */ HeapTuple *vals; /* tuples */ + slist_node next; /* link for internal bookkeeping */ + SubTransactionId subid; /* subxact in which tuptable was created */ } SPITupleTable; /* Plans are opaque structs for standard users of SPI */ diff --git a/src/include/executor/spi_priv.h b/src/include/executor/spi_priv.h index ef7903abd0..7d4c9e9639 100644 --- a/src/include/executor/spi_priv.h +++ b/src/include/executor/spi_priv.h @@ -23,8 +23,10 @@ typedef struct /* current results */ uint32 processed; /* by Executor */ Oid lastoid; - SPITupleTable *tuptable; + SPITupleTable *tuptable; /* tuptable currently being built */ + /* resources of this execution context */ + slist_head tuptables; /* list of all live SPITupleTables */ MemoryContext procCxt; /* procedure context */ MemoryContext execCxt; /* executor context */ MemoryContext savedcxt; /* context of SPI_connect's caller */ diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index f74326a53c..3b2919c543 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -1202,7 +1202,13 @@ exec_stmt_block(PLpgSQL_execstate *estate, PLpgSQL_stmt_block *block) */ SPI_restore_connection(); - /* Must clean up the econtext too */ + /* + * 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 + * eval_tuptable. + */ + estate->eval_tuptable = NULL; exec_eval_cleanup(estate); /* Look for a matching exception handler */ diff --git a/src/pl/plpython/plpy_cursorobject.c b/src/pl/plpython/plpy_cursorobject.c index 910e63b199..2c458d35fd 100644 --- a/src/pl/plpython/plpy_cursorobject.c +++ b/src/pl/plpython/plpy_cursorobject.c @@ -377,8 +377,6 @@ PLy_cursor_iternext(PyObject *self) } PG_CATCH(); { - SPI_freetuptable(SPI_tuptable); - PLy_spi_subtransaction_abort(oldcontext, oldowner); return NULL; } @@ -461,8 +459,6 @@ PLy_cursor_fetch(PyObject *self, PyObject *args) } PG_CATCH(); { - SPI_freetuptable(SPI_tuptable); - PLy_spi_subtransaction_abort(oldcontext, oldowner); return NULL; } diff --git a/src/pl/plpython/plpy_spi.c b/src/pl/plpython/plpy_spi.c index ed1f21cd6a..982bf84e0e 100644 --- a/src/pl/plpython/plpy_spi.c +++ b/src/pl/plpython/plpy_spi.c @@ -439,7 +439,6 @@ PLy_spi_execute_fetch_result(SPITupleTable *tuptable, int rows, int status) { MemoryContextSwitchTo(oldcontext); PLy_typeinfo_dealloc(&args); - SPI_freetuptable(tuptable); Py_DECREF(result); PG_RE_THROW(); }