On Wed, Oct 9, 2019 at 6:56 PM Robert Haas <robertmh...@gmail.com> wrote: > > 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... > >
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 <t...@sss.pgh.pa.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