MIME-Version: 1.0
References: 
 <CA+HiwqE1ntHy2h9zJ9v3MwAkoGAveSERcHWkDTTZnP0kxWqbKQ@mail.gmail.com>
In-Reply-To: 
 <CA+HiwqE1ntHy2h9zJ9v3MwAkoGAveSERcHWkDTTZnP0kxWqbKQ@mail.gmail.com>
From: Kirill Reshke <reshkekirill@gmail.com>
Date: Tue, 14 Apr 2026 21:08:04 +0500
Message-ID: 
 <CALdSSPh=8qLa1kPsBUnPXMmigaCABiNTwmwg1wH+TmQWH=f_Ww@mail.gmail.com>
Subject: Re: GetCachedPlan() refactor: move execution lock acquisition out
To: Amit Langote <amitlangote09@gmail.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CALdSSPh%3D8qLa1kPsBUnPXMmigaCABiNTwmwg1wH%2BTmQWH%3Df_Ww%40mail.gmail.com>
Precedence: bulk

On Tue, 14 Apr 2026 at 13:20, Amit Langote <amitlangote09@gmail.com> wrote:
>
> 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.
>
> [1] https://postgr.es/m/CA+HiwqFGkMSge6TgC9KQzde0ohpAycLQuV7ooitEEpbKB0O_mg@mail.gmail.com
> [2] https://postgr.es/m/4191508.1674157166%40sss.pgh.pa.us
> [3] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=1722d5eb05d8e5d2e064cd1798abcae4f296ca9d
>
> --
> Thanks, Amit Langote


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

> +
> + for (;;)
> + {
> + cplan = GetCachedPlan(plansource, boundParams, owner, queryEnv);
> + if (LockCachedPlan(cplan))
> + break;
> +
> + ReleaseCachedPlan(cplan, owner);
> + }

Should we use CFI here?

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

-- 
Best regards,
Kirill Reshke