Re: Preventing tuple-table leakage in plpgsql

It looks like to me exec_stmt_block creates a subtransaction if the block
has an exception handler by calling BeginInternalSubTransaction. Then
inside the PG_TRY it calls exec_stmts which runs the actual body of the
begin block. If an exception is thrown then I presume we are hitting the
PG_CATCH block where it calls RollbackAndReleaseCurrentSubTransaction.
Inside RollbackAndReleaseCurrentSubTransaction it calls AtEOSubXact_SPI
which frees the procCxt where the tuptabcxt is allocated.

Looking at it seems to suggest that the memory allocated under tuptabcxt
should be freed when we abort the subtransaction? Or did I miss something
here?

BTW, hacking pl_exec and moving the tubtabcxt under CurTransactionContext
does also seem to resolve the memory issue. Which suggests that somewhere
along the way AtEOSubXact_SPI is not called when the subtransaction is
aborted by the catch block, that or I got lost in the code.

On Fri, Jul 5, 2013 at 6:39 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:

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