Re: Assert failure on running a completed portal again

On Sun, 08 Dec 2024 14:25:53 -0500
Tom Lane <tgl@sss.pgh.pa.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@sraoss.co.jp>