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?
