Subject: Re: generic plans and "initial" pruning

From: Robert Haas <[email-masked]>
Date: Thu, 15 Aug 2024 11:34:51 -0400

On Thu, Aug 15, 2024 at 8:57 AM Amit Langote <[email-masked]> wrote:
> TBH, it's more of a hunch that people who are not involved in this
> development might find the new reality, whereby the execution is not
> racefree until ExecutorRun(), hard to reason about.

I'm confused by what you mean here by "racefree". A race means
multiple sessions are doing stuff at the same time and the result
depends on who does what first, but the executor stuff is all
backend-private. Heavyweight locks are not backend-private, but those
would be taken in ExectorStart(), not ExecutorRun(), IIUC.

> With the patch, CreateQueryDesc() and ExecutorStart() are moved to
> PortalStart() so that QueryDescs including the PlanState trees for all
> queries are built before any is run.  Why?  So that if ExecutorStart()
> fails for any query in the list, we can simply throw out the QueryDesc
> and the PlanState trees of the previous queries (NOT run them) and ask
> plancache for a new CachedPlan for the list of queries.  We don't have
> a way to ask plancache.c to replan only a given query in the list.

I agree that moving this from PortalRun() to PortalStart() seems like
a bad idea, especially in view of what you write below.

> * There's no longer CCI() between queries in PortalRunMulti() because
> the snapshots in each query's QueryDesc must have been adjusted to
> reflect the correct command counter.  I've checked but can't really be
> sure if the value in the snapshot is all anyone ever uses if they want
> to know the current value of the command counter.

I don't think anything stops somebody wanting to look at the current
value of the command counter. I also don't think you can remove the
CommandCounterIncrement() calls between successive queries, because
then they won't see the effects of earlier calls. So this sounds
broken to me.

Also keep in mind that one of the queries could call a function which
does something that bumps the command counter again. I'm not sure if
that creates its own hazzard separate from the lack of CCIs, or
whether it's just another part of that same issue. But you can't
assume that each query's snapshot should have a command counter value
one more than the previous query.

While this all seems bad for the partially-initialized-execution-tree
approach, I wonder if you don't have problems here with the other
design, too. Let's say you've the multi-query case and there are 2
queries. The first one (Q1) is SELECT mysterious_function() and the
second one (Q2) is SELECT * FROM range_partitioned_table WHERE
key_column = 42. What if mysterious_function() performs DDL on
range_partitioned_table? I haven't tested this so maybe there are
things going on here that prevent trouble, but it seems like executing
Q1 can easily invalidate the plan for Q2. And then it seems like
you're basically back to the same problem.

> > > 3. The need to add *back* the fields to store the RT indexes of
> > > relations that are not looked at by ExecInitNode() traversal such as
> > > root partitioned tables and non-leaf partitions.
> >
> > I don't remember exactly why we removed those or what the benefit was,
> > so I'm not sure how big of a problem it is if we have to put them
> > back.
>
> We removed those in commit 52ed730d511b after commit f2343653f5b2
> removed redundant execution-time locking of non-leaf relations.  So we
> removed them because we realized that execution time locking is
> unnecessary given that AcquireExecutorLocks() exists and now we want
> to add them back because we'd like to get rid of
> AcquireExecutorLocks(). :-)

My bias is to believe that getting rid of AcquireExecutorLocks() is
probably the right thing to do, but that's not a strongly-held
position and I could be totally wrong about it. The thing is, though,
that AcquireExecutorLocks() is fundamentally stupid, and it's hard to
see how it can ever be any smarter. If we want to make smarter
decisions about what to lock, it seems reasonable to me to think that
the locking code needs to be closer to code that can evaluate
expressions and prune partitions and stuff like that.

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

---

From: Amit Langote <[email-masked]>
Date: Fri, 16 Aug 2024 21:35:56 +0900

> would be taken in ExectorStart(), not ExecutorRun(), IIUC.

Sorry, yes, I meant ExecutorStart().  A backend that wants to execute
a plan tree from a CachedPlan is in a race with other backends that
might modify tables before ExecutorStart() takes the remaining locks.
That race window is bigger when it is ExecutorStart() that will take
the locks, and I don't mean in terms of timing, but in terms of the
other code that can run in between GetCachedPlan() returning a
partially valid plan and ExecutorStart() takes the remaining locks
depending on the calling module.

> then they won't see the effects of earlier calls. So this sounds

I suppose you mean CCI between "running" (calling ExecutorRun on)
successive queries.  Then the patch is indeed broken.  If we're to
make that right, the number of CCIs for the multi-query portals will
have to double given the separation of ExecutorStart() and
ExecutorRun() phases.

> you're basically back to the same problem.

A rule (but not views AFAICS) can lead to the multi-query case (there
might be other ways).  I tried the following, and, yes, the plan for
the query queued by the rule is broken by the execution of that for
the 1st query:

