Skip to content

Commit 3eea7a0

Browse files
tglsfdctglsfdc
committed
Simplify executor's determination of whether to use parallelism.
Our parallel-mode code only works when we are executing a query in full, so ExecutePlan must disable parallel mode when it is asked to do partial execution. The previous logic for this involved passing down a flag (variously named execute_once or run_once) from callers of ExecutorRun or PortalRun. This is overcomplicated, and unsurprisingly some of the callers didn't get it right, since it requires keeping state that not all of them have handy; not to mention that the requirements for it were undocumented. That led to assertion failures in some corner cases. The only state we really need for this is the existing QueryDesc.already_executed flag, so let's just put all the responsibility in ExecutePlan. (It could have been done in ExecutorRun too, leading to a slightly shorter patch -- but if there's ever more than one caller of ExecutePlan, it seems better to have this logic in the subroutine than the callers.) This makes those ExecutorRun/PortalRun parameters unnecessary. In master it seems okay to just remove them, returning the API for those functions to what it was before parallelism. Such an API break is clearly not okay in stable branches, but for them we can just leave the parameters in place after documenting that they do nothing. Per report from Yugo Nagata, who also reviewed and tested this patch. Back-patch to all supported branches. Discussion: https://postgr.es/m/20241206062549.710dc01cf91224809dd6c0e1@sraoss.co.jp
1 parent 4d82750 commit 3eea7a0

File tree

19 files changed

+49
-73
lines changed

19 files changed

+49
-73
lines changed

‎contrib/auto_explain/auto_explain.c

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ static ExecutorEnd_hook_type prev_ExecutorEnd = NULL;
7979
static void explain_ExecutorStart(QueryDesc *queryDesc, int eflags);
8080
static void explain_ExecutorRun(QueryDesc *queryDesc,
8181
ScanDirection direction,
82-
uint64 count, bool execute_once);
82+
uint64 count);
8383
static void explain_ExecutorFinish(QueryDesc *queryDesc);
8484
static void explain_ExecutorEnd(QueryDesc *queryDesc);
8585

