Subject: Re: generic plans and "initial" pruning

From: Amit Langote <[email-masked]>
Date: Thu, 6 Apr 2023 08:23:31 +0900

On Tue, Apr 4, 2023 at 10:29 PM Amit Langote <[email-masked]>
wrote:
> On Tue, Apr 4, 2023 at 6:41 AM Tom Lane <[email-masked]> wrote:
> > A few concrete thoughts:
> >
> > * I understand that your plan now is to acquire locks on all the
> > originally-named tables, then do permissions checks (which will
> > involve only those tables), then dynamically lock just inheritance and
> > partitioning child tables as we descend the plan tree.
>
> Actually, with the current implementation of the patch, *all* of the
> relations mentioned in the plan tree would get locked during the
> ExecInitNode() traversal of the plan tree (and of those in
> plannedstmt->subplans), not just the inheritance child tables.
> Locking of non-child tables done by the executor after this patch is
> duplicative with AcquirePlannerLocks(), so that's something to be
> improved.
>
> > That seems
> > more or less okay to me, but it could be reflected better in the
> > structure of the patch perhaps.
> >
> > * In particular I don't much like the "viewRelations" list, which
> > seems like a wart; those ought to be handled more nearly the same way
> > as other RTEs.  (One concrete reason why is that this scheme is going
> > to result in locking views in a different order than they were locked
> > during original parsing, which perhaps could contribute to deadlocks.)
> > Maybe we should store an integer list of which RTIs need to be locked
> > in the early phase?  Building that in the parser/rewriter would provide
> > a solid guide to the original locking order, so we'd be trivially sure
> > of duplicating that.  (It might be close enough to follow the RT list
> > order, which is basically what AcquireExecutorLocks does today, but
> > this'd be more certain to do the right thing.)  I'm less concerned
> > about lock order for child tables because those are just going to
> > follow the inheritance or partitioning structure.
>
> What you've described here sounds somewhat like what I had implemented
> in the patch versions till v31, though it used a bitmapset named
> minLockRelids that is initialized by setrefs.c.  Your idea of
> initializing a list before planning seems more appealing offhand than
> the code I had added in setrefs.c to populate that minLockRelids
> bitmapset, which would be bms_add_range(1, list_lenth(finalrtable)),
> followed by bms_del_members(set-of-child-rel-rtis).
>
> I'll give your idea a try.

After sleeping on this, I think we perhaps don't need to remember
originally-named relations if only for the purpose of locking them for
execution.  That's because, for a reused (cached) plan,
AcquirePlannerLocks() would have taken those locks anyway.

AcquirePlannerLocks() doesn't lock inheritance children because they would
be added to the range table by the planner, so they should be locked
separately for execution, if needed.  I thought taking the execution-time
locks only when inside ExecInit[Merge]Append would work, but then we have
cases where single-child Append/MergeAppend are stripped of the
Append/MergeAppend nodes by setrefs.c.  Maybe we need a place to remember
such child relations, that is, only in the cases where Append/MergeAppend
elision occurs, in something maybe esoteric-sounding like
PlannedStmt.elidedAppendChildRels or something?

Another set of child relations that are not covered by Append/MergeAppend
child nodes is non-leaf partitions.  I've proposed adding a List of
Bitmapset field to Append/MergeAppend named 'allpartrelids' as part of this
patchset (patch 0001) to track those for execution-time locking.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

---

From: Amit Langote <[email-masked]>
Date: Thu, 8 Jun 2023 23:23:21 +0900

Here is a new version.  Summary of main changes since the last version
that Tom reviewed back in April:

* ExecInitNode() subroutines now return NULL (as opposed to a
partially initialized PlanState node as in the last version) upon
detecting that the CachedPlan that the plan tree is from is no longer
valid due to invalidation messages processed upon taking locks.  Plan
tree subnodes that are fully initialized till the point of detection
are added by ExecInitNode() into a List in EState called
es_inited_plannodes.  ExecEndPlan() now iterates over that list to
close each one individually using ExecEndNode().  ExecEndNode() or its
subroutines thus no longer need to be recursive to close the child
nodes.  Also, with this design, there is no longer the possibility of
partially initialized PlanState trees with partially initialized
individual PlanState nodes, so the ExecEndNode() subroutine changes
that were in the last version to account for partial initialization
are not necessary.

* Instead of setting EXEC_FLAG_GET_LOCKS in es_top_eflags for the
entire duration of InitPlan(), it is now only set in ExecInitAppend()
and ExecInitMergeAppend(), because that's where the subnodes scanning
child tables would be and the executor only needs to lock child tables
to validate a CachedPlan in a race-free manner.  Parent tables that
appear in the query would have been locked by AcquirePlannerLocks().
Child tables whose scan subnodes don't appear under Append/MergeAppend
(due to the latter being removed by setrefs.c for there being only a
single child) are identified in PlannedStmt.elidedAppendChildRelations
and InitPlan() locks each one found there if the plan tree is from a
CachedPlan.

* There's no longer PlannedStmt.viewRelations, because view relations
need not be tracked separately for locking as AcquirePlannerLocks()
covers them.

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 3 Jul 2023 15:27:22 +0200

> On 8 Jun 2023, at 16:23, Amit Langote <[email-masked]> wrote:
> 
> Here is a new version.

The local planstate variable in the hunk below is shadowing the function
parameter planstate which cause a compiler warning:

@@ -1495,18 +1556,15 @@ ExecEndPlan(PlanState *planstate, EState *estate)
 	ListCell   *l;
 
 	/*
-	 * shut down the node-type-specific query processing
-	 */
-	ExecEndNode(planstate);
-
-	/*
-	 * for subplans too
+	 * Shut down the node-type-specific query processing for all nodes that
+	 * were initialized during InitPlan(), both in the main plan tree and those
+	 * in subplans (es_subplanstates), if any.
 	 */
-	foreach(l, estate->es_subplanstates)
+	foreach(l, estate->es_inited_plannodes)
 	{
-		PlanState  *subplanstate = (PlanState *) lfirst(l);
+		PlanState  *planstate = (PlanState *) lfirst(l);

--
Daniel Gustafsson

---

From: Amit Langote <[email-masked]>
Date: Thu, 6 Jul 2023 23:29:10 +0900

> parameter planstate which cause a compiler warning:

Thanks Daniel for the heads up.

Attached new version fixes that and contains a few other notable
changes.  Before going into the details of those changes, let me
reiterate in broad strokes what the patch is trying to do.

The idea is to move the locking of some tables referenced in a cached
(generic) plan from plancache/GetCachedPlan() to the
executor/ExecutorStart().  Specifically, the locking of inheritance
child tables.  Why?  Because partition pruning with "initial pruning
steps" contained in the Append/MergeAppend nodes may eliminate some
child tables that need not have been locked to begin with, though the
pruning can only occur during ExecutorStart().

After applying this patch, GetCachedPlan() only locks the tables that
are directly mentioned in the query to ensure that the
analyzed-rewritten-but-unplanned query tree backing a given CachedPlan
is still valid (cf RevalidateCachedQuery()), but not the tables in the
CachedPlan that would have been added by the planner.  Tables in a
CachePlan that would not be locked currently only include the
inheritance child tables / partitions of the tables mentioned in the
query.  This means that the plan trees in a given CachedPlan returned
by GetCachedPlan() are only partially valid and are subject to
invalidation because concurrent sessions can possibly modify the child
tables referenced in them before ExecutorStart() gets around to
locking them.  If the concurrent modifications do happen,
ExecutorStart() is now equipped to detect them by way of noticing that
the CachedPlan is invalidated and inform the caller to discard and
recreate the CachedPlan.  This entails changing all the call sites of
ExecutorStart() that pass it a plan tree from a CachedPlan to
implement the replan-and-retry-execution loop.

Given the above, ExecutorStart(), which has not needed so far to take
any locks (except on indexes mentioned in IndexScans), now needs to
lock child tables if executing a cached plan which contains them.  In
the previous versions, the patch used a flag passed in
EState.es_top_eflags to signal ExecGetRangeTableRelation() to lock the
table.  The flag would be set in ExecInitAppend() and
ExecInitMergeAppend() for the duration of the loop that initializes
child subplans with the assumption that that's where the child tables
would be opened.  But not all child subplans of Append/MergeAppend
scan child tables (think UNION ALL queries), so this approach can
result in redundant locking.  Worse, I needed to invent
PlannedStmt.elidedAppendChildRelations to separately track child
tables whose Scan nodes' parent Append/MergeAppend would be removed by
setrefs.c in some cases.

So, this new patch uses a flag in the RangeTblEntry itself to denote
if the table is a child table instead of the above roundabout way.
ExecGetRangeTableRelation() can simply look at the RTE to decide
whether to take a lock or not.  I considered adding a new bool field,
but noticed we already have inFromCl to track if a given RTE is for
table/entity directly mentioned in the query or for something added
behind-the-scenes into the range table as the field's description in
parsenodes.h says.  RTEs for child tables are added behind-the-scenes
by the planner and it makes perfect sense to me to mark their inFromCl
as false.  I can't find anything that relies on the current behavior
of inFromCl being set to the same value as the root inheritance parent
(true).  Patch 0002 makes this change for child RTEs.

A few other notes:

* A parallel worker does ExecutorStart() without access to the
CachedPlan that the leader may have gotten its plan tree from.  This
means that parallel workers do not have the ability to detect plan
tree invalidations.  I think that's fine, because if the leader would
have been able to launch workers at all, it would also have gotten all
the locks to protect the (portion of) the plan tree that the workers
would be executing.  I had an off-list discussion about this with
Robert and he mentioned his concern that each parallel worker would
have its own view of which child subplans of a parallel Append are
"valid" that depends on the result of its own evaluation of initial
pruning.   So, there may be race conditions whereby a worker may try
to execute plan nodes that are no longer valid, for example, if the
partition a worker considers valid is not viewed as such by the leader
and thus not locked.  I shared my thoughts as to why that sounds
unlikely at [1], though maybe I'm a bit too optimistic?

* For multi-query portals, you can't now do ExecutorStart()
immediately followed by ExecutorRun() for each query in the portal,
because ExecutorStart() may now fail to start a plan if it gets
invalidated.   So PortalStart() now does ExecutorStart()s for all
queries and remembers the QueryDescs for PortalRun() then to do
ExecutorRun()s using.  A consequence of this is that
CommandCounterIncrement() now must be done between the
ExecutorStart()s of the individual plans in PortalStart() and not
between the ExecutorRun()s in PortalRunMulti().  make check-world
passes with this new arrangement, though I'm not entirely confident
that there are no problems lurking.

[1] https://postgr/es/m/CA+HiwqFA=[email-masked]

---

From: Amit Langote <[email-masked]>
Date: Thu, 13 Jul 2023 21:58:38 +0900

> that there are no problems lurking.

In an absolutely brown-paper-bag moment, I realized that I had not
updated src/backend/executor/README to reflect the changes to the
executor's control flow that this patch makes.   That is, after
scrapping the old design back in January whose details *were*
reflected in the patches before that redesign.

Anyway, the attached fixes that.

Tom, do you think you have bandwidth in the near future to give this
another look?  I think I've addressed the comments that you had given
back in April, though as mentioned in the previous message, there may
still be some funny-looking aspects still remaining.  In any case, I
have no intention of pressing ahead with the patch without another
committer having had a chance to sign off on it.

---

From: Thom Brown <[email-masked]>
Date: Mon, 17 Jul 2023 17:32:51 +0100

> committer having had a chance to sign off on it.

I've only just started taking a look at this, and my first test drive
yields very impressive results:

8192 partitions (3 runs, 10000 rows)
Head 391.294989 382.622481 379.252236
Patched 13088.145995 13406.135531 13431.828051

Looking at your changes to README, I would like to suggest rewording
the following:

+table during planning.  This means that inheritance child tables, which are
+added to the query's range table during planning, if they are present in a
+cached plan tree would not have been locked.

To:

This means that inheritance child tables present in a cached plan
tree, which are added to the query's range table during planning,
would not have been locked.

Also, further down:

s/intiatialize/initialize/

I'll carry on taking a closer look and see if I can break it.

Thom

---

From: Amit Langote <[email-masked]>
Date: Tue, 18 Jul 2023 16:26:35 +0900

Hi Thom,

> Patched 13088.145995 13406.135531 13431.828051

Just to be sure, did you use pgbench --Mprepared with plan_cache_mode
= force_generic_plan in postgresql.conf?

> I'll carry on taking a closer look and see if I can break it.

Thanks for looking.  I've fixed these issues in the attached updated
patch.  I've also changed the position of a newly added paragraph in
src/backend/executor/README so that it doesn't break the flow of the
existing text.

---

From: Thom Brown <[email-masked]>
Date: Tue, 18 Jul 2023 09:36:55 +0100

> = force_generic_plan in postgresql.conf?

I did.

For full disclosure, I also had max_locks_per_transaction set to 10000.

> src/backend/executor/README so that it doesn't break the flow of the

Thanks.

Thom

---

From: Amit Langote <[email-masked]>
Date: Wed, 2 Aug 2023 22:39:45 +0900

While chatting with Robert about this patch set, he suggested that it
would be better to break out some executor refactoring changes from
the main patch (0003) into a separate patch.  To wit, the changes to
make the PlanState tree cleanup in ExecEndPlan() non-recursive by
walking a flat list of PlanState nodes instead of the recursive tree
walk that ExecEndNode() currently does.  That allows us to cleanly
handle the cases where the PlanState tree is only partially
constructed when ExecInitNode() detects in the middle of its
construction that the plan tree is no longer valid after receiving and
processing an invalidation message on locking child tables.  Or at
least more cleanly than the previously proposed approach of adjusting
ExecEndNode() subroutines for the individual node types to gracefully
handle such partially initialized PlanState trees.

With the new approach, node type specific subroutines of ExecEndNode()
need not close its child nodes, because ExecEndPlan() would close each
node that would have been initialized directly.  I couldn't find any
instance of breakage by this decoupling of child node cleanup from
their parent node's cleanup.  Comments in ExecEndGather() and
ExecEndGatherMerge() appear to suggest that outerPlan must be closed
before the local cleanup:

 void
 ExecEndGather(GatherState *node)
 {
-   ExecEndNode(outerPlanState(node));  /* let children clean up first */
+   /* outerPlan is closed separately. */
    ExecShutdownGather(node);
    ExecFreeExprContext(&node->ps);

But I don't think there's a problem, because what ExecShutdownGather()
does seems entirely independent of cleanup of outerPlan.

As for the performance impact of initializing the list of initialized
nodes to use during the cleanup phase, I couldn't find a regression,
nor any improvement by replacing the tree walk by linear scan of a
list.  Actually, ExecEndNode() is pretty far down in the perf profile
anyway, so the performance difference caused by the patch hardly
matters.  See the following contrived example:

create table f();
analyze f;
explain (costs off) select count(*) from f f1, f f2, f f3, f f4, f f5,
f f6, f f7, f f8, f f9, f f10;
                                  QUERY PLAN
------------------------------------------------------------------------------
 Aggregate
   ->  Nested Loop
         ->  Nested Loop
               ->  Nested Loop
                     ->  Nested Loop
                           ->  Nested Loop
                                 ->  Nested Loop
                                       ->  Nested Loop
                                             ->  Nested Loop
                                                   ->  Nested Loop
                                                         ->  Seq Scan on f f1
                                                         ->  Seq Scan on f f2
                                                   ->  Seq Scan on f f3
                                             ->  Seq Scan on f f4
                                       ->  Seq Scan on f f5
                                 ->  Seq Scan on f f6
                           ->  Seq Scan on f f7
                     ->  Seq Scan on f f8
               ->  Seq Scan on f f9
         ->  Seq Scan on f f10
(20 rows)

do $$
begin
for i in 1..100000 loop
perform count(*) from f f1, f f2, f f3, f f4, f f5, f f6, f f7, f f8,
f f9, f f10;
end loop;
end; $$;

Times for the DO:

Unpatched:
Time: 756.353 ms
Time: 745.752 ms
Time: 749.184 ms

Patched:
Time: 737.717 ms
Time: 747.815 ms
Time: 753.456 ms

I've attached the new refactoring patch as 0001.

Another change I've made in the main patch is to change the API of
ExecutorStart() (and ExecutorStart_hook) more explicitly to return a
boolean indicating whether or not the plan initialization was
successful.  That way seems better than making the callers figure that
out by seeing that QueryDesc.planstate is NULL and/or checking
QueryDesc.plan_valid.  Correspondingly, PortalStart() now also returns
true or false matching what ExecutorStart() returned.  I suppose this
better alerts any extensions that use the ExecutorStart_hook to fix
their code to do the right thing.

Having extracted the ExecEndNode() change, I'm also starting to feel
inclined to extract a couple of other bits from the main patch as
separate patches, such as moving the ExecutorStart() call from
PortalRun() to PortalStart() for the multi-query portals.  I'll do
that in the next version.

---

From: Amit Langote <[email-masked]>
Date: Thu, 3 Aug 2023 17:37:39 +0900

> that in the next version.

Here's a patch set where the refactoring to move the ExecutorStart()
calls to be closer to GetCachedPlan() (for the call sites that use a
CachedPlan) is extracted into a separate patch, 0002.  Its commit
message notes an aspect of this refactoring that I feel a bit nervous
about -- needing to also move the CommandCounterIncrement() call from
the loop in PortalRunMulti() to PortalStart() which now does
ExecutorStart() for the PORTAL_MULTI_QUERY case.

---

From: Robert Haas <[email-masked]>
Date: Mon, 7 Aug 2023 11:36:32 -0400

> ExecutorStart() for the PORTAL_MULTI_QUERY case.

I spent some time today reviewing 0001. Here are a few thoughts and
notes about things that I looked at.

First, I wondered whether it was really adequate for ExecEndPlan() to
just loop over estate->es_plan_nodes and call it good. Put
differently, is it possible that we could ever have more than one
relevant EState, say for a subplan or an EPQ execution or something,
so that this loop wouldn't cover everything? I found nothing to make
me think that this is a real danger.

Second, I wondered whether the ordering of cleanup operations could be
an issue. Right now, a node can position cleanup code before, after,
or both before and after recursing to child nodes, whereas with this
design change, the cleanup code will always be run before recursing to
child nodes. Here, I think we have problems. Both ExecGather and
ExecEndGatherMerge intentionally clean up the children before the
parent, so that the child shutdown happens before
ExecParallelCleanup(). Based on the comment and commit
acf555bc53acb589b5a2827e65d655fa8c9adee0, this appears to be
intentional, and you can sort of see why from looking at the stuff
that happens in ExecParallelCleanup(). If the instrumentation data
vanishes before the child nodes have a chance to clean things up,
maybe EXPLAIN ANALYZE won't reflect that instrumentation any more. If
the DSA vanishes, maybe we'll crash if we try to access it. If we
actually reach DestroyParallelContext(), we're just going to start
killing the workers. None of that sounds like what we want.

The good news, of a sort, is that I think this might be the only case
of this sort of problem. Most nodes recurse at the end, after doing
all the cleanup, so the behavior won't change. Moreover, even if it
did, most cleanup operations look pretty localized -- they affect only
the node itself, and not its children. A somewhat interesting case is
nodes associated with subplans. Right now, because of the coding of
ExecEndPlan, nodes associated with subplans are all cleaned up at the
very end, after everything that's not inside of a subplan. But with
this change, they'd get cleaned up in the order of initialization,
which actually seems more natural, as long as it doesn't break
anything, which I think it probably won't, since as I mention in most
cases node cleanup looks quite localized, i.e. it doesn't care whether
it happens before or after the cleanup of other nodes.

I think something will have to be done about the parallel query stuff,
though. I'm not sure exactly what. It is a little weird that Gather
and Gather Merge treat starting and killing workers as a purely
"private matter" that they can decide to handle without the executor
overall being very much aware of it. So maybe there's a way that some
of the cleanup logic here could be hoisted up into the general
executor machinery, that is, first end all the nodes, and then go
back, and end all the parallelism using, maybe, another list inside of
the estate. However, I think that the existence of ExecShutdownNode()
is a complication here -- we need to make sure that we don't break
either the case where that happen before overall plan shutdown, or the
case where it doesn't.

Third, a couple of minor comments on details of how you actually made
these changes in the patch set. Personally, I would remove all of the
"is closed separately" comments that you added. I think it's a
violation of the general coding principle that you should make the
code look like it's always been that way. Sure, in the immediate
future, people might wonder why you don't need to recurse, but 5 or 10
years from now that's just going to be clutter. Second, in the cases
where the ExecEndNode functions end up completely empty, I would
suggest just removing the functions entirely and making the switch
that dispatches on the node type have a switch case that lists all the
nodes that don't need a callback here and say /* Nothing do for these
node types */ break;. This will save a few CPU cycles and I think it
will be easier to read as well.

Fourth, I wonder whether we really need this patch at all. I initially
thought we did, because if we abandon the initialization of a plan
partway through, then we end up with a plan that is in a state that
previously would never have occurred, and we still have to be able to
clean it up. However, perhaps it's a difference without a distinction.
Say we have a partial plan tree, where not all of the PlanState nodes
ever got created. We then just call the existing version of
ExecEndPlan() on it, with no changes. What goes wrong? Sure, we might
call ExecEndNode() on some null pointers where in the current world
there would always be valid pointers, but ExecEndNode() will handle
that just fine, by doing nothing for those nodes, because it starts
with a NULL-check.

Another alternative design might be to switch ExecEndNode to use
planstate_tree_walker to walk the node tree, removing the walk from
the node-type-specific functions as in this patch, and deleting the
end-node functions that are no longer required altogether, as proposed
above. I somehow feel that this would be cleaner than the status quo,
but here again, I'm not sure we really need it. planstate_tree_walker
would just pass over any NULL pointers that it found without doing
anything, but the current code does that too, so while this might be
more beautiful than what we have now, I'm not sure that there's any
real reason to do it. The fact that, like the current patch, it would
change the order in which nodes are cleaned up is also an issue -- the
Gather/Gather Merge ordering issues might be easier to handle this way
with some hack in ExecEndNode() than they are with the design you have
now, but we'd still have to do something about them, I believe.

Sorry if this is a bit of a meandering review, but those are my thoughts.

-- 
Robert Haas

---

From: Tom Lane <[email-masked]>
Date: Mon, 07 Aug 2023 11:44:53 -0400

Robert Haas <[email-masked]> writes:
> that happens in ExecParallelCleanup().

Right, I doubt that changing that is going to work out well.
Hash joins might have issues with it too.

Could it work to make the patch force child cleanup before parent,
instead of after?  Or would that break other places?

On the whole though I think it's probably a good idea to leave
parent nodes in control of the timing, so I kind of side with
your later comment about whether we want to change this at all.

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Mon, 7 Aug 2023 12:25:52 -0400

> Hash joins might have issues with it too.

I thought about the case, because Hash and Hash Join are such closely
intertwined nodes, but I don't see any problem there. It doesn't
really look like it would matter in what order things got cleaned up.
Unless I'm missing something, all of the data structures are just
independent things that we have to get rid of sometime.

> instead of after?  Or would that break other places?

To me, it seems like the overwhelming majority of the code simply
doesn't care. You could pick an order out of a hat and it would be
100% OK. But I haven't gone and looked through it with this specific
idea in mind.

> your later comment about whether we want to change this at all.

My overall feeling here is that what Gather and Gather Merge is doing
is pretty weird. I think I kind of knew that at the time this was all
getting implemented and reviewed, but I wasn't keen to introduce more
infrastructure changes than necessary given that parallel query, as a
project, was still pretty new and I didn't want to give other hackers
more reasons to be unhappy with what was already a lot of very
wide-ranging change to the system. A good number of years having gone
by now, and other people having worked on that code some more, I'm not
too worried about someone calling for a wholesale revert of parallel
query. However, there's a second problem here as well, which is that
I'm still not sure what the right thing to do is. We've fiddled around
with the shutdown sequence for parallel query a number of times now,
and I think there's still stuff that doesn't work quite right,
especially around getting all of the instrumentation data back to the
leader. I haven't spent enough time on this recently enough to be sure
what if any problems remain, though.

So on the one hand, I don't really like the fact that we have an
ad-hoc recursion arrangement here, instead of using
planstate_tree_walker or, as Amit proposes, a List. Giving subordinate
nodes control over the ordering when they don't really need it just
means we have more code with more possibility for bugs and less
certainty about whether the theoretical flexibility is doing anything
in practice. But on the other hand, because we know that at least for
the Gather/GatherMerge case it seems like it probably matters
somewhat, it definitely seems appealing not to change anything as part
of this patch set that we don't really have to.

I've had it firmly in my mind here that we were going to need to
change something somehow -- I mean, the possibility of returning in
the middle of node initialization seems like a pretty major change to
the way this stuff works, and it seems hard for me to believe that we
can just do that and not have to adjust any code anywhere else. Can it
really be true that we can do that and yet not end up creating any
states anywhere with which the current cleanup code is unprepared to
cope? Maybe, but it would seem like rather good luck if that's how it
shakes out. Still, at the moment, I'm having a hard time understanding
what this particular change buys us.

---

From: Amit Langote <[email-masked]>
Date: Tue, 8 Aug 2023 23:32:07 +0900

> notes about things that I looked at.

Thanks for taking a look at this.

> me think that this is a real danger.

Check.

> child nodes.

Because a node is appended to es_planstate_nodes at the end of
ExecInitNode(), child nodes get added before their parent nodes.  So
the children are cleaned up first.

> Here, I think we have problems. Both ExecGather and

Given that children are closed before parent, the order of operations
in ExecEndGather[Merge] is unchanged.

> will be easier to read as well.

I agree with both suggestions.

> that just fine, by doing nothing for those nodes, because it starts

Well, not all cleanup actions for a given node type are a recursive
call to ExecEndNode(), some are also things like this:

    /*
     * clean out the tuple table
     */
    ExecClearTuple(node->ps.ps_ResultTupleSlot);

But should ExecInitNode() subroutines return the partially initialized
PlanState node or NULL on detecting invalidation?  If I'm
understanding how you think this should be working correctly, I think
you mean the former, because if it were the latter, ExecInitNode()
would end up returning NULL at the top for the root and then there's
nothing to pass to ExecEndNode(), so no way to clean up to begin with.
In that case, I think we will need to adjust ExecEndNode() subroutines
to add `if (node->ps.ps_ResultTupleSlot)` in the above code, for
example.  That's something Tom had said he doesn't like very much [1].

Some node types such as Append, BitmapAnd, etc. that contain a list of
subplans would need some adjustment, such as using palloc0 for
as_appendplans[], etc. so that uninitialized subplans have NULL in the
array.

There are also issues around ForeignScan, CustomScan
ExecEndNode()-time callbacks when they are partially initialized -- is
it OK to call the *EndScan callback if the *BeginScan one may not have
been called to begin with?  Though, perhaps we can adjust the
ExecInitNode() subroutines for those to return NULL by opening the
relation and checking for invalidation at the beginning instead of in
the middle.  That should be done for all Scan or leaf-level node
types.

Anyway, I guess, for the patch's purpose, maybe we should bite the
bullet and make those adjustments rather than change ExecEndNode() as
proposed.  I can give that another try.

> now, but we'd still have to do something about them, I believe.

It might be interesting to see if introducing planstate_tree_walker()
in ExecEndNode() makes it easier to reason about ExecEndNode()
generally speaking, but I think you may be that doing so may not
really make matters easier for the partially initialized planstate
tree case.

---

From: Robert Haas <[email-masked]>
Date: Tue, 8 Aug 2023 12:05:26 -0400

> example.  That's something Tom had said he doesn't like very much [1].

Yeah, I understood Tom's goal as being "don't return partially
initialized nodes."

Personally, I'm not sure that's an important goal. In fact, I don't
even think it's a desirable one. It doesn't look difficult to audit
the end-node functions for cases where they'd fail if a particular
pointer were NULL instead of pointing to some real data, and just
fixing all such cases to have NULL-tests looks like purely mechanical
work that we are unlikely to get wrong. And at least some cases
wouldn't require any changes at all.

If we don't do that, the complexity doesn't go away. It just moves
someplace else. Presumably what we do in that case is have
ExecInitNode functions undo any initialization that they've already
done before returning NULL. There are basically two ways to do that.
Option one is to add code at the point where they return early to
clean up anything they've already initialized, but that code is likely
to substantially duplicate whatever the ExecEndNode function already
knows how to do, and it's very easy for logic like this to get broken
if somebody rearranges an ExecInitNode function down the road. Option
two is to rearrange the ExecInitNode functions now, to open relations
or recurse at the beginning, so that we discover the need to fail
before we initialize anything. That restricts our ability to further
rearrange the functions in future somewhat, but more importantly,
IMHO, it introduces more risk right now. Checking that the ExecEndNode
function will not fail if some pointers are randomly null is a lot
easier than checking that changing the order of operations in an
ExecInitNode function breaks nothing.

I'm not here to say that we can't do one of those things. But I think
adding null-tests to ExecEndNode functions looks like *far* less work
and *way* less risk.

There's a second issue here, too, which is when we abort ExecInitNode
partway through, how do we signal that? You're rightly pointing out
here that if we do that by returning NULL, then we don't do it by
returning a pointer to the partially initialized node that we just
created, which means that we either need to store those partially
initialized nodes in a separate data structure as you propose to do in
0001, or else we need to pick a different signalling convention. We
could change (a) ExecInitNode to have an additional argument, bool
*kaboom, or (b) we could make it return bool and return the node
pointer via a new additional argument, or (c) we could put a Boolean
flag into the estate and let the function signal failure by flipping
the value of the flag. If we do any of those things, then as far as I
can see 0001 is unnecessary. If we do none of them but also avoid
creating partially initialized nodes by one of the two techniques
mentioned two paragraphs prior, then 0001 is also unnecessary. If we
do none of them but do create partially initialized nodes, then we
need 0001.

So if this were a restaurant menu, then it might look like this:

Prix Fixe Menu (choose one from each)

First Course - How do we clean up after partial initialization?
(1) ExecInitNode functions produce partially initialized nodes
(2) ExecInitNode functions get refactored so that the stuff that can
cause early exit always happens first, so that no cleanup is ever
needed
(3) ExecInitNode functions do any required cleanup in situ

Second Course - How do we signal that initialization stopped early?
(A) Return NULL.
(B) Add a bool * out-parmeter to ExecInitNode.
(C) Add a Node * out-parameter to ExecInitNode and change the return
value to bool.
(D) Add a bool to the EState.
(E) Something else, maybe.

I think that we need 0001 if we choose specifically (1) and (A). My
gut feeling is that the least-invasive way to do this project is to
choose (1) and (D). My second choice would be (1) and (C), and my
third choice would be (1) and (A). If I can't have (1), I think I
prefer (2) over (3), but I also believe I prefer hiding in a deep hole
to either of them. Maybe I'm not seeing the whole picture correctly
here, but both (2) and (3) look awfully painful to me.

---

From: Amit Langote <[email-masked]>
Date: Fri, 11 Aug 2023 14:31:03 +0900

On Wed, Aug 9, 2023 at 1:05 AM Robert Haas <[email-masked]> wrote:
> if somebody rearranges an ExecInitNode function down the road.

Yeah, I too am not a fan of making ExecInitNode() clean up partially
initialized nodes.

> Option

+1

> 0001,
> or else we need to pick a different signalling convention. We
> the value of the flag.

The failure can already be detected by seeing that
ExecPlanIsValid(estate) is false.  The question is what ExecInitNode()
or any of its subroutines should return once it is.  I think the
following convention works:

Return partially initialized state from ExecInit* function where we
detect the invalidation after calling ExecInitNode() on a child plan,
so that ExecEndNode() can recurse to clean it up.

Return NULL from ExecInit* functions where we detect the invalidation
after opening and locking a relation but before calling ExecInitNode()
to initialize a child plan if there's one at all.  Even if we may set
things like ExprContext, TupleTableSlot fields, they are cleaned up
independently of the plan tree anyway via the cleanup called with
es_exprcontexts, es_tupleTable, respectively.  I even noticed bits
like this in ExecEnd* functions:

-   /*
-    * Free the exprcontext(s) ... now dead code, see ExecFreeExprContext
-    */
-#ifdef NOT_USED
-   ExecFreeExprContext(&node->ss.ps);
-   if (node->ioss_RuntimeContext)
-       FreeExprContext(node->ioss_RuntimeContext, true);
-#endif

So, AFAICS, ExprContext, TupleTableSlot cleanup in ExecNode* functions
is unnecessary but remain around because nobody cared about and got
around to getting rid of it.

> If we do any of those things, then as far as I

I think what I've ended up with in the attached 0001 (WIP) is both
(1), (2), and (D).  As mentioned above, (D) is implemented with the
ExecPlanStillValid() function.

---

From: Amit Langote <[email-masked]>
Date: Fri, 11 Aug 2023 22:50:26 +0900

> ExecPlanStillValid() function.

After removing the unnecessary cleanup code from most node types’ ExecEnd*
functions, one thing I’m tempted to do is remove the functions that do
nothing else but recurse to close the outerPlan, innerPlan child nodes.  We
could instead have ExecEndNode() itself recurse to close outerPlan,
innerPlan child nodes at the top, which preserves the
close-child-before-self behavior for Gather* nodes, and close node type
specific cleanup functions for nodes that do have any local cleanup to do.
Perhaps, we could even use planstate_tree_walker() called at the top
instead of the usual bottom so that nodes with a list of child subplans
like Append also don’t need to have their own ExecEnd* functions.

Thanks, Amit Langote

---

From: Robert Haas <[email-masked]>
Date: Mon, 28 Aug 2023 09:43:41 -0400

On Fri, Aug 11, 2023 at 9:50 AM Amit Langote <[email-masked]> wrote:
> After removing the unnecessary cleanup code from most node types’ ExecEnd* functions, one thing I’m tempted to do is remove the functions that do nothing else but recurse to close the outerPlan, innerPlan child nodes.  We could instead have ExecEndNode() itself recurse to close outerPlan, innerPlan child nodes at the top, which preserves the close-child-before-self behavior for Gather* nodes, and close node type specific cleanup functions for nodes that do have any local cleanup to do.  Perhaps, we could even use planstate_tree_walker() called at the top instead of the usual bottom so that nodes with a list of child subplans like Append also don’t need to have their own ExecEnd* functions.

I think 0001 needs to be split up. Like, this is code cleanup:

-       /*
-        * Free the exprcontext
-        */

This is providing for NULL pointers where we don't currently:

-       list_free_deep(aggstate->hash_batches);
+       if (aggstate->hash_batches)
+               list_free_deep(aggstate->hash_batches);

And this is the early return mechanism per se:

+       if (!ExecPlanStillValid(estate))
+               return aggstate;

I think at least those 3 kinds of changes deserve to be in separate
patches with separate commit messages explaining the rationale behind
each e.g. "Remove unnecessary cleanup calls in ExecEnd* functions.
These calls are no longer required, because <reasons>. Removing them
saves a few CPU cycles and simplifies planned refactoring, so do
that."

---

From: Amit Langote <[email-masked]>
Date: Tue, 5 Sep 2023 16:13:09 +0900

Thanks for taking a look.

> saves a few CPU cycles and simplifies planned refactoring, so do

Breaking up the patch as you describe makes sense, so I've done that:

Attached 0001 removes unnecessary cleanup calls from ExecEnd*() routines.

0002 adds NULLness checks in ExecEnd*() routines on some pointers that
may not be initialized by the corresponding ExecInit*() routines in
the case where it returns early.

0003 adds the early return mechanism based on checking CachedPlan
invalidation, though no CachedPlan is actually passed to the executor
yet, so no functional changes here yet.

Other patches are rebased over these.  One significant change is in
0004 which does the refactoring to make the callers of ExecutorStart()
aware that it may now return with a partially initialized planstate
tree that should not be executed.  I added a new flag
EState.es_canceled to denote that state of the execution to complement
the existing es_finished.  I also needed to add
AfterTriggerCancelQuery() to ensure that we don't attempt to fire a
canceled query's triggers.  Most of these changes are needed only to
appease the various Asserts in these parts of the code and I thought
they are warranted given the introduction of a new state of query
execution.

---

From: Robert Haas <[email-masked]>
Date: Tue, 5 Sep 2023 10:41:02 -0400

> Attached 0001 removes unnecessary cleanup calls from ExecEnd*() routines.

It also adds a few random Assert()s to verify that unrelated pointers
are not NULL. I suggest that it shouldn't do that.

The commit message doesn't mention the removal of the calls to
ExecDropSingleTupleTableSlot. It's not clear to me why that's OK and I
think it would be nice to mention it in the commit message, assuming
that it is in fact OK.

I suggest changing the subject line of the commit to something like
"Remove obsolete executor cleanup code."

> the case where it returns early.

I think you should only add these where it's needed. For example, I
think list_free_deep(NIL) is fine.

The changes to ExecEndForeignScan look like they include stuff that
belongs in 0001.

Personally, I prefer explicit NULL-tests i.e. if (x != NULL) to
implicit ones like if (x), but opinions vary.

---

From: Amit Langote <[email-masked]>
Date: Wed, 6 Sep 2023 18:12:28 +0900

> are not NULL. I suggest that it shouldn't do that.

OK, removed.

> think it would be nice to mention it in the commit message, assuming

That is not OK, so I dropped their removal. I think I confused them
with slots in other functions initialized with
ExecInitExtraTupleSlot() that *are* put into the estate.

> "Remove obsolete executor cleanup code."

Sure.

> think list_free_deep(NIL) is fine.

OK, done.

> The changes to ExecEndForeignScan look like they include stuff that

Oops, yes.  Moved to 0001.

> implicit ones like if (x), but opinions vary.

I agree, so changed all the new tests to use (x != NULL) form.
Typically, I try to stick with whatever style is used in the nearby
code, though I can see both styles being used in the ExecEnd*()
routines.  I opted to use the style that we both happen to prefer.

Attached updated patches.  Thanks for the review.

---

From: Robert Haas <[email-masked]>
Date: Wed, 6 Sep 2023 10:20:06 -0400

> Attached updated patches.  Thanks for the review.

I think 0001 looks ready to commit. I'm not sure that the commit
message needs to mention future patches here, since this code cleanup
seems like a good idea regardless, but if you feel otherwise, fair
enough.

On 0002, some questions:

- In ExecEndLockRows, is the call to EvalPlanQualEnd a concern? i.e.
Does that function need any adjustment?
- In ExecEndMemoize, should there be a null-test around
MemoryContextDelete(node->tableContext) as we have in
ExecEndRecursiveUnion, ExecEndSetOp, etc.?

I wonder how we feel about setting pointers to NULL after freeing the
associated data structures. The existing code isn't consistent about
doing that, and making it do so would be a fairly large change that
would bloat this patch quite a bit. On the other hand, I think it's a
good practice as a general matter, and we do do it in some ExecEnd
functions.

On 0003, I have some doubt about whether we really have all the right
design decisions in detail here:

- Why have this weird rule where sometimes we return NULL and other
times the planstate? Is there any point to such a coding rule? Why not
just always return the planstate?

- Is there any point to all of these early exit cases? For example, in
ExecInitBitmapAnd, why exit early if initialization fails? Why not
just plunge ahead and if initialization failed the caller will notice
that and when we ExecEndNode some of the child node pointers will be
NULL but who cares? The obvious disadvantage of this approach is that
we're doing a bunch of unnecessary initialization, but we're also
speeding up the common case where we don't need to abort by avoiding a
branch that will rarely be taken. I'm not quite sure what the right
thing to do is here.

- The cases where we call ExecGetRangeTableRelation or
ExecOpenScanRelation are a bit subtler ... maybe initialization that
we're going to do later is going to barf if the tuple descriptor of
the relation isn't what we thought it was going to be. In that case it
becomes important to exit early. But if that's not actually a problem,
then we could apply the same principle here also -- don't pollute the
code with early-exit cases, just let it do its thing and sort it out
later. Do you know what the actual problems would be here if we didn't
exit early in these cases?

- Depending on the answers to the above points, one thing we could
think of doing is put an early exit case into ExecInitNode itself: if
(unlikely(!ExecPlanStillValid(whatever)) return NULL. Maybe Andres or
someone is going to argue that that checks too often and is thus too
expensive, but it would be a lot more maintainable than having similar
checks strewn throughout the ExecInit* functions. Perhaps it deserves
some thought/benchmarking. More generally, if there's anything we can
do to centralize these checks in fewer places, I think that would be
worth considering. The patch isn't terribly large as it stands, so I
don't necessarily think that this is a critical issue, but I'm just
wondering if we can do better. I'm not even sure that it would be too
expensive to just initialize the whole plan always, and then just do
one test at the end. That's not OK if the changed tuple descriptor (or
something else) is going to crash or error out in a funny way or
something before initialization is completed, but if it's just going
to result in burning a few CPU cycles in a corner case, I don't know
if we should really care.

- The "At this point" comments don't give any rationale for why we
shouldn't have received any such invalidation messages. That  makes
them fairly useless; the Assert by itself clarifies that you think
that case shouldn't happen. The comment's job is to justify that
claim.

---

From: Amit Langote <[email-masked]>
Date: Mon, 25 Sep 2023 21:57:48 +0900

> seems like a good idea regardless, but if you feel otherwise, fair

OK, I will remove the mention of future patches.

> Does that function need any adjustment?

I think it does with the patch as it stands.  It needs to have an
early exit at the top if parentestate is NULL, which it would be if
EvalPlanQualInit() wasn't called from an ExecInit*() function.

Though, as I answer below your question as to whether there is
actually any need to interrupt all of the ExecInit*() routines,
nothing needs to change in ExecEndLockRows().

> ExecEndRecursiveUnion, ExecEndSetOp, etc.?

Oops, you're right.  Added.

> good practice as a general matter, and we do do it in some ExecEnd

I agree that it might be worthwhile to take the opportunity and make
the code more consistent in this regard.  So, I've included those
changes too in 0002.

> if we should really care.

I thought about this some and figured that adding the
is-CachedPlan-still-valid tests in the following places should suffice
after all:

1. In InitPlan() right after the top-level ExecInitNode() calls
2. In ExecInit*() functions of Scan nodes, right after
ExecOpenScanRelation() calls

CachedPlans can only become invalid because of concurrent changes to
the inheritance child tables referenced in the plan.  Only the
following schema modifications of child tables are possible to be
performed concurrently:

* Addition of a column (allowed only if traditional inheritance child)
* Addition of an index
* Addition of a non-index constraint
* Dropping of a child table (allowed only if traditional inheritance child)
* Dropping of an index referenced in the plan

The first 3 are not destructive enough to cause crashes, weird errors
during ExecInit*(), though the last two can be, so the 2nd set of the
tests after ExecOpenScanRelation() mentioned above.

> that case shouldn't happen. The comment's job is to justify that

I've rewritten the comments.

I'll post the updated set of patches shortly.

---

From: Amit Langote <[email-masked]>
Date: Tue, 26 Sep 2023 22:06:12 +0900

> ExecOpenScanRelation() calls

After sleeping on this, I think we do need the checks after all the
ExecInitNode() calls too, because we have many instances of the code
like the following one:

    outerPlanState(gatherstate) = ExecInitNode(outerNode, estate, eflags);
    tupDesc = ExecGetResultType(outerPlanState(gatherstate));
    <some code that dereferences outDesc>

If outerNode is a SeqScan and ExecInitSeqScan() returned early because
ExecOpenScanRelation() detected that plan was invalidated, then
tupDesc would be NULL in this case, causing the code to crash.

Now one might say that perhaps we should only add the
is-CachedPlan-valid test in the instances where there is an actual
risk of such misbehavior, but that could lead to confusion, now or
later.  It seems better to add them after every ExecInitNode() call
while we're inventing the notion, because doing so relieves the
authors of future enhancements of the ExecInit*() routines from
worrying about any of this.

Attached 0003 should show how that turned out.

Updated 0002 as mentioned in the previous reply -- setting pointers to
NULL after freeing them more consistently across various ExecEnd*()
routines and using the `if (pointer != NULL)` style over the `if
(pointer)` more consistently.

Updated 0001's commit message to remove the mention of its relation to
any future commits.  I intend to push it tomorrow.

Patches 0004 onwards contain changes too, mainly in terms of moving
the code around from one patch to another, but I'll omit the details
of the specific change for now.

---

From: Amit Langote <[email-masked]>
Date: Thu, 28 Sep 2023 17:26:27 +0900

> any future commits.  I intend to push it tomorrow.

Pushed that one.  Here are the rebased patches.

0001 seems ready to me, but I'll wait a couple more days for others to
weigh in.  Just to highlight a kind of change that others may have
differing opinions on, consider this hunk from the patch:

-   MemoryContextDelete(node->aggcontext);
+   if (node->aggcontext != NULL)
+   {
+       MemoryContextDelete(node->aggcontext);
+       node->aggcontext = NULL;
+   }
...
+   ExecEndNode(outerPlanState(node));
+   outerPlanState(node) = NULL;

So the patch wants to enhance the consistency of setting the pointer
to NULL after freeing part.  Robert mentioned his preference for doing
it in the patch, which I agree with.

---

From: Amit Langote <[email-masked]>
Date: Mon, 20 Nov 2023 13:29:53 +0900

> it in the patch, which I agree with.

Rebased.

I haven't been able to reproduce and debug a crash reported by cfbot
that I see every now and then:

https://cirrus-ci.com/task/5673432591892480?logs=cores#L0

[22:46:12.328] Program terminated with signal SIGSEGV, Segmentation fault.
[22:46:12.328] Address not mapped to object.
[22:46:12.838] #0 afterTriggerInvokeEvents
(events=events@entry=0x836db0460, firing_id=1,
estate=estate@entry=0x842eec100, delete_ok=<optimized out>) at
../src/backend/commands/trigger.c:4656
[22:46:12.838] #1 0x00000000006c67a8 in AfterTriggerEndQuery
(estate=estate@entry=0x842eec100) at
../src/backend/commands/trigger.c:5085
[22:46:12.838] #2 0x000000000065bfba in CopyFrom (cstate=0x836df9038)
at ../src/backend/commands/copyfrom.c:1293
...

While a patch in this series does change
src/backend/commands/trigger.c, I'm not yet sure about its relation
with the backtrace shown there.

---

From: Robert Haas <[email-masked]>
Date: Wed, 6 Dec 2023 13:52:59 -0500

Reviewing 0001:

Perhaps ExecEndCteScan needs an adjustment. What if node->leader was never set?

Other than that, I think this is in good shape. Maybe there are other
things we'd want to adjust here, or maybe there aren't, but there
doesn't seem to be any good reason to bundle more changes into the
same patch.

Reviewing 0002 and beyond:

I think it's good that you have tried to divide up a big change into
little pieces, but I'm finding the result difficult to understand. It
doesn't really seem like each patch stands on its own. I keep flipping
between patches to try to understand why other patches are doing
things, which kind of defeats the purpose of splitting stuff up. For
example, 0002 adds a NodeTag field to QueryDesc, but it doesn't even
seem to initialize that field, let alone use it for anything. It adds
a CachedPlan pointer to QueryDesc too, and adapts CreateQueryDesc to
allow one as an argument, but none of the callers actually pass
anything. I suspect that that the first change (adding a NodeTag)
field is a bug, and that the second one is intentional, but it's hard
to tell without flipping through all of the other patches to see how
they build on what 0002 does. And even when something isn't a bug,
it's also hard to tell whether it's the right design, again because
you can't consider each patch in isolation. Ideally, splitting a patch
set should bring related changes together in a single patch and push
unrelated changes apart into different patches, but I don't really see
this particular split having that effect.

There is a chicken and egg problem here, to be fair. If we add code
that can make plan initialization fail without teaching the planner to
cope with failures, then we have broken the server, and if we do the
reverse, then we have a bunch of dead code that we can't test. Neither
is very satisfactory. But I still hope there's some better division
possible than what you have here currently. For instance, I wonder if
it would be possible to add all the stuff to cope with plan
initialization failing and then have a test patch that makes
initialization randomly fail with some probability (or maybe you can
even cause failures at specific points). Then you could test that
infrastructure by running the regression tests in a loop with various
values of the relevant setting.

Another overall comment that I have is that it doesn't feel like
there's enough high-level explanation of the design. I don't know how
much of that should go in comments vs. commit messages vs. a README
that accompanies the patch set vs. whatever else, and I strongly
suspect that some of the stuff that seems confusing now is actually
stuff that at one point I understood and have just forgotten about.
But rediscovering it shouldn't be quite so hard. For example, consider
the question "why are we storing the CachedPlan in the QueryDesc?" I
eventually figured out that it's so that ExecPlanStillValid can call
CachedPlanStillValid which can then consult the cached plan's is_valid
flag. But is that the only access to the CachedPlan that we ever
expect to occur via the QueryDesc? If not, what else is allowable? If
so, why not just store a Boolean in the QueryDesc and arrange for the
plancache to be able to flip it when invalidating? I'm not saying
that's a better design -- I'm saying that it looks hard to understand
your thought process from the patch set. And also, you know, assuming
the current design is correct, could there be some way of dividing up
the patch set so that this one change, where we add the CachedPlan to
the QueryDesc, isn't so spread out across the whole series?

Some more detailed review comments below. This isn't really a full
review because I don't understand the patches well enough for that,
but it's some stuff I noticed.

In 0002:

+     * result-rel info, etc.  Also, we don't pass the parent't copy of the

Typo.

+        /*
+         * All the necessary locks must already have been taken when
+         * initializing the parent's copy of subplanstate, so the CachedPlan,
+         * if any, should not have become invalid during ExecInitNode().
+         */
+        Assert(ExecPlanStillValid(rcestate));

This -- and the other similar instance -- feel very uncomfortable.
There's a lot of action at a distance here. If this assertion ever
failed, how would anyone ever figure out what went wrong? You wouldn't
for example know which object got invalidated, presumably
corresponding to a lock that you failed to take. Unless the problem
were easily reproducible in a test environment, trying to guess what
happened might be pretty awful; imagine seeing this assertion failure
in a customer log file and trying to back-track to the find the
underlying bug. A further problem is that what would actually happen
is you *wouldn't* see this in the customer log file, because
assertions wouldn't be enabled, so you'd just see queries occasionally
returning wrong answers, I guess? Or crashing in some other random
part of the code? Which seems even worse. At a minimum I think this
should be upgraded to a test-and-elog, and maybe there's some value in
trying to think of what should get printed by that elog to facilitate
proper debugging, if it happens.

In 0003:

+                *
+                * OK to ignore the return value; plan can't become invalid,
+                * because there's no CachedPlan.
                 */
-               ExecutorStart(cstate->queryDesc, 0);
+               (void) ExecutorStart(cstate->queryDesc, 0);

This also feels awkward, for similar reasons. Sure, it shouldn't
return false, but also, if it did, you'd just blindly continue. Maybe
there should be test-and-elog here too. Or maybe this is an indication
that we need less action at a distance. Like, if ExecutorStart took
the CachedPlan as an argument instead of feeding it through the
QueryDesc, then you could document that ExecutorStart returns true if
that value is passed as NULL and true or false otherwise. Here,
whether ExecutorStart can return true or false depends on the contents
of the queryDesc ... which, granted, in this case is just built a line
or two before anyway, but if you just passed to to ExecutorStart then
you wouldn't need to feed it through the QueryDesc, it seems to me.
Even better, maybe there should be ExecutorStart() that continues
returning void and ExecutorStartExtended() that takes a cached plan as
an additional argument and returns a bool.

        /*
-        * Check that ExecutorFinish was called, unless in
EXPLAIN-only mode. This
-        * Assert is needed because ExecutorFinish is new as of 9.1, and callers
-        * might forget to call it.
+        * Check that ExecutorFinish was called, unless in
EXPLAIN-only mode or if
+        * execution was canceled. This Assert is needed because
ExecutorFinish is
+        * new as of 9.1, and callers might forget to call it.
         */

Maybe we could drop the second sentence at this point.

In 0005:

+                        * XXX Maybe we should we skip calling
ExecCheckPermissions from
+                        * InitPlan in a parallel worker.

Why? If the thinking is to save overhead, then perhaps try to assess
the overhead. If the thinking is that we don't want it to fail
spuriously, then we have to weight that against the (security) risk of
succeeding spuriously.

+ * Returns true if current transaction holds a lock on the given relation of
+ * mode 'lockmode'.  If 'orstronger' is true, a stronger lockmode is also OK.
+ * ("Stronger" is defined as "numerically higher", which is a bit
+ * semantically dubious but is OK for the purposes we use this for.)

I don't particularly enjoy seeing this comment cut and pasted into
some new place. Especially the tongue-in-cheek parenthetical part.
Better to refer to the original comment or something instead of
cut-and-pasting. Also, why is it appropriate to pass orstronger = true
here? Don't we expect the *exact* lock mode that we have planned to be
held, and isn't it a sure sign of a bug if it isn't? Maybe orstronger
should just be ripped out here (and the comment could then go away
too).

In 0006:

+       /*
+        * RTIs of all partitioned tables whose children are scanned by
+        * appendplans. The list contains a bitmapset for every partition tree
+        * covered by this Append.
+        */

The first sentence of this comment makes this sound like a list of
integers, the RTIs of all partitioned tables that are scanned. The
second sentence makes it sound like a list of bitmapsets, but what
does it mean to take about each partition tree covered by this Append?

This is far from a complete review but I'm running out of steam for
today. I hope that it's at least somewhat useful.

...Robert

---

From: vignesh C <[email-masked]>
Date: Fri, 5 Jan 2024 16:16:27 +0530

> > it in the patch, which I agree with.

There is a leak reported at [1], details for the same is available at [2]:
diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/select_views.out
/tmp/cirrus-ci-build/build/testrun/regress-running/regress/results/select_views.out
--- /tmp/cirrus-ci-build/src/test/regress/expected/select_views.out
2023-12-19 23:00:04.677385000 +0000
+++ /tmp/cirrus-ci-build/build/testrun/regress-running/regress/results/select_views.out
2023-12-19 23:06:26.870259000 +0000
@@ -1288,6 +1288,7 @@
       (102, '2011-10-12', 120),
       (102, '2011-10-28', 200),
       (103, '2011-10-15', 480);
+WARNING:  resource was not closed: relation "customer_pkey"
 CREATE VIEW my_property_normal AS
        SELECT * FROM customer WHERE name = current_user;
 CREATE VIEW my_property_secure WITH (security_barrier) A

[1] - https://cirrus-ci.com/task/6494009196019712
[2] - https://api.cirrus-ci.com/v1/artifact/task/6494009196019712/testrun/build/testrun/regress-running/regress/regression.diffs

Regards,
Vingesh

---

From: "Andrey M. Borodin" <[email-masked]>
Date: Sun, 31 Mar 2024 10:03:31 +0500

> On 6 Dec 2023, at 23:52, Robert Haas <[email-masked]> wrote:
> 
>  I hope that it's at least somewhat useful.
> 

> On 5 Jan 2024, at 15:46, vignesh C <[email-masked]> wrote:
> 
> There is a leak reported 

Hi Amit,

this is a kind reminder that some feedback on your patch[0] is waiting for your reply.
Thank you for your work!

Best regards, Andrey Borodin.

[0] https://commitfest.postgresql.org/47/3478/

---

From: Amit Langote <[email-masked]>
Date: Mon, 8 Apr 2024 17:39:02 +0900

Hi Andrey,

> this is a kind reminder that some feedback on your patch[0] is waiting for your reply.

Thanks for moving this to the next CF.

My apologies (especially to Robert) for not replying on this thread
for a long time.

I plan to start working on this soon.

--
Thanks, Amit Langote

---

From: Alvaro Herrera <[email-masked]>
Date: Wed, 19 Jun 2024 19:09:26 +0200

I had occasion to run the same benchmark you described in the initial
email in this thread.  To do so I applied patch series v49 on top of
07cb29737a4e, which is just one that happened to have the same date as
v49.

I then used a script like this (against a server having
plan_cache_mode=force_generic_mode)

for numparts in 0 1 2 4 8 16 32 48 64 80 81 96 127 128 160 200 256 257 288 300 384 512 1024 1536 2048;  do
    pgbench testdb -i --partitions=$numparts 2>/dev/null
    echo -ne "$numparts\t"
    pgbench -n testdb -S -T30 -Mprepared | grep "^tps" | sed -e 's/^tps = \([0-9.]*\) .*/\1/'
done

and did the same with the commit mentioned above (that is, unpatched).
I got this table as result

 partitions │   patched    │  07cb29737a  
────────────┼──────────────┼──────────────
          0 │ 65632.090431 │ 68967.712741
          1 │ 68096.641831 │ 65356.587223
          2 │ 59456.507575 │ 60884.679464
          4 │    62097.426 │ 59698.747104
          8 │ 58044.311175 │ 57817.104562
         16 │ 59741.926563 │ 52549.916262
         32 │ 59261.693449 │ 44815.317215
         48 │ 59047.125629 │ 38362.123652
         64 │ 59748.738797 │ 34051.158525
         80 │ 59276.839183 │ 32026.135076
         81 │ 62318.572932 │ 30418.122933
         96 │ 59678.857163 │ 28478.113651
        127 │ 58761.960028 │ 24272.303742
        128 │ 59934.268306 │ 24275.214593
        160 │ 56688.790899 │ 21119.043564
        200 │ 56323.188599 │ 18111.212849
        256 │  55915.22466 │ 14753.953709
        257 │ 57810.530461 │ 15093.497575
        288 │ 56874.780092 │ 13873.332162
        300 │ 57222.056549 │ 13463.768946
        384 │  54073.77295 │ 11183.558339
        512 │ 37503.766847 │   8114.32532
       1024 │ 42746.866448 │   4468.41359
       1536 │  39500.58411 │  3049.984599
       2048 │ 36988.519486 │  2269.362006

where already at 16 partitions we can see that things are going downhill
with the unpatched code.  (However, what happens when the table is not
partitioned looks a bit funny.)

I hope we can get this new executor code in 18.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"La primera ley de las demostraciones en vivo es: no trate de usar el sistema.
Escriba un guión que no toque nada para no causar daños." (Jakob Nielsen)