Re: Preventing tuple-table leakage in plpgsql

On Fri, Jul 05, 2013 at 02:47:06PM -0400, Tom Lane wrote:
> Bug #8279 exhibits an intra-transaction memory leak in a plpgsql
> function that repeatedly traps an error. The cause of the leak is that
> the SPITupleTable created during exec_stmt_execsql is never cleaned up.
> (It will get cleaned up at function exit, but that's not soon enough in
> this usage.)

> One approach we could take is to insert PG_TRY blocks to ensure that
> SPI_freetuptable is called on error exit from such functions. (plpython
> seems to have adopted this solution already.) However, that tends to be
> pretty messy notationally, and possibly could represent a noticeable
> performance hit depending on what you assume about the speed of
> sigsetjmp(). Moreover, fixing known trouble spots this way does nothing
> to guard against similar errors elsewhere.

I, too, find that strategy worth avoiding as long as practical.

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

Is there good reason to believe that SPI tuptables are the main interesting
PL/pgSQL allocation to make a point of freeing promptly, error or no error? I
wonder if larger sections of pl_exec.c could run under CurTransactionContext.

> It's also not very clear to me if tuple tables should be thrown away
> automatically at subtransaction commit. We could do that, or leave
> things alone, or add some logic to emit warning bleats about unreleased
> tuple tables (comparable to what is done for many other resource types).
> If we change anything about the commit case, I think we run significant
> risk of breaking third-party code that works now, so maybe it's best
> to leave that alone.

That's not clear to me, either. The safe thing would be to leave the default
unchanged but expose an API to override the tuptable parent context.
Initially, only PL/pgSQL would use it.

> It might also be worth debating whether to back-patch such a fix.
> This issue has been there ever since plpgsql grew the ability to trap
> errors, so the lack of previous reports might mean that it's not worth
> taking risks to fix such leaks in back branches.

I agree that could go either way; let's see what the patch looks like.

Thanks,
nm

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