Andres Freund <andres@anarazel.de> writes:
> On 2017-10-05 11:38:40 -0400, Tom Lane wrote:
>> I wonder why ReorderBufferCommit does this:
>> ...
> I'm not sure what the problem is that you're seeing?
Nah, I take that back. The AbortCurrentTransaction call looks funny
(and is sadly underdocumented) but it's not invalid to call it and
then call RollbackAndReleaseCurrentSubTransaction.
> What we have here is that plpgsql uses SPI to execute
> pg_logical_slot_peek_changes(), which internally does *not* use SPI.
Yeah. I think there are two separate issues:
1. The Assert that's crashing is just wrong and should be removed.
It's a hangover from the previous behavior (before 3d13623d7)
of unconditionally setting _SPI_current->tuptable = NULL there.
2. The "MemoryContextResetAndDeleteChildren(_SPI_current->execCxt)"
call, further up, is deleting a still-live executor state tree,
as well as the logical-decoding context that is a child of that
executor query context. So if you get past the Assert you'll still
crash later on.
I'm not sure about a good way to fix #2 without re-introducing the memory
leaks that call was added to fix (cf 7ec1c5a86). It's slightly annoying
at this point that we got rid of the SPI_push/SPI_pop mechanism, because
we could perhaps have used a check of the _SPI_curid value to distinguish
a SPI context we should clean up from one we shouldn't. But that ship has
sailed, and even if we wanted to undo that change there'd still be the
matter of how to fix the bugs that prompted removing SPI_push/SPI_pop.
The best idea I have at the moment is to introduce a new SPI API
function along the lines of "SPI_cleanup_execution()" which would
do the execCxt cleanup, expect SPI-using callers of internal
subtransactions to do that as part of error cleanup, and drop the
execCxt cleanup from AtEOSubXact_SPI. This is kind of annoying
because it's an API change that probably affects external PLs.
A PL that failed to absorb the change would have a memory leak,
although likely not a very bad one since leakage would only
accumulate in the case of a lot of failed SPI_executes in a row
(each in a subtransaction) with never a success.
Since the SPI_push-ectomy only happened in v10, conceivably we could avoid
the API break in prior branches by solving this with the _SPI_curid check
idea in those branches. I'm not really enamored of that though, since it
means designing and testing two independent fixes.
regards, tom lane
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs