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

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)

‎src/backend/commands/trigger.c

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -5057,21 +5057,6 @@ AfterTriggerBeginQuery(void)
50575057
}
50585058

50595059

5060-
/* ----------
5061-
* AfterTriggerAbortQuery()
5062-
*
5063-
* Called by standard_ExecutorEnd() if the query execution was aborted due to
5064-
* the plan becoming invalid during initialization.
5065-
* ----------
5066-
*/
5067-
void
5068-
AfterTriggerAbortQuery(void)
5069-
{
5070-
/* Revert the actions of AfterTriggerBeginQuery(). */
5071-
afterTriggers.query_depth--;
5072-
}
5073-
5074-
50755060
/* ----------
50765061
* AfterTriggerEndQuery()
50775062
*

‎src/backend/executor/README

Lines changed: 3 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -285,28 +285,6 @@ are typically reset to empty once per tuple. Per-tuple contexts are usually
285285
associated with ExprContexts, and commonly each PlanState node has its own
286286
ExprContext to evaluate its qual and targetlist expressions in.
287287

288-
Relation Locking
289-
----------------
290-
291-
When the executor initializes a plan tree for execution, it doesn't lock
292-
non-index relations if the plan tree is freshly generated and not derived
293-
from a CachedPlan. This is because such locks have already been established
294-
during the query's parsing, rewriting, and planning phases. However, with a
295-
cached plan tree, some relations may remain unlocked. The function
296-
AcquireExecutorLocks() only locks unprunable relations in the plan, deferring
297-
the locking of prunable ones to executor initialization. This avoids
298-
unnecessary locking of relations that will be pruned during "initial" runtime
299-
pruning in ExecDoInitialPruning().
300-
301-
This approach creates a window where a cached plan tree with child tables
302-
could become outdated if another backend modifies these tables before
303-
ExecDoInitialPruning() locks them. As a result, the executor has the added duty
304-
to verify the plan tree's validity whenever it locks a child table after
305-
doing initial pruning. This validation is done by checking the CachedPlan.is_valid
306-
flag. If the plan tree is outdated (is_valid = false), the executor stops
307-
further initialization, cleans up anything in EState that would have been
308-
allocated up to that point, and retries execution after recreating the
309-
invalid plan in the CachedPlan. See ExecutorStartCachedPlan().
310288

311289
Query Processing Control Flow
312290
-----------------------------
@@ -315,13 +293,11 @@ This is a sketch of control flow for full query processing:
315293

316294
CreateQueryDesc
317295

318-
ExecutorStart or ExecutorStartCachedPlan
296+
ExecutorStart
319297
CreateExecutorState
320298
creates per-query context
321-
switch to per-query context to run ExecDoInitialPruning and ExecInitNode
299+
switch to per-query context to run ExecInitNode
322300
AfterTriggerBeginQuery
323-
ExecDoInitialPruning
324-
does initial pruning and locks surviving partitions if needed
325301
ExecInitNode --- recursively scans plan tree
326302
ExecInitNode
327303
recurse into subsidiary nodes
@@ -345,12 +321,7 @@ This is a sketch of control flow for full query processing:
345321

346322
FreeQueryDesc
347323

348-
As mentioned in the "Relation Locking" section, if the plan tree is found to
349-
be stale after locking partitions in ExecDoInitialPruning(), the control is
350-
immediately returned to ExecutorStartCachedPlan(), which will create a new plan
351-
tree and perform the steps starting from CreateExecutorState() again.
352-
353-
Per above comments, it's not really critical for ExecEndPlan to free any
324+
Per above comments, it's not really critical for ExecEndNode to free any
354325
memory; it'll all go away in FreeExecutorState anyway. However, we do need to
355326
be careful to close relations, drop buffer pins, etc, so we do need to scan
356327
the plan state tree to find these sorts of resources.

0 commit comments

Comments
 (0)