Re: GetCachedPlan() refactor: move execution lock acquisition out

On Wed, Apr 15, 2026 at 11:14 AM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Wed, Apr 15, 2026 at 1:08 AM Kirill Reshke <reshkekirill(at)gmail(dot)com> wrote:
> > > +static bool
> > > +LockCachedPlan(CachedPlan *cplan)
> > > +{
> > > + AcquireExecutorLocks(cplan->stmt_list, true);
> > > + if (!cplan->is_valid)
> > > + {
> > > + AcquireExecutorLocks(cplan->stmt_list, false);
> > > + return false;
> > > + }
> > > + return true;
> > > +}
> >
> > simply `return cplan->is_valid ` would be more Postgres-y here, isnt it?
>
> Agreed, will fix too.

Thinking more about this bit, checking only cplan->is_valid after
locking is not really equivalent to what CheckCachedPlan() was doing
before.

CheckCachedPlan() can also reset plan->is_valid based on other state
(role, xmin), whereas in v1 the new check relied only on lock inval
callbacks to have set is_valid to false. That means the new check is
not enough. It may happen to be okay for the current callers in this
patch, but it is not something I think is safe to rely on once there
are future callers that do arbitrary work, such as ExecutorPrep(),
even if carefully coded, between the original GetCachedPlan() /
CheckCachedPlan() step and the later recheck after locking.

Separately, I also realized that v1 was introducing redundant lock
acquisition for freshly built plans, which is not really a no behavior
change refactoring either.

So in v2 I ended up reworking two parts more substantially:

* The patch now factors the reused-generic-plan validity logic into
RecheckCachedPlan().

* It also adds an is_reused output argument to GetCachedPlan(), so
callers can distinguish the reused-plan case from a freshly built plan
and avoid the extra lock step in the latter case.

--
Thanks, Amit Langote