I wrote:
> Hmm ... good point. The other plan I'd been considering was to add
> explicit tracking inside spi.c of all tuple tables created within the
> current procedure, and then have AtEOSubXact_SPI flush any that were
> created inside a failed subxact. The tables would still be children of
> the procCxt and thus could not be leaked past SPI_finish. When you
> suggested attaching to subtransaction contexts I thought that would let
> us get away without this additional bookkeeping logic, but maybe we
> should bite the bullet and add the extra logic. A change that's meant
> to remove leak risks really shouldn't be introducing other, new leak
> risks. (An additional advantage is we could detect attempts to free
> the same tuptable more than once, which would be a good thing ...)
Here's a draft cut at that. I've checked that this fixes the reported
memory leak. This uses ilist.h, so it couldn't easily be backported
to before 9.3, but we weren't going to do that anyway.
Worth noting is that SPI_freetuptable() is now much more picky about what
it's being passed: it won't free anything that is not a tuptable of the
currently connected SPI procedure. This doesn't appear to be a problem
for anything in core or contrib, but I wonder whether anyone thinks we
need to relax that? If so, we could allow it to search down the SPI
context stack, but I'm not sure I see a use-case for allowing an inner
SPI procedure to free a tuple table made by an outer one. In general,
I think this version of SPI_freetuptable() is a lot safer than what
we had before, and possibly likely to find bugs in calling code.
Another point worth making is that this version of the patch deletes the
tuple tables during AtEOSubXact_SPI(), earlier in cleanup than would
happen with the prior version. That increases the risk that external
code might try to delete an already-deleted tuple table, if it tries
to call SPI_freetuptable() during subxact cleanup. The new code won't
crash, although come to think of it it will probably throw an error
because you're not connected anymore. (Maybe this is a reason to not
insist on being connected, but just silently search whatever the top
stack context is?)
This still lacks doc changes, too.
regards, tom lane
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index f30b2cd93330d72e0a412d6517aec08c96cc753e..f179922dd4bd4eb36212ebd91413ba28de44b2fc 100644
*** a/src/backend/executor/spi.c
--- b/src/backend/executor/spi.c
*************** SPI_connect(void)
*** 126,131 ****
--- 126,132 ----
_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();
*************** SPI_finish(void)
*** 166,172 ****
/* Restore memory context as it was before procedure call */
MemoryContextSwitchTo(_SPI_current->savedcxt);
! /* Release memory used in procedure call */
MemoryContextDelete(_SPI_current->execCxt);
_SPI_current->execCxt = NULL;
MemoryContextDelete(_SPI_current->procCxt);
--- 167,173 ----
/* Restore memory context as it was before procedure call */
MemoryContextSwitchTo(_SPI_current->savedcxt);
! /* Release memory used in procedure call (including tuptables) */
MemoryContextDelete(_SPI_current->execCxt);
_SPI_current->execCxt = NULL;
MemoryContextDelete(_SPI_current->procCxt);
*************** AtEOSubXact_SPI(bool isCommit, SubTransa
*** 282,292 ****
*/
if (_SPI_current && !isCommit)
{
/* 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;
}
}
--- 283,316 ----
*/
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 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.
! */
! 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);
}
}
*************** SPI_freetuple(HeapTuple tuple)
*** 1021,1028 ****
void
SPI_freetuptable(SPITupleTable *tuptable)
{
! if (tuptable != NULL)
! MemoryContextDelete(tuptable->tuptabcxt);
}
--- 1045,1089 ----
void
SPI_freetuptable(SPITupleTable *tuptable)
{
! slist_mutable_iter siter;
! bool found = false;
!
! /* ignore call if NULL pointer */
! if (tuptable == NULL)
! return;
!
! /* must be connected */
! if (_SPI_curid + 1 != _SPI_connected)
! elog(ERROR, "improper call to SPI_freetuptable");
! if (_SPI_current != &(_SPI_stack[_SPI_curid + 1]))
! elog(ERROR, "SPI stack corrupted");
!
! /* ensure tuptable is active, then remove it from list */
! 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;
! }
! }
! 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);
}
*************** spi_dest_startup(DestReceiver *self, int
*** 1656,1661 ****
--- 1717,1724 ----
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,
*************** spi_dest_startup(DestReceiver *self, int
*** 1666,1673 ****
MemoryContextSwitchTo(tuptabcxt);
_SPI_current->tuptable = tuptable = (SPITupleTable *)
! palloc(sizeof(SPITupleTable));
tuptable->tuptabcxt = tuptabcxt;
tuptable->alloced = tuptable->free = 128;
tuptable->vals = (HeapTuple *) palloc(tuptable->alloced * sizeof(HeapTuple));
tuptable->tupdesc = CreateTupleDescCopy(typeinfo);
--- 1729,1746 ----
MemoryContextSwitchTo(tuptabcxt);
_SPI_current->tuptable = tuptable = (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 d4f1272cd81ae13eb52a16327357bfdf15ad7621..81310e377f6b769c5aa306cf9aab932813dbeef8 100644
*** a/src/include/executor/spi.h
--- b/src/include/executor/spi.h
***************
*** 13,18 ****
--- 13,19 ----
#ifndef SPI_H
#define SPI_H
+ #include "lib/ilist.h"
#include "nodes/parsenodes.h"
#include "utils/portal.h"
*************** typedef struct SPITupleTable
*** 24,29 ****
--- 25,32 ----
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 ef7903abd09a94906d48bfcbf3ea223aacdd9423..7d4c9e963933401bf278363bf51a9db63c78086f 100644
*** a/src/include/executor/spi_priv.h
--- b/src/include/executor/spi_priv.h
*************** typedef struct
*** 23,30 ****
/* current results */
uint32 processed; /* by Executor */
Oid lastoid;
! SPITupleTable *tuptable;
MemoryContext procCxt; /* procedure context */
MemoryContext execCxt; /* executor context */
MemoryContext savedcxt; /* context of SPI_connect's caller */
--- 23,32 ----
/* current results */
uint32 processed; /* by Executor */
Oid lastoid;
! 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 5b142e3bee6ccf929f3d4ec8ef3bf6c46467dc90..aa6b0af2e08ba59358ec8bfa7182bae5cef9cb8a 100644
*** a/src/pl/plpgsql/src/pl_exec.c
--- b/src/pl/plpgsql/src/pl_exec.c
*************** exec_stmt_block(PLpgSQL_execstate *estat
*** 1202,1208 ****
*/
SPI_restore_connection();
! /* Must clean up the econtext too */
exec_eval_cleanup(estate);
/* Look for a matching exception handler */
--- 1202,1214 ----
*/
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
! * 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 910e63b19954cae697043d1641365de9b480cdc0..2c458d35fdb1a19fad1a46670487d6468f63384b 100644
*** a/src/pl/plpython/plpy_cursorobject.c
--- b/src/pl/plpython/plpy_cursorobject.c
*************** PLy_cursor_iternext(PyObject *self)
*** 377,384 ****
}
PG_CATCH();
{
- SPI_freetuptable(SPI_tuptable);
-
PLy_spi_subtransaction_abort(oldcontext, oldowner);
return NULL;
}
--- 377,382 ----
*************** PLy_cursor_fetch(PyObject *self, PyObjec
*** 461,468 ****
}
PG_CATCH();
{
- SPI_freetuptable(SPI_tuptable);
-
PLy_spi_subtransaction_abort(oldcontext, oldowner);
return NULL;
}
--- 459,464 ----
diff --git a/src/pl/plpython/plpy_spi.c b/src/pl/plpython/plpy_spi.c
index ed1f21cd6a51a83bd57157f16af003654b47b46c..982bf84e0e544661f41d98ca7a6eeeddda288036 100644
*** a/src/pl/plpython/plpy_spi.c
--- b/src/pl/plpython/plpy_spi.c
*************** PLy_spi_execute_fetch_result(SPITupleTab
*** 439,445 ****
{
MemoryContextSwitchTo(oldcontext);
PLy_typeinfo_dealloc(&args);
- SPI_freetuptable(tuptable);
Py_DECREF(result);
PG_RE_THROW();
}
--- 439,444 ----
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