create table foo (a int);
create table bar (a int);
create or replace function foo_trig_func () returns trigger as $$
begin drop table bar cascade; return new.*; end; $$ language plpgsql;
create trigger foo_trig before insert on foo execute function foo_trig_func();
create rule insert_foo AS ON insert TO foo do also insert into bar
values (new.*);
set plan_cache_mode to force_generic_plan ;
prepare q as insert into foo values (1);
execute q;
NOTICE:  drop cascades to rule insert_foo on table foo
ERROR:  relation with OID 16418 does not exist

The ERROR comes from trying to run (actually "initialize") the cached
plan for `insert into bar values (new.*);` which is due to the rule.

Though, it doesn't have to be a cached plan for the breakage to
happen.  You can see the same error without the prepared statement:

insert into foo values (1);
NOTICE:  drop cascades to rule insert_foo on table foo
ERROR:  relation with OID 16418 does not exist

Another example:

create or replace function foo_trig_func () returns trigger as $$
begin alter table bar add b int; return new.*; end; $$ language
plpgsql;
execute q;
ERROR:  table row type and query-specified row type do not match
DETAIL:  Query has too few columns.

insert into foo values (1);
ERROR:  table row type and query-specified row type do not match
DETAIL:  Query has too few columns.

This time the error occurs in ExecModifyTable(), so when "running" the
plan, but again the code that's throwing the error is just "lazy"
initialization of the ProjectionInfo when inserting into bar.

So it is possible for the executor to try to run a plan that has
become invalid since it was created, so...

> expressions and prune partitions and stuff like that.

One perhaps crazy idea [1]:

What if we remove AcquireExecutorLocks() and move the responsibility
of taking the remaining necessary locks into the executor (those on
any inheritance children that are added during planning and thus not
accounted for by AcquirePlannerLocks()), like the patch already does,
but don't make it also check if the plan has become invalid, which it
can't do anyway unless it's from a CachedPlan.  That means we instead
let the executor throw any errors that occur when trying to either
initialize the plan because of the changes that have occurred to the
objects referenced in the plan, like what is happening in the above
example.  If that case is going to be rare anway, why spend energy on
checking the validity and replan, especially if that's not an easy
thing to do as we're finding out.  In the above example, we could say
that it's a user error to create a rule like that, so it should not
happen in practice, but when it does, the executor seems to deal with
it correctly by refusing to execute a broken plan .  Perhaps it's more
worthwhile to make the executor behave correctly in face of plan
invalidation than teach the rest of the system to deal with the
executor throwing its hands up when it runs into an invalid plan?
Again, I think this may be a crazy line of thinking but just wanted to
get it out there.

--
Thanks, Amit Langote

[1] I recall Michael Paquier mentioning something like this to me once
when I was describing this patch and thread to him.

---

From: Robert Haas <[email-masked]>
Date: Mon, 19 Aug 2024 12:39:02 -0400

> become invalid since it was created, so...

I'm not sure what the "so what" here is.

> Again, I think this may be a crazy line of thinking but just wanted to

I don't know whether this is crazy or not. I think there are two
issues. One, the set of checks that we have right now might not be
complete, and we might just not have realized that because it happens
infrequently enough that we haven't found all the bugs. If that's so,
then a change like this could be a good thing, because it might force
us to fix stuff we should be fixing anyway. I have a feeling that some
of the checks you hit there were added as bug fixes long after the
code was written originally, so my confidence that we don't have more
bugs isn't especially high.

And two, it matters a lot how frequent the errors will be in practice.
I think we normally try to replan rather than let a stale plan be used
because we want to not fail, because users don't like failure. If the
design you propose here would make failures more (or less) frequent,
then that's a problem (or awesome).

---

From: Tom Lane <[email-masked]>
Date: Mon, 19 Aug 2024 12:54:14 -0400

> I'm not sure what the "so what" here is.

The fact that there are holes in our protections against that doesn't
make it a good idea to walk away from the protections.  That path
leads to crashes and data corruption and unhappy users.

What the examples here are showing is that AcquireExecutorLocks
is incomplete because it only provides defenses against DDL
initiated by other sessions, not by our own session.  We have
CheckTableNotInUse but I'm not sure if it could be applied here.
We certainly aren't calling that in anywhere near as systematic
a way as we have for acquiring locks.

Maybe we should rethink the principle that a session's locks
never conflict against itself, although I fear that might be
a nasty can of worms.

Could it work to do CheckTableNotInUse when acquiring an
exclusive table lock?  I don't doubt that we'd have to fix some
code paths, but if the damage isn't extensive then that
might offer a more nearly bulletproof approach.

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Mon, 19 Aug 2024 13:38:28 -0400

> never conflict against itself, although I fear that might be

It might not be that bad. It could replace the CheckTableNotInUse()
protections that we have today but maybe cover more cases, and it
could do so without needing any changes to the shared lock manager.
Say every time you start a query you give that query an ID number, and
all locks taken by that query are tagged with that ID number in the
local lock table, and maybe some flags indicating why the lock was
taken. When a new lock acquisition comes along you can say "oh, this
lock was previously taken so that we could do thus-and-so" and then
use that to fail with the appropriate error message. That seems like
it might be more powerful than the refcnt check within
CheckTableNotInUse().

