Subject: Re: generic plans and "initial" pruning

From: Tomas Vondra <[email-masked]>
Date: Wed, 4 Dec 2024 18:20:23 +0100

On 12/4/24 14:34, Amit Langote wrote:
> Hi Tomas,
> 
> On Mon, Dec 2, 2024 at 3:36 AM Tomas Vondra <[email-masked]> wrote:
>> Hi,
>>
>> I took a look at this patch, mostly to familiarize myself with the
>> pruning etc. I have a bunch of comments, but all of that is minor,
>> perhaps even nitpicking - with prior feedback from David, Tom and
>> Robert, I can't really compete with that.
> 
> Thanks for looking at this.  These are helpful.
> 
>> FWIW the patch needs a rebase, there's a minor bitrot - but it was
>> simply enough to fix for a review / testing.
>>
>>
>> 0001
>> ----
>>
>> 1) But if we don't expect this error to actually happen, do we really
>> need to make it ereport()? Maybe it should be plain elog(). I mean, it's
>> "can't happen" and thus doesn't need translations etc.
>>
>>     if (!bms_equal(relids, pruneinfo->relids))
>>         ereport(ERROR,
>>                 errcode(ERRCODE_INTERNAL_ERROR),
>>                 errmsg_internal("mismatching PartitionPruneInfo found at
>> part_prune_index %d",
>>                                 part_prune_index),
>>                 errdetail_internal("plan node relids %s, pruneinfo
>> relids %s",
>>                                    bmsToString(relids),
>>                                    bmsToString(pruneinfo->relids)));
> 
> I'm fine with elog() here even if it causes the message to be longer:
> 
> elog(ERROR, "mismatching PartitionPruneInfo found at part_prune_index
> %d (plan node relids %s, pruneinfo relids %s)
> 

I'm not forcing you to do elog, if you think ereport() is better. I'm
only asking because AFAIK the "policy" is that ereport is for cases that
think can happen (and thus get translated), while elog(ERROR) is for
cases that we believe shouldn't happen.

So every time I see "ereport" I ask myself "how could this happen" which
doesn't seem to be the case here.

>> Perhaps it should even be an assert?
> 
> I am not sure about that.  Having a message handy might be good if a
> user ends up hitting this case for whatever reason, like trying to run
> a corrupted plan.
> 

I'm a bit skeptical about this, TBH. If we assume the plan is
"corrupted", why should we notice in this particular place? I mean, it
could be corrupted in a million different ways, and the chance that it
got through all the earlier steps is like 1 in a 1.000.000.

>> 2) unnecessary newline added to execPartition.h
> 
> Perhaps you meant "removed".  Fixed.
> 

Yes, sorry. I misread the diff.

>> 5) PlannerGlobal
>>
>>     /* List of PartitionPruneInfo contained in the plan */
>>     List       *partPruneInfos;
>>
>> Why does this say "contained in the plan" unlike the other fields? Is
>> there some sort of difference? I'm not saying it's wrong.
> 
> Ok, maybe the following is a bit more helpful and like the comment for
> other fields:
> 
>     /* "flat" list of PartitionPruneInfos */
>     List       *partPruneInfos;
> 

WFM

>> 0002
>> ----
>>
>> 1) Isn't it weird/undesirable partkey_datum_from_expr() loses some of
>> the asserts? Would the assert be incorrect in the new implementation, or
>> are we removing it simply because we happen to not have one of the fields?
> 
> The former -- the asserts would be incorrect in the new implementation
> -- because in the new implementation a standalone ExprContext is used
> that is independent of the parent PlanState (when available) for both
> types of runtime pruning.
> 
> The old asserts, particularly the second one, weren't asserting
> something very useful anyway, IMO.  What I mean is that the
> ExprContext provided in the PartitionPruneContext to be the same as
> the parent PlanState's ps_ExprContext isn't critical to the code that
> follows.  Nor whether the PlanState is available or not.
> 

OK, thanks for explaining

>> 2) inconsistent spelling: run-time vs. runtime
> 
> I assume you meant in this comment:
> 
> * estate                       The EState for the query doing runtime pruning
> 
> Fixed by using run-time, which is a more commonly used term in the
> source code than runtime.
> 

Not quite. I was looking at runtime/run-time in the patch files, but now
I realize some of that is preexisting ... Still, maybe the patch should
stick to one spelling.

>> 2) I'm not quite sure what "exec" partition pruning is?
>>
>> /*
>>  * ExecInitPartitionPruning
>>  *   Initialize the data structures needed for runtime "exec" partition
>>  *   pruning and return the result of initial pruning, if available.
>>
>> Is that the same thing as "runtime pruning"?
> 
> "Exec" pruning refers to pruning performed during execution, using
> PARAM_EXEC parameters. In contrast, "init" pruning occurs during plan
> initialization, using parameters whose values remain constant during
> execution, such as PARAM_EXTERN parameters and stable functions.
> 
> Before this patch, the ExecInitPartitionPruning function, called
> during ExecutorStart(), performed "init" pruning and set up state in
> the PartitionPruneState for subsequent "exec" pruning during
> ExecutorRun(). With this patch, "init" pruning is performed well
> before this function is called, leaving its sole responsibility to
> setting up the state for "exec" pruning. It may be worth renaming the
> function to better reflect this new role, rather than updating only
> the comment.
> 
> Actually, that is what I decided to do in the attached, along with
> some other adjustments like moving ExecDoInitialPruning() to
> execPartition.c from execMain.c, fixing up some obsolete comments,
> etc.
> 

I don't see any attachment :-(

Anyway, if I understand correctly, the "runtime pruning" has two
separate cases - initial pruning and exec pruning. Is that right?
> 
>>
>> 2) It may not be quite clear why ExecInitUpdateProjection() switches to
>> mt_updateColnosLists. Should that be explained in a comment, somewhere?
> 
> There is a comment in the ModifyTableState struct definition:
> 
>     /*
>      * List of valid updateColnosLists.  Contains only those belonging to
>      * unpruned relations from ModifyTable.updateColnosLists.
>      */
>     List       *mt_updateColnosLists;
> 
> It seems redundant to reiterate this in ExecInitUpdateProjection().
> 

Ah, I see. Makes sense.

> 
>> 0005
>> ----
>>
>> 1) auto_explain.c - So what happens if the plan gets invalidated? The
>> hook explain_ExecutorStart returns early, but then what? Does that break
>> the user session somehow, or what?
> 
> It will get called again after ExecutorStartExt() loops back to do
> ExecutorStart() with a new updated plan tree.
> 
>> 2) Isn't it a bit fragile if this requires every extension to update
>> and add the ExecPlanStillValid() calls to various places?
> 
> The ExecPlanStillValid() call only needs to be added immediately after
> the call to standard_ExecutorStart() in an extension's
> ExecutorStart_hook() implementation.
> 
>> What if an
>> extension doesn't do that? What weirdness will happen?
> 
> The QueryDesc.planstate won't contain a PlanState tree for starters
> and other state information that InitPlan() populates in EState based
> on the PlannedStmt.
> 

OK, and the consequence is that the query will fail, right?

>> Maybe it'd be
>> possible to at least check this in some other executor hook? Or at least
>> we could ensure the check was done in assert-enabled builds? Or
>> something to make extension authors aware of this?
> 
> I've added a note in the commit message, but if that's not enough, one
> idea might be to change the return type of ExecutorStart_hook so that
> the extensions that implement it are forced to be adjusted. Say, from
> void to bool to indicate whether standard_ExecutorStart() succeeded
> and thus created a "valid" plan.  I had that in the previous versions
> of the patch.  Thoughts?
> 

Maybe. My concern is that this case (plan getting invalidated) is fairly
rare, so it's entirely plausible the extension will seem to work just
fine without the code update for a long time.

Sure, changing the APIs is allowed, I'm just wondering if maybe there
might be a way to not have this issue, or at least notice the missing
call early.

I haven't tried, wouldn't it be better to modify ExecutorStart() to do
the retries internally? I mean, the extensions wouldn't need to check if
the plan is still valid, ExecutorStart() would take care of that. Yeah,
it might need some new arguments, but that's more obvious.

>> Aside from going through the patches, I did a simple benchmark to see
>> how this works in practice. I did a simple test, with pgbench -S and
>> variable number of partitions/clients. I also varied the number of locks
>> per transaction, because I was wondering if it may interact with the
>> fast-path improvements. See the attached xeon.sh script and CSV with
>> results from the 44/88-core machine.
>>
>> There's also two PDFs visualizing the results, to show the impact as a
>> difference between "master" (no patches) vs. "pruning" build with v57
>> applied. As usual, "green" is good (faster), read is "bad" (slower).
>>
>> For most combinations of parameters, there's no impact on throughput.
>> Anything in 99-101% is just regular noise, possibly even more. I'm
>> trying to reduce the noise a bit more, but this seems acceptable. I'd
>> like to discuss three "cases" I see in the results:
> 
> Thanks for doing these benchmarks.  I'll reply separately to discuss
> the individual cases.
> 
>> costing / auto mode
>> -------------------
>>
>> Anyway, this leads me to a related question - not quite a "bug" in the
>> patch, but something to perhaps think about. And that's costing, and
>> what "auto" should do.
>>
>> There are two PNG charts, showing throughput for runs with -M prepared
>> and 1000 partitions. Each chart shows throughput for the three cache
>> modes, and different client counts. There's a clear distinction between
>> "master" and "patched" runs - the "generic" plans performed terribly, by
>> orders of magnitude. With the patches it beats the "custom" plans.
>>
>> Which is great! But it also means that while "auto" used to do the right
>> thing, with the patches that's not the case.
>>
>> AFAIK that's because we don't consider the runtime pruning when costing
>> the plans, so the cost is calculated as if no pruning happened. And so
>> it seems way more expensive than it should ... and it loses with the
>> custom scans. Is that correct, or do I understand this wrong?
> 
> That's correct. The planner does not consider runtime pruning when
> assigning costs to Append or MergeAppend paths in
> create_{merge}append_path().
> 
>> Just to be clear, I'm not claiming the patch has to deal with this. I
>> suppose it can be handled as a future improvement, and I'm not even sure
>> there's a good way to consider this during costing. For example, can we
>> estimate how many partitions will be pruned?
> 
> There have been discussions about this in the 2017 development thread
> of run-time pruning [1] and likely at some later point in other
> threads.  One simple approach mentioned at [1] is to consider that
> only 1 partition will be scanned for queries containing WHERE partkey
> = $1, because only 1 partition can contain matching rows with that
> condition.
> 
> I agree that this should be dealt with sooner than later so users get
> generic plans even without having to use force_generic_plan.
> 
> I'll post the updated patches tomorrow.
> 

Cool, thanks!

regards
-- 
Tomas Vondra

---

From: Tom Lane <[email-masked]>
Date: Wed, 04 Dec 2024 12:32:13 -0500

> cases that we believe shouldn't happen.

The proposed coding looks fine from that perspective, because it uses
errmsg_internal and errdetail_internal which don't give rise to
translatable strings.  Having said that, if we think this is a
"can't happen" case then it's fair to wonder why go to such lengths
to format it prettily.  Also, I'd argue that the error message
style guidelines still apply, but this errdetail doesn't conform.

			regards, tom lane

---

From: Amit Langote <[email-masked]>
Date: Thu, 5 Dec 2024 11:14:12 +0900

> style guidelines still apply, but this errdetail doesn't conform.

Thinking about this further, perhaps an Assert is sufficient here. An
Append/MergeAppend node's part_prune_index not pointing to the correct
entry in the global "flat" list of PartitionPruneInfos would indicate
a bug. It seems unlikely that user actions could cause this issue.

-- 
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Thu, 5 Dec 2024 15:53:32 +0900

> got through all the earlier steps is like 1 in a 1.000.000.

Yeah, I am starting to think the same.  Btw, the idea to have a check
and elog() / ereport() came from Alvaro upthread:
https://www.postgresql.org/message-id/20221130181201.mfinyvtob3j5i2a6%40alvherre.pgsql

> separate cases - initial pruning and exec pruning. Is that right?

That's correct.  These patches are about performing "initial" pruning
at a different time and place so that we can take the deferred locks
on the unpruned partitions before we perform ExecInitNode() on any of
the plan trees in the PlannedStmt.

> OK, and the consequence is that the query will fail, right?

No, the core executor will retry the execution with a new updated
plan.  In the absence of the early return, the extension might even
crash when accessing such incomplete QueryDesc.

What the patch makes the ExecutorStart_hook do is similar to how
InitPlan() will return early when locks taken on partitions that
survive initial pruning invalidate the plan.

> fine without the code update for a long time.

You might see the errors like the one below when the core executor or
a hook tries to initialize or process in some other way a known
invalid plan, for example, because an unpruned partition's index got
concurrently dropped before the executor got the lock:

ERROR: could not open relation with OID xxx

> it might need some new arguments, but that's more obvious.

