Thread: Preventing tuple-table leakage in plpgsql

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

The submitter proposes fixing this by inserting some more
SPI_freetuptable calls in exec_stmt_execsql, but that's not much of a
fix.  The patch only covers the two ereports associated with "strict"
mode (and not, say, any errors thrown in functions called by
exec_stmt_execsql).  Nor does it do anything for similar leakage
scenarios elsewhere.  I count at least four other functions with the
same kind of oversight in plpgsql, and there are suspicious-looking
usages elsewhere as well.

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.

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.

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.

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.

Comments, better ideas?
        regards, tom lane

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

Noah Misch <noah@leadboat.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.

Also, because the tuple tables don't go away until
CleanupSubTransaction(), code that explicitly frees them will still work
as long as it does that before rolling back the subxact.  Thus, the
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.  Ditto for the change in
AtEOSubXact_SPI.  Unfortunately, the change in pl_exec.c *is* necessary
to avoid a coredump, because that call was being made after rolling back
the subxact.

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.

The patch still requires documentation changes, and there's also one
more error-cleanup SPI_freetuptable() call that ought to go away, in
PLy_spi_execute_fetch_result.  But that one needs the fix posited in
http://www.postgresql.org/message-id/21017.1374273434@sss.pgh.pa.us
first.

Comments?

            regards, tom lane

On Thu, Jul 11, 2013 at 09:14:38PM -0400, Chad Wagner wrote:
> 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.

Right.  AtEOSubXact_SPI() cleans up any SPI connections originating in the
ending subtransaction.  It leaves alone connections from higher subtransaction
levels; SPI has no general expectation that those have lost relevance.

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

In your code from bug #8279, I expect it to be freed when the DO block exits.
The backend might not actually shrink then, but repeated calls to a similar DO
block within the same transaction should not cause successive increases in the
process's memory footprint.

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

Hmm.  An SPI_push()+SPI_connect() every time PL/pgSQL starts a subtransaction
would be another way to fix it, yes.

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

Noah Misch <noah@leadboat.com> writes:
> On Thu, Jul 11, 2013 at 09:14:38PM -0400, Chad Wagner wrote:
>> Should SPI_connect be called again after the subtransaction is created?  And
>> SPI_finish before the subtransaction is committed or aborted?

> Hmm.  An SPI_push()+SPI_connect() every time PL/pgSQL starts a subtransaction
> would be another way to fix it, yes.

That sounds like a dangerous idea to me.  The procedure would then be
working actively with queries from two different SPI levels, which I'm
pretty sure would cause issues.  It's possible that plpgsql's SPI access
is sufficiently lexically-local that statements within the BEGIN block
couldn't use any SPI resources created by statements outside it nor vice
versa.  But then again maybe not, and in any case we couldn't imagine
that that would be a workable restriction for non-plpgsql scenarios.
        regards, tom lane

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

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

Hmm ... good point.  The other plan I'd been considering was to add
explicit tracking inside spi.c of all tuple tables created within the
current procedure, and then have AtEOSubXact_SPI flush any that were
created inside a failed subxact.  The tables would still be children of
the procCxt and thus could not be leaked past SPI_finish.  When you
suggested attaching to subtransaction contexts I thought that would let
us get away without this additional bookkeeping logic, but maybe we
should bite the bullet and add the extra logic.  A change that's meant
to remove leak risks really shouldn't be introducing other, new leak
risks.  (An additional advantage is we could detect attempts to free
the same tuptable more than once, which would be a good thing ...)

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

Hm, possibly, depending on just when the error was thrown.
        regards, tom lane

On Sun, Jul 21, 2013 at 12:40:38PM -0400, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > 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.
> 
> Hmm ... good point.  The other plan I'd been considering was to add
> explicit tracking inside spi.c of all tuple tables created within the
> current procedure, and then have AtEOSubXact_SPI flush any that were
> created inside a failed subxact.  The tables would still be children of
> the procCxt and thus could not be leaked past SPI_finish.  When you
> suggested attaching to subtransaction contexts I thought that would let
> us get away without this additional bookkeeping logic, but maybe we
> should bite the bullet and add the extra logic.

Is there reason to believe we wouldn't eventually find a half dozen other
allocations calling for similar bespoke treatment?  Does something make tuple
tables special among memory allocations, or are they just the garden-variety
allocation that happens to bother the test case at hand?

> A change that's meant
> to remove leak risks really shouldn't be introducing other, new leak
> risks.

Yes; that gives one pause.

Thanks,
nm

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

Noah Misch <noah@leadboat.com> writes:
> On Sun, Jul 21, 2013 at 12:40:38PM -0400, Tom Lane wrote:
>> Hmm ... good point.  The other plan I'd been considering was to add
>> explicit tracking inside spi.c of all tuple tables created within the
>> current procedure, and then have AtEOSubXact_SPI flush any that were
>> created inside a failed subxact.