But that seems somewhat incidental to what this thread is about. IIUC,
Amit's original design involved having the plan cache call some new
executor function to do partition pruning before lock acquisition, and
then passing that data structure around, including back to the
executor, so that we didn't repeat the pruning we already did, which
would be a bad thing to do not only because it would incur CPU cost
but also because really bad things would happen if we got a different
answer the second time. IIUC, you didn't think that was going to work
out nicely, and suggested instead moving the pruning+locking to
ExecutorStart() time. But now Amit is finding problems with that
approach, because by the time we reach PortalRun() for the
PORTAL_MULTI_QUERY case, it's too late to replan, because we can't ask
the plancache to replan just one query from the list; and if we try to
fix that by moving ExecutorStart() to PortalStart(), then there are
other problems. Do you have a view on what the way forward might be?

This thread has gotten a tad depressing, honestly. All of the opinions
about what we ought to do seem to be based on the firm conviction that
X or Y or Z will not work, rather than on the confidence that A or B
or C will work. Yet I'm inclined to believe this problem is solvable.

---

From: Tom Lane <[email-masked]>
Date: Mon, 19 Aug 2024 13:52:07 -0400

Robert Haas <[email-masked]> writes:
> But that seems somewhat incidental to what this thread is about.

Perhaps.  But if we're running into issues related to that, it might
be good to set aside the long-term goal for a bit and come up with
a cleaner answer for intra-session locking.  That could allow the
pruning problem to be solved more cleanly in turn, and it'd be
an improvement even if not.

> Do you have a view on what the way forward might be?

I'm fresh out of ideas at the moment, other than having a hope that
divide-and-conquer (ie, solving subproblems first) might pay off.

> or C will work. Yet I'm inclined to believe this problem is solvable.

Yeah.  We are working in an extremely not-green field here, which
means it's a lot easier to see pre-existing reasons why X will not
work than to have confidence that it will work.  But hey, if this
were easy then we'd have done it already.

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Mon, 19 Aug 2024 14:20:46 -0400

> an improvement even if not.

Maybe, but the pieces aren't quite coming together for me. Solving
this would mean that if we execute a stale plan, we'd be more likely
to get a good error and less likely to get a bad, nasty-looking
internal error, or a crash. That's good on its own terms, but we don't
really want user queries to produce errors at all, so I don't think
we'd feel any more free to rearrange the order of operations than we
do today.

> divide-and-conquer (ie, solving subproblems first) might pay off.

Fair enough, but why do you think that the original approach of
creating a data structure from within the plan cache mechanism
(probably via a call into some new executor entrypoint) and then
feeding that through to ExecutorRun() time can't work? Is it possible
you latched onto some non-optimal decisions that the early versions of
the patch made, rather than there being a fundamental problem with the
concept?

I actually thought the do-it-at-executorstart-time approach sounded
pretty good, even though we might have to abandon planstate tree
initialization partway through, right up until Amit started talking
about moving ExecutorStart() from PortalRun() to PortalStart(), which
I have a feeling is going to create a bigger problem than we can
solve. I think if we want to save that approach, we should try to
figure out if we can teach the plancache to replan one query from a
list without replanning the others, which seems like it might allow us
to keep the order of major operations unchanged. Otherwise, it makes
sense to me to have another go at the other approach, at least to make
sure we understand clearly why it can't work.

> were easy then we'd have done it already.

Yeah, true.

---

From: Amit Langote <[email-masked]>
Date: Tue, 20 Aug 2024 22:00:38 +0900

> I'm not sure what the "so what" here is.

I meant that if the executor has to deal with broken plans anyway, we
might as well lean into that fact by choosing not to handle only the
cached plan case in a certain way.  Yes, I understand that that's not
a good justification.

> bugs isn't especially high.

This makes sense.

> then that's a problem (or awesome).

I think we'd modify plancache.c to postpone the locking of only
prunable relations (i.e., partitions), so we're looking at only a
handful of concurrent modifications that are going to cause execution
errors.  That's because we disallow many DDL modifications of
partitions unless they are done via recursion from the parent, so the
space of errors in practice would be smaller compared to if we were to
postpone *all* cached plan locks to ExecInitNode() time.  DROP INDEX
a_partion_only_index comes to mind as something that might cause an
error.  I've not tested if other partition-only constraints can cause
unsafe behaviors.

Perhaps, we can add the check for CachedPlan.is_valid after every
table_open() and index_open() in the executor that takes a lock or at
all the places we discussed previously and throw the error (say:
"cached plan is no longer valid") if it's false.  That's better than
running into and throwing into some random error by soldiering ahead
with its initialization / execution, but still a loss in terms of user
experience because we're adding a new failure mode, however rare.

-- 
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Tue, 20 Aug 2024 22:14:58 +0900

> we'd feel any more free to rearrange the order of operations than we

Yeah, it's unclear whether executing a potentially stale plan is an
acceptable tradeoff compared to replanning, especially if it occurs
rarely. Personally, I would prefer that it is.

> feeding that through to ExecutorRun() time can't work?

That would be ExecutorStart().  The data structure need not be
referenced after ExecInitNode().

> Is it possible

+1

-- 
Thanks, Amit Langote

---