@@ -321,15 +321,15 @@ explain_ExecutorStart(QueryDesc *queryDesc, int eflags)
321321
*/
322322
static void
323323
explain_ExecutorRun(QueryDesc *queryDesc, ScanDirection direction,
324-
uint64 count, bool execute_once)
324+
uint64 count)
325325
{
326326
nesting_level++;
327327
PG_TRY();
328328
{
329329
if (prev_ExecutorRun)
330-
prev_ExecutorRun(queryDesc, direction, count, execute_once);
330+
prev_ExecutorRun(queryDesc, direction, count);
331331
else
332-
standard_ExecutorRun(queryDesc, direction, count, execute_once);
332+
standard_ExecutorRun(queryDesc, direction, count);
333333
}
334334
PG_FINALLY();
335335
{

‎contrib/pg_stat_statements/pg_stat_statements.c

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -335,7 +335,7 @@ static PlannedStmt *pgss_planner(Query *parse,
335335
static void pgss_ExecutorStart(QueryDesc *queryDesc, int eflags);
336336
static void pgss_ExecutorRun(QueryDesc *queryDesc,
337337
ScanDirection direction,
338-
uint64 count, bool execute_once);
338+
uint64 count);
339339
static void pgss_ExecutorFinish(QueryDesc *queryDesc);
340340
static void pgss_ExecutorEnd(QueryDesc *queryDesc);
341341
static void pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
@@ -1021,16 +1021,15 @@ pgss_ExecutorStart(QueryDesc *queryDesc, int eflags)
10211021
* ExecutorRun hook: all we need do is track nesting depth
10221022
*/
10231023
static void
1024-
pgss_ExecutorRun(QueryDesc *queryDesc, ScanDirection direction, uint64 count,
1025-
bool execute_once)
1024+
pgss_ExecutorRun(QueryDesc *queryDesc, ScanDirection direction, uint64 count)
10261025
{
10271026
nesting_level++;
10281027
PG_TRY();
10291028
{
10301029
if (prev_ExecutorRun)
1031-
prev_ExecutorRun(queryDesc, direction, count, execute_once);
1030+
prev_ExecutorRun(queryDesc, direction, count);
10321031
else
1033-
standard_ExecutorRun(queryDesc, direction, count, execute_once);
1032+
standard_ExecutorRun(queryDesc, direction, count);
10341033
}
10351034
PG_FINALLY();
10361035
{

‎src/backend/commands/copyto.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -880,7 +880,7 @@ DoCopyTo(CopyToState cstate)
880880
else
881881
{
882882
/* run the plan --- the dest receiver will send tuples */
883-
ExecutorRun(cstate->queryDesc, ForwardScanDirection, 0, true);
883+
ExecutorRun(cstate->queryDesc, ForwardScanDirection, 0);
884884
processed = ((DR_copy *) cstate->queryDesc->dest)->processed;
885885
}
886886

‎src/backend/commands/createas.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -340,7 +340,7 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
340340
ExecutorStart(queryDesc, GetIntoRelEFlags(into));
341341

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

345345
/* save the rowcount if we're given a qc to fill */
346346
if (qc)

‎src/backend/commands/explain.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -719,7 +719,7 @@ ExplainOnePlan(PlannedStmt *plannedstmt, IntoClause *into, ExplainState *es,
719719
dir = ForwardScanDirection;
720720

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

724724
/* run cleanup too */
725725
ExecutorFinish(queryDesc);

‎src/backend/commands/extension.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -912,7 +912,7 @@ execute_sql_string(const char *sql, const char *filename)
912912
dest, NULL, NULL, 0);
913913

914914
ExecutorStart(qdesc, 0);
915-
ExecutorRun(qdesc, ForwardScanDirection, 0, true);
915+
ExecutorRun(qdesc, ForwardScanDirection, 0);
916916
ExecutorFinish(qdesc);
917917
ExecutorEnd(qdesc);
918918

‎src/backend/commands/matview.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -446,7 +446,7 @@ refresh_matview_datafill(DestReceiver *dest, Query *query,
446446
ExecutorStart(queryDesc, 0);
447447

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

451451
processed = queryDesc->estate->es_processed;
452452

‎src/backend/commands/portalcmds.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -427,7 +427,7 @@ PersistHoldablePortal(Portal portal)
427427
NULL);
428428

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

432432
queryDesc->dest->rDestroy(queryDesc->dest);
433433
queryDesc->dest = NULL;

‎src/backend/commands/prepare.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -252,7 +252,7 @@ ExecuteQuery(ParseState *pstate,
252252
*/
253253
PortalStart(portal, paramLI, eflags, GetActiveSnapshot());
254254

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

257257
PortalDrop(portal, false);
258258

‎src/backend/executor/execMain.c

Lines changed: 22 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -77,14 +77,12 @@ static void InitPlan(QueryDesc *queryDesc, int eflags);
7777
static void CheckValidRowMarkRel(Relation rel, RowMarkType markType);
7878
static void ExecPostprocessPlan(EState *estate);
7979
static void ExecEndPlan(PlanState *planstate, EState *estate);
80-
static void ExecutePlan(EState *estate, PlanState *planstate,
81-
bool use_parallel_mode,
80+
static void ExecutePlan(QueryDesc *queryDesc,
8281
CmdType operation,
8382
bool sendTuples,
8483
uint64 numberTuples,
8584
ScanDirection direction,
86-
DestReceiver *dest,
87-
bool execute_once);
85+
DestReceiver *dest);
8886
static bool ExecCheckOneRelPerms(RTEPermissionInfo *perminfo);
8987
static bool ExecCheckPermissionsModified(Oid relOid, Oid userid,
9088
Bitmapset *modifiedCols,
@@ -294,18 +292,17 @@ standard_ExecutorStart(QueryDesc *queryDesc, int eflags)
294292
*/
295293
void
296294
ExecutorRun(QueryDesc *queryDesc,
297-
ScanDirection direction, uint64 count,
298-
bool execute_once)
295+
ScanDirection direction, uint64 count)
299296
{
300297
if (ExecutorRun_hook)
301-
(*ExecutorRun_hook) (queryDesc, direction, count, execute_once);
298+
(*ExecutorRun_hook) (queryDesc, direction, count);
302299
else
303-
standard_ExecutorRun(queryDesc, direction, count, execute_once);
300+
standard_ExecutorRun(queryDesc, direction, count);
304301
}
305302

306303
void
307304
standard_ExecutorRun(QueryDesc *queryDesc,
308-
ScanDirection direction, uint64 count, bool execute_once)
305+
ScanDirection direction, uint64 count)
309306
{
310307
EState *estate;
311308
CmdType operation;
@@ -354,21 +351,12 @@ standard_ExecutorRun(QueryDesc *queryDesc,
354351
* run plan
355352
*/
356353
if (!ScanDirectionIsNoMovement(direction))
357-
{
358-
if (execute_once && queryDesc->already_executed)
359-
elog(ERROR, "can't re-execute query flagged for single execution");
360-
queryDesc->already_executed = true;
361-
362-
ExecutePlan(estate,
363-
queryDesc->planstate,
364-
queryDesc->plannedstmt->parallelModeNeeded,
354+
ExecutePlan(queryDesc,
365355
operation,
366356
sendTuples,
367357
count,
368358
direction,
369-
dest,
370-
execute_once);
371-
}
359+
dest);
372360

373361
/*
374362
* Update es_total_processed to keep track of the number of tuples
@@ -1601,22 +1589,19 @@ ExecCloseRangeTableRelations(EState *estate)
16011589
* moving in the specified direction.
16021590
*
16031591
* Runs to completion if numberTuples is 0
1604-
*
1605-
* Note: the ctid attribute is a 'junk' attribute that is removed before the
1606-
* user can see it
16071592
* ----------------------------------------------------------------
16081593
*/
16091594
static void
1610-
ExecutePlan(EState *estate,
1611-
PlanState *planstate,
1612-
bool use_parallel_mode,
1595+
ExecutePlan(QueryDesc *queryDesc,
16131596
CmdType operation,
16141597
bool sendTuples,
16151598
uint64 numberTuples,
16161599
ScanDirection direction,
1617-
DestReceiver *dest,
1618-
bool execute_once)
1600+
DestReceiver *dest)
16191601
{
1602+
EState *estate = queryDesc->estate;
1603+
PlanState *planstate = queryDesc->planstate;
1604+
bool use_parallel_mode;
16201605
TupleTableSlot *slot;
16211606
uint64 current_tuple_count;
16221607

@@ -1631,11 +1616,17 @@ ExecutePlan(EState *estate,
16311616
estate->es_direction = direction;
16321617

16331618
/*
1634-
* If the plan might potentially be executed multiple times, we must force
1635-
* it to run without parallelism, because we might exit early.
1619+
* Set up parallel mode if appropriate.
1620+
*
1621+
* Parallel mode only supports complete execution of a plan. If we've
1622+
* already partially executed it, or if the caller asks us to exit early,
1623+
* we must force the plan to run without parallelism.
16361624
*/
1637-
if (!execute_once)
1625+
if (queryDesc->already_executed || numberTuples != 0)
16381626
use_parallel_mode = false;
1627+
else
1628+
use_parallel_mode = queryDesc->plannedstmt->parallelModeNeeded;
1629+
queryDesc->already_executed = true;
16391630

16401631
estate->es_use_parallel_mode = use_parallel_mode;
16411632
if (use_parallel_mode)

0 commit comments

Comments
 (0)