abort-time portal cleanup

I've been studying At{Sub,}{Abort,Cleanup}_Portals() for the last few
days and have come to the conclusion that the code is not entirely up
to our usual standards. I believe that a good deal of the reason for
this is attributable to the poor quality of the code comments in this
area, although there are perhaps some other contributing factors as
well, such as bullheadedness on my part and perhaps others.

The trouble starts with the header comment for AtAbort_Portals, which
states that "At this point we run the cleanup hook if present, but we
can't release the portal's memory until the cleanup call." At the time
this logic was introduced in commit
de28dc9a04c4df5d711815b7a518501b43535a26 (2003-05-02),
AtAbort_Portals() affected all non-held portals without caring whether
they were active or not, and, UserAbortTransactionBlock() called
AbortTransaction() directly, so typing "ROLLBACK;" would cause
AtAbort_Portals() to be reached from within PortalRun(). Even if
PortalRun() managed to return without crashing, the caller would next
try to call PortalDrop() on what was now an invalid pointer. However,
commit 8f9f1986034a2273e09ad10671e10d1adda21d1f (2004-09-16) changed
things so that UserAbortEndTransaction() just sets things up so that
the subsequent call to CommitTransactionCommand() would call
AbortTransaction() instead of trying to do it right away, and that
doesn't happen until after we're done with the portal. As far as I
can see, that change made this comment mostly false, but the comment
has nevertheless managed to survive for another ~15 years. I think we
can, and in fact should, just drop the portal right here.

As far as actually making that work, there are a few wrinkles. The
first is that we might be in the middle of FATAL. In that case, unlike
the ROLLBACK case, a call to PortalRun() is still on the stack, but
we'll exit the process rather than returning, so the fact that we've
created a dangling pointer for the caller won't matter. However, as
shown by commit ad9a274778d2d88c46b90309212b92ee7fdf9afe (2018-02-01)
and the report that led up to it at
https://www.postgresql.org/message-id/20180128034531.h6o4w3727ifof3jy%40alap3.anarazel.de
it's not a good idea to try to clean up the portal in that case,
because we might've already started shutting down critical systems.
It seems not only risky but also unnecessary: our process-local state
is about to go away, and the executor shouldn't need to clean up any
shared memory state that won't also get cleaned up by some other
mechanism. So, it seems to me that if we reach AtAbort_Portals()
during FATAL processing, we should either (1) do nothing at all and
just return or (2) forget about all the existing portals without
cleaning them up and then return. The second option seems a little
safer to me, because it guarantees that if we somehow reach code that
might try to look up a portal later, it won't find anything. But I
think it's arguable.

The second wrinkle is that there might be an active portal. Apart
from the FATAL case already mentioned, I think the only way this can
happen is some C code that calls purposefully calls AbortTransaction()
in the middle of executing a command. It can't be an ERROR, because
then the portal would be marked as failed before we get here, and it
can't be an explicit ROLLBACK, because as noted above, that case was
changed 15 years ago. It's got to be some other case where C code
calls AbortTransaction() voluntarily in the middle of a statement. For
over a decade, there were no cases at all of this type, but the code
in this function catered to hypothetical cases by marking the portal
failed. By 2016, Noah had figured out that this was bogus, and that
any future cases would likely require different handling, but Tom and
I shouted him down:

http://postgr.es/m/67674.1454259004@sss.pgh.pa.us

The end result of that discussion was commit
41baee7a9312eefb315b6b2973ac058c9efaa9cf (2016-02-05) which left the
code as it was but added comments nothing that it was wrong. It
actually wasn't entirely wrong, because it handled the FATAL case
mentioned above by the byzantine mechanism of invoking the portal's
cleanup callback after first setting the status to PORTAL_FAILED.
Since the only existing cleanup callback arranges to do nothing if the
status is PORTAL_FAILED, this worked out to a complicated way of
(correctly) skipping callback in the FATAL case.

But, probably because that wasn't documented in any understandable
way, possibly because nobody really understood it, when commit
8561e4840c81f7e345be2df170839846814fa004 (2018-01-22) added support
for transaction control in procedures, it just removed the code
marking active portals as failed, just as Noah had predicted would be
necessary ~2 years earlier. Andres noticed that this broke the FATAL
case and tracked it back to the removal of this code, resulting it it
getting put back, but just for the FATAL case, in commit
ad9a274778d2d88c46b90309212b92ee7fdf9afe (2018-02-01). See also
discussion at:

https://www.postgresql.org/message-id/20180128034531.h6o4w3727ifof3jy%40alap3.anarazel.de

I think that the code here still isn't really right. Apart from the
fact that the comments don't explain anything very clearly and the
FATAL case is handled in a way that looks overly complicated and
accidental, the handling in AtSubAbort_Portals() hasn't been updated,
so it's now inconsistent with AtAbort_Portals() as to both substance
and comments. I think that's only held together because stored
procedures don't yet support explicit control over subtransactions,
only top-level transactions.

Stepping back a bit, stored procedures are a good example of a command
that uses multiple transactions internally. We have a few others, such
as VACUUM, but at present, that case only commits transactions
internally; it does not roll them back internally. If it did, it
would want the same thing that procedures want, namely, to leave the
active portal alone. It doesn't quite work to leave the active portal
completely alone, because the portal has got a pointer to a
ResourceOwner which is about to be freed by end-of-transaction
cleanup; at the least, we've got to clear the pointer to that when the
multi-transaction statement eventually finishes in some future
transaction, it doesn't try to do anything with a dangling pointer.
But note that the resource owner can't really be in use in this
situation anyway; if it is, then the transaction abort is about to
blow away resources that the statement still needs. Similarly, the
statement can't be using any transaction-local memory, because that
too is about to get blown away. The only thing that can work at all
here is a statement that's been carefully designed to be able to
survive starting and ending transactions internally. Such a statement
must not rely on any transaction-local resources. The only thing I'm
sure we have to do is set portal->resowner to NULL. Calling the
cleanup hook, as the current code does, can't be right, because we'd
be cleaning up something that isn't going away. I think it only works
now because this is another case where the cleanup hook arranges to do
nothing in the cases where calling it is wrong in the first place. The
current code also calls PortalReleaseCachedPlan in this case; I'm not
100% certain whether that's appropriate or not.

Attached is a patch that (1) removes At(Sub)Cleanup_Portals() entirely
in favor of dropping portals on the spot in At(Sub)Abort_Portals(),
(2) overhauls the comments in this area, and (3) makes
AtSubAbort_Portals() consistent with AtAbort_Portals(). One possible
concern here - which Andres mentioned to me during off-list discussion
- is that someone might create a portal for a ROLLBACK statement, and
then try to execute that portal after an error has occurred. In
theory, keeping the portal around between abort time and cleanup time
might allow this to work, but it actually doesn't work, so there's no
functional regression. As noted by Amit Kapila also during off-list
discussion, IsTransactionStmtList() only works if portal->stmts is
set, and PortalReleaseCachedPlan() clears portal->stmts, so once we've
aborted the transaction, any previously-created portals are
unrecognizable as transaction statements.

This area is incredibly confusing to understand, so it's entirely
possible that I've gotten some things wrong. However, I think it's
worth the effort to try to do some cleanup here, because I think it's
overly complex and under-documented. On top of the commits already
mentioned, some of which I think demonstrate that the issues here
haven't been completely understood, I found other bug fix commits that
look like bugs that might've never happened in the first place if this
weren't all so confusing.

Feedback appreciated,

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company