MIME-Version: 1.0
References: 
 <CA+HiwqE1ntHy2h9zJ9v3MwAkoGAveSERcHWkDTTZnP0kxWqbKQ@mail.gmail.com>
 <CALdSSPh=8qLa1kPsBUnPXMmigaCABiNTwmwg1wH+TmQWH=f_Ww@mail.gmail.com>
In-Reply-To: 
 <CALdSSPh=8qLa1kPsBUnPXMmigaCABiNTwmwg1wH+TmQWH=f_Ww@mail.gmail.com>
From: Amit Langote <amitlangote09@gmail.com>
Date: Wed, 15 Apr 2026 11:14:45 +0900
Message-ID: 
 <CA+HiwqH7TcHb=P7qHF-9jnXD3APLUWbD4QS_OJ3d-5qaC+Puzg@mail.gmail.com>
Subject: Re: GetCachedPlan() refactor: move execution lock acquisition out
To: Kirill Reshke <reshkekirill@gmail.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CA%2BHiwqH7TcHb%3DP7qHF-9jnXD3APLUWbD4QS_OJ3d-5qaC%2BPuzg%40mail.gmail.com>
Precedence: bulk

Hi,

On Wed, Apr 15, 2026 at 1:08=E2=80=AFAM Kirill Reshke <reshkekirill@gmail.c=
om> wrote:
> On Tue, 14 Apr 2026 at 13:20, Amit Langote <amitlangote09@gmail.com> wrot=
e:
> > Over in the "generic plans and initial pruning" thread [1], I came to
> > think that the cleanest way to address the architectural issue behind
> > that thread is to make GetCachedPlan() do less execution-related work.
> > Specifically, it should stop acquiring execution locks itself, and
> > leave that decision to callers.
> >
> > GetCachedPlan() has long combined plan acquisition with execution
> > setup, and that coupling makes it harder to improve either side
> > cleanly. Tom pointed in this direction back in 2023 [2]:
> >
> > "...the whole problem arises because the system is wrongly factored.
> > We should get rid of AcquireExecutorLocks altogether, allowing the
> > plancache to hand back a generic plan that it's not certain of the
> > validity of, and instead integrate the responsibility for acquiring
> > locks into executor startup."
> >
> > This patch takes the first half of that suggestion by moving execution
> > locking out of the plancache and into its callers.
> >
> > The attached patch does that, with no intended behavioral change:
> >
> > * Remove AcquireExecutorLocks() from CheckCachedPlan(), so
> > GetCachedPlan() returns a plan without holding execution locks.
> >
> > * Add a new internal helper in plancache.c, LockCachedPlan(), which
> > acquires execution locks, rechecks validity, and returns false if the
> > plan was invalidated, so the caller can retry.
> >
> > * Add GetCachedPlanLocked() as a convenience wrapper that fetches a
> > plan, calls LockCachedPlan(), and retries on invalidation. This
> > becomes the normal entry point for callers that want a plan ready for
> > execution.
> >
> > * Convert existing GetCachedPlan() callers to use GetCachedPlanLocked()=
.
> >
> > The second half of Tom's suggestion, moving that responsibility into
> > executor startup, is trickier. ExecutorStart() is a stable API with
> > extension hooks, so changing its contract has a fairly high bar. I
> > tried that route once already [3].
> >
> > A working variant that adds an ExecutorPrep() entry point, with a
> > wrapper implementing pruning-aware locking on top, was discussed in
> > the original thread [1].  But rather than propose that whole stack at
> > once, I think it is better to do this in phases: first decouple plan
> > acquisition from execution locking, then revisit the ExecutorPrep()
> > piece separately if this goes in.
> >
> > The eventual goal is still pruning-aware locking. I'm posting this
> > piece separately because the original thread has become fairly long,
> > and this part seems easier to review on its own.
>
> Hi! I have slightly checked v1. Can't tell if refactoring does make
> cached plan maintenance more straightforward because of lack of
> experience in this area, but here's my 2c

Thanks for taking a look.

Could you say a bit more about what aspect of cached plan maintenance
you have in mind? My main motivation here is to separate plan
acquisition from execution-time locking, so that locking policy no
longer has to live inside GetCachedPlan().

> > +
> > + for (;;)
> > + {
> > + cplan =3D GetCachedPlan(plansource, boundParams, owner, queryEnv);
> > + if (LockCachedPlan(cplan))
> > + break;
> > +
> > + ReleaseCachedPlan(cplan, owner);
> > + }
>
> Should we use CFI here?

Good point. On a closer look, I think the retry loop can go away
entirely. If LockCachedPlan() fails, releasing the plan and calling
GetCachedPlan() again is enough, because that will rebuild the plan
and the needed locks will already be held by the planner. I'll
simplify that in the next version.

> > +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.

I'll hold off on posting a v2 for now.

--=20
Thanks, Amit Langote