From: Tom Lane <tgl@sss.pgh.pa.us>
To: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
cc: julian.schauder@gmx.de, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15703: Segfault in cancelled CALL-Statements
In-reply-to: <8678.1553097373@sss.pgh.pa.us>
References: <15703-c12c5bc0ea34ba26@postgresql.org>
 <8678.1553097373@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Wed, 20 Mar 2019 11:56:13 -0400"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <20458.1553535374.1@sss.pgh.pa.us>
Date: Mon, 25 Mar 2019 13:36:14 -0400
Message-ID: <20459.1553535374@sss.pgh.pa.us>
Precedence: bulk

I wrote:
> I find HoldPinnedPortals suspicious in another way: is it really
> OK for it to mark *all* pinned portals as auto-held?  What if we're
> in a nest of procedures run by different PLs (of which only the
> innermost will have any idea we're committing)?

After taking another look at this, I'm convinced that it's flat out
broken.  It cannot be up to individual PLs to decide whether or not
to call HoldPinnedPortals before issuing a commit.  If a PL that needs
that calls (a function belonging to) some other PL that thinks it
doesn't need that, and the second PL issues a commit or rollback,
we'll have a failure for whatever portal(s) the first PL left pinned.

I think, therefore, that we should get rid of HoldPinnedPortals and
just assign the responsibility for holding pinned portals to the
normal code paths, probably PreCommit_Portals and AtAbort_Portals.

Another issue in this area is whether it's really safe to hold a portal
during transaction abort at all.  I'm inclined to think that it is not.
HoldPortal can call arbitrary user code, and the idea that we'll let that
happen during an already-failed transaction seems borderline insane from
here.  However, I'm not very sure what we can do instead.

As things stand right now, the calls of HoldPinnedPortals from places
like exec_stmt_rollback are safe in this sense, because we haven't yet
started to abort the current transaction.  However, that just raises
the question of why we need those calls at all.  If a transaction
abort happens due to some kind of error, we aren't going to do any
pinned-portal holding, so why should we need it in exec_stmt_rollback?

Maybe the rule needs to be "we'll do HoldPinnedPortals in SPI_rollback,
but not if you reach transaction abort via an actual error".  Seems
a bit weird though.

Thoughts?

			regards, tom lane