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