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.wag...@gmail.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 <n...@leadboat.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 >> > >