> Is there reason to believe we wouldn't eventually find a half dozen other
> allocations calling for similar bespoke treatment?  Does something make tuple
> tables special among memory allocations, or are they just the garden-variety
> allocation that happens to bother the test case at hand?

It's hard to speculate about the memory management habits of third-party
SPI-using code.  But in plpgsql, the convention is that random bits of
memory should be allocated in a short-term context separate from the SPI
procCxt --- typically, the estate->eval_econtext expression context,
which exec_stmt_block already takes care to clean up when catching an
exception.  So the problem is that that doesn't work for tuple tables,
which have procCxt lifespan.  The fact that they tend to be big (at
least 8K apiece) compounds the issue.
        regards, tom lane

On Mon, Jul 22, 2013 at 10:02:30PM -0400, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > On Sun, Jul 21, 2013 at 12:40:38PM -0400, Tom Lane wrote:
> >> Hmm ... good point.  The other plan I'd been considering was to add
> >> explicit tracking inside spi.c of all tuple tables created within the
> >> current procedure, and then have AtEOSubXact_SPI flush any that were
> >> created inside a failed subxact.
> 
> > Is there reason to believe we wouldn't eventually find a half dozen other
> > allocations calling for similar bespoke treatment?  Does something make tuple
> > tables special among memory allocations, or are they just the garden-variety
> > allocation that happens to bother the test case at hand?
> 
> It's hard to speculate about the memory management habits of third-party
> SPI-using code.  But in plpgsql, the convention is that random bits of
> memory should be allocated in a short-term context separate from the SPI
> procCxt --- typically, the estate->eval_econtext expression context,
> which exec_stmt_block already takes care to clean up when catching an
> exception.  So the problem is that that doesn't work for tuple tables,
> which have procCxt lifespan.  The fact that they tend to be big (at
> least 8K apiece) compounds the issue.

Reasonable to treat them specially, per your plan, then.

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

I wrote:
> Hmm ... good point.  The other plan I'd been considering was to add
> explicit tracking inside spi.c of all tuple tables created within the
> current procedure, and then have AtEOSubXact_SPI flush any that were
> created inside a failed subxact.  The tables would still be children of
> the procCxt and thus could not be leaked past SPI_finish.  When you
> suggested attaching to subtransaction contexts I thought that would let
> us get away without this additional bookkeeping logic, but maybe we
> should bite the bullet and add the extra logic.  A change that's meant
> to remove leak risks really shouldn't be introducing other, new leak
> risks.  (An additional advantage is we could detect attempts to free
> the same tuptable more than once, which would be a good thing ...)

Here's a draft cut at that.  I've checked that this fixes the reported
memory leak.  This uses ilist.h, so it couldn't easily be backported
to before 9.3, but we weren't going to do that anyway.

Worth noting is that SPI_freetuptable() is now much more picky about what
it's being passed: it won't free anything that is not a tuptable of the
currently connected SPI procedure.  This doesn't appear to be a problem
for anything in core or contrib, but I wonder whether anyone thinks we
need to relax that?  If so, we could allow it to search down the SPI
context stack, but I'm not sure I see a use-case for allowing an inner
SPI procedure to free a tuple table made by an outer one.  In general,
I think this version of SPI_freetuptable() is a lot safer than what
we had before, and possibly likely to find bugs in calling code.

Another point worth making is that this version of the patch deletes the
tuple tables during AtEOSubXact_SPI(), earlier in cleanup than would
happen with the prior version.  That increases the risk that external
code might try to delete an already-deleted tuple table, if it tries
to call SPI_freetuptable() during subxact cleanup.  The new code won't
crash, although come to think of it it will probably throw an error
because you're not connected anymore.  (Maybe this is a reason to not
insist on being connected, but just silently search whatever the top
stack context is?)

This still lacks doc changes, too.

            regards, tom lane

I wrote:
> Another point worth making is that this version of the patch deletes the
> tuple tables during AtEOSubXact_SPI(), earlier in cleanup than would
> happen with the prior version.  That increases the risk that external
> code might try to delete an already-deleted tuple table, if it tries
> to call SPI_freetuptable() during subxact cleanup.  The new code won't
> crash, although come to think of it it will probably throw an error
> because you're not connected anymore.  (Maybe this is a reason to not
> insist on being connected, but just silently search whatever the top
> stack context is?)

After further reflection I think that's the prudent way to do it, so
I've adjusted SPI_freetuptable to not insist on being connected.
Pushed with that change:
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=3d13623d75d3206c8f009353415043a191ebab39
        regards, tom lane