Re: Preventing tuple-table leakage in plpgsql

Noah Misch <noah@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.1374273434@sss.pgh.pa.us
first.

Comments?

            regards, tom lane