From 8dc44320c7d4b20f50200d7b21c98e4058b8d6d7 Mon Sep 17 00:00:00 2001 From: Amit Langote Date: Sat, 4 Apr 2026 18:38:34 +0900 Subject: [PATCH v11 1/4] Move execution lock acquisition out of GetCachedPlan() GetCachedPlan() previously acquired execution locks on all plan relations as part of cached plan validation. Move this responsibility to callers, making GetCachedPlan() return a valid plan without holding execution locks. Add AcquireExecutorLocks() as the caller-facing function: it locks all relations in the plan, checks that the plan is still valid afterward, and returns false if it was invalidated so the caller can retry with a fresh plan. For portal-backed callers, add PortalLockCachedPlan() in pquery.c which wraps the lock-check-retry loop and handles the case where replanning changes the portal strategy. Store the CachedPlanSource pointer in PortalData so retry can call GetCachedPlan() without the caller threading it through. Adjust all non-portal GetCachedPlan() callers (SPI, EXPLAIN EXECUTE, SQL functions) to call AcquireExecutorLocks() explicitly after fetching the plan. No behavioral change. This separates plan retrieval from execution setup, allowing a later commit to substitute pruning-aware locking for eligible plans. --- src/backend/commands/portalcmds.c | 1 + src/backend/commands/prepare.c | 14 +++++- src/backend/executor/functions.c | 14 ++++-- src/backend/executor/spi.c | 22 ++++++++-- src/backend/tcop/postgres.c | 2 + src/backend/tcop/pquery.c | 68 ++++++++++++++++++++++++++++- src/backend/utils/cache/plancache.c | 44 ++++++++++++++----- src/backend/utils/mmgr/portalmem.c | 7 +++ src/include/utils/plancache.h | 1 + src/include/utils/portal.h | 3 ++ 10 files changed, 155 insertions(+), 21 deletions(-) diff --git a/src/backend/commands/portalcmds.c b/src/backend/commands/portalcmds.c index 01efac3319e..cf5deec4943 100644 --- a/src/backend/commands/portalcmds.c +++ b/src/backend/commands/portalcmds.c @@ -118,6 +118,7 @@ PerformCursorOpen(ParseState *pstate, DeclareCursorStmt *cstmt, ParamListInfo pa queryString, CMDTAG_SELECT, /* cursor's query is always a SELECT */ list_make1(plan), + NULL, NULL); /*---------- diff --git a/src/backend/commands/prepare.c b/src/backend/commands/prepare.c index 876aad2100a..03d7a98fc58 100644 --- a/src/backend/commands/prepare.c +++ b/src/backend/commands/prepare.c @@ -207,6 +207,7 @@ ExecuteQuery(ParseState *pstate, query_string, entry->plansource->commandTag, plan_list, + entry->plansource, cplan); /* @@ -632,8 +633,17 @@ ExplainExecuteQuery(ExecuteStmt *execstmt, IntoClause *into, ExplainState *es, } /* Replan if needed, and acquire a transient refcount */ - cplan = GetCachedPlan(entry->plansource, paramLI, - CurrentResourceOwner, pstate->p_queryEnv); + for (;;) + { + cplan = GetCachedPlan(entry->plansource, paramLI, + CurrentResourceOwner, + pstate->p_queryEnv); + plan_list = cplan->stmt_list; + + if (AcquireExecutorLocks(cplan)) + break; + ReleaseCachedPlan(cplan, CurrentResourceOwner); + } INSTR_TIME_SET_CURRENT(planduration); INSTR_TIME_SUBTRACT(planduration, planstart); diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c index 88109348817..2afb814a435 100644 --- a/src/backend/executor/functions.c +++ b/src/backend/executor/functions.c @@ -654,6 +654,7 @@ static bool init_execution_state(SQLFunctionCachePtr fcache) { CachedPlanSource *plansource; + CachedPlan *cplan; execution_state *preves = NULL; execution_state *lasttages = NULL; int nstmts; @@ -696,10 +697,15 @@ init_execution_state(SQLFunctionCachePtr fcache) * CurrentResourceOwner will be the same when ShutdownSQLFunction runs.) */ fcache->cowner = CurrentResourceOwner; - fcache->cplan = GetCachedPlan(plansource, - fcache->paramLI, - fcache->cowner, - NULL); + for (;;) + { + cplan = GetCachedPlan(plansource, fcache->paramLI, + fcache->cowner, NULL); + if (AcquireExecutorLocks(cplan)) + break; + ReleaseCachedPlan(cplan, fcache->cowner); + } + fcache->cplan = cplan; /* * If necessary, make esarray[] bigger to hold the needed state. diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c index 52f3b11301c..268cd10bde8 100644 --- a/src/backend/executor/spi.c +++ b/src/backend/executor/spi.c @@ -1686,6 +1686,7 @@ SPI_cursor_open_internal(const char *name, SPIPlanPtr plan, query_string, plansource->commandTag, stmt_list, + plansource, cplan); /* @@ -2106,6 +2107,16 @@ SPI_plan_get_cached_plan(SPIPlanPtr plan) _SPI_current->queryEnv); Assert(cplan == plansource->gplan); + if (!AcquireExecutorLocks(cplan)) + { + /* Plan invalidated during locking; get a fresh one. */ + ReleaseCachedPlan(cplan, + plan->saved ? CurrentResourceOwner : NULL); + cplan = GetCachedPlan(plansource, NULL, + plan->saved ? CurrentResourceOwner : NULL, + _SPI_current->queryEnv); + } + /* Pop the error context stack */ error_context_stack = spierrcontext.previous; @@ -2574,9 +2585,14 @@ _SPI_execute_plan(SPIPlanPtr plan, const SPIExecuteOptions *options, * Replan if needed, and increment plan refcount. If it's a saved * plan, the refcount must be backed by the plan_owner. */ - cplan = GetCachedPlan(plansource, options->params, - plan_owner, _SPI_current->queryEnv); - + for (;;) + { + cplan = GetCachedPlan(plansource, options->params, + plan_owner, _SPI_current->queryEnv); + if (AcquireExecutorLocks(cplan)) + break; + ReleaseCachedPlan(cplan, plan_owner); + } stmt_list = cplan->stmt_list; /* diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 10be60011ad..aaebefcdf7a 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -1231,6 +1231,7 @@ exec_simple_query(const char *query_string) query_string, commandTag, plantree_list, + NULL, NULL); /* @@ -2030,6 +2031,7 @@ exec_bind_message(StringInfo input_message) query_string, psrc->commandTag, cplan->stmt_list, + psrc, cplan); /* Portal is defined, set the plan ID based on its contents. */ diff --git a/src/backend/tcop/pquery.c b/src/backend/tcop/pquery.c index d8fc75d0bb9..1b22515d56e 100644 --- a/src/backend/tcop/pquery.c +++ b/src/backend/tcop/pquery.c @@ -59,6 +59,7 @@ static uint64 DoPortalRunFetch(Portal portal, long count, DestReceiver *dest); static void DoPortalRewind(Portal portal); +static bool PortalLockCachedPlan(Portal portal); /* @@ -462,6 +463,8 @@ PortalStart(Portal portal, ParamListInfo params, */ portal->strategy = ChoosePortalStrategy(portal->stmts); +restart: + /* * Fire her up according to the strategy */ @@ -487,6 +490,11 @@ PortalStart(Portal portal, ParamListInfo params, /* * Create QueryDesc in portal's context; for the moment, set * the destination to DestNone. + * + * If the portal is backed by a cached plan, acquire execution + * locks via PortalLockCachedPlan(). If the plan is + * invalidated during locking, it replans and may change the + * portal strategy, requiring us to restart PortalStart(). */ queryDesc = CreateQueryDesc(linitial_node(PlannedStmt, portal->stmts), portal->sourceText, @@ -496,6 +504,14 @@ PortalStart(Portal portal, ParamListInfo params, params, portal->queryEnv, 0); + if (portal->cplan) + { + if (PortalLockCachedPlan(portal)) + { + PopActiveSnapshot(); + goto restart; + } + } /* * If it's a scrollable cursor, executor needs to support @@ -534,6 +550,11 @@ PortalStart(Portal portal, ParamListInfo params, case PORTAL_ONE_RETURNING: case PORTAL_ONE_MOD_WITH: + if (portal->cplan) + { + if (PortalLockCachedPlan(portal)) + goto restart; + } /* * We don't start the executor until we are told to run the @@ -577,7 +598,20 @@ PortalStart(Portal portal, ParamListInfo params, break; case PORTAL_MULTI_QUERY: - /* Need do nothing now */ + + /* + * GetCachedPlan() no longer acquires execution locks, so we + * must do it here. Multi-statement plans always use + * conservative locking (all partitions locked); pruning-aware + * locking is not feasible because PortalRunMulti() executes + * statements sequentially with CCI between them. + */ + if (portal->cplan) + { + if (PortalLockCachedPlan(portal)) + goto restart; + } + portal->tupDesc = NULL; break; } @@ -1785,3 +1819,35 @@ EnsurePortalSnapshotExists(void) /* PushActiveSnapshotWithLevel might have copied the snapshot */ portal->portalSnapshot = GetActiveSnapshot(); } + +/* + * PortalLockCachedPlan + * Acquire execution locks for a cached-plan-backed portal, + * retrying with a fresh plan if the current one is invalidated. + * + * Returns true if replanning changed portal->strategy, meaning the + * caller must redispatch. Returns false once locks are held. + */ +static bool +PortalLockCachedPlan(Portal portal) +{ + PortalStrategy start_strategy = portal->strategy; + + if (AcquireExecutorLocks(portal->cplan)) + return false; + + /* Replan. Locks will be taken freshly. */ + ReleaseCachedPlan(portal->cplan, portal->resowner); + portal->cplan = NULL; + portal->stmts = NIL; + portal->cplan = GetCachedPlan(portal->plansource, + portal->portalParams, + portal->resowner, + portal->queryEnv); + portal->stmts = portal->cplan->stmt_list; + portal->strategy = ChoosePortalStrategy(portal->stmts); + if (portal->strategy != start_strategy) + return true; + + return false; +} diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c index 698e7c1aa22..f7fe366859c 100644 --- a/src/backend/utils/cache/plancache.c +++ b/src/backend/utils/cache/plancache.c @@ -100,7 +100,7 @@ static bool choose_custom_plan(CachedPlanSource *plansource, ParamListInfo boundParams); static double cached_plan_cost(CachedPlan *plan, bool include_planner); static Query *QueryListGetPrimaryStmt(List *stmts); -static void AcquireExecutorLocks(List *stmt_list, bool acquire); +static void AcquireExecutorLocksInt(List *stmt_list, bool acquire); static void AcquirePlannerLocks(List *stmt_list, bool acquire); static void ScanQueryForLocks(Query *parsetree, bool acquire); static bool ScanQueryWalker(Node *node, bool *acquire); @@ -945,8 +945,9 @@ RevalidateCachedQuery(CachedPlanSource *plansource, * Caller must have already called RevalidateCachedQuery to verify that the * querytree is up to date. * - * On a "true" return, we have acquired the locks needed to run the plan. - * (We must do this for the "true" result to be race-condition-free.) + * On a "true" return, the generic plan may be reused as a valid cached + * plan. Any execution-time setup, including lock acquisition, is the + * caller's responsibility. */ static bool CheckCachedPlan(CachedPlanSource *plansource) @@ -983,8 +984,6 @@ CheckCachedPlan(CachedPlanSource *plansource) */ Assert(plan->refcount > 0); - AcquireExecutorLocks(plan->stmt_list, true); - /* * If plan was transient, check to see if TransactionXmin has * advanced, and if so invalidate it. @@ -1003,9 +1002,6 @@ CheckCachedPlan(CachedPlanSource *plansource) /* Successfully revalidated and locked the query. */ return true; } - - /* Oops, the race case happened. Release useless locks. */ - AcquireExecutorLocks(plan->stmt_list, false); } /* @@ -1282,8 +1278,11 @@ cached_plan_cost(CachedPlan *plan, bool include_planner) * plan or a custom plan for the given parameters: the caller does not know * which it will get. * - * On return, the plan is valid and we have sufficient locks to begin - * execution. + * On return, the plan is valid but no execution locks are held. + * The caller must call AcquireExecutorLocks() before executing. + * For freshly built plans (custom or new generic), the planner + * already holds the needed locks, so AcquireExecutorLocks() is + * redundant but harmless. * * On return, the refcount of the plan has been incremented; a later * ReleaseCachedPlan() call is expected. If "owner" is not NULL then @@ -1906,9 +1905,11 @@ QueryListGetPrimaryStmt(List *stmts) /* * AcquireExecutorLocks: acquire locks needed for execution of a cached plan; * or release them if acquire is false. + * + * This locks all relations in a given PlannedStmt's range table. */ static void -AcquireExecutorLocks(List *stmt_list, bool acquire) +AcquireExecutorLocksInt(List *stmt_list, bool acquire) { ListCell *lc1; @@ -1955,6 +1956,27 @@ AcquireExecutorLocks(List *stmt_list, bool acquire) } } +/* + * AcquireExecutorLocks + * Acquire execution locks on all relations in a cached plan. + * + * Returns true if the plan is still valid after locking. Returns + * false if the plan was invalidated while locks were being acquired, + * in which case the locks have been released and the caller should + * discard this plan and retry with a fresh one from GetCachedPlan(). + */ +bool +AcquireExecutorLocks(CachedPlan *cplan) +{ + AcquireExecutorLocksInt(cplan->stmt_list, true); + if (!cplan->is_valid) + { + AcquireExecutorLocksInt(cplan->stmt_list, false); + return false; + } + return true; +} + /* * AcquirePlannerLocks: acquire locks needed for planning of a querytree list; * or release them if acquire is false. diff --git a/src/backend/utils/mmgr/portalmem.c b/src/backend/utils/mmgr/portalmem.c index 493f9b0ee19..613f3be30b3 100644 --- a/src/backend/utils/mmgr/portalmem.c +++ b/src/backend/utils/mmgr/portalmem.c @@ -272,6 +272,10 @@ CreateNewPortal(void) * the passed plan trees have adequate lifetime. Typically this is done by * copying them into the portal's context. * + * If plansource is provided, it is the CachedPlanSource that produced + * cplan. PortalLockCachedPlan() uses it to fetch a fresh plan if the + * current one is invalidated during execution lock acquisition. + * * The caller is also responsible for ensuring that the passed prepStmtName * (if not NULL) and sourceText have adequate lifetime. * @@ -286,6 +290,7 @@ PortalDefineQuery(Portal portal, const char *sourceText, CommandTag commandTag, List *stmts, + CachedPlanSource *plansource, CachedPlan *cplan) { Assert(PortalIsValid(portal)); @@ -299,6 +304,7 @@ PortalDefineQuery(Portal portal, portal->commandTag = commandTag; SetQueryCompletion(&portal->qc, commandTag, 0); portal->stmts = stmts; + portal->plansource = plansource; portal->cplan = cplan; portal->status = PORTAL_DEFINED; } @@ -517,6 +523,7 @@ PortalDrop(Portal portal, bool isTopCommit) /* drop cached plan reference, if any */ PortalReleaseCachedPlan(portal); + portal->plansource = NULL; /* * If portal has a snapshot protecting its data, release that. This needs diff --git a/src/include/utils/plancache.h b/src/include/utils/plancache.h index 7a4a85c8038..e0fc403e717 100644 --- a/src/include/utils/plancache.h +++ b/src/include/utils/plancache.h @@ -241,6 +241,7 @@ extern CachedPlan *GetCachedPlan(CachedPlanSource *plansource, ParamListInfo boundParams, ResourceOwner owner, QueryEnvironment *queryEnv); +extern bool AcquireExecutorLocks(CachedPlan *cplan); extern void ReleaseCachedPlan(CachedPlan *plan, ResourceOwner owner); extern bool CachedPlanAllowsSimpleValidityCheck(CachedPlanSource *plansource, diff --git a/src/include/utils/portal.h b/src/include/utils/portal.h index a7bedb12c18..3af535362cd 100644 --- a/src/include/utils/portal.h +++ b/src/include/utils/portal.h @@ -137,6 +137,8 @@ typedef struct PortalData CommandTag commandTag; /* command tag for original query */ QueryCompletion qc; /* command completion data for executed query */ List *stmts; /* list of PlannedStmts */ + CachedPlanSource *plansource; /* CachedPlanSource, for replanning on + * invalidation */ CachedPlan *cplan; /* CachedPlan, if stmts are from one */ ParamListInfo portalParams; /* params to pass to query */ @@ -240,6 +242,7 @@ extern void PortalDefineQuery(Portal portal, const char *sourceText, CommandTag commandTag, List *stmts, + CachedPlanSource *plansource, CachedPlan *cplan); extern PlannedStmt *PortalGetPrimaryStmt(Portal portal); extern void PortalCreateHoldStore(Portal portal); -- 2.47.3