Skip to content

Commit 1722d5e

Browse files
amitlanamitlan
committed
Revert "Don't lock partitions pruned by initial pruning"
As pointed out by Tom Lane, the patch introduced fragile and invasive design around plan invalidation handling when locking of prunable partitions was deferred from plancache.c to the executor. In particular, it violated assumptions about CachedPlan immutability and altered executor APIs in ways that are difficult to justify given the added complexity and overhead. This also removes the firstResultRels field added to PlannedStmt in commit 28317de, which was intended to support deferred locking of certain ModifyTable result relations. Reported-by: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/605328.1747710381@sss.pgh.pa.us
1 parent f3622b6 commit 1722d5e

File tree

33 files changed

+89
-672
lines changed

33 files changed

+89
-672
lines changed

‎contrib/auto_explain/auto_explain.c

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ static ExecutorRun_hook_type prev_ExecutorRun = NULL;
8181
static ExecutorFinish_hook_type prev_ExecutorFinish = NULL;
8282
static ExecutorEnd_hook_type prev_ExecutorEnd = NULL;
8383

84-
static bool explain_ExecutorStart(QueryDesc *queryDesc, int eflags);
84+
static void explain_ExecutorStart(QueryDesc *queryDesc, int eflags);
8585
static void explain_ExecutorRun(QueryDesc *queryDesc,
8686
ScanDirection direction,
8787
uint64 count);
@@ -261,11 +261,9 @@ _PG_init(void)
261261
/*
262262
* ExecutorStart hook: start up logging if needed
263263
*/
264-
static bool
264+
static void
265265
explain_ExecutorStart(QueryDesc *queryDesc, int eflags)
266266
{
267-
bool plan_valid;
268-
269267
/*
270268
* At the beginning of each top-level statement, decide whether we'll
271269
* sample this statement. If nested-statement explaining is enabled,
@@ -301,13 +299,9 @@ explain_ExecutorStart(QueryDesc *queryDesc, int eflags)
301299
}
302300

303301
if (prev_ExecutorStart)
304-
plan_valid = prev_ExecutorStart(queryDesc, eflags);
302+
prev_ExecutorStart(queryDesc, eflags);
305303
else
306-
plan_valid = standard_ExecutorStart(queryDesc, eflags);
307-
308-
/* The plan may have become invalid during standard_ExecutorStart() */
309-
if (!plan_valid)
310-
return false;
304+
standard_ExecutorStart(queryDesc, eflags);
311305

312306
if (auto_explain_enabled())
313307
{
@@ -325,8 +319,6 @@ explain_ExecutorStart(QueryDesc *queryDesc, int eflags)
325319
MemoryContextSwitchTo(oldcxt);
326320
}
327321
}
328-
329-
return true;
330322
}
331323

