Re: Preventing tuple-table leakage in plpgsql

It looks like to me when AtEOSubXact_SPI is called the
_SPI_current->connectSubId is always 1 (since it is only set when
SPI_connect is called, which is only once for plpgsql), but the
CurrentSubTransactionId is incremented each time a subtransaction is
started.

As a result, the memory for procCxt is only freed when I presume the
TopTransaction is aborted or committed.

Should SPI_connect be called again after the subtransaction is created?
And SPI_finish before the subtransaction is committed or aborted?

On Thu, Jul 11, 2013 at 8:46 PM, Chad Wagner <chad(dot)wagner(at)gmail(dot)com> wrote:

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