Re: Assert failure on running a completed portal again - Mailing list pgsql-hackers

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.

            regards, tom lane

diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index f8113d116d..3ccdbde112 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -75,14 +75,13 @@ static void InitPlan(QueryDesc *queryDesc, int eflags);
 static void CheckValidRowMarkRel(Relation rel, RowMarkType markType);
 static void ExecPostprocessPlan(EState *estate);
 static void ExecEndPlan(PlanState *planstate, EState *estate);
-static void ExecutePlan(EState *estate, PlanState *planstate,
+static void ExecutePlan(QueryDesc *queryDesc,
                         bool use_parallel_mode,
                         CmdType operation,
                         bool sendTuples,
                         uint64 numberTuples,
                         ScanDirection direction,
-                        DestReceiver *dest,
-                        bool execute_once);
+                        DestReceiver *dest);
 static bool ExecCheckOneRelPerms(RTEPermissionInfo *perminfo);
 static bool ExecCheckPermissionsModified(Oid relOid, Oid userid,
                                          Bitmapset *modifiedCols,
@@ -283,6 +282,9 @@ standard_ExecutorStart(QueryDesc *queryDesc, int eflags)
  *        retrieved tuples, not for instance to those inserted/updated/deleted
  *        by a ModifyTable plan node.
  *
+ *        execute_once is ignored, and is present only to avoid an API break
+ *        in stable branches.
+ *
  *        There is no return value, but output tuples (if any) are sent to
  *        the destination receiver specified in the QueryDesc; and the number
  *        of tuples processed at the top level can be found in
@@ -357,21 +359,13 @@ standard_ExecutorRun(QueryDesc *queryDesc,
      * run plan
      */
     if (!ScanDirectionIsNoMovement(direction))
-    {
-        if (execute_once && queryDesc->already_executed)
-            elog(ERROR, "can't re-execute query flagged for single execution");
-        queryDesc->already_executed = true;
-
-        ExecutePlan(estate,
-                    queryDesc->planstate,
+        ExecutePlan(queryDesc,
                     queryDesc->plannedstmt->parallelModeNeeded,
                     operation,
                     sendTuples,
                     count,
                     direction,
-                    dest,
-                    execute_once);
-    }
+                    dest);

     /*
      * Update es_total_processed to keep track of the number of tuples
@@ -1600,22 +1594,19 @@ ExecCloseRangeTableRelations(EState *estate)
  *        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
  * ----------------------------------------------------------------
  */
 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;

@@ -1630,11 +1621,13 @@ ExecutePlan(EState *estate,
     estate->es_direction = direction;

     /*
-     * If the plan might potentially be executed multiple times, we must force
-     * it to run without parallelism, because we might exit early.
+     * Parallel mode only supports complete execution of a plan.  If we've
+     * already partially executed it, or if the caller asks us to exit early,
+     * we must force the plan to run without parallelism.
      */
-    if (!execute_once)
+    if (queryDesc->already_executed || numberTuples != 0)
         use_parallel_mode = false;
+    queryDesc->already_executed = true;

     estate->es_use_parallel_mode = use_parallel_mode;
     if (use_parallel_mode)
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index a750dc800b..1887b11982 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -1278,7 +1278,7 @@ exec_simple_query(const char *query_string)
         (void) PortalRun(portal,
                          FETCH_ALL,
                          true,    /* always top level */
-                         true,
+                         true,    /* ignored */
                          receiver,
                          receiver,
                          &qc);
@@ -2255,7 +2255,7 @@ exec_execute_message(const char *portal_name, long max_rows)
     completed = PortalRun(portal,
                           max_rows,
                           true, /* always top level */
-                          !execute_is_fetch && max_rows == FETCH_ALL,
+                          true, /* ignored */
                           receiver,
                           receiver,
                           &qc);
diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c
index 0c45fcf318..0a72006823 100644
--- a/src/backend/tcop/pquery.c
+++ b/src/backend/tcop/pquery.c
@@ -670,6 +670,8 @@ PortalSetResultFormat(Portal portal, int nFormats, int16 *formats)
  * isTopLevel: true if query is being executed at backend "top level"
  * (that is, directly from a client command message)
  *
+ * run_once: ignored, present only to avoid an API break in stable branches.
+ *
  * dest: where to send output of primary (canSetTag) query
  *
  * altdest: where to send output of non-primary queries
@@ -714,10 +716,6 @@ PortalRun(Portal portal, long count, bool isTopLevel, bool run_once,
      */
     MarkPortalActive(portal);

-    /* Set run_once flag.  Shouldn't be clear if previously set. */
-    Assert(!portal->run_once || run_once);
-    portal->run_once = run_once;
-
     /*
      * Set up global portal context pointers.
      *
@@ -922,7 +920,7 @@ PortalRunSelect(Portal portal,
         {
             PushActiveSnapshot(queryDesc->snapshot);
             ExecutorRun(queryDesc, direction, (uint64) count,
-                        portal->run_once);
+                        false);
             nprocessed = queryDesc->estate->es_processed;
             PopActiveSnapshot();
         }
@@ -962,7 +960,7 @@ PortalRunSelect(Portal portal,
         {
             PushActiveSnapshot(queryDesc->snapshot);
             ExecutorRun(queryDesc, direction, (uint64) count,
-                        portal->run_once);
+                        false);
             nprocessed = queryDesc->estate->es_processed;
             PopActiveSnapshot();
         }
@@ -1406,9 +1404,6 @@ PortalRunFetch(Portal portal,
      */
     MarkPortalActive(portal);

-    /* If supporting FETCH, portal can't be run-once. */
-    Assert(!portal->run_once);
-
     /*
      * Set up global portal context pointers.
      */
diff --git a/src/include/executor/execdesc.h b/src/include/executor/execdesc.h
index 0a7274e26c..79e8b63111 100644
--- a/src/include/executor/execdesc.h
+++ b/src/include/executor/execdesc.h
@@ -48,7 +48,7 @@ typedef struct QueryDesc
     EState       *estate;            /* executor's query-wide state */
     PlanState  *planstate;        /* tree of per-plan-node state */

-    /* This field is set by ExecutorRun */
+    /* This field is set by ExecutePlan */
     bool        already_executed;    /* true if previously executed */

     /* This is always set NULL by the core system, but plugins can change it */
diff --git a/src/include/utils/portal.h b/src/include/utils/portal.h
index 29f49829f2..6c63515074 100644
--- a/src/include/utils/portal.h
+++ b/src/include/utils/portal.h
@@ -145,7 +145,7 @@ typedef struct PortalData
     /* Features/options */
     PortalStrategy strategy;    /* see above */
     int            cursorOptions;    /* DECLARE CURSOR option bits */
-    bool        run_once;        /* portal will only be run once */
+    bool        run_once;        /* unused */

     /* Status data */
     PortalStatus status;        /* see above */