332324
/*

‎contrib/pg_stat_statements/pg_stat_statements.c

Lines changed: 4 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -335,7 +335,7 @@ static PlannedStmt *pgss_planner(Query *parse,
335335
const char *query_string,
336336
int cursorOptions,
337337
ParamListInfo boundParams);
338-
static bool pgss_ExecutorStart(QueryDesc *queryDesc, int eflags);
338+
static void pgss_ExecutorStart(QueryDesc *queryDesc, int eflags);
339339
static void pgss_ExecutorRun(QueryDesc *queryDesc,
340340
ScanDirection direction,
341341
uint64 count);
@@ -989,19 +989,13 @@ pgss_planner(Query *parse,
989989
/*
990990
* ExecutorStart hook: start up tracking if needed
991991
*/
992-
static bool
992+
static void
993993
pgss_ExecutorStart(QueryDesc *queryDesc, int eflags)
994994
{
995-
bool plan_valid;
996-
997995
if (prev_ExecutorStart)
998-
plan_valid = prev_ExecutorStart(queryDesc, eflags);
996+
prev_ExecutorStart(queryDesc, eflags);
999997
else
1000-
plan_valid = standard_ExecutorStart(queryDesc, eflags);
1001-
1002-
/* The plan may have become invalid during standard_ExecutorStart() */
1003-
if (!plan_valid)
1004-
return false;
998+
standard_ExecutorStart(queryDesc, eflags);
1005999

10061000
/*
10071001
* If query has queryId zero, don't track it. This prevents double
@@ -1024,8 +1018,6 @@ pgss_ExecutorStart(QueryDesc *queryDesc, int eflags)
10241018
MemoryContextSwitchTo(oldcxt);
10251019
}
10261020
}
1027-
1028-
return true;
10291021
}
10301022

10311023
/*

‎doc/src/sgml/release-18.sgml

Lines changed: 0 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -588,27 +588,6 @@ Improve the locking performance of queries that access many relations (Tomas Von
588588
</para>
589589
</listitem>
590590

591-
<!--
592-
Author: Amit Langote <amitlan@postgresql.org>
593-
2025-01-30 [bb3ec16e1] Move PartitionPruneInfo out of plan nodes into PlannedSt
594-
Author: Amit Langote <amitlan@postgresql.org>
595-
2025-01-31 [d47cbf474] Perform runtime initial pruning outside ExecInitNode()
596-
Author: Amit Langote <amitlan@postgresql.org>
597-
2025-02-07 [cbc127917] Track unpruned relids to avoid processing pruned relatio
598-
Author: Amit Langote <amitlan@postgresql.org>
599-
2025-02-20 [525392d57] Don't lock partitions pruned by initial pruning
600-
-->
601-
602-
<listitem>
603-
<para>
604-
Avoid the locking of pruned partitions during execution (Amit Langote)
605-
<ulink url="&commit_baseurl;bb3ec16e1">&sect;</ulink>
606-
<ulink url="&commit_baseurl;d47cbf474">&sect;</ulink>
607-
<ulink url="&commit_baseurl;cbc127917">&sect;</ulink>
608-
<ulink url="&commit_baseurl;525392d57">&sect;</ulink>
609-
</para>
610-
</listitem>
611-
612591
<!--
613592
Author: David Rowley <drowley@postgresql.org>
614593
2024-08-20 [adf97c156] Speed up Hash Join by making ExprStates support hashing

‎src/backend/commands/copyto.c

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -835,7 +835,7 @@ BeginCopyTo(ParseState *pstate,
835835
((DR_copy *) dest)->cstate = cstate;
836836

837837
/* Create a QueryDesc requesting no output */
838-
cstate->queryDesc = CreateQueryDesc(plan, NULL, pstate->p_sourcetext,
838+
cstate->queryDesc = CreateQueryDesc(plan, pstate->p_sourcetext,
839839
GetActiveSnapshot(),
840840
InvalidSnapshot,
841841
dest, NULL, NULL, 0);
@@ -845,8 +845,7 @@ BeginCopyTo(ParseState *pstate,
845845
*
846846
* ExecutorStart computes a result tupdesc for us
847847
*/
848-
if (!ExecutorStart(cstate->queryDesc, 0))
849-
elog(ERROR, "ExecutorStart() failed unexpectedly");
848+
ExecutorStart(cstate->queryDesc, 0);
850849

851850
tupDesc = cstate->queryDesc->tupDesc;
852851
}

‎src/backend/commands/createas.c

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -334,13 +334,12 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
334334
UpdateActiveSnapshotCommandId();
335335

336336
/* Create a QueryDesc, redirecting output to our tuple receiver */
337-
queryDesc = CreateQueryDesc(plan, NULL, pstate->p_sourcetext,
337+
queryDesc = CreateQueryDesc(plan, pstate->p_sourcetext,
338338
GetActiveSnapshot(), InvalidSnapshot,
339339
dest, params, queryEnv, 0);
340340

341341
/* call ExecutorStart to prepare the plan for execution */
342-
if (!ExecutorStart(queryDesc, GetIntoRelEFlags(into)))
343-
elog(ERROR, "ExecutorStart() failed unexpectedly");
342+
ExecutorStart(queryDesc, GetIntoRelEFlags(into));
344343

345344
/* run the plan to completion */
346345
ExecutorRun(queryDesc, ForwardScanDirection, 0);

‎src/backend/commands/explain.c

Lines changed: 5 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -369,8 +369,7 @@ standard_ExplainOneQuery(Query *query, int cursorOptions,
369369
}
370370

371371
/* run it (if needed) and produce output */
372-
ExplainOnePlan(plan, NULL, NULL, -1, into, es, queryString, params,
373-
queryEnv,
372+
ExplainOnePlan(plan, into, es, queryString, params, queryEnv,
374373
&planduration, (es->buffers ? &bufusage : NULL),
375374
es->memory ? &mem_counters : NULL);
376375
}
@@ -492,9 +491,7 @@ ExplainOneUtility(Node *utilityStmt, IntoClause *into, ExplainState *es,
492491
* to call it.
493492
*/
494493
void
495-
ExplainOnePlan(PlannedStmt *plannedstmt, CachedPlan *cplan,
496-
CachedPlanSource *plansource, int query_index,
497-
IntoClause *into, ExplainState *es,
494+
ExplainOnePlan(PlannedStmt *plannedstmt, IntoClause *into, ExplainState *es,
498495
const char *queryString, ParamListInfo params,
499496
QueryEnvironment *queryEnv, const instr_time *planduration,
500497
const BufferUsage *bufusage,
@@ -550,7 +547,7 @@ ExplainOnePlan(PlannedStmt *plannedstmt, CachedPlan *cplan,
550547
dest = None_Receiver;
551548

552549
/* Create a QueryDesc for the query */
553-
queryDesc = CreateQueryDesc(plannedstmt, cplan, queryString,
550+
queryDesc = CreateQueryDesc(plannedstmt, queryString,
554551
GetActiveSnapshot(), InvalidSnapshot,
555552
dest, params, queryEnv, instrument_option);
556553

@@ -564,17 +561,8 @@ ExplainOnePlan(PlannedStmt *plannedstmt, CachedPlan *cplan,
564561
if (into)
565562
eflags |= GetIntoRelEFlags(into);
566563

567-
/* Prepare the plan for execution. */
568-
if (queryDesc->cplan)
569-
{
570-
ExecutorStartCachedPlan(queryDesc, eflags, plansource, query_index);
571-
Assert(queryDesc->planstate);
572-
}
573-
else
574-
{
575-
if (!ExecutorStart(queryDesc, eflags))
576-
elog(ERROR, "ExecutorStart() failed unexpectedly");
577-
}
564+
/* call ExecutorStart to prepare the plan for execution */
565+
ExecutorStart(queryDesc, eflags);
578566

579567
/* Execute the plan for statistics if asked for */
580568
if (es->analyze)

‎src/backend/commands/extension.c

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -993,13 +993,11 @@ execute_sql_string(const char *sql, const char *filename)
993993
QueryDesc *qdesc;
994994

995995
qdesc = CreateQueryDesc(stmt,
996-
NULL,
997996
sql,
998997
GetActiveSnapshot(), NULL,
999998
dest, NULL, NULL, 0);
1000999

1001-
if (!ExecutorStart(qdesc, 0))
1002-
elog(ERROR, "ExecutorStart() failed unexpectedly");
1000+
ExecutorStart(qdesc, 0);
10031001
ExecutorRun(qdesc, ForwardScanDirection, 0);
10041002
ExecutorFinish(qdesc);
10051003
ExecutorEnd(qdesc);

‎src/backend/commands/matview.c

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -438,13 +438,12 @@ refresh_matview_datafill(DestReceiver *dest, Query *query,
438438
UpdateActiveSnapshotCommandId();
439439

440440
/* Create a QueryDesc, redirecting output to our tuple receiver */
441-
queryDesc = CreateQueryDesc(plan, NULL, queryString,
441+
queryDesc = CreateQueryDesc(plan, queryString,
442442
GetActiveSnapshot(), InvalidSnapshot,
443443
dest, NULL, NULL, 0);
444444

445445
/* call ExecutorStart to prepare the plan for execution */
446-
if (!ExecutorStart(queryDesc, 0))
447-
elog(ERROR, "ExecutorStart() failed unexpectedly");
446+
ExecutorStart(queryDesc, 0);
448447

449448
/* run the plan */
450449
ExecutorRun(queryDesc, ForwardScanDirection, 0);

‎src/backend/commands/portalcmds.c

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,6 @@ PerformCursorOpen(ParseState *pstate, DeclareCursorStmt *cstmt, ParamListInfo pa
117117
queryString,
118118
CMDTAG_SELECT, /* cursor's query is always a SELECT */
119119
list_make1(plan),
120-
NULL,
121120
NULL);
122121

123122
/*----------

‎src/backend/commands/prepare.c

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -205,8 +205,7 @@ ExecuteQuery(ParseState *pstate,
205205
query_string,
206206
entry->plansource->commandTag,
207207
plan_list,
208-
cplan,
209-
entry->plansource);
208+
cplan);
210209

211210
/*
212211
* For CREATE TABLE ... AS EXECUTE, we must verify that the prepared
@@ -586,7 +585,6 @@ ExplainExecuteQuery(ExecuteStmt *execstmt, IntoClause *into, ExplainState *es,
586585
MemoryContextCounters mem_counters;
587586
MemoryContext planner_ctx = NULL;
588587
MemoryContext saved_ctx = NULL;
589-
int query_index = 0;
590588

591589
if (es->memory)
592590
{
@@ -659,8 +657,7 @@ ExplainExecuteQuery(ExecuteStmt *execstmt, IntoClause *into, ExplainState *es,
659657
PlannedStmt *pstmt = lfirst_node(PlannedStmt, p);
660658

661659
if (pstmt->commandType != CMD_UTILITY)
662-
ExplainOnePlan(pstmt, cplan, entry->plansource, query_index,
663-
into, es, query_string, paramLI, pstate->p_queryEnv,
660+
ExplainOnePlan(pstmt, into, es, query_string, paramLI, pstate->p_queryEnv,
664661
&planduration, (es->buffers ? &bufusage : NULL),
665662
es->memory ? &mem_counters : NULL);
666663
else
@@ -671,8 +668,6 @@ ExplainExecuteQuery(ExecuteStmt *execstmt, IntoClause *into, ExplainState *es,
671668
/* Separate plans with an appropriate separator */
672669
if (lnext(plan_list, p) != NULL)
673670
ExplainSeparatePlans(es);
674-
675-
query_index++;
676671
}
677672

678673
if (estate)

0 commit comments

Comments
 (0)