On Wed, Jan 27, 2016 at 11:47 PM, Noah Misch <noah@leadboat.com> wrote:
> On Wed, Jan 27, 2016 at 11:04:33PM -0500, Robert Haas wrote:
>> + Assert(portal->status != PORTAL_ACTIVE);
>> if (portal->status == PORTAL_ACTIVE)
>> MarkPortalFailed(portal);
>>
>> Now that just looks kooky to me. We assert that the portal isn't
>> active, but then cater to the possibility that it might be anyway?
>
> Right.
>
>> That seems totally contrary to our usual programming practices, and a
>> bad idea for that reason.
>
> It is contrary to our usual programming practices, I agree. I borrowed the
> idea from untenured code (da3751c8, 2015-11-11) in load_relcache_init_file():
>
> if (nailed_rels != NUM_CRITICAL_SHARED_RELS ||
> nailed_indexes != NUM_CRITICAL_SHARED_INDEXES)
> {
> elog(WARNING, "found %d nailed shared rels and %d nailed shared indexes in init file, but
expected%d and %d respectively",
> nailed_rels, nailed_indexes,
> NUM_CRITICAL_SHARED_RELS, NUM_CRITICAL_SHARED_INDEXES);
> /* Make sure we get developers' attention about this */
> Assert(false);
>
> I liked this pattern. It's a good fit for cases that we design to be
> impossible yet for which we have a workaround if they do happen. That being
> said, if you feel it's bad, I would be fine using elog(FATAL). I envision
> this as a master-only change in any case. No PGXN module references
> PORTAL_ACTIVE or MarkPortalActive(), so it's unlikely that extension code will
> notice the change whether in Assert() form or in elog() form. What is best?
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. 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.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company