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?


> 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,
                        /* Make sure we get developers' attention about this */

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?

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

Reply via email to