One approach could be to move some code from standard_ExecutorStart()
into ExecutorStart(). Specifically, the code responsible for setting
up enough state in the EState to perform ExecDoInitialPruning(), which
takes locks that might invalidate the plan. If the plan does become
invalid, the hook and standard_ExecutorStart() are not called.
Instead, the caller, ExecutorStartExt() in this case, creates a new
plan.

This avoids the need to add ExecPlanStillValid() checks anywhere,
whether in core or extension code. However, it does mean accessing the
PlannedStmt earlier than InitPlan(), but the current placement of the
code is not exactly set in stone.

-- 
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Thu, 5 Dec 2024 20:28:10 +0900

> code is not exactly set in stone.

I tried this approach and found that it essentially disables testing
of this patch using the delay_execution module, which relies on the
ExecutorStart_hook(). The way the testing works is that the hook in
delay_execution.c pauses the execution of a cached plan to allow a
concurrent session to drop an index referenced in the plan. When
unpaused, execution initialization resumes by calling
standard_ExecutorStart(). At this point, obtaining the lock on the
partition whose index has been dropped invalidates the plan, which the
hook detects and reports. It then also reports the successful
re-execution of an updated plan that no longer references the dropped
index.  Hmm.

-- 
Thanks, Amit Langote

---

From: Tomas Vondra <[email-masked]>
Date: Thu, 5 Dec 2024 14:53:09 +0100

On 12/5/24 07:53, Amit Langote wrote:
>> ...

Isn't that what I said? My question was what happens if the extension
does not add the new ExecPlanStillValid() call - sorry if that wasn't
clear. If it can crash, that's what I meant by "fail".

> ERROR: could not open relation with OID xxx

Yeah, but how likely is that? How often get plans invalidated in regular
application workload. People don't create or drop indexes very often,
for example ...

Again, I'm not saying requiring the call would be unacceptable, I'm sure
we made similar changes in the past. But if it wasn't needed without too
much contortion, that would be nice.

regards

-- 
Tomas Vondra

---

From: Tomas Vondra <[email-masked]>
Date: Thu, 5 Dec 2024 15:07:12 +0100

> re-execution of an updated plan that no longer references the dropped

It's not clear to me why the change disables this testing, and I can't
try without a patch. Could you explain?

thanks

-- 
Tomas Vondra

---

From: Amit Langote <[email-masked]>
Date: Fri, 6 Dec 2024 17:18:47 +0900

> try without a patch. Could you explain?

Sorry, PFA the delta patch for the change I described above.  It
applies on top of v58 series of patches that I posted yesterday.
You'll notice that delay_execution test fails if you apply and do
check-world.

As for how the change breaks the testing, here is a before and after
of the flow of a isolation test in
src/test/modules/delay_execution/specs/cached-plan-inval.spec (s1 is
the session used to run a cached plan, s2 to perform concurrent DDL
that invalidates the plan):

* Before (working):

1. s2 takes advisory lock
2. s1 runs cached plan -> goes to ExecutorStart_hook -> waits for the
advisory lock
3. s2 drops an index referenced in the plan
4. s2 unlocks advisory lock
5. s1 locks unpruned partitions -> detects plan invalidation due to
dropped index.

* After (stops working because initial pruning and locking are done
before calling ExecutorStart_hook):

1. s2 takes advisory lock
2. s1 runs cached plan -> locks unpruned partitions -> goes to
ExecutorStart_hook to get advisory lock -> waits for advisory lock
3. s2 drops an index referenced in the plan -> waits for lock on the
unpruned partition -> deadlock!

One idea I had after sending the email yesterday is to introduce
ExecutorStartCachedPlan_hook for the advisory lock based waiting.
ExecutorStartCachedPlan() is the new function that you will find in
v58-0004 that wraps ExecutorStart() to handle plan invalidation.  This
new hook would be called before ExecutorStartCachedPlan() calls
ExecutorStart(), so the original testing flow can still work.

Another idea might be to use injection points infra to introduce the
wait instead of the combination of a executor hook and advisory lock.

-- 
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Fri, 6 Dec 2024 17:26:49 +0900

> clear. If it can crash, that's what I meant by "fail".

Ok, I see.  So, I suppose you meant to confirm if the invalid plan
won't silently be executed returning wrong results.  Yes, I don't
think that would happen given the kinds of invalidations that are
possible.  The various checks in the ExecInitNode() path, such as the
one that catches a missing index, will prevent the plan from running.
I may not have searched exhaustively enough though.

> application workload. People don't create or drop indexes very often,

Yeah, that's a valid point.  Andres once mentioned that ANALYZE can
invalidate plans and that can occur frequently in busy systems.

> much contortion, that would be nice.

I tend to agree.

Another change introduced by the patch that extensions might need to
mind (noted in the commit message of v58-0004) is the addition of the
es_unpruned_relids field to EState. This field tracks the RT indexes
of relations that are locked and therefore safe to access during
execution. Importantly, it does not include the RT indexes of leaf
partitions that are pruned during "initial" pruning and thus remain
unlocked.

This change means that executor extensions can no longer assume that
all relations in the range table are locked and safe to access.
Instead, extensions must account for the possibility that some
relations, specifically pruned partitions, are not locked. Normally,
executor code accesses relations using ExecGetRangeTableRelation(),
which does not take a lock before returning the Relation pointer,
assuming that locks are already managed upstream.

-- 
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Mon, 9 Dec 2024 16:10:24 +0900

> ExecutorStart(), so the original testing flow can still work.

Here's that patch with this idea implemented that fixes the
delay_execution test breakage.  Applies on top of v58 series of
patches.

However, as mentioned in my previous reply, since extensions might
need to adjust their ExecutorStart hook code to check if the RT index
is in EState.es_unpruned_relids when accessing child relations
directly via ExecGetRangeTableRelation(), I can accept them also
adding a check for ExecPlanStillValid() in their ExecutorStart hook.
So we may not want to add a new hook even if only for testing.

-- 
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Thu, 12 Dec 2024 16:58:54 +0900

> So we may not want to add a new hook even if only for testing.

One thing I realized about the es_unpruned_relids bitmapset is that
ExecGetRangeTableRelation() should verify that any RT index passed to
it is a member of the bitmapset. If the RT index is not included, the
function should throw an error to catch such cases. I made that change
in 0004. This approach can help identify extensions that manipulate RT
entries belonging to potentially pruned partitions, provided they use
the ExecGetRangeTableRelation() interface to open those relations.

To summarize how extensions can be affected:

1. Plan invalidation during standard_ExecutorStart(): A plan tree
originating from a CachedPlan can become invalid during
standard_ExecutorStart() due to locks taken on leaf partitions that
survive initial pruning. Extensions should be updated to handle this
scenario by checking ExecPlanStillValid(estate) immediately after
calling standard_ExecutorStart() in their ExecutorStart_hook. If it
returns false, the extensions should avoid further processing.

2. Validation of RT indexes: If the plan tree remains valid, any
direct manipulation of relations using RT indexes must first verify
that the RT index is present in the EState.es_unpruned_relids
bitmapset. This bitmapset includes: a) RT indexes of relations that
are originally unprunable (and locked during GetCachedPlan()), and
b) RT indexes of leaf partitions that survive initial partition
pruning. This step is crucial because pruned relations are not locked.
Additionally, with the update in 0004, attempting to open pruned
relations using ExecGetRangeTableRelation() will result in an error.

I’d love to hear from anyone maintaining executor hooks, such as those
from Timescale, Citus, or other extension developers. Please give this
patch set (0001-0004) a try and let me know if you run into any issues
or have feedback.  0005 is a sketch of an approach that eliminates the
need for extensions to check ExecPlanStillValid() in their
ExecutorStart_hook.

--
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Thu, 23 Jan 2025 16:15:48 +0900

> need for extensions to check ExecPlanStillValid() in their

I’ve rebased over recent changes to setrefs.c (commit bf826ea0629).
During the rebase, I realized that the patch
0002-Initialize-PartitionPruneContexts-lazily wasn’t a good idea after
all.

The test case added by bf826ea0629 highlighted an issue: initializing
pruning expressions lazily during execution could leave the
Append/MergeAppend node’s PlanState.subPlan uninitialized at
ExecInitNode() time. Initially, I thought this would have only
cosmetic consequences -- such as changes in test case output where
SubPlans referenced in "exec" pruning expressions wouldn’t appear --
but I may have underestimated the problem. As a result, I’ve abandoned
that approach and the patch in favor of initializing all pruning
expressions during plan initialization.

Additionally, I revisited the impact of the main patch on
ExecutorStart_hooks. It seems better to change the return type from
void to bool, returning the result of
ExecPlanStillValid(queryDesc->estate). This change has the added
benefit of breaking extensions that use ExecutorStart_hook at compile
time, encouraging authors to update their code. The updated commit
message includes details on additional checks extensions must
implement, particularly for cases where they might access pruned and
thus unlocked relations.

I've stared at the refactoring patches 0001 and 0002 for long enough
at this point that I'd like to commit them early next week, barring
further comments or objections.  I'll keep staring at 0003.

--
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Fri, 31 Jan 2025 17:31:44 +0900

> further comments or objections.  I'll keep staring at 0003.

I have now pushed 0001 and 0002.

I broke 0003 into two patches:

Patch to track unpruned relations in the executor, allowing the
overhead of processing pruned partitions to be skipped during plan
initialization. This is particularly relevant for top-level nodes such
as ModifyTable and LockRows, which -- unlike Append / MergeAppend --
do not ignore initially pruned partitions. Since initial pruning is
now performed separately from plan initialization and earlier in
InitPlan(), we can fix this by checking whether a given child result
relation or RowMark belongs to a pruned partition and skipping it.

Patch to defer locking of prunable relations from GetCachedPlan() to
InitPlan(), preventing partitions pruned by initial pruning from being
locked unnecessarily.

With the attached 0001, I can see that saving the overhead of
initializing ResultRelInfos for pruned partitions in
ExecInitModifyTable() results in a noticeable speedup for pgbench
-Mprepared with partitions, especially at higher partition counts
where the overhead is more significant. The numbers I have here are a
bit noisy, but they provide a general idea of the performance benefit
of skipping initially pruned partitions during plan initialization.

Setup:

plan_cache_mode = force_generic_plan
max_locks_per_transaction = 1000

for i in 100 200 500 1000 2000; do
echo -ne "$i\t"
pgbench -i --partitions=$i > /dev/null 2>&1;
pgbench -n -Mprepared -T 10 | grep tps;
done

With master:
100 tps = 2837.095192 (without initial connection time)
200 tps = 2614.143255 (without initial connection time)
500 tps = 1960.666074 (without initial connection time)
1000 tps = 1390.691229 (without initial connection time)
2000 tps = 884.882656 (without initial connection time)

With 0001:
100 tps = 2889.600827 (without initial connection time)
200 tps = 2720.895632 (without initial connection time)
500 tps = 2096.177756 (without initial connection time)
1000 tps = 1659.265873 (without initial connection time)
2000 tps = 1148.976177 (without initial connection time)

With 0002:
100 tps = 3070.137629 (without initial connection time)
200 tps = 4589.336857 (without initial connection time)
500 tps = 2977.339119 (without initial connection time)
1000 tps = 2885.417560 (without initial connection time)
2000 tps = 3832.111167 (without initial connection time)

-- 
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Thu, 6 Feb 2025 11:35:47 +0900

> 2000 tps = 3832.111167 (without initial connection time)

Per cfbot-ci, the new test case output in 0002 needed to be updated.

I plan to push 0001 tomorrow, barring any objections.

-- 
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Wed, 12 Feb 2025 20:53:50 +0900

> I plan to push 0001 tomorrow, barring any objections.

I pushed that last Friday. With bb3ec16e, d47cbf47, and cbc12791 now in:

* Pruning information is now stored separately from parent plan nodes
in PlannedStmt.

* Initial runtime pruning occurs as a separate step, independent of
and before plan initialization in InitPlan().

* The RT indexes of unprunable relations and those of partitions that
survive initial pruning are stored in a global bitmapset in EState,
allowing us to avoid work that was previously done for pruned
partitions. This was difficult before because initial pruning wasn’t
performed before the parent plan node was initialized, meaning that
the work we aimed to save had already been done.

The final remaining piece is to skip taking locks on partitions pruned
during initial pruning, and the attached patch addresses that.

I’d like to commit the patch next week, barring objections.

--
Thanks, Amit Langote

---

From: Alexander Lakhin <[email-masked]>
Date: Fri, 14 Feb 2025 23:00:00 +0200

Hello Amit,

> I plan to push 0001 tomorrow, barring any objections.

Please try the following script:
CREATE TABLE pt (a int, b int) PARTITION BY range (a);
CREATE TABLE tp1 PARTITION OF pt FOR VALUES FROM (1) TO (2);
CREATE TABLE tp2 PARTITION OF pt FOR VALUES FROM (2) TO (3);

