Re: Assert failure on running a completed portal again

On Sun, 08 Dec 2024 14:25:53 -0500
Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> I wrote:
> > After looking at this further, I think this whole "run_once"
> > business is badly designed, worse documented, and redundant
> > (as well as buggy). It can be replaced with three self-contained
> > lines in ExecutePlan, as per the attached.
>
> > (Obviously, the API changes in this wouldn't do for the back
> > branches. But we could leave those arguments in place as
> > unused dummies.)
>
> Here's a cut-down version that I think would be OK to use in the
> back branches.

I confirmed both versions of the pach fixed the issue using pgproto.
Also, I feel your fix makes the codes easier to understand at least for me.

I have a few minor comment:

* moving in the specified direction.
*
* Runs to completion if numberTuples is 0
- *
- * 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?

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.

Regards,
Yugo Nagata

--
Yugo NAGATA <nagata(at)sraoss(dot)co(dot)jp>