On Fri, Jan 29, 2016 at 11:19 PM, Noah Misch <n...@leadboat.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
