On Thu, Apr 26, 2018 at 06:36:01PM -0400, Tom Lane wrote:
> Uh, no, the memory *isn't* reused later. The coding in AtEOXact_SPI
> assumes that it can just null out _SPI_stack because whatever that
> might've been pointing to is transaction-lifespan memory and will
> go away without extra work here. Which was true in every prior
> release, because SPI_connect caused the stack to be allocated in
> TopTransactionContext. You've changed it to be in TopMemoryContext
> and not adjusted the cleanup to match.
Yep, thanks for jumping in the ship.
> We could change the coding in AtEOXact_SPI so that it preserves
> _SPI_stack and _SPI_stack_depth and only resets _SPI_connected,
> but I'm not sure what's the point of keeping the stack storage
> if _SPI_connected gets reset; that means we no longer think
> there's any data there. One way or the other this code is now
> quite schizophrenic.
Would we want to handle memory contexts differently between an atomic
and a non-atomic connection to the SPI? For an atomic call, there is
little point in keeping the stack in the session context as this is
invalid data by the end of the transaction commit, and back-branches
rely on the transaction's context removed at when a transaction finishes
to cleanup the memory of the SPI stack.
> Possibly what you really want is for the stack storage to be permanent
> and for _SPI_connected to get reset to something that's not necessarily
> -1, so that AtEOXact_SPI is more like AtEOSubXact_SPI. But then you need
> a mechanism for figuring out how much of the stack ought to outlive the
> current transaction. I would bet money that that
> "_SPI_current->internal_xact" thing is wrong/inadequate.
>
> The comments in both SPI_connect and AtEOXact_SPI about memory management
> seem to need work, too.
Yeah, error handling also needs some extra lookup. Do you still want
the memory to be around when a transaction fails with an internal ERROR?
Having consistency between the way atomic and non-atomic connections are
handled would be nice, for both the creation of the SPI stack and its
deletion. The final result should not impact the user experience
particularly for background workers or people would get ramping silent
failures because of memory exhaustion.
--
Michael