From: Robert Haas <[email-masked]>
Date: Tue, 20 Aug 2024 10:53:45 -0400

> error.  I've not tested if other partition-only constraints can cause

This seems like a valid point to some extent, but in other contexts
we've had discussions about how we don't actually guarantee all that
much uniformity between a partitioned table and its partitions, and
it's been questioned whether we made the right decisions there. So I'm
not entirely sure that the surface area for problems here will be as
narrow as you're hoping -- I think we'd need to go through all of the
ALTER TABLE variants and think it through. But maybe the problems
aren't that bad.

It does seem like constraints can change the plan. Imagine the
partition had a CHECK(false) constraint before and now doesn't, or
something.

---

From: Amit Langote <[email-masked]>
Date: Wed, 21 Aug 2024 21:45:24 +0900

> ALTER TABLE variants and think it through. But maybe the problems

Many changeable properties that are reflected in the RelationData of a
partition after getting the lock on it seem to cause no issues as long
as the executor code only looks at RelationData, which is true for
most Scan nodes.  It also seems true for ModifyTable which looks into
RelationData for relation properties relevant to insert/deletes.

The two things that don't cope are:

* Index Scan nodes with concurrent DROP INDEX of partition-only indexes.

* Concurrent DROP CONSTRAINT of partition-only CHECK and NOT NULL
constraints can lead to incorrect result as I write below.

> partition had a CHECK(false) constraint before and now doesn't, or

Yeah, if the CHECK constraint gets dropped concurrently, any new rows
that got added after that will not be returned by executing a stale
cached plan, because the plan would have been created based on the
assumption that such rows shouldn't be there due to the CHECK
constraint.  We currently don't explicitly check that the constraints
that were used during planning still exist before executing the plan.

Overall, I'm starting to feel less enthused by the idea throwing an
error in the executor due to known and unknown hazards of trying to
execute a stale plan.  Even if we made a note in the docs of such
hazards, any users who run into these rare errors are likely to head
to -bugs or -hackers anyway.

Tom said we should perhaps look at the hazards caused by intra-session
locking, but we'd still be left with the hazards of missing index and
constraints,  AFAICS, due to DROP from other sessions.

So, the options:

* The replanning aspect of the lock-in-the-executor design would be
simpler if a CachedPlan contained the plan for a single query rather
than a list of queries, as previously mentioned. This is particularly
due to the requirements of the PORTAL_MULTI_QUERY case. However, this
option might be impractical.

* Polish the patch for the old design of doing the initial pruning
before AcquireExecutorLocks() and focus on hashing out any bugs and
issues of that design.

-- 
Thanks, Amit Langote

---

From: Robert Haas <[email-masked]>
Date: Wed, 21 Aug 2024 09:10:06 -0400

> option might be impractical.

It might be, but maybe it would be worth a try? I mean,
GetCachedPlan() seems to just call pg_plan_queries() which just loops
over the list of query trees and does the same thing for each one. If
we wanted to replan a single query, why couldn't we do
fake_querytree_list = list_make1(list_nth(querytree_list, n)) and then
call pg_plan_queries(fake_querytree_list)? Or something equivalent to
that. We could have a new GetCachedSinglePlan(cplan, n) to do this.

> before AcquireExecutorLocks() and focus on hashing out any bugs and

That's also an option. It probably has issues too, but I don't know
what they are exactly.

---

From: Amit Langote <[email-masked]>
Date: Fri, 23 Aug 2024 21:48:27 +0900

> that. We could have a new GetCachedSinglePlan(cplan, n) to do this.

I've been hacking to prototype this, and it's showing promise. It
helps make the replan loop at the call sites that start the executor
with an invalidatable plan more localized and less prone to
action-at-a-distance issues. However, the interface and contract of
the new function in my prototype are pretty specialized for the replan
loop in this context—meaning it's not as general-purpose as
GetCachedPlan(). Essentially, what you get when you call it is a
'throwaway' CachedPlan containing only the plan for the query that
failed during ExecutorStart(), not a plan integrated into the original
CachedPlanSource's stmt_list. A call site entering the replan loop
will retry the execution with that throwaway plan, release it once
done, and resume looping over the plans in the original list. The
invalid plan that remains in the original list will be discarded and
replanned in the next call to GetCachedPlan() using the same
CachedPlanSource. While that may sound undesirable, I'm inclined to
think it's not something that needs optimization, given that we're
expecting this code path to be taken rarely.

I'll post a version of a revamped locks-in-the-executor patch set
using the above function after debugging some more.

--
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Thu, 29 Aug 2024 22:34:17 +0900

> using the above function after debugging some more.

Here it is.

0001 implements changes to defer the locking of runtime-prunable
relations to the executor.  The new design introduces a bitmapset
field in PlannedStmt to distinguish at runtime between relations that
are prunable whose locking can be deferred until ExecInitNode() and
those that are not and must be locked in advance.  The set of prunable
relations can be constructed by looking at all the PartitionPruneInfos
in the plan and checking which are subject to "initial" pruning steps.
The set of unprunable relations is obtained by subtracting those from
the set of all RT indexes.  This design gets rid of one annoying
aspect of the old design which was the need to add specialized fields
to store the RT indexes of partitioned relations that are not
otherwise referenced in the plan tree. That was necessary because in
the old design, I had removed the function AcquireExecutorLocks()
altogether to defer the locking of all child relations to execution.
In the new design such relations are still locked by
AcquireExecutorLocks().

