Re: abort-time portal cleanup

On Wed, Oct 9, 2019 at 6:56 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> 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...
> >

Yeah, this is the reason, I mentioned it.

> 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.
>

+1.

> 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?
>

If read the commit message of commit 7981c34279 [1] which introduced
this, then we might get some clue. It is quite possible that we need
same handling for PORTAL_NEW, PORTAL_DEFINED, etc. but it seems we
just hit the problem mentioned in commit 7981c34279 for PORTAL_READY
state. I think as per commit, if we don't mark it failed, then with
auto_explain things can go wrong.

[1] -
commit 7981c34279fbddc254cfccb9a2eec4b35e692a12
Author: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Date: Thu Feb 18 03:06:46 2010 +0000

Force READY portals into FAILED state when a transaction or
subtransaction is aborted, if they were created within the failed
xact. This prevents ExecutorEnd from being run on them, which is a
good idea because they may contain references to tables or other
objects that no longer exist. In particular this is hazardous when
auto_explain is active, but it's really rather surprising that nobody
has seen an issue with this before. I'm back-patching this to 8.4,
since that's the first version that contains auto_explain or an
ExecutorEnd hook, but I wonder whether we shouldn't back-patch
further.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com