On Sat, Mar 7, 2026 at 6:54 PM Amit Langote <[email protected]> wrote: > Attached is v6 of the patch series. I've been working toward > committing this, so I wanted to lay out the ExecutorPrep() design and > the key trade-offs before doing so. > > When a cached generic plan references a partitioned table, > GetCachedPlan() locks all partitions upfront via > AcquireExecutorLocks(), even those that initial pruning will > eliminate. But initial partition pruning only runs later during > ExecutorStart(). Moving pruning earlier requires some executor setup > (range table, permissions, pruning state), and ExecutorPrep() is the > vehicle for that. Unlike the approach reverted in last May, this > keeps the CachedPlan itself unchanged -- all per-execution state flows > through a separate CachedPlanPrepData that the caller provides. > > The approach also keeps GetCachedPlan()'s interface > backward-compatible: the new CachedPlanPrepData argument is optional. > If a caller passes NULL, all partitions are locked as before and > nothing changes. This means existing callers and any new code that > calls GetCachedPlan() without caring about pruning-aware locking just > works. > > The risk is on the other side: if a caller does pass a > CachedPlanPrepData, GetCachedPlan() will lock only the surviving > partitions and populate prep_estates with the EStates that > ExecutorPrep() created. The caller then must make those EStates > available to ExecutorStart() -- via QueryDesc->estate, > portal->prep_estates, or the equivalent path for SPI and SQL > functions. If it fails to do so, ExecutorStart() will call > ExecutorPrep() again, which may compute different pruning results than > the original call, potentially expecting locks on relations that were > never acquired. The executor would then operate on relations it > doesn't hold locks on. > > So the contract is: if you opt in to pruning-aware locking by passing > CachedPlanPrepData, you must complete the pipeline by delivering the > prep EStates to the executor. In the current patch, all the call sites > that pass a CachedPlanPrepData (portals, SPI, EXECUTE, SQL functions, > EXPLAIN) do thread the EStates through correctly, and I've tried to > make the plumbing straightforward enough that it's hard to get wrong. > But it is a new invariant that didn't exist before, and a caller that > gets it wrong would fail silently rather than with an obvious error. > > To catch such violations, I've added a debug-only check in > standard_ExecutorStart() that fires when no prep EState was provided. > It iterates over the plan's rtable and verifies that every lockable > relation is actually locked. It should always be true if > AcquireExecutorLocks() locked everything, but would fail if > pruning-aware locking happened upstream and the caller dropped the > prep EState. The check is skipped in parallel workers, which acquire > relation locks lazily in ExecGetRangeTableRelation(). > > + if (queryDesc->estate == NULL) > + { > +#ifdef USE_ASSERT_CHECKING > + if (!IsParallelWorker()) > + { > + ListCell *lc; > + > + foreach(lc, queryDesc->plannedstmt->rtable) > + { > + RangeTblEntry *rte = lfirst_node(RangeTblEntry, lc); > + > + if (rte->rtekind == RTE_RELATION || > + (rte->rtekind == RTE_SUBQUERY && rte->relid != > InvalidOid)) > + Assert(CheckRelationOidLockedByMe(rte->relid, > + rte->rellockmode, > + true)); > + } > + } > +#endif > + queryDesc->estate = ExecutorPrep(queryDesc->plannedstmt, > + queryDesc->params, > + CurrentResourceOwner, > + true, > + eflags); > + } > +#ifdef USE_ASSERT_CHECKING > + else > + { > + /* > + * A prep EState was provided, meaning pruning-aware locking > + * should have locked at least the unpruned relations. > + */ > + if (!IsParallelWorker()) > + { > + int rtindex = -1; > + > + while ((rtindex = > bms_next_member(queryDesc->estate->es_unpruned_relids, > + rtindex)) >= 0) > + { > + RangeTblEntry *rte = exec_rt_fetch(rtindex, > queryDesc->estate); > + > + Assert(rte->rtekind == RTE_RELATION || > + (rte->rtekind == RTE_SUBQUERY && > + rte->relid != InvalidOid)); > + Assert(CheckRelationOidLockedByMe(rte->relid, > + rte->rellockmode, true)); > + } > + } > + } > +#endif > > So the invariant is: if no prep EState was provided, every relation in > the plan is locked; if one was provided, at least the unpruned > relations are locked. Both are checked in assert builds. > > I think this covers the main concerns, but I may be missing something. > If anyone sees a problem with this approach, I'd like to hear about > it.
Here's v7. Some plancache.c changes that I'd made were in the wrong patch in v6; this version puts them where they belong. -- Thanks, Amit Langote
v7-0003-Add-test-for-partition-lock-behavior-with-generic.patch
Description: Binary data
v7-0005-Make-SQL-function-executor-track-ExecutorPrep-sta.patch
Description: Binary data
v7-0002-Introduce-ExecutorPrep-and-refactor-executor-star.patch
Description: Binary data
v7-0004-Use-pruning-aware-locking-in-cached-plans.patch
Description: Binary data
v7-0006-Reuse-partition-pruning-results-in-parallel-worke.patch
Description: Binary data
v7-0001-Refactor-partition-pruning-initialization-for-cla.patch
Description: Binary data