0002 is the old patch to make ExecEndNode() robust against partially
initialized PlanState nodes by adding NULL checks.

0003 is the patch to add changes to deal with the CachedPlan becoming
invalid before the deferred locks on prunable relations are taken.
I've moved the replan loop into a new wrapper-over-ExecutorStart()
function instead of having the same logic at multiple sites.  The
replan logic uses the GetSingleCachedPlan() described in the quoted
text.  The callers of the new ExecutorStart()-wrapper, which I've
dubbed ExecutorStartExt(), need to pass the CachedPlanSource and a
query_index, which is the index of the query being executed in the
list CachedPlanSource.query_list.  They are needed by
GetSingleCachedPlan().  The changes outside the executor are pretty
minimal in this design and all the difficulties of having to loop back
to GetCachedPlan() are now gone.  I like how this turned out.

One idea that I think might be worth trying to reduce the footprint of
0003 is to try to lock the prunable relations in a step of InitPlan()
separate from ExecInitNode(), which can be implemented by doing the
initial runtime pruning in that separate step.  That way, we'll have
all the necessary locks before calling ExecInitNode() and so we don't
need to sprinkle the CachedPlanStillValid() checks all over the place
and worry about missed checks and dealing with partially initialized
PlanState trees.

-- 
Thanks, Amit Langote

---

From: Junwang Zhao <[email-masked]>
Date: Sat, 31 Aug 2024 20:30:34 +0800

Hi,

> and worry about missed checks and dealing with partially initialized

