On Sun, Jan 03, 2016 at 12:35:56AM -0500, Noah Misch wrote:
> On Sat, Jan 02, 2016 at 07:22:13PM -0500, Tom Lane wrote:
> > Noah Misch <noah@leadboat.com> writes:
> > > I am inclined to add an Assert(portal->status != PORTAL_ACTIVE) to emphasize
> > > that this is backup only. MarkPortalActive() callers remain responsible for
> > > updating the status to something else before relinquishing control.
> >
> > No, I do not think that would be an improvement. There is no contract
> > saying that this must be done earlier, IMO.
>
> Indeed, nobody wrote a contract. The assertion would record what has been the
> sole standing practice for eleven years (since commit a393fbf9). It would
> prompt discussion if a proposed patch would depart from that practice, and
> that is a good thing. Also, every addition of dead code should label that
> code to aid future readers.
Here's the patch I have in mind. AtAbort_Portals() has an older
MarkPortalFailed() call, and the story is somewhat different there per its new
comment. That call is plausible to reach with no bug involved, but
MarkPortalFailed() would then be the wrong thing.
nm