From: | Noah Misch <noah(at)leadboat(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | pgsql-hackers(at)postgresql(dot)org, Chad Wagner <chad(dot)wagner(at)gmail(dot)com> |
Subject: | Re: Preventing tuple-table leakage in plpgsql |
Date: | 2013-07-21 15:19:32 |
Message-ID: | 20130721151932.GA126816@tornado.leadboat.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Fri, Jul 19, 2013 at 07:34:14PM -0400, Tom Lane wrote:
> Noah Misch <noah(at)leadboat(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Noah Misch | 2013-07-21 16:07:29 | Re: InvokeObjectPostAlterHook() vs. CommandCounterIncrement() |
Previous Message | Tom Lane | 2013-07-21 15:13:09 | Re: Preventing tuple-table leakage in plpgsql |