MERGE INTO pt
USING (SELECT pg_backend_pid() AS pid) AS q JOIN tp1 ON (q.pid = tp1.a)
ON pt.a = tp1.a
WHEN MATCHED THEN DELETE;

which fails for me with segfault:
Program terminated with signal SIGSEGV, Segmentation fault.
#0  ExecInitMerge (mtstate=0x5a9b9fbccae0, estate=0x5a9b9fbcbe20) at nodeModifyTable.c:3680
3680                    relationDesc = RelationGetDescr(resultRelInfo->ri_RelationDesc);
(gdb) bt
#0  ExecInitMerge (mtstate=0x5a9b9fbccae0, estate=0x5a9b9fbcbe20) at nodeModifyTable.c:3680
#1  0x00005a9b67e6dfb5 in ExecInitModifyTable (node=0x5a9b9fbd5858, estate=0x5a9b9fbcbe20, eflags=0) at 
nodeModifyTable.c:4906
#2  0x00005a9b67e273f7 in ExecInitNode (node=0x5a9b9fbd5858, estate=0x5a9b9fbcbe20, eflags=0) at execProcnode.c:177
#3  0x00005a9b67e1b9d2 in InitPlan (queryDesc=0x5a9b9fbb9970, eflags=0) at execMain.c:1092
#4  0x00005a9b67e1a524 in standard_ExecutorStart (queryDesc=0x5a9b9fbb9970, eflags=0) at execMain.c:268
#5  0x00005a9b67e1a223 in ExecutorStart (queryDesc=0x5a9b9fbb9970, eflags=0) at execMain.c:142
...

starting from cbc127917.

