On Tue, Oct 8, 2019 at 2:10 PM Andres Freund <and...@anarazel.de> wrote: > On 2019-10-07 12:14:52 -0400, Robert Haas wrote: > > > - if (portal->status == PORTAL_READY) > > > - MarkPortalFailed(portal); > > > > > > Why it is safe to remove this check? It has been explained in commit > > > 7981c342 why we need that check. I don't see any explanation in email > > > or patch which justifies this code removal. Is it because you removed > > > PortalCleanup? If so, that is still called from PortalDrop? > > > > All MarkPortalFailed() does is change the status to PORTAL_FAILED and > > call the cleanup hook. PortalDrop() calls the cleanup hook, and we > > don't need to change the status if we're removing it completely. > > Note that currently PortalCleanup() behaves differently depending on > whether the portal is set to failed or not...
Urk, yeah, I forgot about that. I think that's a wretched hack that somebody ought to untangle at some point, but maybe for purposes of this patch it makes more sense to just put the MarkPortalFailed call back. It's unclear to me why there's a special case here specifically for PORTAL_READY. Like, why is PORTAL_NEW or PORTAL_DEFINED or PORTAL_DONE any different? It seems like if we're aborting the transaction, we should not be calling ExecutorFinish()/ExecutorEnd() for anything. We could achieve that result by just nulling out the cleanup hook unconditionally instead of having this complicated dance where we mark ready portals failed, which calls the cleanup hook, which decides not to do anything because the portal has been marked failed. It'd be great if there were a few more comments in this file explaining what the thinking behind all this was. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
