On Sat, Jan 02, 2016 at 01:46:07PM -0500, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > On Thu, Sep 03, 2015 at 11:04:11PM -0400, Tom Lane wrote:
> >> *************** AtSubAbort_Portals(SubTransactionId mySu
>
> >> --- 909,966 ----
> >> {
> >> Portal portal = hentry->portal;
> >>
> >> + /* Was it created in this subtransaction? */
> >> if (portal->createSubid != mySubid)
> >> + {
> >> + /* No, but maybe it was used in this subtransaction? */
> >> + if (portal->activeSubid == mySubid)
> >> + {
> > ...
> >> + if (portal->status == PORTAL_ACTIVE)
> >> + MarkPortalFailed(portal);
>
> > Do you have a test case that reaches this particular MarkPortalFailed() call?
> > My attempts stumbled over the fact that, before we reach here, each of the
> > three MarkPortalActive() callers will have already called MarkPortalFailed()
> > in its PG_CATCH block. ("make check" does not reach this call.)
>
> Offhand I think that's just belt-and-suspenders-too coding. As you say,
> we'd typically have failed active portals already before getting here.
> But the responsibility of this routine is to *guarantee* that no broken
> portals remain active, so I'd not want to remove this check.
>
> Do you have a particular reason for asking?
After reading the log message for and comments added in commit c5454f9, I
understood that the commit changed PostgreSQL to fail portals that did not
fail before. That is to say, queries that formerly completed without error
would now elicit errors. I was looking into what sorts of queries it affected
in this way. If the new MarkPortalFailed() is indeed dead code, then the
commit affects no query that way. The meat of the commit is then the
ResourceOwnerNewParent() call, which lets queries ERROR cleanly instead of
seeing assertion failures or undefined behavior.
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.
