Noah Misch <n...@leadboat.com> writes:
> On Fri, Jul 05, 2013 at 02:47:06PM -0400, Tom Lane wrote:
>> So I'm inclined to propose that SPI itself should offer some mechanism
>> for cleaning up tuple tables at subtransaction abort. We could just
>> have it automatically throw away tuple tables made in the current
>> subtransaction, or we could allow callers to exercise some control,
>> perhaps by calling a function that says "don't reclaim this tuple table
>> automatically". I'm not sure if there's any real use-case for such a
>> call though.
> I suppose that would be as simple as making spi_dest_startup() put the
> tuptabcxt under CurTransactionContext? The function to prevent reclamation
> would then just do a MemoryContextSetParent().
I experimented with this, and found out that it's not quite that simple.
In a SPI procedure that hasn't created a subtransaction (eg, a plpgsql
function without an exception block), if we attach tuple tables to the
outer transaction's CurTransactionContext then they fail to go away
during SPI_finish(), creating leaks in code that was previously correct.
However, we can use your idea when running inside a subtransaction,
while still attaching the tuple table to the procedure's own procCxt
when no subtransaction is involved. The attached draft patch does it
that way, and successfully resolves the complained-of leakage case.
I like this better than what I originally had in mind, because it
produces no change in semantics in the case where a SPI procedure
doesn't create any subtransactions, which I imagine is at least 99.44%
of third-party SPI-using code.
Also, because the tuple tables don't go away until
CleanupSubTransaction(), code that explicitly frees them will still work
as long as it does that before rolling back the subxact. Thus, the
patch's changes to remove SPI_freetuptable() calls in
plpy_cursorobject.c are not actually necessary for correctness, it's
just that we no longer need them. Ditto for the change in
AtEOSubXact_SPI. Unfortunately, the change in pl_exec.c *is* necessary
to avoid a coredump, because that call was being made after rolling back
the subxact.
All in all, the risk of breaking anything outside core code seems very
small, and I'm inclined to think that we don't need to provide an API
function to reparent a tuple table. But having said that, I'm also
inclined to not back-patch this further than 9.3, just in case.
The patch still requires documentation changes, and there's also one
more error-cleanup SPI_freetuptable() call that ought to go away, in
PLy_spi_execute_fetch_result. But that one needs the fix posited in
http://www.postgresql.org/message-id/21017.1374273...@sss.pgh.pa.us
first.
Comments?
regards, tom lane
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index f30b2cd93330d72e0a412d6517aec08c96cc753e..3f4d1e625d738ad0c08d0245a0169451d29a631d 100644
*** a/src/backend/executor/spi.c
--- b/src/backend/executor/spi.c
*************** AtEOSubXact_SPI(bool isCommit, SubTransa
*** 284,291 ****
{
/* 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;
}
}
--- 284,291 ----
{
/* free Executor memory the same as _SPI_end_call would do */
MemoryContextResetAndDeleteChildren(_SPI_current->execCxt);
! /* forget any partially created tuple-table */
! /* (we don't need to free it, subxact cleanup will do so) */
_SPI_current->tuptable = NULL;
}
}
*************** void
*** 1641,1648 ****
spi_dest_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
{
SPITupleTable *tuptable;
! MemoryContext oldcxt;
MemoryContext tuptabcxt;
/*
* When called by Executor _SPI_curid expected to be equal to
--- 1641,1649 ----
spi_dest_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
{
SPITupleTable *tuptable;
! MemoryContext parentcxt;
MemoryContext tuptabcxt;
+ MemoryContext oldcxt;
/*
* When called by Executor _SPI_curid expected to be equal to
*************** spi_dest_startup(DestReceiver *self, int
*** 1656,1669 ****
if (_SPI_current->tuptable != NULL)
elog(ERROR, "improper call to spi_dest_startup");
! oldcxt = _SPI_procmem(); /* switch to procedure memory context */
! tuptabcxt = AllocSetContextCreate(CurrentMemoryContext,
"SPI TupTable",
ALLOCSET_DEFAULT_MINSIZE,
ALLOCSET_DEFAULT_INITSIZE,
ALLOCSET_DEFAULT_MAXSIZE);
! MemoryContextSwitchTo(tuptabcxt);
_SPI_current->tuptable = tuptable = (SPITupleTable *)
palloc(sizeof(SPITupleTable));
--- 1657,1681 ----
if (_SPI_current->tuptable != NULL)
elog(ERROR, "improper call to spi_dest_startup");
! /*
! * If we're in the same subtransaction our SPI procedure was called in,
! * attach the tuple table context to the procedure's procCxt. This
! * ensures it'll go away at SPI_finish(). If we're in a subtransaction
! * established within the procedure, attach the tuple table to the
! * subtransaction's CurTransactionContext, so that it'll go away
! * immediately if the subtransaction fails.
! */
! if (_SPI_current->connectSubid == GetCurrentSubTransactionId())
! parentcxt = _SPI_current->procCxt;
! else
! parentcxt = CurTransactionContext;
! tuptabcxt = AllocSetContextCreate(parentcxt,
"SPI TupTable",
ALLOCSET_DEFAULT_MINSIZE,
ALLOCSET_DEFAULT_INITSIZE,
ALLOCSET_DEFAULT_MAXSIZE);
! oldcxt = MemoryContextSwitchTo(tuptabcxt);
_SPI_current->tuptable = tuptable = (SPITupleTable *)
palloc(sizeof(SPITupleTable));
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 ----
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
