Hi, On Thu, Apr 16, 2026 at 11:35 AM Chao Li <[email protected]> wrote: > > On Apr 15, 2026, at 21:46, Amit Langote <[email protected]> wrote: > > On Wed, Apr 15, 2026 at 11:14 AM Amit Langote <[email protected]> > > wrote: > >> On Wed, Apr 15, 2026 at 1:08 AM Kirill Reshke <[email protected]> > >> 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. > > V2 overall looks good, it preserves the current behavior. > > I just have a question. RecheckCachedPlan is only called by LockCachedPlan > and CheckCachedPlan, and both callers are static, why RecheckCachedPlan is > exported?
Oh, I thought someone would ask that. Yes, it could stay static in plancache.c for now and only be exported later, when some future smarter cached-plan locker outside plancache.c actually needs it. -- Thanks, Amit Langote
