Re: abort-time portal cleanup

On Tue, Sep 24, 2019 at 5:31 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> After this cleanup, I think we don't need At(Sub)Abort_Portals in
> AbortOutOfAnyTransaction() for the states TBLOCK_(SUB)ABORT and
> friends. This is because AbortTransaction itself would have zapped the
> portal.

Not if the ROLLBACK itself failed - in that case, the portal would
have been active at the time, and thus not subject to removal. And, as
the existing comments in xact.c state, that's exactly why that
function call is there.

> 2. You seem to forgot removing AtCleanup_Portals() from portal.h

Oops. Fixed in the attached version.

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

> 4.
> + * If the status is PORTAL_ACTIVE, then we must be
> executing a command
> + * that uses multiple transactions internally. In that
> case, the
> + * command in question must be one that does not
> internally rely on
> + * any transaction-lifetime resources, because they
> would disappear
> + * in the upcoming transaction-wide cleanup.
> */
> if (portal->status == PORTAL_ACTIVE)
>
> I am not able to understand how we can reach with the portal state as
> 'active' for a multi-transaction command. It seems wherever we mark
> portal as active, we don't relinquish the control unless its state is
> changed. Can you share some example where this can happen?

Yeah -- a plpgsql function or procedure that does "ROLLBACK;"
internally. The calling code doesn't relinquish control, but it does
reach AbortTransaction().

If you want to see it happen, just put an elog() inside that block and
run make -C src/pl/plpgsql check.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company