On Wed, Jan 27, 2016 at 11:47 PM, Noah Misch <n...@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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to