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, 
>                         /* 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:

Reply via email to