Re: Assert failure on running a completed portal again

Yugo NAGATA <nagata(at)sraoss(dot)co(dot)jp> writes:
> On Sun, 08 Dec 2024 14:25:53 -0500
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> - *
> - * Note: the ctid attribute is a 'junk' attribute that is removed before the
> - * user can see it
> * ----------------------------------------------------------------
> */

> This comment remove seems not related to the fix of ExecutePlan. Should this
> actually have been done by 8a5849b7ff24c?

Yeah, I'm sure it should have been removed a long time ago.
Presumably it's there because ctid was once an explicit part
of ExecutePlan's API; but that's surely not been so for a long
time, and I don't see that this sentence has any remaining
relevance where it is.

You could say that getting rid of that dead comment should be
a separate commit, but I thought that was a bit too pedantic.

> static void
> -ExecutePlan(EState *estate,
> - PlanState *planstate,
> +ExecutePlan(QueryDesc *queryDesc,
> bool use_parallel_mode,
> CmdType operation,
> bool sendTuples,
> uint64 numberTuples,
> ScanDirection direction,
> - DestReceiver *dest,
> - bool execute_once)
> + DestReceiver *dest)
> {
> + EState *estate = queryDesc->estate;
> + PlanState *planstate = queryDesc->planstate;
> TupleTableSlot *slot;
> uint64 current_tuple_count

> I guess planstate is removed due to the redundancy that this is included
> in queryDesc. If so, I think we can also remove use_parallel_mode for the
> same reason.

Certainly there's a bit of aesthetic judgment involved here. In
principle, now that we're passing down the QueryDesc, we could remove
the operation and dest arguments too. I chose not to do that on
the grounds that ExecutorRun is deciding what ExecutePlan should do,
and at least in principle it could decide to pass something else
for these arguments than what it passes today. But it's hard to
see how it would pass a different EState or PlanState tree than
what is in the QueryDesc, so I thought it was okay to remove those
arguments. Somebody else writing this patch might have done that
differently, but I think that's fine.

A different line of thought is that the reason we need this change
is that the already_executed flag is in the wrong place. If we
moved it into EState, which feels like a more natural place, we
could avoid refactoring ExecutePlan's API. But that seemed more
invasive in the big picture, and it'd involve more risk in the
back branches, so I didn't go that way.

Thanks for reviewing and testing the patch!

regards, tom lane