Re: Preventing tuple-table leakage in plpgsql

On Fri, Jul 19, 2013 at 07:34:14PM -0400, Tom Lane wrote:
> Noah Misch <noah(at)leadboat(dot)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.

Reasonable enough. Code that does use subtransactions will need to be more
careful than before to manually free tuple tables in the non-error case.
Failure to do so has been creating a leak that lasts until SPI_finish(), but
it will now be able to cause a transaction-lifespan leak.

> 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.

If PLy_spi_subtransaction_commit() were to throw an error (granted, unlikely),
would we not reference freed memory at those code sites as they stand today?

> Unfortunately, the change in pl_exec.c *is* necessary
> to avoid a coredump, because that call was being made after rolling back
> the subxact.

Brief search for similar patterns in external PLs:

plr - no subtransaction use
plv8 - no SPI_freetuptable()
plphp - uses both, but usage looks compatible
pljava - calls "SPI_freetuptable(SPI_tuptable)", but never a tuptable pointer
it stored away. Should be compatible, then.

> 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.

I wouldn't be confident in back-patching further than that. It's not hard to
imagine a non-core PL needing a compensating change like the one you made to
PL/pgSQL.

Thanks,
nm

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com