Re: generic plans and "initial" pruning

On Sat, Mar 7, 2026 at 6:54 PM Amit Langote <amitlangote09@gmail.com> 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