Re: Assert failure on running a completed portal again

Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Dec 10, 2024 at 2:53 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'm thinking about something like this:

> That seems pretty good, although the last sentence seems like it might
> be a little too alarming. Maybe add "although we know of no specific
> problem" or something like that.

OK, I'll tone it down a bit.

> The portal mechanism is a hot mess, IMHO, and needs some serious
> redesign, or at least cleanup. For example, the fact that
> portal->cleanup is always either PortalCleanup or NULL is an
> "interesting" design choice.

While I'm not here to defend that code particularly, that part
doesn't seem totally insane to me.  The intent clearly was to
support more than one form of cleanup.  Maybe after 25 years
we can conclude that we'll never need more than one form, but
I'm not going to fault whoever wrote it for not having an
operational crystal ball.

> MarkPortalDone() and MarkPortalFailed()
> looked like they do the same amount of cleanup but, ha ha, no they
> don't, because the one and only cleanup hook peeks behind the curtain
> to figure out who is calling it.

If you're talking about

     * Shut down executor, if still running.  We skip this during error abort,
     * since other mechanisms will take care of releasing executor resources,
     * and we can't be sure that ExecutorEnd itself wouldn't fail.

it's hardly the fault of the Portal logic that ExecutorEnd is unsafe
to call during abort.  (ExecutorEnd shares that property with a
boatload of other code, too.)

Anyway, if you feel like rewriting that stuff, step right up.
My feeling about it is that the law of conservation of cruft
will prevent a replacement from being all that much cleaner,
but maybe I'm wrong.

			regards, tom lane