From: | Robert Haas <robertmhaas(at)gmail(dot)com> |
---|---|
To: | Noah Misch <noah(at)leadboat(dot)com> |
Cc: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Fwd: Core dump with nested CREATE TEMP TABLE |
Date: | 2016-01-30 12:37:45 |
Message-ID: | CA+TgmoaQxsniwCCbUV4-OcZeJr3fbOwb45Y3Ye3dAwLR=hDmnw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Fri, Jan 29, 2016 at 11:19 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> 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.
I won't argue with that, but it strikes me that if I were reviewing a
patch to do such a thing, I'd surely compare the new caller against
the existing ones, so any failure to adhere to the same design pattern
would be caught that way. I expect other reviewers and/or committers
would almost certainly do something similar. If we presuppose
incompetence on the part of future reviewers and committers, no action
taken now can secure us.
>> 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.
> + */
You may be right, but then again Tom had a different opinion, even
after seeing your patch, and he's no dummy.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
From | Date | Subject | |
---|---|---|---|
Next Message | Petr Jelinek | 2016-01-30 12:37:51 | Re: Sequence Access Method WIP |
Previous Message | Michael Paquier | 2016-01-30 12:36:27 | Re: extend pgbench expressions with functions |