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