@@ -1241,7 +1244,7 @@ GetCachedPlan(CachedPlanSource *plansource,
ParamListInfo boundParams,
  if (customplan)
  {
  /* Build a custom plan */
- plan = BuildCachedPlan(plansource, qlist, boundParams, queryEnv);
+ plan = BuildCachedPlan(plansource, qlist, boundParams, queryEnv, true);

Is the *true* here a typo? Seems it should be *false* for custom plan?

-- 
Regards
Junwang Zhao

---

From: Amit Langote <[email-masked]>
Date: Mon, 2 Sep 2024 17:19:39 +0900

> Is the *true* here a typo? Seems it should be *false* for custom plan?

That's correct, thanks for catching that.  Will fix.

-- 
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Thu, 5 Sep 2024 18:55:47 +0900

> That's correct, thanks for catching that.  Will fix.

Done.

I've also rewritten the new GetSingleCachedPlan() function in 0003.
The most glaring bug in the previous version was that the transient
CachedPlan it creates cannot be seen by PlanCacheRelCallback() et al
functions because it was intentionally not linked to the
CachedPlanSource, so if the CachedPlan would not be invalidated even
if some prunable relation got changed before it is locked during
ExecutorStart().  I've added a new list standalone_plan_list to add
these to and changed the inval callback functions to invalidate any
plans contained in them.

Another thing I found out through testing is that CachedPlanSource can
have become invalid since leaving GetCachedPlan() (actually even
before returning from that function) because of
PlanCacheSysCallback(), which drops/invalidates *all* plans when a
syscache is invalidated.  There are comments in plancache.c (see
BuildCachedPlan()) saying that such invalidations are, in theory,
false positives, but that gave me a pause nonetheless.

Finally, instead of calling GetCachedPlan() from GetSingleCachedPlan()
to create a plan for only the query whose plan got invalidated, which
required a bunch of care to ensure that the CachedPlanSource is not
overwritten with the information about this single-query planning,
I've made GetSingleCachedPlan() create the PlannedStmt and the
detached CachedPlan object on its own, borrowing the minimal necessary
code from BuildCachedPlan() to do so.

-- 
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Tue, 17 Sep 2024 21:57:03 +0900

> and worry about missed checks and dealing with partially initialized

I've worked on this and found that it results in a much simpler design.

Attached are 0001 and 0002, which contain patches to refactor the
runtime pruning code. These changes move initial pruning outside of
ExecInitNode() and use the results during ExecInitNode() to determine
the set of child subnodes to initialize.

With that in place, the patches (0003, 0004) that move the locking of
prunable relations from plancache.c into the executor becomes simpler.
It no longer needs to modify any code called by ExecInitNode(). Since
no new locks are taken during ExecInitNode(), I didn't have to worry
about changing all the code involved in PlanState tree initialization
to add checks for CachedPlan validity.  The check is only needed after
performing initial pruning, and if the CachedPlan is invalid,
ExecInitNode() won’t be called in the first place.

-- 
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Thu, 19 Sep 2024 17:39:51 +0900

> ExecInitNode() won’t be called in the first place.

Sorry, I had missed merging some hunks into 0002 that fixed obsolete
comments.  Fixed in the attached v54.

Regarding 0002, I was a bit bothered by the need to add a new function
just to iterate over the PartitionPruningDatas and the
PartitionedRelPruningData they contain, solely to initialize the
PartitionPruneContext needed for exec pruning. To address this, I
propose 0003, which moves the initialization of those contexts to be
done "lazily" in find_matching_subplan_recurse(), where they are
actually used. To make this work, I added an is_valid flag to
PartitionPruneContext, which is checked as follows in the code block
where it's initialized:

+        if (unlikely(!pprune->exec_context.is_valid))

I didn't notice any overhead of adding this to
find_matching_partitions_recurse() which is called for every instance
of exec pruning, so I think it's worthwhile to consider 0003.

I realized that I had missed considering, in the
delay-locking-to-executor patch (now 0004), that there may be plan
objects belonging to pruned partitions, such as RowMarks and
ResultRelInfos, which should not be initialized.
ExecGetRangeTableRelation() invoked with the RT indexes in these
objects would cause crashes in Assert builds since the pruned
partitions would not have been locked. I've updated the patch to
ignore RowMarks and result relations (in ModifyTable.resultRelations)
for pruned child relations, which required adding more accounting info
to EState to store the bitmapset of unpruned RT indexes. For
ResultRelInfos, I took the approach of memsetting them to 0 for pruned
result relations and adding checks at multiple sites to ensure the
ResultRelInfo being handled is valid. I recall previously proposing
lazy initialization for these objects when first needed [1], which
would make the added code unnecessary, but I might save that for
another time.

--
Thanks, Amit Langote
[1] https://postgr.es/m/[email-masked]

---

From: Amit Langote <[email-masked]>
Date: Thu, 19 Sep 2024 21:10:04 +0900

On Thu, Sep 19, 2024 at 5:39 PM Amit Langote <[email-masked]> wrote:
> For
> ResultRelInfo being handled is valid.

After some reflection, I realized that nobody would think that that
approach is very robust. In the attached, I’ve modified
ExecInitModifyTable() to allocate ResultRelInfos only for unpruned
relations, instead of allocating for all in
ModifyTable.resultRelations and setting pruned ones to 0. This
approach feels more robust.

-- 
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Fri, 20 Sep 2024 17:10:03 +0900

On Thu, Sep 19, 2024 at 9:10 PM Amit Langote <[email-masked]> wrote:
> After some reflection,

Not enough reflection, evidently...

> I realized that nobody would think that that

Except, I forgot that ModifyTable has other lists that parallel
resultRelations (of the same length) viz. withCheckOptionLists,
returningLists, and updateColnosLists, which need to be similarly
truncated to only consider unpruned relations.  I've updated 0004 to
do so.  This was broken even in the other design where locking is
delayed all the way until ExecInitAppend does initial pruning(),
because ResultRelInfos are created before initializing the plan
subtree containing the Append node, which would try to lock and open
*all* partitions.

Also, I've switched the order of 0002 and 0003 to avoid a situation
where I add a function in 0002 only to remove it in 0003.  By doing
the refactoring to initialize PartitionPruneContexts lazily first, the
patch to move the initial pruning to occur before ExecInitNode()
became much simpler as it doesn't need to touch the code related to
exec pruning.

-- 
Thanks, Amit Langote

---

From: Robert Haas <[email-masked]>
Date: Thu, 10 Oct 2024 16:15:40 -0400

Hi Amit,

This is not a full review (sorry!) but here are a few comments.

In general, I don't have a problem with this direction. I thought
Tom's previous proposal of abandoning ExecInitNode() in medias res if
we discover that we need to replan was doable and I still think that,
but ISTM that this approach needs to touch less code, because
abandoning ExecInitNode() partly through means we could have leftover
state to clean up in any node in the PlanState tree, and as we've
discussed, ExecEndNode() isn't necessarily prepared to clean up a
PlanState tree that was only partially processed by ExecInitNode(). As
far as I can see in the time I've spent looking at this today, 0001
looks pretty unobjectionable (with some exceptions that I've noted
below). I also think 0003 looks pretty safe. It seems like partition
pruning moves backward across a pretty modest amount of code that does
pretty well-defined things. Basically, initialization-time pruning now
happens before other types of node initialization, and before setting
up row marks. I do however find the changes in 0002 to be less
obviously correct and less obviously safe; see below for some notes
about that.

In 0001, the name root_parent_relids doesn't seem very clear to me,
and neither does the explanation of what it does. You say
"'root_parent_relids' identifies the relation to which both the parent
plan and the PartitionPruneInfo given by 'part_prune_index' belong."
But it's a set, so what does it mean to identify "the" relation? It's
a set of relations, not just one. And why does the name include the
word "root"? It's neither the PlannerGlobal object, which we often
call root, nor is it the root of the partitioning hierarchy. To me, it
looks like it's just the set of relids that we can potentially prune.
I don't see why this isn't just called "relids", like the field from
which it's copied:

+       pruneinfo->root_parent_relids = parentrel->relids;

It just doesn't seem very root-y or very parent-y.

-       node->part_prune_info = partpruneinfo;
+

Extra blank line.

In 0002, the handling of ExprContexts seems a little bit hard to
understand. Sometimes we're using the PlanState's ExprContext, and
sometimes we're using a separate context owned by the
PartitionedRelPruningData's context, and it's not exactly clear why
that is or what the consequences are. Likewise I wouldn't mind some
more comments or explanation in the commit message of the changes in
this patch related to EState objects. I can't help wondering if the
changes here could have either semantic implications (like expression
evaluation can produce different results than before) or performance
implications (because we create objects that we didn't previously
create). As noted above, this is really my only design-level concern
about 0001-0003.

Typo: partrtitioned

Regrettably, I have not looked seriously at 0004 and 0005, so I can't
comment on those.

---

From: Amit Langote <[email-masked]>
Date: Fri, 11 Oct 2024 16:30:23 +0900

Robert,

> This is not a full review (sorry!) but here are a few comments.

Thank you for taking a look.

> PlanState tree that was only partially processed by ExecInitNode().

I will say that I feel more comfortable committing and be responsible
for the refactoring I'm proposing in 0001-0003 than the changes
required to take locks during ExecInitNode(), as seen in the patches
up to version v52..

> As
> a set of relations, not just one.

The intention is to ensure that the bitmapset in PartitionPruneInfo
corresponds to the apprelids bitmapset in the Append or MergeAppend
node that owns the PartitionPruneInfo. Essentially, root_parent_relids
is used to cross-check that both sets align, ensuring that the pruning
logic applies to the same relations as the parent plan.

> And why does the name include the

Maybe just "relids" suffices with a comment updated like this:

 * relids               RelOptInfo.relids of the parent plan node (e.g. Append
 *                      or MergeAppend) to which his PartitionPruneInfo node
 *                      belongs. Used to ensure that the pruning logic matches
 *                      the parent plan's apprelids.

> -       node->part_prune_info = partpruneinfo;

Fixed.

> create).

I have taken another look at whether there's any real need to use
separate ExprContexts for initial and runtime pruning and ISTM there
isn't, so we can make "exec" pruning use the same ExprContext as what
"init" would have used.  There *is* a difference however in how we
initializing the partition key expressions for initial and runtime
pruning, but it's not problematic to use the same ExprContext.

I'll update the commentary a bit more.

> Typo: partrtitioned

Fixed.

> Regrettably, I have not looked seriously at 0004 and 0005, so I can't

Ok, I'm updating 0005 to change how the CachedPlan is handled when it
becomes invalid during InitPlan(). Currently (v56), a separate
transient CachedPlan is created for the query being initialized when
invalidation occurs. However, it seems better to update the original
CachedPlan in place to avoid extra bookkeeping for transient plans—an
approach Robert suggested in an off-list discussion.

Will post a new version next week.

-- 
Thanks, Amit Langote

---

From: Robert Haas <[email-masked]>
Date: Tue, 15 Oct 2024 10:38:10 -0400

>  *                      the parent plan's apprelids.

LGTM.

---

From: Tom Lane <[email-masked]>
Date: Tue, 15 Oct 2024 11:22:42 -0400

> LGTM.

"his" -> "this", surely?

			regards, tom lane

---

From: Amit Langote <[email-masked]>
Date: Fri, 25 Oct 2024 21:30:24 +0900

> Will post a new version next week.

Sorry for the delay.

I've completed hacking on the approach to update the existing
CachedPlan in-place when it’s invalidated during plan initialization
in its stmt_list. Previously, we created transient (living for that
execution) CachedPlans for each query/plan, tracked separately from
the original CachedPlan, so that invalidation callbacks could
reference them. This meant that the original CachedPlan would continue
to hold invalid plans until the next GetCachedPlan() call.

With the new approach, the original CachedPlan is updated directly:
new PlannedStmts are installed into the existing stmt_list, allowing
any callers iterating over that list to continue unaffected. The new
UpdateCachedPlan() function now creates new plans for all queries in
the CachedPlan’s owning CachedPlanSource, replacing the previous
plans, and marks it valid.  So the CachedPlan becomes valid
immediately instead of in the next GetCachedPlan().

One caveat is that, without a dedicated memory context for the
PlannedStmts in stmt_list, the old ones leak into CacheMemoryContext.
However, since UpdateCachedPlan() is rarely invoked, I haven’t focused
on addressing this leak. If needed, we could introduce an additional
memory context next to CachedPlan.context, which would allow freeing
the PlannedStmts without affecting the stmt_list. For now, I’ve
ensured that stmt_list itself is not overwritten in
UpdateCachedPlan().

UpdateCachedPlan() is added in 0005.

I've kept 0005, the patch to retry execution with an updated plan if
the plan becomes invalid after taking locks on prunable relations
(deferred until initial pruning), separate for now. However, I plan to
eventually merge it into 0004, the patch implementing deferred
locking.

I've also fixed the comment in 0003 about PartitionPruneInfo.relid as
Tom pointed out, which now reads:

 * relids               RelOptInfo.relids of the parent plan node (e.g. Append
 *                      or MergeAppend) to which this PartitionPruneInfo node
 *                      belongs.  The pruning logic ensures that this matches
 *                      the parent plan node's apprelids.

I've stared at the refactoring patches 0001-0003 long enough at this
point to think that they are good for committing.

-- 
Thanks, Amit Langote

---

From: Tomas Vondra <[email-masked]>
Date: Sun, 1 Dec 2024 19:36:21 +0100

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.

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)));

