Re: [PATCH] plpython function causes server panic

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Mar 22, 2024 at 4:37 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I think these things are already dealt with. However, one thing
>> worth questioning is that CommitSubTransaction() will just silently
>> kill any workers started during the current subxact, and likewise
>> CommitTransaction() zaps workers without complaint. Shouldn't these
>> instead throw an error about how you didn't close parallel mode,
>> and then the corresponding Abort function does the cleanup?
>> I did not change that behavior here, but it seems dubious.

> I'm not sure. I definitely knew when I wrote this code that we often
> emit warnings about resources that aren't cleaned up at (sub)commit
> time rather than just silently releasing them, and I feel like the
> fact that I didn't implement that behavior here was probably a
> deliberate choice to avoid some problem.

Ah, right, it's reasonable to consider this an end-of-xact resource
leak, which we generally handle with WARNING not ERROR. And I see
that AtEOXact_Parallel and AtEOSubXact_Parallel already do

if (isCommit)
elog(WARNING, "leaked parallel context");

However, the calling logic seems a bit shy of a load, in that it
trusts IsInParallelMode() completely to decide whether to check for
leaked parallel contexts. So we'd miss the case where somebody did
ExitParallelMode without having cleaned up workers. It's not like
AtEOXact_Parallel and AtEOSubXact_Parallel cost a lot when they have
nothing to do, so I think we should call them unconditionally, and
separately from that issue a warning if parallelModeLevel isn't zero
(and we're committing).

regards, tom lane