Re: abort-time portal cleanup

On Tue, Oct 8, 2019 at 2:10 PM Andres Freund <andres(at)anarazel(dot)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