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

I wrote:
> Don't like this much.  I'm not sure I believe any part of this
> approach to deciding whether the portal is "run once".  In any
> case, a proper fix for this needs to include actually documenting
> what that field means and does.

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.)

            regards, tom lane

diff --git a/contrib/auto_explain/auto_explain.c b/contrib/auto_explain/auto_explain.c
index 623a674f99..f2eaa8e494 100644
--- a/contrib/auto_explain/auto_explain.c
+++ b/contrib/auto_explain/auto_explain.c
@@ -79,7 +79,7 @@ static ExecutorEnd_hook_type prev_ExecutorEnd = NULL;
 static void explain_ExecutorStart(QueryDesc *queryDesc, int eflags);
 static void explain_ExecutorRun(QueryDesc *queryDesc,
                                 ScanDirection direction,
-                                uint64 count, bool execute_once);
+                                uint64 count);
 static void explain_ExecutorFinish(QueryDesc *queryDesc);
 static void explain_ExecutorEnd(QueryDesc *queryDesc);

@@ -321,15 +321,15 @@ explain_ExecutorStart(QueryDesc *queryDesc, int eflags)
  */
 static void
 explain_ExecutorRun(QueryDesc *queryDesc, ScanDirection direction,
-                    uint64 count, bool execute_once)
+                    uint64 count)
 {
     nesting_level++;
     PG_TRY();
     {
         if (prev_ExecutorRun)
-            prev_ExecutorRun(queryDesc, direction, count, execute_once);
+            prev_ExecutorRun(queryDesc, direction, count);
         else
-            standard_ExecutorRun(queryDesc, direction, count, execute_once);
+            standard_ExecutorRun(queryDesc, direction, count);
     }
     PG_FINALLY();
     {
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 49c657b3e0..602cae54ff 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -335,7 +335,7 @@ static PlannedStmt *pgss_planner(Query *parse,
 static void pgss_ExecutorStart(QueryDesc *queryDesc, int eflags);
 static void pgss_ExecutorRun(QueryDesc *queryDesc,
                              ScanDirection direction,
-                             uint64 count, bool execute_once);
+                             uint64 count);
 static void pgss_ExecutorFinish(QueryDesc *queryDesc);
 static void pgss_ExecutorEnd(QueryDesc *queryDesc);
 static void pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
@@ -1021,16 +1021,15 @@ pgss_ExecutorStart(QueryDesc *queryDesc, int eflags)
  * ExecutorRun hook: all we need do is track nesting depth
  */
 static void
-pgss_ExecutorRun(QueryDesc *queryDesc, ScanDirection direction, uint64 count,
-                 bool execute_once)
+pgss_ExecutorRun(QueryDesc *queryDesc, ScanDirection direction, uint64 count)
 {
     nesting_level++;
     PG_TRY();
     {
         if (prev_ExecutorRun)
-            prev_ExecutorRun(queryDesc, direction, count, execute_once);
+            prev_ExecutorRun(queryDesc, direction, count);
         else
-            standard_ExecutorRun(queryDesc, direction, count, execute_once);
+            standard_ExecutorRun(queryDesc, direction, count);
     }
     PG_FINALLY();
     {
diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index f55e6d9675..161a0f8b0a 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -880,7 +880,7 @@ DoCopyTo(CopyToState cstate)
     else
     {
         /* run the plan --- the dest receiver will send tuples */
-        ExecutorRun(cstate->queryDesc, ForwardScanDirection, 0, true);
+        ExecutorRun(cstate->queryDesc, ForwardScanDirection, 0);
         processed = ((DR_copy *) cstate->queryDesc->dest)->processed;
     }

diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index 5c92e48a56..466376af9b 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -340,7 +340,7 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
         ExecutorStart(queryDesc, GetIntoRelEFlags(into));

         /* run the plan to completion */
-        ExecutorRun(queryDesc, ForwardScanDirection, 0, true);
+        ExecutorRun(queryDesc, ForwardScanDirection, 0);

         /* save the rowcount if we're given a qc to fill */
         if (qc)
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index a3f1d53d7a..3078f5c1a3 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -719,7 +719,7 @@ ExplainOnePlan(PlannedStmt *plannedstmt, IntoClause *into, ExplainState *es,
             dir = ForwardScanDirection;

         /* run the plan */
-        ExecutorRun(queryDesc, dir, 0, true);
+        ExecutorRun(queryDesc, dir, 0);

         /* run cleanup too */
         ExecutorFinish(queryDesc);
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index af6bd8ff42..e683c520a8 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -912,7 +912,7 @@ execute_sql_string(const char *sql, const char *filename)
                                         dest, NULL, NULL, 0);

                 ExecutorStart(qdesc, 0);
-                ExecutorRun(qdesc, ForwardScanDirection, 0, true);
+                ExecutorRun(qdesc, ForwardScanDirection, 0);
                 ExecutorFinish(qdesc);
                 ExecutorEnd(qdesc);

diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index 010097873d..694da8291e 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -446,7 +446,7 @@ refresh_matview_datafill(DestReceiver *dest, Query *query,
     ExecutorStart(queryDesc, 0);

     /* run the plan */
-    ExecutorRun(queryDesc, ForwardScanDirection, 0, true);
+    ExecutorRun(queryDesc, ForwardScanDirection, 0);

     processed = queryDesc->estate->es_processed;

diff --git a/src/backend/commands/portalcmds.c b/src/backend/commands/portalcmds.c
index ac52ca25e9..dc68264242 100644
--- a/src/backend/commands/portalcmds.c
+++ b/src/backend/commands/portalcmds.c
@@ -427,7 +427,7 @@ PersistHoldablePortal(Portal portal)
                                         NULL);

         /* Fetch the result set into the tuplestore */
-        ExecutorRun(queryDesc, direction, 0, false);
+        ExecutorRun(queryDesc, direction, 0);

         queryDesc->dest->rDestroy(queryDesc->dest);
         queryDesc->dest = NULL;
diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c
index a93f970a29..39a71c1de2 100644
--- a/src/backend/commands/prepare.c
+++ b/src/backend/commands/prepare.c
@@ -252,7 +252,7 @@ ExecuteQuery(ParseState *pstate,
      */
     PortalStart(portal, paramLI, eflags, GetActiveSnapshot());

-    (void) PortalRun(portal, count, false, true, dest, dest, qc);
+    (void) PortalRun(portal, count, false, dest, dest, qc);

     PortalDrop(portal, false);

diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 5ca856fd27..312f622f32 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -77,14 +77,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,
@@ -294,18 +293,17 @@ standard_ExecutorStart(QueryDesc *queryDesc, int eflags)
  */
 void
 ExecutorRun(QueryDesc *queryDesc,
-            ScanDirection direction, uint64 count,
-            bool execute_once)
+            ScanDirection direction, uint64 count)
 {
     if (ExecutorRun_hook)
-        (*ExecutorRun_hook) (queryDesc, direction, count, execute_once);
+        (*ExecutorRun_hook) (queryDesc, direction, count);
     else
-        standard_ExecutorRun(queryDesc, direction, count, execute_once);
+        standard_ExecutorRun(queryDesc, direction, count);
 }

 void
 standard_ExecutorRun(QueryDesc *queryDesc,
-                     ScanDirection direction, uint64 count, bool execute_once)
+                     ScanDirection direction, uint64 count)
 {
     EState       *estate;
     CmdType        operation;
@@ -354,21 +352,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
@@ -1601,22 +1591,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;

@@ -1631,11 +1618,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/executor/execParallel.c b/src/backend/executor/execParallel.c
index bfb3419efb..846ec727de 100644
--- a/src/backend/executor/execParallel.c
+++ b/src/backend/executor/execParallel.c
@@ -1471,8 +1471,7 @@ ParallelQueryMain(dsm_segment *seg, shm_toc *toc)
      */
     ExecutorRun(queryDesc,
                 ForwardScanDirection,
-                fpes->tuples_needed < 0 ? (int64) 0 : fpes->tuples_needed,
-                true);
+                fpes->tuples_needed < 0 ? (int64) 0 : fpes->tuples_needed);

     /* Shut down the executor */
     ExecutorFinish(queryDesc);
diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c
index 8d1fda2ddc..3b2f78b219 100644
--- a/src/backend/executor/functions.c
+++ b/src/backend/executor/functions.c
@@ -894,7 +894,7 @@ postquel_getnext(execution_state *es, SQLFunctionCachePtr fcache)
         /* Run regular commands to completion unless lazyEval */
         uint64        count = (es->lazyEval) ? 1 : 0;

-        ExecutorRun(es->qd, ForwardScanDirection, count, !fcache->returnsSet || !es->lazyEval);
+        ExecutorRun(es->qd, ForwardScanDirection, count);

         /*
          * If we requested run to completion OR there was no tuple returned,
diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 2fb2e73604..c1d8fd08c6 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -2929,7 +2929,7 @@ _SPI_pquery(QueryDesc *queryDesc, bool fire_triggers, uint64 tcount)

     ExecutorStart(queryDesc, eflags);

-    ExecutorRun(queryDesc, ForwardScanDirection, tcount, true);
+    ExecutorRun(queryDesc, ForwardScanDirection, tcount);

     _SPI_current->processed = queryDesc->estate->es_processed;

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 42af768045..e0a603f42b 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -1283,7 +1283,6 @@ exec_simple_query(const char *query_string)
         (void) PortalRun(portal,
                          FETCH_ALL,
                          true,    /* always top level */
-                         true,
                          receiver,
                          receiver,
                          &qc);
@@ -2260,7 +2259,6 @@ 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,
                           receiver,
                           receiver,
                           &qc);
diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c
index 0c45fcf318..89d704df8d 100644
--- a/src/backend/tcop/pquery.c
+++ b/src/backend/tcop/pquery.c
@@ -157,7 +157,7 @@ ProcessQuery(PlannedStmt *plan,
     /*
      * Run the plan to completion.
      */
-    ExecutorRun(queryDesc, ForwardScanDirection, 0, true);
+    ExecutorRun(queryDesc, ForwardScanDirection, 0);

     /*
      * Build command completion status data, if caller wants one.
@@ -681,7 +681,7 @@ PortalSetResultFormat(Portal portal, int nFormats, int16 *formats)
  * suspended due to exhaustion of the count parameter.
  */
 bool
-PortalRun(Portal portal, long count, bool isTopLevel, bool run_once,
+PortalRun(Portal portal, long count, bool isTopLevel,
           DestReceiver *dest, DestReceiver *altdest,
           QueryCompletion *qc)
 {
@@ -714,10 +714,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.
      *
@@ -921,8 +917,7 @@ PortalRunSelect(Portal portal,
         else
         {
             PushActiveSnapshot(queryDesc->snapshot);
-            ExecutorRun(queryDesc, direction, (uint64) count,
-                        portal->run_once);
+            ExecutorRun(queryDesc, direction, (uint64) count);
             nprocessed = queryDesc->estate->es_processed;
             PopActiveSnapshot();
         }
@@ -961,8 +956,7 @@ PortalRunSelect(Portal portal,
         else
         {
             PushActiveSnapshot(queryDesc->snapshot);
-            ExecutorRun(queryDesc, direction, (uint64) count,
-                        portal->run_once);
+            ExecutorRun(queryDesc, direction, (uint64) count);
             nprocessed = queryDesc->estate->es_processed;
             PopActiveSnapshot();
         }
@@ -1406,9 +1400,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/executor/executor.h b/src/include/executor/executor.h
index 69c3ebff00..892cf055cd 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -78,8 +78,7 @@ extern PGDLLIMPORT ExecutorStart_hook_type ExecutorStart_hook;
 /* Hook for plugins to get control in ExecutorRun() */
 typedef void (*ExecutorRun_hook_type) (QueryDesc *queryDesc,
                                        ScanDirection direction,
-                                       uint64 count,
-                                       bool execute_once);
+                                       uint64 count);
 extern PGDLLIMPORT ExecutorRun_hook_type ExecutorRun_hook;

 /* Hook for plugins to get control in ExecutorFinish() */
@@ -200,9 +199,9 @@ ExecGetJunkAttribute(TupleTableSlot *slot, AttrNumber attno, bool *isNull)
 extern void ExecutorStart(QueryDesc *queryDesc, int eflags);
 extern void standard_ExecutorStart(QueryDesc *queryDesc, int eflags);
 extern void ExecutorRun(QueryDesc *queryDesc,
-                        ScanDirection direction, uint64 count, bool execute_once);
+                        ScanDirection direction, uint64 count);
 extern void standard_ExecutorRun(QueryDesc *queryDesc,
-                                 ScanDirection direction, uint64 count, bool execute_once);
+                                 ScanDirection direction, uint64 count);
 extern void ExecutorFinish(QueryDesc *queryDesc);
 extern void standard_ExecutorFinish(QueryDesc *queryDesc);
 extern void ExecutorEnd(QueryDesc *queryDesc);
diff --git a/src/include/tcop/pquery.h b/src/include/tcop/pquery.h
index 073fb323bc..37ba3160e5 100644
--- a/src/include/tcop/pquery.h
+++ b/src/include/tcop/pquery.h
@@ -36,7 +36,7 @@ extern void PortalSetResultFormat(Portal portal, int nFormats,
                                   int16 *formats);

 extern bool PortalRun(Portal portal, long count, bool isTopLevel,
-                      bool run_once, DestReceiver *dest, DestReceiver *altdest,
+                      DestReceiver *dest, DestReceiver *altdest,
                       QueryCompletion *qc);

 extern uint64 PortalRunFetch(Portal portal,
diff --git a/src/include/utils/portal.h b/src/include/utils/portal.h
index 29f49829f2..afe605a8ed 100644
--- a/src/include/utils/portal.h
+++ b/src/include/utils/portal.h
@@ -145,7 +145,6 @@ typedef struct PortalData
     /* Features/options */
     PortalStrategy strategy;    /* see above */
     int            cursorOptions;    /* DECLARE CURSOR option bits */
-    bool        run_once;        /* portal will only be run once */

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