(I've discovered this anomaly with SQLsmith.)

Best regards,
Alexander Lakhin
Neon (https://neon.tech)

---

From: Amit Langote <[email-masked]>
Date: Sat, 15 Feb 2025 16:51:09 +0900

Hi Alexander,

On Sat, Feb 15, 2025 at 6:00 AM Alexander Lakhin <[email-masked]> wrote:
> #1  0x00005a9b67e6dfb5 in ExecInitModifyTable (node=0x5a9b9fbd5858, estate=0x5a9b9fbcbe20, eflags=0) at nodeModifyTable.c:4906

Thanks! It looks like I missed updating the MERGE-related lists in ModifyTable.

I've attached a fix with a test added based on your example. I plan to
push this on Monday.

-- 
Thanks, Amit Langote

---

From: Junwang Zhao <[email-masked]>
Date: Sun, 16 Feb 2025 12:37:07 +0800

Hi Amit,

> I've attached a fix with a test added based on your example. I plan to

I applied the patch and the problem solved, I have a small question that
should the following line

```
if (node->mergeActionLists == NIL)
```

be changed to

```
if (mtstate->mt_mergeActionLists == NIL)
```

ISTM that if we have pruned all the merge actions, there is no harm we
omit setting mtstate->mt_merge_subcommands to 0.

-- 
Regards
Junwang Zhao

---

From: Amit Langote <[email-masked]>
Date: Mon, 17 Feb 2025 16:15:33 +0900

Hi Junwang,

On Sun, Feb 16, 2025 at 1:37 PM Junwang Zhao <[email-masked]> wrote:
> I applied the patch and the problem solved,

Thanks for checking.

> I have a small question that

Yeah, that seems harmless, so done.

I have pushed the fix now.

-- 
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Fri, 21 Feb 2025 12:40:09 +0900

> I’d like to commit the patch next week, barring objections.

I pushed the final piece yesterday.

Thank you all who have commented on this thread, reviewed the patches
in its various incarnations, and offered advice here or offlist.

-- 
Thanks, Amit Langote

---

From: Tom Lane <[email-masked]>
Date: Fri, 21 Feb 2025 01:04:39 -0500

> I pushed the final piece yesterday.

trilobite reports that this fails under -DCLOBBER_CACHE_ALWAYS:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=trilobite&dt=2025-02-20%2019%3A37%3A12

			regards, tom lane

---

From: Amit Langote <[email-masked]>
Date: Fri, 21 Feb 2025 15:36:41 +0900

> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=trilobite&dt=2025-02-20%2019%3A37%3A12

Looking, thanks for the heads up.

-- 
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Fri, 21 Feb 2025 17:07:09 +0900

> Looking, thanks for the heads up.

The short of it is that the cached-plan-inval test in the
delay_execution suite can never be made to work under
CLOBBER_CACHE_ALWAYS. The test assumes that locks on partitions for a
reused generic plan are not taken until InitPlan(). However, under
CLOBBER_CACHE_ALWAYS, generic plans are never reused, so the test's
assumption never holds.

I see two possible ways to address this:

1. Find a way to disable the cached-plan-inval test in
CLOBBER_CACHE_ALWAYS builds. However, I haven't found any other test
that does this.

2. Remove the test altogether, though that might be too drastic.

Thoughts?

--
Thanks, Amit Langote

---

From: Tom Lane <[email-masked]>
Date: Fri, 21 Feb 2025 10:55:03 -0500

> CLOBBER_CACHE_ALWAYS, generic plans are never reused, so the test's

Ugh.

> 2. Remove the test altogether, though that might be too drastic.

Well, you could force matters with "set debug_discard_caches = 0"
within the test, but I think that's just a band-aid that would
not make the test fully stable.  The point of CLOBBER_CACHE_ALWAYS
is to model random arrival of cache flush events, which is *always*
a possibility due to background activity (autovacuum for instance).

We do have a couple of other regression tests that rely on
"set debug_discard_caches = 0", and I've not seen many buildfarm
failures tracing to that, but I don't trust it a whole lot.

How badly do you want to keep this test case?  It seems fairly
rickety to me, even without this particular concern.

			regards, tom lane

---

From: Amit Langote <[email-masked]>
Date: Sat, 22 Feb 2025 11:13:24 +0900

> rickety to me, even without this particular concern.

Hmm, yeah, I have to admit that even if we address this specific
issue, the risk of this test failing again outweighs the likelihood of
it catching a real breakage in the deferred lock mechanism.

I'll remove the test for now.

-- 
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Sat, 22 Feb 2025 15:29:25 +0900

> I'll remove the test for now.

Done. I'll try to think of a more robust testing approach for this,
but I’m not very optimistic :-(.

-- 
Thanks, Amit Langote

---

From: Alexander Lakhin <[email-masked]>
Date: Sat, 22 Feb 2025 17:00:01 +0200

Hello Amit,

> I pushed the final piece yesterday.

Please look at new error, produced by the following script,
starting from 525392d57:
CREATE TABLE t(id int) PARTITION BY RANGE (id);
CREATE INDEX idx on t(id);
CREATE TABLE tp_1 PARTITION OF t FOR VALUES FROM (10) TO (20);
CREATE TABLE tp_2 PARTITION OF t FOR VALUES FROM (20) TO (30) PARTITION BY RANGE(id);
CREATE TABLE tp_2_1 PARTITION OF tp_2 FOR VALUES FROM (21) to (22);
CREATE TABLE tp_2_2 PARTITION OF tp_2 FOR VALUES FROM (22) to (23);
CREATE FUNCTION stable_one() RETURNS INT AS $$ BEGIN RETURN 1; END; $$ LANGUAGE plpgsql STABLE;

SELECT min(id) OVER (PARTITION BY id ORDER BY id) FROM t WHERE id >= stable_one();

ERROR:  XX000: trying to open a pruned relation
LOCATION:  ExecGetRangeTableRelation, execUtils.c:830

This issue was discovered with SQLsmith.

Best regards,
Alexander Lakhin
Neon (https://neon.tech)

---

From: Tender Wang <[email-masked]>
Date: Sun, 23 Feb 2025 01:02:56 +0800

> This issue was discovered with SQLsmith.

The error message was added in commit  525392d57. In this case,
the estate->es_unpruned_relids only includes 1, which is the offset of
table t.
In register_partpruneinfo(), we collect glob->prunableRelids; in this case,
it contains 2,3,4,5. Then we will do:
result->unprunableRelids = bms_difference(glob->allRelids,
 glob->prunableRelids);
so the result->unprunableRelids only contains 1.

But tp_2 is also partition table, and its partpruneinfo created by
create_append_plan() is put into the head of global list.
So we first process it in ExecDoInitialPruning().  Then error reports
because we only contain 1 in estate->es_unpruned_relids.

-- 
Thanks,
Tender Wang

---

From: Amit Langote <[email-masked]>
Date: Sun, 23 Feb 2025 17:35:49 +0900

>> This issue was discovered with SQLsmith.

Thanks for the report.

> The error message was added in commit  525392d57. In this case, the estate->es_unpruned_relids only includes 1, which is the offset of table t.
> In register_partpruneinfo(), we collect glob->prunableRelids; in this case, it contains 2,3,4,5. Then we will do:
> But tp_2 is also partition table, and its partpruneinfo created by create_append_plan() is put into the head of global list.
> So we first process it in ExecDoInitialPruning().  Then error reports because we only contain 1 in estate->es_unpruned_relids.

Thanks for checking.

The RT index of tp_2 should appear in PlannedStmt.unprunableRelids,
because it needs to be opened in CreatePartitionPruneState() for
setting up its PartitionPruneInfo. We use ExecGetRangeTableRelation()
to open, which expects the relation to be locked, so the error.

To ensure tp_2 appears in PlannedStmt.unprunableRelids, we should
prevent make_partitionedrel_pruneinfo() from placing the RT index into
leafpart_rti_map[], as the current condition for inclusion doesn’t
account for whether the partition is itself partitioned.

I've come up with the attached.

-- 
Thanks, Amit Langote

---

From: Tender Wang <[email-masked]>
Date: Sun, 23 Feb 2025 20:46:03 +0800

Amit Langote <[email-masked]> 于2025年2月23日周日 16:36写道：

> > The error message was added in commit  525392d57. In this case, the
> estate->es_unpruned_relids only includes 1, which is the offset of table t.
> > In register_partpruneinfo(), we collect glob->prunableRelids; in this
> case, it contains 2,3,4,5. Then we will do:

LGTM.

-- 
Thanks,
Tender Wang

---

From: Amit Langote <[email-masked]>
Date: Tue, 25 Feb 2025 11:51:31 +0900

>> I've come up with the attached.

Pushed after some tweaks to comments and the test case.

-- 
Thanks, Amit Langote

---

From: Tom Lane <[email-masked]>
Date: Mon, 19 May 2025 23:06:21 -0400

> Pushed after some tweaks to comments and the test case.

My attention was drawn to commit 525392d57 after observing that
Valgrind complained about a memory leak in some code that commit added
to BuildCachedPlan().  I tried to make sense of said code so I could
remove the leak, and eventually arrived at the attached patch, which
is part of a series of leak-fixing things hence the high sequence
number.

Unfortunately, the bad things I speculated about in the added comments
seem to be reality.  The second attached file is a test case that
triggers

TRAP: failed Assert("list_length(plan_list) == list_length(plan->stmt_list)"), File: "plancache.c", Line: 1259, PID: 602087

because it adds a DO ALSO rule that causes the rewriter to generate
more PlannedStmts than it did before.

This is quite awful, because it does more than simply break the klugy
(and undocumented) business about keeping the top-level List in a
different context.  What it means is that any outside code that is
busy iterating that List is very fundamentally broken: it's not clear
what List index it ought to resume at, except that "the one it was at"
is demonstrably incorrect.

I also don't really believe the (also undocumented) assumption that
such outside code is in between executions of PlannedStmts of the
List and hence can tolerate those being ripped out and replaced.
I have not attempted to build an example, because the one I have
seems sufficiently damning.  But I bet that a recursive function
could be constructed in such a way that an outer execution is
still in progress when an inner call triggers UpdateCachedPlan.

Another small problem (much more easily fixable than the above,
probably) is that summarily setting "plan->is_valid = true"
at the end is not okay.  We could already have received an
invalidation that should result in marking the plan stale.
(Holding locks on the tables involved is not sufficient to
prevent that, as there are other sources of inval events.)

It's possible that this code can be fixed, but I fear it's
going to involve some really fundamental redesign, which
probably shouldn't be happening after beta1.  I think there
is no alternative but to revert for v18.

			regards, tom lane

From a680e6b6885378beb0164e465b50afd81558ebc5 Mon Sep 17 00:00:00 2001
From: Tom Lane <[email-masked]>
Date: Mon, 19 May 2025 00:02:20 -0400
Subject: [PATCH v2 10/20] Partially fix some extremely broken code from
 525392d57.

Avoid leaking memory in the stmt_context during BuildCachedPlan.
Sadly, this code has problems a lot worse than that (per the
documentation I added), so I suspect 525392d57 will get reverted
and we won't need this patch.

Author: Tom Lane <[email-masked]>
Discussion: https://postgr.es/m/[email-masked]
---
 src/backend/utils/cache/plancache.c | 37 ++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/src/backend/utils/cache/plancache.c b/src/backend/utils/cache/plancache.c
index 9bcbc4c3e97..40ba3e9df7c 100644
--- a/src/backend/utils/cache/plancache.c
+++ b/src/backend/utils/cache/plancache.c
@@ -1109,22 +1109,32 @@ BuildCachedPlan(CachedPlanSource *plansource, List *qlist,
 	 */
 	if (!plansource->is_oneshot)
 	{
+		List	   *stmt_plist;
+
 		plan_context = AllocSetContextCreate(CurrentMemoryContext,
 											 "CachedPlan",
 											 ALLOCSET_START_SMALL_SIZES);
 		MemoryContextCopyAndSetIdentifier(plan_context, plansource->query_string);
 
-		stmt_context = AllocSetContextCreate(CurrentMemoryContext,
+		stmt_context = AllocSetContextCreate(plan_context,
 											 "CachedPlan PlannedStmts",
 											 ALLOCSET_START_SMALL_SIZES);
 		MemoryContextCopyAndSetIdentifier(stmt_context, plansource->query_string);
-		MemoryContextSetParent(stmt_context, plan_context);
 
+		/*
+		 * Copy plans into the stmt_context.
+		 */
 		MemoryContextSwitchTo(stmt_context);
-		plist = copyObject(plist);
+		stmt_plist = copyObject(plist);
 
+		/*
+		 * We actually need the top-level List object to be in the long-lived
+		 * plan_context, in case UpdateCachedPlan wants to update it; see
+		 * comments therein.  Do a shallow copy to make that happen.
+		 */
 		MemoryContextSwitchTo(plan_context);
-		plist = list_copy(plist);
+		plist = list_copy(stmt_plist);
+		list_free(stmt_plist);	/* be tidy */
 	}
 	else
 		plan_context = CurrentMemoryContext;
@@ -1251,12 +1261,22 @@ UpdateCachedPlan(CachedPlanSource *plansource, int query_index,
 
 	/*
 	 * Planning work is done in the caller's memory context.  The resulting
-	 * PlannedStmt is then copied into plan->stmt_context after throwing away
-	 * the old ones.
+	 * PlannedStmt(s) are then copied into plan->stmt_context after throwing
+	 * away the old ones.  But note that we re-use the long-lived
+	 * plan->stmt_list list to hold the pointers to the PlannedStmts.  This
+	 * kluge avoids breaking code that is iterating over that list, so long as
+	 * it's between statements and not currently using one of the contained
+	 * PlannedStmts.
+	 *
+	 * XXX this is, if not actively broken, at least unbelievably fragile.
+	 * Aside from the likelihood that the just-stated assumption doesn't hold
+	 * universally, there is not a good reason to believe that the length of
+	 * the plan list is constant.
 	 */
 	plan_list = pg_plan_queries(query_list, plansource->query_string,
 								plansource->cursor_options, NULL);
-	Assert(list_length(plan_list) == list_length(plan->stmt_list));
+	if (list_length(plan_list) != list_length(plan->stmt_list))
+		elog(ERROR, "UpdateCachedPlan(): plan list length changed");
 
 	MemoryContextReset(plan->stmt_context);
 	oldcxt = MemoryContextSwitchTo(plan->stmt_context);
@@ -1276,7 +1296,8 @@ UpdateCachedPlan(CachedPlanSource *plansource, int query_index,
 
 	/*
 	 * We've updated all the plans that might have been invalidated, so mark
-	 * the CachedPlan as valid.
+	 * the CachedPlan as valid.  XXX wrong: we could already have hit a new
+	 * invalidation event.
 	 */
 	plan->is_valid = true;
 
-- 
2.43.5

drop table if exists test_table;

CREATE TABLE test_table (a int);

create or replace function doit(r int, a int) returns bool
language plpgsql as $$
begin
  raise notice 'r = %, a = %', r, a;
  if (r = 10) then
    CREATE RULE make_noise AS ON DELETE TO test_table
	DO ALSO INSERT INTO test_table SELECT 2;
    raise notice 'made rule';
  end if;
  if (r = 20 and a = 1) then
    CREATE RULE make_noise_2 AS ON DELETE TO test_table
	DO ALSO INSERT INTO test_table SELECT 3;
    raise notice 'made rule 2';
  end if;
  return true;
end$$;

set plan_cache_mode to force_generic_plan;

DO $$
BEGIN
    FOR r IN 1..30 LOOP
        TRUNCATE test_table;
        INSERT INTO test_table SELECT 1;
        DELETE FROM test_table where doit(r,a);
    END LOOP;
END$$;

table test_table;

---

From: Tomas Vondra <[email-masked]>
Date: Tue, 20 May 2025 09:59:53 +0200

> seem to be reality.  The second attached file is a test case that

FYI I added this as a PG18 open item:

  https://wiki.postgresql.org/wiki/PostgreSQL_18_Open_Items

regards

-- 
Tomas Vondra

---

From: Amit Langote <[email-masked]>
Date: Tue, 20 May 2025 22:25:41 +0900

Hi Tom,

> prevent that, as there are other sources of inval events.)

Thanks for pointing out the hole in the current handling of
CachedPlan->stmt_list. You're right that the approach of preserving
the list structure while replacing its contents in-place doesn’t hold
up when the rewriter adds or removes statements dynamically. There
might be other cases that neither of us have tried.  I don’t think
that mechanism is salvageable.

To address the issue without needing a full revert, I’m considering
dropping UpdateCachedPlan() and removing the associated MemoryContext
dance to preserve CachedPlan->stmt_list structure. Instead, the
executor would replan the necessary query into a transient list of
PlannedStmts, leaving the original CachedPlan untouched. That avoids
mutating shared plan state during execution and still enables deferred
locking in the vast majority of cases.

There are two variants of this approach. In the simpler form, the
transient PlannedStmt list exists only in executor-local memory and
isn’t registered with the invalidation machinery. That might be
acceptable in practice, since all referenced relations are locked at
that point -- but it would mean any invalidation events delivered
during execution are ignored. The more robust variant is to build a
one-query standalone CachedPlan using something like
GetTransientCachedPlanForQuery(), which I had proposed back in [1].
This gets added to a standalone_plan_list so that invalidation
callbacks can still reach it. I dropped that design earlier [2] due to
the cleanup overhead, but I’d be happy to bring it back in a
simplified form if that seems preferable.

One open question in either case is what to do if the number of
PlannedStmts in the rewritten plan changes as with your example. Would
it be reasonable to just go ahead and execute the additional
statements from the transient plan, even though the original
CachedPlan wouldn’t have known about them until the next use? That
would avoid introducing any new failure behavior while still handling
the invalidation correctly for the current execution.

> is no alternative but to revert for v18.

...Beyond that, I think I’ve run out of clean options for making
deferred locking executor-local while keeping invalidation safe. I
know you'd previously objected (with good reason) to making
GetCachedPlan() itself run pruning logic to determine which partitions
to lock -- and to the idea of carrying or sharing the result of that
pruning back to the executor via interface changes in the path from
plancache.c through its callers down to ExecutorStart(). So I’ve
steered away from revisiting that direction. But if we’re not
comfortable with either of the transient replanning options, then we
may end up shelving the deferred locking idea entirely -- which would
be unfortunate, given how much it helps workloads that rely on generic
plans over large partitioned tables.

Let me know what you think -- I’ll hold off on posting a revert or a
replacement until we’ve agreed on the path forward.

-- 
Thanks, Amit Langote

[1] https://www.postgresql.org/message-id/CA%2BHiwqGSOge3eT3kcm_nxCSA3Ut%2Bd0jtchi8g8J9uXi-kyC7Jw%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CA%2BHiwqHRRFQN6yZ54fBydOTM6ncqZBCmewZ6n519RjRdDsO44g%40mail.gmail.com

---

From: Tom Lane <[email-masked]>
Date: Tue, 20 May 2025 11:38:31 -0400

> locking in the vast majority of cases.

Yeah, I think messing with the CachedPlan is just fundamentally wrong.
It breaks the invariant that the executor should not scribble on what
it's handed --- maybe not as obviously as some other cases, but it's
still not a good design.

I kind of feel that we ought to take two steps back and think
about what it even means to have a generic plan in this situation.
Perhaps we should simply refuse to use that code path if there are
prunable partitioned tables involved?

> replacement until we’ve agreed on the path forward.

I had not looked at 525392d57 in any detail before (the claim in
the commit message that I reviewed it is a figment of someone's
imagination).  Now that I have, I'm still going to argue for revert.
Aside from the points above, I really hate what's been done to the
fundamental executor APIs.  The fact that ExecutorStart callers have
to know about this is as ugly as can be.  I also don't like the
fact that it's added overhead in cases where there can be no benefit
(notice that my test case doesn't even involve a partitioned table).

I still like the core idea of deferring locking, but I don't like
anything about this implementation of it.  It seems like there has
to be a better and simpler way.

			regards, tom lane

---

From: Amit Langote <[email-masked]>
Date: Wed, 21 May 2025 19:22:03 +0900

> it's handed --- maybe not as obviously as some other cases, but it's

Fair enough. I’ll revert this and some related changes shortly.  WIP
patch attached.

> prunable partitioned tables involved?

Sorry, I’m not sure I fully understand -- especially what you mean by
“that code path.” If you're referring to the generic plan creation and
reuse path in general, I'd point out that initial runtime pruning was
introduced largely to improve the efficiency of generic plan execution
(albeit without addressing the locking bottleneck at the time -- David
Rowley had explored that earlier). So simply disallowing generic plans
when partitions are involved feels like an odd direction, given that a
major motivation for initial pruning was to make those cases faster.

Custom plans can win when parameters are available, of course, but
there's a major use case involving stable expressions like now() with
time-based partitions, where plan_cache_mode = auto will still choose
a generic plan. So I wouldn’t say that optimizing generic plan
execution -- especially the goal of this project -- is wasted effort
in practice.

> imagination).

Apologies if I gave the misleading impression that you were on board
with the current design. I meant only to acknowledge your earlier
engagement with the general idea, which I appreciated. I marked it as
“(old versions)” in the commit metadata to reflect that -- clearly I
should’ve been more precise.  I know that the meaning of Reviewed-by
and other tags is evolving and I clearly haven't kept up.

>  Now that I have, I'm still going to argue for revert.

I tried to keep the overhead low by ensuring that the only additional
thing we'd be doing in the regular path is a CachedPlan->is_valid
boolean check in a couple of places, and that further work would only
happen if invalidation actually occurred. That said, I realize the
patch makes invalidation handling apply in more cases than before,
which may itself be seen as added overhead. But I may have
misunderstood your concern -- perhaps it's more about the layering
violation than the raw cycles?

> to be a better and simpler way.

It's good to hear that you still like the core idea -- I’d really
appreciate it if you're willing to continue bearing with me as I try
to rework this in a way that's cleaner and better aligned with the
overall design. I'd welcome any thoughts you have along the way. I
know this has been a difficult project, and I don't mean to come
across as taking any of it lightly. I'm still hopeful there's a path
forward, but I completely understand the need to reset here.

-- 
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Wed, 21 May 2025 19:22:26 +0900

>   https://wiki.postgresql.org/wiki/PostgreSQL_18_Open_Items

Thanks Tomas.

-- 
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Thu, 22 May 2025 17:12:24 +0900

> Fair enough. I’ll revert this and some related changes shortly.  WIP

I have pushed out the revert now.

Note that I’ve only reverted the changes related to deferring locks on
prunable partitions. I’m planning to leave the preparatory commits
leading up to that one in place unless anyone objects. For reference,
here they are in chronological order (the last 3 are bug fixes):

bb3ec16e14d Move PartitionPruneInfo out of plan nodes into PlannedStmt
d47cbf474ec Perform runtime initial pruning outside ExecInitNode()
cbc127917e0 Track unpruned relids to avoid processing pruned relations
75dfde13639 Fix an oversight in cbc127917 to handle MERGE correctly
cbb9086c9ef Fix bug in cbc127917 to handle nested Append correctly
28317de723b Ensure first ModifyTable rel initialized if all are pruned

I think separating initial pruning from plan node initialization is
still worthwhile on its own, as evidenced by the improvements in
cbc127917e.

-- 
Thanks, Amit Langote

---

From: Tomas Vondra <[email-masked]>
Date: Thu, 22 May 2025 15:04:45 +0200

> I have pushed out the revert now.

Thank you.

> still worthwhile on its own, as evidenced by the improvements in

I'm OK with that in principle, assuming the benefits outweigh the risk
of making backpatching harder. The patches don't seem exceptionally
large / invasive, but I don't know how often we modify these parts.

regards

-- 
Tomas Vondra

---

From: Robert Haas <[email-masked]>
Date: Thu, 22 May 2025 09:50:22 -0400

> to be a better and simpler way.

Without particularly defending this implementation, and certainly
without defending its bugs, I just want to say that I'm not convinced
by the idea that there has to be a better and simpler way. We --
principally Amit, but also me and you and others -- have been trying
to find the best way of doing this for probably 5 years now. If you do
something during executor startup, you have to be prepared for
executor startup to force a replan, and if you do something before
executor startup, then you're duplicating executor logic into a new
phase that needs to communicate its results forward to execution
proper. Either approach is awkward and that awkwardness seems to
inevitably bleed into the plan cache specifically. I'd be beyond
delighted if you want to help chart a path through the awkwardness
here, since you know this stuff better than anybody, but I am
skeptical that there is a truly marvelous approach which we've just
managed to overlook for all this time.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Amit Langote <[email-masked]>
Date: Fri, 23 May 2025 11:17:48 +0900

> large / invasive, but I don't know how often we modify these parts.

Thanks. I agree it's something to be mindful of, but I don’t expect
the reimplementation of the locking deferral to require changes to
this part of the code again. So barring any surprises, it shouldn't be
the case that the pruning code ends up looking significantly different
in v19.

Also, the actual pruning logic hasn’t changed much -- just where it’s
called from.

Let me know if any of that still raises concerns.

-- 
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Fri, 20 Jun 2025 21:30:18 +0900

> still worthwhile on its own, as evidenced by the improvements in

I've been thinking about how to address the concerns Tom raised about
the reverted patch.  Here's a summary of where my thinking currently
stands.

* CachedPlan invalidation handling:

The first issue is the part of the old design where a CachedPlan
invalidated during executor startup -- while locking unpruned
partitions -- was modified in place to replace the stale PlannedStmts
in its stmt_list with new ones obtained by replanning all queries in
the enclosing CachedPlanSource's query_list. I did that mainly to
ensure that replanning happens as soon as the executor discovers the
plan is invalid, instead of returning to the caller and requiring them
to go back to plancache.c to trigger replanning. There were many
issues with making that approach work in practice, because different
callers of the executor have different ways of running plans from a
CachedPlan -- with pquery.c in particular being hard to refactor
cleanly to support that flow.

The first alternative I came up with is to place only the query whose
PlannedStmt is being initialized into a standalone CachedPlanSource
and create a corresponding standalone CachedPlan. "Standalone" here
means that both objects are "saved" independently of the original
CachedPlanSource and CachedPlan, but are still tracked by the
invalidation callbacks.

But thinking about it more recently, what's actually important is not
whether we construct a new CachedPlan at all, but simply that we
replan just the one query that needs to be run, and use the resulting
PlannedStmt directly. The planner will have taken all required locks,
so we don't need to register the plan with the invalidation machinery
-- concurrent invalidations can't affect correctness.

In that case, the replanned PlannedStmt can be treated as transient
executor-local state, with no need to carry any of the plan cache
infrastructure along with it.  To support that, I further assume that,
because replanning and execution happen essentially back-to-back,
there's no opportunity for role-based or xmin-based invalidation (as
is checked for a CachedPlan in CheckCachedPlan()) to affect the plan
in between. If that reasoning holds, then we don't need to register
the replanned statement with the invalidation machinery at all.

Because we wouldn't have touched the original CachedPlan at all, the
stale PlannedStmts in it wouldn't be replaced until the next
GetCachedPlan() call triggers replanning. I'm willing to accept that
as a tradeoff for a less invasive design to handle replanning in the
executor.

Finally, it's worth noting that the executor is always passed the
entire CachedPlan, regardless of which individual statement is being
executed. Without per-statement validity tracking, it's hard for the
executor to tell whether replanning is actually needed for a given
query when the CachedPlan is marked invalid (is_valid=false), making
it impossible to selectively replan just one. To support that, what I
would need is validity tracking at the level of individual
PlannedStmts -- and perhaps even Querys -- in the source's query_list,
with the current is_valid flag effectively serving as the logical AND
of all the individual flags. We didn't need that in the old design,
because we'd replace all statements to mark the CachedPlan valid again
-- though Tom was right to point out flaws in the assumption that
setting is_valid like that was actually safe.

* ExecutorStart() interface damage control:

The other aspect I’ve been thinking about is how to contain the
changes required inside ExecutorStart(), and limit the disruption to
ExecutorStart_hooks in particular, while keeping changes for outside
callers narrowly scoped. In the previous patch, pruning, locking, and
invalidation checking were all done inside InitPlan(), which is called
by standard_ExecutorStart() -- an implementation choice that was
potentially disruptive to extensions using ExecutorStart_hook. Since
such hooks are expected to call standard_ExecutorStart() to perform
core plan initialization, they would have to check afterward whether
the plan had actually been initialized successfully, in case an
invalidation occurred during InitPlan(). That wasn’t optional, and it
made it easy for hook authors to miss the fact that
standard_ExecutorStart() could return without initializing the plan,
breaking expectations that were previously reliable.

Separately, for top-level callers of the executor, the patch
introduced a new entry point, ExecutorStartCachedPlan(), to avoid
requiring each caller to implement its own replanning loop. But that
approach was also awkward, since it required switching to a
nonstandard function just to get correct behavior.

What I’m thinking now is that we should instead move the logic for
pruning, deferred locking, and replanning directly into
ExecutorStart() itself. In the reverted patch, callers were affected
mainly because they had to choose between ExecutorStart() and a new
entry point, ExecutorStartCachedPlan(), which existed solely to handle
invalidation and replanning. That divergence from the standard API
made things awkward at the call site.

In contrast, the design I’m proposing avoids any need for new executor
entry points -- ExecutorStart() retains its original signature and
behavior, with the added benefit that replanning and pruning are now
handled internally before hooks or standard initialization logic are
invoked. The design requires moving some code from
standard_ExecutorStart() -- specifically the code that sets up the
EState and parameters -- and from InitPlan() -- namely, the parts that
initialize the range table, partition pruning state, and perform
ExecDoInitialPruning().

The callers of ExecutorStart() do still need to ensure that they pass
the CachedPlan, the CachedPlanSource, and the query_index in QueryDesc
via CreateQueryDesc(). The executor’s external API remains unchanged.

Importantly, this restructuring would not require any behavioral
changes for existing ExecutorStart_hook implementations. From a hook’s
point of view, this is a code motion change only. Hooks are still
invoked at the same point, but they’re now guaranteed to receive a
plan that is valid and ready for execution. This avoids the control
flow surprises introduced by the reverted patch -- specifically, the
need for hooks to detect whether standard_ExecutorStart() had
completed successfully -- while preserving the executor’s API and
execution contract as they exist in master.

I’ll hold off on writing any code for now -- just wanted to lay out
this direction and hear what others think, especially Tom.

--
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Thu, 17 Jul 2025 21:11:08 +0900

> this direction and hear what others think, especially Tom.

The refinements I described in my email above might help mitigate some
of those executor-related issues. However, I'm starting to wonder if
it's worth reconsidering our decision to handle pruning, locking, and
validation entirely at executor startup, which was the approach taken
in the reverted patch.

The alternative approach, doing initial pruning and locking within
plancache.c itself (which I floated a while ago), might be worth
revisiting. It avoids the complications we've discussed around the
executor API and preserves the clear separation of concerns that
plancache.c provides, though it does introduce some new layering
concerns, which I describe further below.

To support this, we'd need a mechanism to pass pruning results to the
executor alongside each PlannedStmt. For each PartitionPruneInfo in
the plan, that would include the corresponding PartitionPruneState and
the bitmapset of surviving relids determined by initial pruning. Given
that a CachedPlan can contain multiple PlannedStmts, this would
effectively be a list of pruning results, one per statement. One
reasonable way to handle that might be to define a parallel data
structure, separate from PlannedStmt, constructed by plancache.c and
carried via QueryDesc. The memory and lifetime management would mirror
how ParamListInfo is handled today, leaving the executor API unchanged
and avoiding intrusive changes to PlannedStmt.

However, one potentially problematic aspect of this design is managing
the lifecycle of the relations referenced by PartitionPruneState.
Currently, partitioned table relations are opened by the executor
after entering ExecutorStart() and closed automatically by
ExecEndPlan(), allowing cleanup of pruning states implicitly. If we
perform initial pruning earlier, we'd need to keep these relations
open longer, necessitating explicit cleanup calls (e.g., a new
FinishPartitionPruneState()) invoked by the caller of the executor,
such as from ExecutorEnd() or even higher-level callers. This
introduces some questionable layering by shifting responsibility for
relation management tasks, which ideally belong within the executor,
into its callers.

My sense is that the complexity involved in carrying pruning results
via this parallel data structure was one of the concerns Tom raised
previously, alongside the significant pruning code refactoring that
the earlier patch required. The latter, at least, should no longer be
necessary given recent code improvements.

I think that's about as many approaches as I can think of, and would
really appreciate others' thoughts on these alternatives.

-- 
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Tue, 22 Jul 2025 15:43:26 +0900

> necessary given recent code improvements.

One point I forgot to mention about this approach is that we'd also
need to ensure permissions on parent relations are checked before
performing initial pruning in plancache.c, since pruning may involve
evaluating user-provided expressions. So in effect, we'd need to
invoke not just ExecDoInitialPruning(), but also
ExecCheckPermissions(), or some variant of it, prior to executor
startup. While manageable, it does add slightly to the complexity.

-- 
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Wed, 12 Nov 2025 23:17:43 +0900

Hi,

> startup. While manageable, it does add slightly to the complexity.

Sorry for the absence. I've now implemented the approach mentioned
above and split it into a series of reasonably isolated patches.

The key idea is to avoid taking unnecessary locks when reusing a
cached plan. To achieve that, we need to perform initial partition
pruning during cached plan reuse in plancache.c so that only surviving
partitions are locked. This requires some plumbing to reuse the result
of this "early" pruning during executor startup, because repeating the
pruning logic would be both inefficient and potentially inconsistent
-- what if you get different results the second time? (I don't have
proof that this can happen, but some earlier emails mention the
theoretical risk, so better to be safe.)

So this patch introduces ExecutorPrep(), which allows executor
metadata such as initial pruning results (valid subplan indexes) and
full unpruned_relids to be computed ahead of execution and reused
later by ExecutorStart() and during QueryDesc setup in parallel
workers using the results shared by the leader. The parallel query bit
was discussed previously at [1], though I didn’t have a solution I
liked then.

This revives an idea that was last implemented in the patch (v30)
posted on Dec 16, 2022. In retrospect, I understand the hesitation Tom
might have had about the patch at the time -- its changes to enable
early pruning and then feed the results into ExecutorStart() were less
than pretty. Thanks to the initial pruning code refactoring that I
committed in Postgres 18, those changes now seem much more principled
and modular IMO.

The patch set is structured as follows:

* Refactor partition pruning initialization (0001): separates the
setup of the pruning state from its execution by introducing
ExecCreatePartitionPruneStates(). This makes the pruning logic easier
to reuse and adds flexibility to do only the setup but skip pruning in
some cases.

* Introduce ExecutorPrep infrastructure (0002): adds ExecutorPrep()
and ExecPrep as a formal way to perform executor setup ahead of
execution. This enables caching or transferring pruning results and
other metadata without triggering execution. ExecutorStart() can now
consume precomputed prep state from the EState created during
ExecutorPrep().  ExecPrepCleanup() handles cleanup when the plan is
invalidated during prep and so not executed; the state is cleaned up
in the regular ExecutorEnd() path otherwise.

* Allow parallel workers to reuse leader pruning results (0003): lets
workers reuse the leader’s initial pruning results (valid subplan
indexes) and unpruned_relids via ExecutorPrep().  This adds a
verification step to check that leader and worker decisions match,
throwing an error if they don’t -- so "reuse" is a bit of a lie.
Should that check be debug-only? (Maybe not.) As mentioned above, this
was previously discussed at [1].

* Enable pruning-aware locking in cached / generic plan reuse (0004):
extends GetCachedPlan() and CheckCachedPlan() to call ExecutorPrep()
on each PlannedStmt in the CachedPlan, locking only surviving
partitions. Adds CachedPlanPrepData to pass this through plan cache
APIs and down to execution via QueryDesc. Also reinstates the
firstResultRel locking rule added in 28317de72 but later lost due to
revert of the earlier pruning patch, to ensure correctness when all
target partitions are pruned.

This approach keeps plan caching and validation logic self-contained
in plancache.c, avoids invasive executor API changes.

Benchmark results:

echo "plan_cache_mode = force_generic_plan" >> $PGDATA/postgresql.conf
for p in 32 64 128 256 512 1024; do pgbench -i --partitions=$p >
/dev/null 2>&1; echo -ne "$p\t"; pgbench -n -S -T10 -Mprepared | grep
tps; done

Master

32 tps = 23841.822407 (without initial connection time)
64 tps = 21578.619816 (without initial connection time)
128 tps = 18090.500707 (without initial connection time)
256 tps = 14152.248201 (without initial connection time)
512 tps = 9432.708423 (without initial connection time)
1024 tps = 5873.696475 (without initial connection time)

Patched

32 tps = 24724.245798 (without initial connection time)
64 tps = 24858.206407 (without initial connection time)
128 tps = 24652.655269 (without initial connection time)
256 tps = 23656.756615 (without initial connection time)
512 tps = 22299.865769 (without initial connection time)
1024 tps = 21911.704317 (without initial connection time)

Comments welcome.

[1] https://www.postgresql.org/message-id/CA%2BHiwqFA%3DswkzgGK8AmXUNFtLeEXFJwFyY3E7cTxvL46aa1OTw%40mail.gmail.com

--
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Mon, 17 Nov 2025 21:50:01 +0900

...
> in the regular ExecutorEnd() path otherwise.

In v1 patch, I had not made ExecutorStart() call ExecutorPrep() to do
the prep work (creating EState, setting up es_relations, checking
permissions) when QueryDesc did not carry the results of
ExecutorPrep() from some earlier stage. Instead, InitPlan() would
detect that prep was absent and perform the missing setup itself. On
second thought it is cleaner for ExecutorStart() to detect the absence
of prep and call ExecutorPrep() directly, matching how prep would be
created when coming from plancache et al.

v2 changes the patch to do that.

> target partitions are pruned.

Looking at the changes to executor/function.c, I also noticed that I
had mistakenly allocated the ExecutorPrep state in
SQLFunctionCache.fcontext whereas the correct context for execution
related state is SQLFunctionCache.subcontext.  In the updated patch,
I've made postquel_start() reparent the prep EState's es_query_cxt to
subcontext from fcontext. I also did not have a test case that
exercised cached plan reuse for SQL functions, so I added one. I split
the function.c's GetCachedPlan() + CachedPlanPrepData plumbing into a
new patch 0005 so it can be reviewed separately, since it is the only
non-mechanical call-site change.

> 1024 tps = 21911.704317 (without initial connection time)

Re-ran to include 0 partition case and more partitions than 1024:

echo "plan_cache_mode = force_generic_plan" >> $PGDATA/postgresql.conf
for p in 0 8 16 32 64 128 256 512 1024 2048 4096; do pgbench -i
--partitions=$p > /dev/null 2>&1; echo -ne "$p\t"; pgbench -n -S -T10
-Mprepared | grep tps; done

Master

0 tps = 23600.068719 (without initial connection time)
8 tps = 22548.439906 (without initial connection time)
16 tps = 22807.337363 (without initial connection time)
32 tps = 22837.789996 (without initial connection time)
64 tps = 22915.846820 (without initial connection time)
128 tps = 22958.472655 (without initial connection time)
256 tps = 22432.432730 (without initial connection time)
512 tps = 20327.618690 (without initial connection time)
1024 tps = 20554.932475 (without initial connection time)
2048 tps = 19947.061061 (without initial connection time)
4096 tps = 17294.369829 (without initial connection time)

Patched

0 tps = 23869.906654 (without initial connection time)
8 tps = 22682.498914 (without initial connection time)
16 tps = 22714.445711 (without initial connection time)
32 tps = 21653.589371 (without initial connection time)
64 tps = 20571.267545 (without initial connection time)
128 tps = 17138.088269 (without initial connection time)
256 tps = 13027.168426 (without initial connection time)
512 tps = 8689.486966 (without initial connection time)
1024 tps = 5450.525617 (without initial connection time)
2048 tps = 3034.383108 (without initial connection time)
4096 tps = 1560.110609 (without initial connection time)

Tabular format (+ve pct_change means patched better)

 partitions    master        patched       pct_change
 ----------------------------------------------------
 0             23869.91      23600.07       -1.1%
 8             22682.50      22548.44       -0.6%
 16            22714.45      22807.34       +0.4%
 32            21653.59      22837.79       +5.5%
 64            20571.27      22915.85      +11.4%
 128           17138.09      22958.47      +34.0%
 256           13027.17      22432.43      +72.2%
 512            8689.49      20327.62     +133.9%
 1024           5450.53      20554.93     +277.1%
 2048           3034.38      19947.06     +557.4%
 4096           1560.11      17294.37    +1008.5%

I also did some runs for custom plans. The custom plan path should
behave about the same on master and patched since the early
ExecutorPrep() business only applies to generic plan reuse cases.

echo "plan_cache_mode = force_custom_plan" >> $PGDATA/postgresql.conf
for p in 0 8 16 32 64 128 256 512 1024 2048 4096; do pgbench -i
--partitions=$p > /dev/null 2>&1; echo -ne "$p\t"; pgbench -n -S -T10
-Mprepared | grep tps; done

Master

pgbench -n -S -T10 -Mprepared | grep tps; done
0 tps = 22346.419557 (without initial connection time)
8 tps = 20959.115560 (without initial connection time)
16 tps = 21390.573290 (without initial connection time)
32 tps = 21358.292393 (without initial connection time)
64 tps = 21288.742635 (without initial connection time)
128 tps = 21167.721447 (without initial connection time)
256 tps = 21256.618661 (without initial connection time)
512 tps = 19401.261197 (without initial connection time)
1024 tps = 19169.135145 (without initial connection time)
2048 tps = 19504.102179 (without initial connection time)
4096 tps = 18880.855783 (without initial connection time)

Patched

0 tps = 22852.634752 (without initial connection time)
8 tps = 21596.432690 (without initial connection time)
16 tps = 21428.779996 (without initial connection time)
32 tps = 20629.225272 (without initial connection time)
64 tps = 21301.644733 (without initial connection time)
128 tps = 21098.543942 (without initial connection time)
256 tps = 21394.364662 (without initial connection time)
512 tps = 19475.152170 (without initial connection time)
1024 tps = 19585.768438 (without initial connection time)
2048 tps = 19810.211969 (without initial connection time)
4096 tps = 19160.981608 (without initial connection time)

In tabular format:

 partitions    master        patched       pct_change
 ----------------------------------------------------
 0             22346.42      22852.63      +2.3%
 8             20959.12      21596.43      +3.0%
 16            21390.57      21428.78      +0.2%
 32            21358.29      20629.23      -3.4%
 64            21288.74      21301.64      +0.1%
 128           21167.72      21098.54      -0.3%
 256           21256.62      21394.36      +0.6%
 512           19401.26      19475.15      +0.4%
 1024          19169.14      19585.77      +2.2%
 2048          19504.10      19810.21      +1.6%
 4096          18880.86      19160.98      +1.5%

Numbers look within noise range as expected.

-- 
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Thu, 20 Nov 2025 16:30:26 +0900

> non-mechanical call-site change.

I also noticed a bug in the prep cleanup logic that runs when a cached
plan becomes invalid during the prep phase. Patch 0005 fixes that and
adds a regression test that exercises the invalidation path. This will
be folded into 0004 later.

-- 
Thanks, Amit Langote

---

From: Tender Wang <[email-masked]>
Date: Sun, 23 Nov 2025 20:17:11 +0800

> be folded into 0004 later.

I spent time looking at these patches.

I search all places that call GetCachedPlan(), and we always pass
&cprep(CachedPlanPrepData) to GetCachedPlan().
In PrepAndCheckCachedPlan(), if the plan_cache_mode is force_generic_plan,
the LockPolicy is always LOCK_UNPRUNED. Because *cprep has never been NULL.
It seems that the LockPolicy has no chance to be LOCK_ALL. Do I miss
something here?
-- 
Thanks,
Tender Wang

---

From: Chao Li <[email-masked]>
Date: Mon, 24 Nov 2025 11:29:53 +0800

Hi, Amit,

Locking only surviving partitions sounds a good optimization. I started to review this patch, but I cannot finish reviewing in one day. I will post my comments as long as I finished some commits.

> On Nov 20, 2025, at 15:30, Amit Langote <[email-masked]> wrote:
> 
> <v3-0004-Use-pruning-aware-locking-in-cached-plans.patch><v3-0005-Add-test-exercising-prep-cleanup-on-cached-plan-i.patch><v3-0002-Introduce-ExecutorPrep-and-refactor-executor-star.patch><v3-0006-Make-SQL-function-executor-track-ExecutorPrep-sta.patch><v3-0003-Reuse-partition-pruning-results-in-parallel-worke.patch><v3-0001-Refactor-partition-pruning-initialization-for-cla.patch>

0001 splits creations of es_part_prune_states into a new function ExecCreatePartitionPruneStates(). With that, you are trying to make the code clearer as you stated in the commit comment. However, the new function is not called, meaning 0001 is not self-contained, feels unusual to me according to the patches I have reviewed so far. I would suggest have ExecDoInitialPruning() call ExecCreatePartitionPruneStates() when es_part_prune_states is still NIL., so that current logic is unchanged, and 0001 can be pushed independently.

0002 moves check permission etc logic from InitPlan() to the new function ExecutorPrep(). The commit message says “executor setup logic unchanged”. Because in old code, before permission check, there was no PushActiveSnapshot(), but in the patch, before check permission, PushActiveSnapshot() is done, which may introduce different behavior, I just wonder why PushActiveSnapshot() is added?

Actually, I am still trying to understand 0002-0004, it would take me some time to fully understand the patch. I’d raise the above comments first. I will continue reviewing this patch tomorrow.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

---

From: Amit Langote <[email-masked]>
Date: Tue, 25 Nov 2025 10:56:43 +0900

On Sun, Nov 23, 2025 at 9:17 PM Tender Wang <[email-masked]> wrote:
> I search all places that call GetCachedPlan(), and we always pass &cprep(CachedPlanPrepData) to GetCachedPlan().
> In PrepAndCheckCachedPlan(), if the plan_cache_mode is force_generic_plan, the LockPolicy is always LOCK_UNPRUNED. Because *cprep has never been NULL.
> It seems that the LockPolicy has no chance to be LOCK_ALL. Do I miss something here?

Yes, eventually LockPolicy may end up redundant and we might not need
AcquireExecutorLocksPolicy() at all, with a single locking path
covering both cases.

My goal initially was to stage the changes across call sites: keep a
LOCK_ALL path for callers that still use the old lock everything up
front behaviour, and gradually convert other callers to pass a
non-NULL CachedPlanPrepData and handle the prep_list it may return, so
that GetCachedPlan() can perform LOCK_UNPRUNED locking internally.
That is why GetCachedPlan() accepts a possibly NULL cprep and why
LockPolicy exists as a separate knob.

For example, I decided to split out function.c refactoring of plan
cache usage into its own patch. That made me realise that new users of
GetCachedPlan() may appear that first adopt the simpler LOCK_ALL
behaviour and only later switch to UNPRUNED when pruning aware locking
becomes useful for them. Keeping the two paths preserves that
incremental route and avoids forcing every new user to adopt
CachedPlanPrepData and UNPRUNED locking up front. I am undecided yet
if that two path structure is a good idea, but I am inclined to keep
it for now. I would be happy to hear opinions on this.

-- 
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Tue, 25 Nov 2025 17:31:04 +0900

Hi Evan,

> Locking only surviving partitions sounds a good optimization. I started to review this patch, but I cannot finish reviewing in one day. I will post my comments as long as I finished some commits.

Thank you very much for taking the time to review.

> 0001 splits creations of es_part_prune_states into a new function ExecCreatePartitionPruneStates(). With that, you are trying to make the code clearer as you stated in the commit comment. However, the new function is not called, meaning 0001 is not self-contained, feels unusual to me according to the patches I have reviewed so far.

Oops, that is not intentional.

> I would suggest have ExecDoInitialPruning() call ExecCreatePartitionPruneStates() when es_part_prune_states is still NIL., so that current logic is unchanged, and 0001 can be pushed independently.

0002 adds a call to ExecDoInitialPruning() in ExecutorPrep(), preceded
by a call to ExecCreatePartitionPruneStates(), and that is how I think
it should be. So in the attached updated 0001, I have made InitPlan()
call ExecCreatePartitionPruneStates() before calling
ExecDoInitialPruning().

> 0002 moves check permission etc logic from InitPlan() to the new function ExecutorPrep(). The commit message says “executor setup logic unchanged”. Because in old code, before permission check, the…

That is a valid concern.

I found it necessary because the initial pruning code (which runs in
ExecDoInitialPruning()) may require ActiveSnapshot to be valid if
pruning expressions end up calling code that invokes
EnsurePortalSnapshotExists(). That requirement already existed when
ExecDoInitialPruning() was driven from ExecutorStart(), but
ExecutorPrep() can now be called from places that do not otherwise
push a snapshot. The snapshot push is only there to cover those
callers. It does not change permission checking itself, it just
ensures ExecutorPrep() runs with the same preconditions that
ExecutorStart() always had.

> Actually, I am still trying to understand 0002-0004, it would take me some time to fully understand the patch. I’d raise the above comments first. I will continue reviewing this patch tomorrow.

Thanks, I appreciate your review.

-- 
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Wed, 11 Feb 2026 13:05:50 +0900

Hi,

Here is v5 of this patch series.

Changes since v4:

* Removed the ExecPrep struct. ExecutorPrep() now returns an EState
directly, which is carried through QueryDesc->estate and prep_estates
lists. This simplifies the interface and avoids the ownership tracking
(owns_estate) that ExecPrep required.

* Moved the PARAM_EXEC values setup from
ExecCreatePartitionPruneState() to InitExecPartitionPruneContexts().
This allows ExecutorPrep() to create pruning state before the executor
has set up PARAM_EXEC slots.

* Added a standalone test (0003) that inspects pg_locks to show
baseline locking behavior: currently both pruning-on and pruning-off
lock all child partitions. The pruning-aware locking patch (0004) then
updates the expected output, making the behavioral change visible as a
diff.

* Absorbed the plan invalidation test (v4's 0005) into the locking
patch (0004), since that's where CachedPlanPrepCleanup lives.

* Moved the parallel worker reuse patch to the end of the series
(0006), since the core functionality doesn't depend on it.

* Fixed the missing i++ in CheckInitialPruningResultsInWorker(),
renamed it from ExecCheckInitialPruningResults(), and made it a
debug-only check (#ifdef USE_ASSERT_CHECKING) that verifies the
worker's pruning result matches the leader's.

The series is:

0001: Refactor partition pruning initialization for clarity
0002: Introduce ExecutorPrep and refactor executor startup
0003: Add test for partition lock behavior with generic cached plans
0004: Use pruning-aware locking in cached plans
0005: Make SQL function executor track ExecutorPrep state
0006: Reuse partition pruning results in parallel workers

The core of this series is ExecutorPrep() in 0002 and its use by
GetCachedPlan() in 0004. This is where I'd appreciate the most
scrutiny.

ExecutorPrep() factors out permission checks, range table
initialization, and initial partition pruning from InitPlan() into a
callable helper that can run before ExecutorStart(). This is what
makes pruning-aware locking possible: GetCachedPlan() calls
ExecutorPrep() during plan validation to determine which partitions
survive initial pruning, then locks only those (plus firstResultRel
targets). The resulting EStates are passed through CachedPlanPrepData
and eventually adopted by ExecutorStart(), so the pruning work isn't
repeated.

The risk is that this pulls part of what we traditionally consider
"execution" into a phase that runs during plan cache validation.
Specifically:

* Snapshot use: ExecutorPrep() requires an active snapshot for pruning
expressions that may call PL functions. During plan cache validation,
we use a transaction snapshot rather than the portal's execution
snapshot. I believe this is safe because initial pruning has always
used whatever snapshot happens to be active at the time
ExecDoInitialPruning() runs, but it's a subtle change in when that
happens.

* EState memory lifetime: The EState created by ExecutorPrep() lives
longer than it used to, since it's created during GetCachedPlan() but
consumed later during ExecutorStart(). CachedPlanPrepData manages
this: it tracks the EStates and their resource owner, and
CachedPlanPrepCleanup() frees them if the plan is invalidated before
execution reaches ExecutorStart(). I've tried to ensure that the
EState's memory context hierarchy stays consistent with the current
system through the reparenting done in portal/SPI/function paths.

* Plan invalidation: If the plan is invalidated between ExecutorPrep()
and ExecutorStart() (e.g., DDL on a partition during pruning), the
prep state is discarded by CachedPlanPrepCleanup() and GetCachedPlan()
retries. The invalidation regression test in 0004 exercises this path.

As of 0002, ExecutorStart() is the only caller of ExecutorPrep(), so
there is no behavioral change until 0004 wires it into
GetCachedPlan(). This makes it possible to review the refactoring in
isolation.

I would really appreciate review from folks familiar with the executor
lifecycle and plan cache internals. The approach touches the boundary
between plan caching and execution, and getting the lifetime and
snapshot semantics right is important.

A note on 0006: it is intentionally last and independently droppable.
It passes the leader's pruning results to parallel workers via
nodeToString/stringToNode, which is the simplest approach but adds
serialization overhead to all parallel queries regardless of whe

--
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Sat, 7 Mar 2026 18:54:27 +0900

Hi,

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.

--
Thanks,
Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Mon, 9 Mar 2026 13:41:51 +0900

> If anyone sees a problem with this approach, I'd like to hear about

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

---

From: Amit Langote <[email-masked]>
Date: Fri, 20 Mar 2026 02:20:49 +0900

> patch in v6; this version puts them where they belong.

Attached is an updated set. One more fix: I added an Assert in
SPI_cursor_open_internal()'s !plan->saved path to verify that
prep_estates is NIL. Unsaved plans always take the custom plan path,
so pruning-aware locking never applies, but it's worth guarding
explicitly since the copyObject/ReleaseCachedPlan sequence that
follows would not be safe otherwise. Also changed
SPI_plan_get_cached_plan() to pass NULL for cprep, since it only
returns the CachedPlan pointer and has no way to deliver prep_estates
to anyone.

Stepping back -- the core question is whether running executor logic
(pruning) inside GetCachedPlan() is acceptable at all. The plan cache
and executor have always had a clean boundary: plan cache locks
everything, executor runs. This optimization necessarily crosses that
line, because the information needed to decide which locks to skip
(pruning results) can only come from executor machinery.

The proposed approach has GetCachedPlan() call ExecutorPrep() to do a
limited subset of executor work (range table init, permissions,
pruning), carry the results out through CachedPlanPrepData, and leave
the CachedPlan itself untouched. The executor already has a multi-step
protocol: start/run/end. prep/start/run/end is just a finer
decomposition of what InitPlan() was already doing inside
ExecutorStart().

Of the attached patches, I'm targeting 0001-0003 for commit. 0004 (SQL
function support) and 0005 (parallel worker reuse) are useful
follow-ons but not essential.  The optimization works without them for
most cases, and they can be reviewed and committed separately.

If there's a cleaner way to avoid locking pruned partitions without
the plumbing this patch adds, I haven't found it in the year since the
revert.  I'd welcome a pointer if you see one.  Failing that, I think
this is the right trade-off, but it's a judgment call about where to
hold your nose.

Tom, I'd value your opinion on whether this approach is something
you'd be comfortable seeing in the tree.

--
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Wed, 25 Mar 2026 16:39:47 +0900

> you'd be comfortable seeing in the tree.

Attached is an updated set with some cleanup after another pass.

- Removed ExecCreatePartitionPruneStates() from 0001. In 0001-0003,
ExecDoInitialPruning() handles both setup and pruning internally; the
split isn't needed yet.

- Tightened commit messages to describe what each commit does now, not
what later commits will use it for. In particular, 0002 is upfront
that the portal/SPI/EXPLAIN plumbing is scaffolding that 0003 lights
up.

- Updated setrefs.c comment for firstResultRels to drop a blanket
claim about one ModifyTable per query level.

As before, 0001-0003 is the focus, maybe 0004 which teaches the new
GetCachedPlan() pruning-aware contract to its relatively new user in
function.c.

-- 
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Thu, 26 Mar 2026 18:24:28 +0900

> GetCachedPlan() pruning-aware contract to its relatively new user in

While reviewing the patch more carefully, I realized there's a
correctness issue when rule rewriting causes a single statement to
expand into multiple PlannedStmts in one CachedPlan.

PortalRunMulti() executes those statements sequentially, with
CommandCounterIncrement() between them, so Q2's ExecutorStart()
normally sees the effects of Q1.

With the patch, though, AcquireExecutorLocksUnpruned() runs
ExecutorPrep() on all PlannedStmts in one pass during GetCachedPlan(),
before any statement executes. If a later statement has
initial-pruning expressions that read data modified by an earlier one,
pruning can see stale results.

There's also a memory lifetime issue: PortalRunMulti() calls
MemoryContextDeleteChildren(portalContext) between statements, which
destroys EStates prepared for later statements.

Here's a concrete case demonstrating the semantic issue:

  create table multistmt_pt (a int, b int) partition by list (a);
  create table multistmt_pt_1 partition of multistmt_pt for values in (1);
  create table multistmt_pt_2 partition of multistmt_pt for values in (2);
  insert into multistmt_pt values (1, 0), (2, 0);

  create table prune_config (val int);
  insert into prune_config values (1);

  create function get_prune_val() returns int as $$
    select val from prune_config;
  $$ language sql stable;

  -- rule action runs first, updating prune_config before the
  -- original statement's pruning would normally be evaluated
  create rule config_upd_rule as on update to multistmt_pt
    do also update prune_config set val = 2;

  set plan_cache_mode to force_generic_plan;
  prepare multi_q as
    update multistmt_pt set b = b + 1 where a = get_prune_val();
  execute multi_q;  -- creates the generic plan

  -- reset for the real test
  update prune_config set val = 1;
  update multistmt_pt set b = 0;

  -- second execute reuses the plan
  execute multi_q;
  select * from multistmt_pt order by a;

Without the patch: the rule action updates prune_config to val=2
first, then after CCI the original statement's initial pruning calls
get_prune_val(), gets 2, prunes to multistmt_pt_2, and updates it
correctly: (1, 0), (2, 1).

With the patch as it stood: both statements' pruning runs during
GetCachedPlan() before either executes. The original statement's
pruning sees val=1, prunes to multistmt_pt_1, and multistmt_pt_2 is
never touched.

The fix is to skip pruning-aware locking for CachedPlans containing
multiple PlannedStmts, falling back to locking all partitions.
Single-statement plans are unchanged.

Since multi-statement plans are now excluded, CachedPlanPrepData no
longer needs a list of EStates -- it carries a single EState pointer.
This simplifies the plumbing throughout: PortalData,
PortalDefineQuery, SPI, and EXPLAIN all pass a single optional EState
instead of walking parallel lists. The next_prep_estate() helper is
gone.

Attached is the updated set.

--
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Fri, 27 Mar 2026 18:00:20 +0900

> Single-statement plans are unchanged.

For good measure, I also verified that Tom's test case from last May
[1] that prompted the revert of the previous commit works correctly
with this patch. When the DO ALSO rule is created mid-execution, the
plan gets invalidated and rebuilt as a multi-statement CachedPlan,
which triggers the fallback to locking all partitions. No assertions,
no crashes.

-- 
Thanks, Amit Langote

[1] https://postgr.es/m/[email-masked]

---

From: Amit Langote <[email-masked]>
Date: Sat, 4 Apr 2026 21:10:37 +0900

Attached is a redesigned version. While working on the previous
design, I grew increasingly uncomfortable with CachedPlanPrepData --
it was smuggling executor state out of GetCachedPlan() through an
out-parameter, which papered over the real problem: GetCachedPlan()
was doing too much. The main change in this version is architectural:
GetCachedPlan() no longer acquires execution locks. Callers now own
that responsibility, which is natural because each call site iterates
stmt_list differently and manages execution state in its own way --
and it lets them choose between conservative lock-all and
pruning-aware locking where appropriate.

Non-portal call sites remain on the conservative path for now.
_SPI_execute_plan requires care around snapshot setup, which happens
after plan fetch rather than before. SQL functions have a different
issue: init_execution_state() fetches the plan while postquel_start()
handles execution, with execution_state containers in between, making
it harder to thread a prepped QueryDesc through. The portal path and
EXPLAIN EXECUTE cover the most common
prepared-statement-with-partitions workloads; the remaining sites can
be converted incrementally.

This is now starting to feel closer to what Tom suggested back in
January 2023 [1], where he proposed getting rid of
AcquireExecutorLocks() inside GetCachedPlan() entirely and pushing
lock acquisition out to callers. He noted that "we'd be pushing the
responsibility for looping back and re-planning out to fairly
high-level calling code" and that "we'd definitely be changing some
fundamental APIs." That is the direction I came around to over the
last couple of weeks while wrestling with CachedPlanPrepData.  The
reverted approach also tried to follow Tom's direction but moved
locking into ExecutorStart(), which forced it to handle plan
invalidation from inside the executor by mutating the CachedPlan
in-place. This version moves locking out to the callers instead, so
the executor and plan cache never reach into each other.

The series is now four patches:

0001: Move execution lock acquisition out of GetCachedPlan(). Adds
AcquireExecutorLocks() as a caller-facing function with validity check
and retry. Adds PortalLockCachedPlan() in pquery.c to centralize the
portal retry logic. All callers are converted. No behavioral change.

0002: Refactor executor's initial partition pruning setup. Cleanup
only, no behavioral change.

0003: Introduce ExecutorPrep() and refactor executor startup. Factors
range table init, permission checks, and initial pruning out of
InitPlan(). Scaffolding for 0004; all callers still go through the
normal ExecutorStart() path.

0004: Use pruning-aware locking for single-statement cached plans.
Adds ExecutorPrepAndLock() which locks unprunable relations, runs
ExecutorPrep() to determine surviving partitions, then locks only
those. Extends PortalLockCachedPlan() with a pruning-aware path for
eligible plans. Multi-statement CachedPlans (from rule rewriting)
always use conservative locking. In principle, this could be relaxed
if the planner can prove that no pruning expression reads state
modified by an earlier statement, but that is left for a future patch.
Includes regression tests.

In case it's not clear, I'm not targeting v19 at this point.  I'd like
to get this into v20 CF1 and would welcome review from anyone
interested.

--
Thanks,
Amit Langote

[1] https://www.postgresql.org/message-id/4191508.1674157166%40sss.pgh.pa.us

---

From: Thom Brown <[email-masked]>
Date: Wed, 27 May 2026 13:03:24 +0100

> to get this into v20 CF1 and would welcome review from anyone

After not having looked at this in close to 2 years, I thought I'd
give it another look. Not found any user-facing issues, and I'm liking
seeing so few locks in pg_locks. I can see that with pruning disabled,
the fallback works, pruning-aware locking is working via SPI through
plpgsql, running ALTER between executions and also invalidating
indexes force replans, and it's looking good.

But I also think there might be a bug in patch 0001, but I'd
appreciate checking my reasoning because I'm not fully confident I've
been diligent enough.

When PortalStart() opens a SELECT cursor that's backed by a cached
plan, it does roughly the following. It builds a queryDesc (an
executor-side struct), one of whose fields is a pointer into the plan
tree inside the portal's cached plan. Then it calls
PortalLockCachedPlan() to acquire the necessary locks, and finally
hands the queryDesc over to the executor.

My worry is about what happens if the cached plan turns out to be
stale, for instance because someone ran DDL on a referenced table. In
that case PortalLockCachedPlan() throws the old plan away (via
ReleaseCachedPlan) and fetches a freshly-built replacement, updtating
the portal's own pointers to match. But the queryDesc from earlier
isn't touched. Its plan pointer still references the old, now-released
plan. From what I can see, once that old plan's last reference is
dropped its memory can be freed, which would leave the executor
reading from freed memory in the next step.

The bit I'm least sure about is whether the old plan's memory really
does get reclaimed straight away when its refcount hits zero. If
something keeps it alive longer then this isn't a bug, or at least not
as bad as I'm making out. I had a look but couldn't convince myself
either way from the code alone. To actually hit this you'd need a
cursor on a cached plan, plus an invalidation arriving in the small
window between the portal being set up and the cursor being opened.
The race condition is brief, and I've not been able to hit it in
testing.

The thing that got me thinking this is real: patch 0004 modifies
PortalLockCachedPlan() so that whenever it replans, it also rebuilds
the queryDesc. That's pretty much the fix I'd expect for this, which
makes me suspect somebody hit it at some point. But 0004 only applies
that fix on the new pruning-aware code path, and it was mentioned in
the thread that 0001 to 0003 might land before 0004. If so, master
would carry the bug in the gap between the two.

I suspect a way to deal with it would be to move the CreateQueryDesc
call in the SELECT case to after PortalLockCachedPlan() returns, which
is what the other portal strategies already seem to do. Alternatively,
you could bring 0004's changes in this area into 0001 and have
PortalLockCachedPlan() always rebuild the queryDesc when it replans.

If I've got this wrong and there's some lifetime mechanism I missed
that keeps the old plan's memory alive, then it's a non-issue and I'm
misreading the code. If I have got it wrong, could you please add
comments to make what is going on clearer?

Regards

Thom

---

From: Amit Langote <[email-masked]>
Date: Thu, 28 May 2026 17:13:53 +0900

Hi Thom,

On Wed, May 27, 2026 at 9:03 PM Thom Brown <[email-masked]> wrote:
> give it another look.

Thanks for taking a look.

> Not found any user-facing issues, and I'm liking

It's a real bug.

You're right that if PortalLockCachedPlan() replans, the QueryDesc
created before the call still points at the old PlannedStmt from the
released plan.  And yes, 0004 happens to fix it by rebuilding the
QueryDesc inside PortalLockCachedPlan(), but 0001 through 0003 are
broken on their own.

Attached is an updated set with the fix: CreateQueryDesc now runs
after PortalLockCachedPlan() returns, as you suggested.  That said,
I'll probably focus first on settling the plancache refactoring that
spun off from this thread [1], and then start a new thread for the
pruning-aware locking work on top of it, incorporating parts of this
series.

-- 
Thanks, Amit Langote

[1] https://www.postgresql.org/message-id/CA%2BHiwqE1ntHy2h9zJ9v3MwAkoGAveSERcHWkDTTZnP0kxWqbKQ%40mail.gmail.com

---

From: Thom Brown <[email-masked]>
Date: Thu, 28 May 2026 14:13:46 +0100

> pruning-aware locking work on top of it, incorporating parts of this

Thanks.

I've done another pass. I see a reference to
AcquireExecutorLocksUnpruned(), but I can't find this function. Is
this supposed to be AcquireExecutorLocksPrepared()?

And also I have a question about the new firstResultRels code

If I've followed it right, the bit in setrefs.c records the
lowest-numbered RT index from leaf_result_relids as the
per-ModifyTable fallback that's used when all real targets get pruned
away, and the executor side looks it up via
linitial_int(node->resultRelations). For that to work those two have
to pick the same RT index, and the comment justifies it with
"partition expansion preserves RT index order". Where is that
preservation guaranteed?

And with the assertion in ExecInitModifyTable:

Assert(list_member_int(estate->es_plannedstmt->firstResultRels, rti));

With writable CTEs producing more than one ModifyTable node the list
has several entries, so all the assert really checks is that some
recorded entry matches, not that the one recorded for this particular
node matches. If that's correct, then in a case where the wrong entry
happened to line up the right relation wouldn't be locked and nothing
would complain. Is there something that keeps these in order
somewhere?

Thom

---

From: Amit Langote <[email-masked]>
Date: Fri, 29 May 2026 17:56:51 +0900

> this supposed to be AcquireExecutorLocksPrepared()?

You're right, stale comment. It should say
AcquireExecutorLocksPrepared(). Fixed.

> "partition expansion preserves RT index order". Where is that

The ordering comes from expand_inherited_rtentry(), which adds child
partitions to the range table sequentially in partition bound order.
Since ModifyTable.resultRelations is built from the same expansion,
its first element is the lowest-numbered RT index among the leaf
partitions for that node. That is the same value
bms_next_member(leaf_result_relids, -1) returns from the Bitmapset,
because Bitmapset iteration returns members in ascending order. I've
added a comment in setrefs.c pointing to expand_inherited_rtentry() as
the source of this guarantee.

> would complain. Is there something that keeps these in order

This is a fair observation -- the Assert checks membership in the
global list rather than per-node correspondence. But node A's rti
can't accidentally pass the Assert by matching an entry recorded for
node B. Each ModifyTable node gets its own partition expansion with
distinct RT entries. In a writable CTE like:

  WITH upd1 AS (UPDATE t SET ...),
       upd2 AS (UPDATE t SET ...)
  UPDATE t SET ...

each UPDATE creates a separate set of leaf partition RT entries --
upd1 might get RT indexes 5,6,7, upd2 gets 8,9,10, and the main UPDATE
gets 11,12,13. The global firstResultRels list would be [5, 8, 11].
When ExecInitModifyTable falls back to linitial_int(resultRelations)
for a given node, it finds that node's own entry, because the RT index
sets are disjoint across nodes.

That said, it's worth being explicit about what protections exist at
each layer, since this is safety-critical code:

1. AcquireExecutorLocksPrepared(), added by 0004, locks every entry in
firstResultRels unconditionally. So regardless of which rti a
ModifyTable node falls back to, the relation will be locked.

2. ExecGetRangeTableRelation() has two checks when opening a relation.
For non-result relations (isResultRel=false), it checks
es_unpruned_relids and raises an ERROR in release builds if the
relation was pruned. For result relations (isResultRel=true), that
check is intentionally skipped -- it has to be, because at least one
result relation per ModifyTable node must remain openable even when
all partitions are pruned, since executor code paths like ExecMerge()
and ExecInitPartitionInfo() rely on resultRelInfo[0] being initialized
(see commit 28317de723b). The remaining protection for result
relations is Assert(CheckRelationLockedByMe()) inside table_open,
which fires in debug builds.

3. I've tightened ExecInitModifyTable to close this gap: the
all-pruned fallback path now raises an elog(ERROR) in release builds
if linitial_int(resultRelations) is not found in firstResultRels,
rather than just an Assert. This gives result relations a
production-visible check comparable to what es_unpruned_relids
provides for scan relations.

So the net effect is that for scan relations, opening a
pruned-and-unlocked relation is caught by an ERROR in production via
es_unpruned_relids. For result relations on the all-pruned fallback
path, it's now also caught by an ERROR in production via the
firstResultRels check in ExecInitModifyTable. The locking in
AcquireExecutorLocksPrepared() ensures the relation is always locked
regardless.

Thanks again for the review.  A close look at these aspects by someone
other than me is very useful.

-- 
Thanks, Amit Langote

---

From: Thom Brown <[email-masked]>
Date: Fri, 29 May 2026 11:30:29 +0100

> other than me is very useful.

Ah, the disjoint RT-entries point is what I was missing. I'd been
reading firstResultRels as a flat list where in theory any entry could
line up with any node's lookup, which is what made the assert feel
potentially insufficient. If each ModifyTable's expansion produces its
own non-overlapping set of leaf RT indexes then membership in the
global list really is equivalent to membership in this node's own
entry, and the assert is sufficient as it stands. Walking through the
writable-CTE case helped.

Thanks

Thom

---

From: Ilmar Yunusov <[email-masked]>
Date: Tue, 02 Jun 2026 17:54:09 +0000

The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:       tested, failed
Spec compliant:           not tested
Documentation:            not tested

Hi,

I looked at v13, focusing on apply/build status and relation-lock behavior for
reused generic plans after initial partition pruning.

I used the v13 series from Amit's 2026-05-29 message, on origin/master at
4b0bf0788b066a4ca1d4f959566678e44ec93422.

The series applies cleanly with git am, and git diff --check reports no
issues.

I first built with:

./configure --prefix="$PWD/pg-install" --without-readline --without-zlib --without-icu
make -s -j8
make -s install

make -C src/test/regress check

passed; all 245 tests passed, including plancache and partition_prune.

I also built a cassert/debug tree with:

./configure --prefix="$PWD/pg-install" --without-readline --without-zlib --without-icu --enable-cassert --enable-debug 'CFLAGS=-O0 -g'
make -s -j8
make -s install

and ran:

make -C src/test/regress check

which also passed; all 245 tests passed.

For the lock behavior, I used a list-partitioned table with force_generic_plan.
After the generic plan had been built and then reused, EXECUTE held only the
matching child partition lock. For example, EXECUTE q(1) held only the
following child lock:

manual_prunelock_p1

EXPLAIN EXECUTE behaved the same way on a reused generic plan; EXPLAIN EXECUTE
q(2) removed the other subplans and held only the following child lock:

manual_prunelock_p2

With enable_partition_pruning = off and a newly prepared statement, executing
the same SELECT held all child partition locks:

manual_prunelock_p1, manual_prunelock_p2, manual_prunelock_p3

I also ran a bounded cassert/debug stress check around plan invalidation. It
did 20 cycles where a child index was created and dropped before EXECUTE, and
20 similar cycles before EXPLAIN EXECUTE. In each cycle, the first execution
after invalidation/replanning held all child partition locks, and the next
execution reusing the generic plan held only the matching child partition lock.
That matches my reading that the patch is reducing locks for reused generic
plans, not for the execution that has to rebuild the plan.

One behavior I wanted to confirm: prepared UPDATE execution still held all
child partition locks in my manual check, including on the second execution
where the generic plan was being reused.

The test was:

prepare upd(int, text) as
  update stress_prunelock_p set b = $2 where a = $1;

Then both:

execute upd(3, 'updated-row-3');

and an all-pruned value:

execute upd(99, 'no-row');

held:

stress_prunelock_p1, stress_prunelock_p2, stress_prunelock_p3,
stress_prunelock_p4

pg_prepared_statements showed generic_plans increasing for this prepared
statement, so this was not a custom-plan case.

Is this expected for ModifyTable/result relations in v13, or did I miss an
eligibility condition that prevents pruning-aware locking from being used for
this prepared UPDATE case? I saw the recent firstResultRels discussion, but I
was not sure whether those changes are intended only to make pruned
result-relation initialization safe, or whether actual prepared DML execution
is expected to see reduced child partition locking as well.

I did not review the broader plancache refactoring design, did not run
installcheck-world, and did not test concurrent DDL from a separate session.

Regards,
Ilmar Yunusov

The new status of this patch is: Waiting on Author

---

From: Amit Langote <[email-masked]>
Date: Thu, 4 Jun 2026 09:25:51 +0900

Hi Ilmar,

> plans, not for the execution that has to rebuild the plan.

Thanks for thorough testing.

> is expected to see reduced child partition locking as well.

Yes, this is expected; the pruning-aware path currently only kicks in
for the portal strategy used by SELECT. I hadn't noticed that
UPDATE/DELETE ends up on a different strategy that bypasses the new
pruning-aware locking path. I need to think about how best to handle
this; the DML portal strategies defer executor startup to a later
point, so it may require some restructuring.

-- 
Thanks, Amit Langote