Re: Assert failure on running a completed portal again

On Tue, Dec 10, 2024 at 2:53 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > Thanks for the research, and +1 if you feel like adding more commentary.
>
> I'm thinking about something like this:
>
> diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
> index 13f3fcdaee..7ebb17fc2e 100644
> --- a/src/backend/executor/execMain.c
> +++ b/src/backend/executor/execMain.c
> @@ -348,7 +348,15 @@ standard_ExecutorRun(QueryDesc *queryDesc,
> dest->rStartup(dest, operation, queryDesc->tupDesc);
>
> /*
> - * run plan
> + * Run plan, unless direction is NoMovement.
> + *
> + * Note: pquery.c selects NoMovement if a prior call already reached
> + * end-of-data in the user-specified fetch direction. This is important
> + * because various parts of the executor can misbehave if called again
> + * after reporting EOF. For example, heapam.c would actually restart a
> + * heapscan and return all its data afresh. There is also reason to be
> + * concerned about whether parallel query mode would operate properly if a
> + * fresh execution request arrives after completing the query.
> */
> if (!ScanDirectionIsNoMovement(direction))
> ExecutePlan(queryDesc,

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.

> It's slightly annoying that the executor is depending on the Portal
> machinery to guarantee that it won't do something wrong. However,
> that was true in the previous formulation that relied on
> Portal.run_once, too. I don't see another way that wouldn't
> amount to duplicating the Portal-level state in the executor,
> and I don't think it's worth doing that.

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. 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. This code looks like it was written
by somebody who didn't initially understand the requirements and then
kept hitting it with a sledgehammer until it stopped complaining
without ever stopping to reconsider the basic design.

--
Robert Haas
EDB: http://www.enterprisedb.com