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
