Re: Fwd: Core dump with nested CREATE TEMP TABLE - Mailing list pgsql-hackers

On Thu, Jan 28, 2016 at 11:12:15AM -0500, Robert Haas wrote:
> I'm honestly failing to understand why we should change anything at
> all.  I don't believe that doing something more severe than marking
> the portal failed actually improves anything.  I suppose if I had to
> pick between what you proposed before and elog(FATAL) I'd pick the
> latter, but I see no real reason to cut off future code (or
> third-party code) at the knees like that.  I don't see it as either
> necessary or desirable to forbid something just because there's no
> clear present use case for it.

As you say, forbidding things makes friction in the event that someone comes
along wanting to do the forbidden thing.  Forbidding things also simplifies
the system, making it easier to verify.  This decision should depend on the
API's audience.  We have prominently-public APIs like lsyscache.h,
stringinfo.h and funcapi.h.  They should cater to reasonably-foreseeable use
cases, not just what yesterday's users have actually used.  We then have
narrow-interest APIs like subselect.h and view.h.  For those, the value of a
simpler system exceeds the risk-adjusted cost of friction.  They should impose
strict constraints on their callers.

Portal status belongs to the second category.  PortalRun(), PortalRunFetch()
and PersistHoldablePortal() are the three core functions that place portals
into PORTAL_ACTIVE status.  No PGXN module does so; none so much as references
a PortalStatus constant or MarkPortal{Active,Done,Failed}() function.  If
someone adds a fourth core portal runner, the system will be simpler and
better if that function replicates the structure of the existing three.

> The code you quote emits a warning
> about a reasonably forseeable situation that can never be right, but
> there's no particular reason to think that MarkPortalFailed is the
> wrong thing to do here if that situation came up.

I have two reasons to expect these MarkPortalFailed() calls will be the wrong
thing for hypothetical code reaching them.  First, PortalRun() and peers reset
ActivePortal and PortalContext on error in addition to fixing portal status
with MarkPortalFailed().  If code forgets to update the status, there's a
substantial chance it forgot the other two.  My patch added a comment
explaining the second reason:

+        /*
+         * See similar code in AtSubAbort_Portals().  This would fire if code
+         * orchestrating multiple top-level transactions within a portal, such
+         * as VACUUM, caught errors and continued under the same portal with a
+         * fresh transaction.  No part of core PostgreSQL functions that way,
+         * though it's a fair thing to want.  Such code would wish the portal
+         * to remain ACTIVE, as in PreCommit_Portals(); we don't cater to
+         * that.  Instead, like AtSubAbort_Portals(), interpret this as a bug.
+         */