Perhaps it should even be an assert?

2) unnecessary newline added to execPartition.h

3) this comment in EState doesn't seem very helpful

    List       *es_part_prune_infos;    /* PlannedStmt.partPruneInfos */

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.

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?

2) inconsistent spelling: run-time vs. runtime

3) PartitionPruneContext.is_valid - I think I'd rename the flag to
"initialized" or something like that. The "is_valid" is a bit confusing,
because it might seem the context can get invalidated later, but AFAICS
that's not the case - we just initialize it lazily.

0003
----

1) In InitPlan I'd move

    estate->es_part_prune_infos = plannedstmt->partPruneInfos;

   before the comment, which is more about ExecDoInitialPruning.

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

0004
----

1) typo: paraller/parallel

2) What about adding an assert to ExecFindMatchingSubPlans, to check
valisubplan_rtis is not NULL? It's just mentioned in a comment, but
better to explicitly enforce that?

2) It may not be quite clear why ExecInitUpdateProjection() switches to
mt_updateColnosLists. Should that be explained in a comment, somewhere?

3) unnecessary newline in ExecLookupResultRelByOid

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?

2) Isn't it a bit fragile if this requires every extension to update
and add the ExecPlanStillValid() calls to various places? What if an
extension doesn't do that? What weirdness will happen? 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?

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:

1) bad #1

IIRC the patch should not affect results for "force_custom_plan" cache
mode (and "auto", which does mostly the same thing, I think). And for
most runs that's true, with results ~100% of master. But there's a
couple curious exceptions - e.g. results for 0 partitions and 16 locks
show a consistent regression of ~10% (in the "-M prepared" mode).

I'm not terribly worried about this because it only shows for 16 locks,
and the default is 64. If someone reduces this GUC value, they should
expect some impact.

Still, it only shows in the "auto" case. I wonder why is that. Strange.

2) bad #2

There's also a similar regression in the "force_generic_plan" without
partitions (with "-M prepared"). This seems more consistent and affects
all the lock counts.

3) good

There's an area os massive improvements (in the 2-200x range) with 100+
partitions. The fast-path patch helped a bit, but this is much better,
of course.

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?

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?

regards

-- 
Tomas Vondra

---

From: Amit Langote <[email-masked]>
Date: Wed, 4 Dec 2024 22:34:16 +0900

Hi Tomas,

> Robert, I can't really compete with that.

Thanks for looking at this.  These are helpful.

>                                    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)

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

> 2) unnecessary newline added to execPartition.h

Perhaps you meant "removed".  Fixed.

>     List       *es_part_prune_infos;    /* PlannedStmt.partPruneInfos */

Agreed, fixed to be like the comment for es_rteperminfos:

List       *es_part_prune_infos;    /* List of PartitionPruneInfo */

> 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;

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

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

> that's not the case - we just initialize it lazily.

Agree that "initialized" is better, so renamed.

>    before the comment, which is more about ExecDoInitialPruning.

Makes sense, done.

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

> 1) typo: paraller/parallel

Oops, fixed.

> better to explicitly enforce that?

Good idea, done.

> 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().

> 3) unnecessary newline in ExecLookupResultRelByOid

Removed.

> the user session somehow, or what?

It will get called again after ExecutorStartExt() loops back to do
ExecutorStart() with a new updated plan tree.

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

> Maybe it'd be

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?

> 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

That's correct. The planner does not consider runtime pruning when
assigning costs to Append or MergeAppend paths in
create_{merge}append_path().

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

--
Thanks, Amit Langote

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

---

From: Amit Langote <[email-masked]>
Date: Thu, 5 Dec 2024 21:03:09 +0900

> I'll post the updated patches tomorrow.

Here is an updated set.

I said that Assert suffices in ExecInitPartitionPruning() but on
further thought and after finding the following in ExecInitExprRec()
that an elog() won't hurt.

                    /* planner messed up */
                    elog(ERROR, "Aggref found in non-Agg plan node");

Like this:

+    /* Obtain the pruneinfo we need. */
+    pruneinfo = list_nth_node(PartitionPruneInfo, estate->es_part_prune_infos,
+                              part_prune_index);
+
+    /* Its relids better match the plan node's or the planner messed up. */
+    if (!bms_equal(relids, pruneinfo->relids))
+        elog(ERROR, "wrong pruneinfo with relids=%s found at
part_prune_index=%d contained in plan node with relids=%s",
+             bmsToString(pruneinfo->relids), part_prune_index,
+             bmsToString(relids));

I've merged what were 0004 and 0005 in v57 together because they would
be eventually committed together and I wanted to write a unified
commit message.

One notable change is that I've renamed ExecutorStartExt() to
ExecutorStartCachedPlan() and changed its callers to only call it if a
CachedPlan is available, calling ExecutorStart() otherwise.

I'm still looking at Tomas's perf numbers and haven't confirmed some
of the findings myself.

--
Thanks, Amit Langote