Subject: Re: generic plans and "initial" pruning

From: Tom Lane <[email-masked]>
Date: Thu, 19 Jan 2023 14:39:26 -0500

I spent some time re-reading this whole thread, and the more I read
the less happy I got.  We are adding a lot of complexity and introducing
coding hazards that will surely bite somebody someday.  And after awhile
I had what felt like an epiphany: the whole problem arises because the
system is wrongly factored.  We should get rid of AcquireExecutorLocks
altogether, allowing the plancache to hand back a generic plan that
it's not certain of the validity of, and instead integrate the
responsibility for acquiring locks into executor startup.  It'd have
to be optional there, since we don't need new locks in the case of
executing a just-planned plan; but we can easily add another eflags
bit (EXEC_FLAG_GET_LOCKS or so).  Then there has to be a convention
whereby the ExecInitNode traversal can return an indicator that
"we failed because the plan is stale, please make a new plan".

There are a couple reasons why this feels like a good idea:

* There's no need for worry about keeping the locking decisions in sync
with what executor startup does.

* We don't need to add the overhead proposed in the current patch to
pass forward data about what got locked/pruned.  While that overhead
is hopefully less expensive than the locks it saved acquiring, it's
still overhead (and in some cases the patch will fail to save acquiring
any locks, making it certainly a net negative).

* In a successfully built execution state tree, there will simply
not be any nodes corresponding to pruned-away, never-locked subplans.
As long as code like EXPLAIN follows the state tree and doesn't poke
into plan nodes that have no matching state, it's secure against the
sort of problems that Robert worried about upthread.

While I've not attempted to write any code for this, I can also
think of a few issues that'd have to be resolved:

* We'd be pushing the responsibility for looping back and re-planning
out to fairly high-level calling code.  There are only half a dozen
callers of GetCachedPlan, so there's not that many places to be
touched; but in some of those places the subsequent executor-start call
is not close by, so that the necessary refactoring might be pretty
painful.  I doubt there's anything insurmountable, but we'd definitely
be changing some fundamental APIs.

* In some cases (views, at least) we need to acquire lock on relations
that aren't directly reflected anywhere in the plan tree.  So there'd
have to be a separate mechanism for getting those locks and rechecking
validity afterward.  A list of relevant relation OIDs might be enough
for that.

* We currently do ExecCheckPermissions() before initializing the
plan state tree.  It won't do to check permissions on relations we
haven't yet locked, so that responsibility would have to be moved.
Maybe that could also be integrated into the initialization recursion?
Not sure.

* In the existing usage of AcquireExecutorLocks, if we do decide
that the plan is stale then we are able to release all the locks
we got before we go off and replan.  I'm not certain if that behavior
needs to be preserved, but if it does then that would require some
additional bookkeeping in the executor.

* This approach is optimizing on the assumption that we usually
won't need to replan, because if we do then we might waste a fair
amount of executor startup overhead before discovering we have
to throw all that state away.  I think that's clearly the right
way to bet, but perhaps somebody else has a different view.

Thoughts?

			regards, tom lane

---

From: Amit Langote <[email-masked]>
Date: Fri, 20 Jan 2023 12:13:53 +0900

On Fri, Jan 20, 2023 at 4:39 AM Tom Lane <[email-masked]> wrote:
> the less happy I got.

Thanks a lot for your time on this.

>  We are adding a lot of complexity and introducing

Interesting.  The current implementation relies on
PlanCacheRelCallback() marking a generic CachedPlan as invalid, so
perhaps there will have to be some sharing of state between the
plancache and the executor for this to work?

> sort of problems that Robert worried about upthread.

I think this is true with the patch as proposed too, but I was still a
bit worried about what an ExecutorStart_hook may be doing with an
uninitialized plan tree.  Maybe we're mandating that the hook must
call standard_ExecutorStart() and only work with the finished
PlanState tree?

> be changing some fundamental APIs.

Yeah.  I suppose mostly the same place that the current patch is
touching to pass around the PartitionPruneResult nodes.

> validity afterward.  A list of relevant relation OIDs might be enough

Hmm, a list of only the OIDs wouldn't preserve the lock mode, so maybe
a list or bitmapset of the RTIs, something along the lines of
PlannedStmt.minLockRelids in the patch?

It perhaps even makes sense to make a special list in PlannedStmt for
only the views?

> Maybe that could also be integrated into the initialization recursion?

Ah, I remember mentioning moving that into ExecGetRangeTableRelation()
[1], but I guess that misses relations that are not referenced in the
plan tree, such as views.  Though maybe that's not a problem if we
track views separately as mentioned above.

> additional bookkeeping in the executor.

I think maybe we'll want to continue to release the existing locks,
because if we don't, it's possible we may keep some locks uselessly if
replanning might lock a different set of relations.

> way to bet, but perhaps somebody else has a different view.

Not sure if you'd like, because it would still keep the
PartitionPruneResult business, but this will be less of a problem if
we do the initial pruning at the beginning of InitPlan(), followed by
locking, before doing anything else.  We would have initialized the
QueryDesc and the EState, but only minimally.  That also keeps the
PartitionPruneResult business local to the executor.

Would you like me to hack up a PoC or are you already on that?

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

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

---

From: Tom Lane <[email-masked]>
Date: Thu, 19 Jan 2023 22:31:49 -0500

Amit Langote <[email-masked]> writes:
>> responsibility for acquiring locks into executor startup.

> plancache and the executor for this to work?

Yeah.  Thinking a little harder, I think this would have to involve
passing a CachedPlan pointer to the executor, and what the executor
would do after acquiring each lock is to ask the plancache "hey, do
you still think this CachedPlan entry is valid?".  In the case where
there's a problem, the AcceptInvalidationMessages call involved in
lock acquisition would lead to a cache inval that clears the validity
flag on the CachedPlan entry, and this would provide an inexpensive
way to check if that happened.

It might be possible to incorporate this pointer into PlannedStmt
instead of passing it separately.

> call standard_ExecutorStart() and only work with the finished

It would certainly be incumbent on any such hook to not touch
not-yet-locked parts of the plan tree.  I'm not particularly concerned
about that sort of requirements change, because we'd be breaking APIs
all through this area in any case.

> Hmm, a list of only the OIDs wouldn't preserve the lock mode,

Good point.  I wonder if we could integrate this with the
RTEPermissionInfo data structure?

> Would you like me to hack up a PoC or are you already on that?

I'm not planning to work on this myself, I was hoping you would.

			regards, tom lane

---

From: Amit Langote <[email-masked]>
Date: Fri, 20 Jan 2023 12:52:07 +0900

> way to check if that happened.

OK, thanks, this is useful.

> instead of passing it separately.

Yeah, that would be less churn.  Though, I wonder if you still hold
that PlannedStmt should not be scribbled upon outside the planner as
you said upthread [1]?

> all through this area in any case.

OK.  Perhaps something that should be documented around ExecutorStart().

> RTEPermissionInfo data structure?

You mean adding a rellockmode field to RTEPermissionInfo?

> I'm not planning to work on this myself, I was hoping you would.

Alright, I'll try to get something out early next week.  Thanks for
all the pointers.

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

---

From: Tom Lane <[email-masked]>
Date: Thu, 19 Jan 2023 22:58:36 -0500

> that PlannedStmt should not be scribbled upon outside the planner as

Well, the whole point of that rule is that the executor can't modify
a plancache entry.  If the plancache itself sets a field in such an
entry, that doesn't seem problematic from here.

But there's other possibilities if that bothers you; QueryDesc
could hold the field, for example.  Also, I bet we'd want to copy
it into EState for the main initialization recursion.

			regards, tom lane

---

From: Amit Langote <[email-masked]>
Date: Fri, 20 Jan 2023 16:19:08 +0900

> it into EState for the main initialization recursion.

QueryDesc sounds good to me, and yes, also a copy in EState in any case.

So I started looking at the call sites of CreateQueryDesc() and
stopped to look at ExecParallelGetQueryDesc().  AFAICS, we wouldn't
need to pass the CachedPlan to a parallel worker's rerun of
InitPlan(), because 1) it doesn't make sense to call the plancache in
a parallel worker, 2) the leader should already have taken all the
locks in necessary for executing a given plan subnode that it intends
to pass to a worker in ExecInitGather().  Does that make sense?

---

From: Amit Langote <[email-masked]>
Date: Fri, 27 Jan 2023 16:01:05 +0900

> Alright, I'll try to get something out early next week.  Thanks for

Sorry for the delay.  Attached is what I've come up with so far.

I didn't actually go with calling the plancache on every lock taken on
a relation, that is, in ExecGetRangeTableRelation().  One thing about
doing it that way that I didn't quite like (or didn't see a clean
enough way to code) is the need to complicate the ExecInitNode()
traversal for handling the abrupt suspension of the ongoing setup of
the PlanState tree.

So, I decided to keep the current model of locking all the relations
that need to be locked before doing anything else in InitPlan(), much
as how AcquireExecutorLocks() does it.   A new function called from
the top of InitPlan that I've called ExecLockRelationsIfNeeded() does
that locking after performing the initial pruning in the same manner
as the earlier patch did.  That does mean that I needed to keep all
the adjustments of the pruning code that are required for such
out-of-ExecInitNode() invocation of initial pruning, including those
PartitionPruneResult to carry the result of that pruning for
ExecInitNode()-time reuse, though they no longer need be passed
through many unrelated interfaces.

Anyways, here's a description of the patches:

0001 adjusts various call sites of ExecutorStart() to cope with the
possibility of being asked to recreate a CachedPlan, if one is
involved.  The main objective here is to have as little stuff as
sensible happen between GetCachedPlan() that returned the CachedPlan
and ExecutorStart() so as to minimize the chances of missing cleaning
up resources that must not be missed.

0002 is preparatory refactoring to make out-of-ExecInitNode()
invocation of pruning possible.

0003 moves the responsibility of CachedPlan validation locking into
ExecutorStart() as described above.

---

From: Amit Langote <[email-masked]>
Date: Thu, 2 Feb 2023 23:49:58 +0900

> traversal for handling the abrupt suspension of the ongoing setup of

OK, I gave this one more try and attached is what I came up with.

This adds a ExecPlanStillValid(), which is called right after anything
that may in turn call ExecGetRangeTableRelation() which has been
taught to lock a relation if EXEC_FLAG_GET_LOCKS has been passed in
EState.es_top_eflags.  That includes all ExecInitNode() calls, and a
few other functions that call ExecGetRangeTableRelation() directly,
such as ExecOpenScanRelation().  If ExecPlanStillValid() returns
false, that is, if EState.es_cachedplan is found to have been
invalidated after a lock being taken by ExecGetRangeTableRelation(),
whatever funcion called it must return immediately and so must its
caller and so on.  ExecEndPlan() seems to be able to clean up after a
partially finished attempt of initializing a PlanState tree in this
way.  Maybe my preliminary testing didn't catch cases where pointers
to resources that are normally put into the nodes of a PlanState tree
are now left dangling, because a partially built PlanState tree is not
accessible to ExecEndPlan; QueryDesc.planstate would remain NULL in
such cases.  Maybe there's only es_tupleTable and es_relations that
needs to be explicitly released and the rest is taken care of by
resetting the ExecutorState context.

On testing, I'm afraid we're going to need something like
src/test/modules/delay_execution to test that concurrent changes to
relation(s) in PlannedStmt.relationOids that occur somewhere between
RevalidateCachedQuery() and InitPlan() result in the latter to be
aborted and that it is handled correctly.  It seems like it is only
the locking of partitions (that are not present in an unplanned Query
and thus not protected by AcquirePlannerLocks()) that can trigger
replanning of a CachedPlan, so any tests we write should involve
partitions.  Should this try to test as many plan shapes as possible
though given the uncertainty around ExecEndPlan() robustness or should
manual auditing suffice to be sure that nothing's broken?

On possibly needing to move permission checking to occur *after*
taking locks, I realized that we don't really need to, because no
relation that needs its permissions should be unlocked by the time we
get to ExecCheckPermissions(); note we only check permissions of
tables that are present in the original parse tree and
RevalidateCachedQuery() should have locked those.  I found a couple of
exceptions to that invariant in that views sometimes appear not to be
in the set of relations that RevalidateCachedQuery() locks.  So, I
invented PlannedStmt.viewRelations, a list of RT indexes of view RTEs
that is populated in setrefs.c. ExecLockViewRelations() called before
ExecCheckPermissions() locks those.

---

From: Amit Langote <[email-masked]>
Date: Fri, 3 Feb 2023 22:01:09 +0900

> resetting the ExecutorState context.

In the attached updated patch, I've made the functions that check
ExecPlanStillValid() to return NULL (if returning something) instead
of returning partially initialized structs.  Those partially
initialized structs were not being subsequently looked at anyway.

> manual auditing suffice to be sure that nothing's broken?

I've added a test case under src/modules/delay_execution by adding a
new ExecutorStart_hook that works similarly as
delay_execution_planner().  The test works by allowing a concurrent
session to drop an object being referenced in a cached plan being
initialized while the ExecutorStart_hook waits to get an advisory
lock.  The concurrent drop of the referenced object is detected during
ExecInitNode() and thus triggers replanning of the cached plan.

I also fixed a bug in the ExplainExecuteQuery() while testing and some comments.

---

From: Andres Freund <[email-masked]>
Date: Tue, 7 Feb 2023 10:08:55 -0800

Hi,

> I also fixed a bug in the ExplainExecuteQuery() while testing and some comments.

The tests seem to frequently hang on freebsd:
https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest%2F42%2F3478

Greetings,

Andres Freund

---

From: Amit Langote <[email-masked]>
Date: Wed, 8 Feb 2023 16:01:30 +0530

> https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest%2F42%2F3478

Thanks for the heads up.  I’ve noticed this one too, though couldn’t find
the testrun artifacts like I could get for some other failures (on other
cirrus machines).  Has anyone else been a similar situation?

>
> <https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest%2F42%2F3478>

---

From: Amit Langote <[email-masked]>
Date: Thu, 2 Mar 2023 22:52:53 +0900

On Wed, Feb 8, 2023 at 7:31 PM Amit Langote <[email-masked]> wrote:
> Thanks for the heads up.  I’ve noticed this one too, though couldn’t find the testrun artifacts like I could get for some other failures (on other cirrus machines).  Has anyone else been a similar situation?

I think I have figured out what might be going wrong on that cfbot
animal after building with the same CPPFLAGS as that animal locally.
I had forgotten to update _out/_readRangeTblEntry() to account for the
patch's change that a view's RTE_SUBQUERY now also preserves relkind
in addition to relid and rellockmode for the locking consideration.

Also, I noticed that a multi-query Portal execution with rules was
failing (thanks to a regression test added in a7d71c41db) because of
the snapshot used for the 2nd query onward not being updated for
command ID change under patched model of multi-query Portal execution.
To wit, under the patched model, all queries in the multi-query Portal
case undergo ExecutorStart() before any of it is run with
ExecutorRun().  The patch hadn't changed things however to update the
snapshot's command ID for the 2nd query onwards, which caused the
aforementioned test case to fail.

This new model does however mean that the 2nd query onwards must use
PushCopiedSnapshot() given the current requirement of
UpdateActiveSnapshotCommandId() that the snapshot passed to it must
not be referenced anywhere else.  The new model basically requires
that each query's QueryDesc points to its own copy of the
ActiveSnapshot.  That may not be a thing in favor of the patched model
though.  For now, I haven't been able to come up with a better
alternative.

---

From: Amit Langote <[email-masked]>
Date: Tue, 14 Mar 2023 19:07:41 +0900

> though.  For now, I haven't been able to come up with a better

Here's a new version addressing the following 2 points.

* Like views, I realized that non-leaf relations of partition trees
scanned by an Append/MergeAppend would need to be locked separately,
because ExecInitNode() traversal of the plan tree would not account
for them.  That is, they are not opened using
ExecGetRangeTableRelation() or ExecOpenScanRelation().  One exception
is that some (if not all) of those non-leaf relations may be
referenced in PartitionPruneInfo and so locked as part of initializing
the corresponding PartitionPruneState, but I decided not to complicate
the code to filter out such relations from the set locked separately.
To carry the set of relations to lock, the refactoring patch 0001
re-introduces the List of Bitmapset field named allpartrelids into
Append/MergeAppend nodes, which we had previously removed on the
grounds that those relations need not be locked separately (commits
f2343653f5b, f003a7522bf).

* I decided to initialize QueryDesc.planstate even in the cases where
ExecInitNode() traversal is aborted in the middle on detecting
CachedPlan invalidation such that it points to a partially initialized
PlanState tree.  My earlier thinking that each PlanState node need not
be visited for resource cleanup in such cases was naive after all.  To
that end, I've fixed the ExecEndNode() subroutines of all Plan node
types to account for potentially uninitialized fields.  There are a
couple of cases where I'm a bit doubtful though.  In
ExecEndCustomScan(), there's no indication in CustomScanState whether
it's OK to call EndCustomScan() when BeginCustomScan() may not have
been called.  For ForeignScanState, I've assumed that
ForeignScanState.fdw_state being set can be used as a marker that
BeginForeignScan would have been called, though maybe that's not a
solid assumption.

I'm also attaching a new (small) patch 0003 that eliminates the
loop-over-rangetable in ExecCloseRangeTableRelations() in favor of
iterating over a new List field of EState named es_opened_relations,
which is populated by ExecGetRangeTableRelation() with only the
relations that were opened.  This speeds up
ExecCloseRangeTableRelations() significantly for the cases with many
runtime-prunable partitions.

---

From: Amit Langote <[email-masked]>
Date: Wed, 22 Mar 2023 21:48:49 +0900

> runtime-prunable partitions.

Here's another version with some cosmetic changes, like fixing some
factually incorrect / obsolete comments and typos that I found.  I
also noticed that I had missed noting near some table_open() that
locks taken with those can't possibly invalidate a plan (such as
lazily opened partition routing target partitions) and thus need the
treatment that locking during execution initialization requires.

---

From: Amit Langote <[email-masked]>
Date: Mon, 27 Mar 2023 17:18:20 +0900

> treatment that locking during execution initialization requires.

Rebased over 3c05284d83b2 ("Invent GENERIC_PLAN option for EXPLAIN.").

---

From: Amit Langote <[email-masked]>
Date: Mon, 27 Mar 2023 23:00:38 +0900

> > > BeginForeignScan would have been called, though maybe that's not a

It seems I hadn't noted in the ExecEndNode()'s comment that all node
types' recursive subroutines need to  handle the change made by this
patch that the corresponding ExecInitNode() subroutine may now return
early without having initialized all state struct fields.

Also noted in the documentation for CustomScan and ForeignScan that
the Begin*Scan callback may not have been called at all, so the
End*Scan should handle that gracefully.

---

From: Tom Lane <[email-masked]>
Date: Mon, 03 Apr 2023 17:41:41 -0400

Amit Langote <[email-masked]> writes:
> [ v38 patchset ]

I spent a little bit of time looking through this, and concluded that
it's not something I will be wanting to push into v16 at this stage.
The patch doesn't seem very close to being committable on its own
terms, and even if it was now is not a great time in the dev cycle
to be making significant executor API changes.  Too much risk of
having to thrash the API during beta, or even change it some more
in v17.  I suggest that we push this forward to the next CF with the
hope of landing it early in v17.

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

* I don't understand the need for changes like this:

 	/* clean up tuple table */
-	ExecClearTuple(node->ps.ps_ResultTupleSlot);
+	if (node->ps.ps_ResultTupleSlot)
+		ExecClearTuple(node->ps.ps_ResultTupleSlot);

ISTM that the process ought to involve taking a lock (if needed)
before we have built any execution state for a given plan node,
and if we find we have to fail, returning NULL instead of a
partially-valid planstate node.  Otherwise, considerations of how
to handle partially-valid nodes are going to metastasize into all
sorts of places, almost certainly including EXPLAIN for instance.
I think we ought to be able to limit the damage to "parent nodes
might have NULL child links that you wouldn't have expected".
That wouldn't faze ExecEndNode at all, nor most other code.

* More attention is needed to comments.  For example, in a couple of
places in plancache.c you have removed function header comments
defining API details and not replaced them with any info about the new
details, despite the fact that those details are more complex than the
old.

> End*Scan should handle that gracefully.

Yeah, I think we need to avoid adding such requirements.  It's the
sort of thing that would far too easily get past developer testing
and only fail once in a blue moon in the field.

			regards, tom lane

---

From: Amit Langote <[email-masked]>
Date: Tue, 4 Apr 2023 22:29:12 +0900

> hope of landing it early in v17.

OK, thanks a lot for your feedback.

> 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

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.

> That wouldn't faze ExecEndNode at all, nor most other code.

Hmm, yes, taking a lock before allocating any of the stuff to add into
the planstate seems like it's much easier to reason about than the
alternative I've implemented.

> details, despite the fact that those details are more complex than the

OK, yeah, maybe I've added a bunch of explanations in execMain.c that
should perhaps have been in plancache.c.

> and only fail once in a blue moon in the field.

OK, got it.

---

From: David Rowley <[email-masked]>
Date: Sun, 19 May 2024 12:39:24 +1200

> "we failed because the plan is stale, please make a new plan".

I also reread the entire thread up to this point yesterday. I've also
been thinking about this recently as Amit has mentioned it to me a few
times over the past few months.

With the caveat of not yet having looked at the latest patch, my
thoughts are that having the executor startup responsible for taking
locks is a bad idea and I don't think we should go down this path. My
reasons are:

1. No ability to control the order that the locks are obtained. The
order in which the locks are taken will be at the mercy of the plan
the planner chooses.
2. It introduces lots of complexity regarding how to cleanly clean up
after a failed executor startup which is likely to make exec startup
slower and the code more complex
3. It puts us even further down the path of actually needing an
executor startup phase.

For #1, the locks taken for SELECT queries are less likely to conflict
with other locks obtained by PostgreSQL, but at least at the moment if
someone is getting deadlocks with a DDL type operation, they can
change their query or DDL script so that locks are taken in the same
order.  If we allowed executor startup to do this then if someone
comes complaining that PG18 deadlocks when PG17 didn't we'd just have
to tell them to live with it.  There's a comment at the bottom of
find_inheritance_children_extended() just above the qsort() which
explains about the deadlocking issue.

I don't have much extra to say about #2.  As mentioned, I've not
looked at the patch. On paper, it sounds possible, but it also sounds
bug-prone and ugly.

For #3, I've been thinking about what improvements we can do to make
the executor more efficient. In [1], Andres talks about some very
interesting things. In particular, in his email items 3) and 5) are
relevant here. If we did move lots of executor startup code into the
planner, I think it would be possible to one day get rid of executor
startup and have the plan record how much memory is needed for the
non-readonly part of the executor state and tag each plan node with
the offset in bytes they should use for their portion of the executor
working state. This would be a single memory allocation for the entire
plan.  The exact details are not important here, but I feel like if we
load up executor startup with more responsibilities, it'll just make
doing something like this harder.  The init run-time pruning code that
I worked on likely already has done that, but I don't think it's
closed the door on it as it might just mean allocating more executor
state memory than we need to. Providing the plan node records the
offset into that memory, I think it could be made to work, just with
the inefficiency of having a (possibly) large unused hole in that
state memory.

As far as I understand it, your objection to the original proposal is
just on the grounds of concerns about introducing hazards that could
turn into bugs.  I think we could come up with some way to make the
prior method of doing pruning before executor startup work. I think
what Amit had before your objection was starting to turn into
something workable and we should switch back to working on that.

David

[1] https://www.postgresql.org/message-id/20180525033538.6ypfwcqcxce6zkjj%40alap3.anarazel.de

---

From: Tom Lane <[email-masked]>
Date: Sat, 18 May 2024 21:27:36 -0400

David Rowley <[email-masked]> writes:
> locks is a bad idea and I don't think we should go down this path.

OK, it's certainly still up for argument, but ...

> order in which the locks are taken will be at the mercy of the plan

I do not think I buy this argument, because plancache.c doesn't
provide any "ability to control the order" today, and never has.
The order in which AcquireExecutorLocks re-gets relation locks is only
weakly related to the order in which the parser/planner got them
originally.  The order in which AcquirePlannerLocks re-gets the locks
is even less related to the original.  This doesn't cause any big
problems that I'm aware of, because these locks are fairly weak.

I think we do have a guarantee that for partitioned tables, parents
will be locked before children, and that's probably valuable.
But an executor-driven lock order could preserve that property too.

> slower and the code more complex

Perhaps true, I'm not sure.  But the patch we'd been discussing
before this proposal was darn complex as well.

> 3. It puts us even further down the path of actually needing an

Huh?  We have such a thing already.

> explains about the deadlocking issue.

The reason it's important there is that function is (sometimes)
used for lock modes that *are* exclusive.

> working state.

I'm fairly skeptical about that idea.  The entire reason we have an
issue here is that we want to do runtime partition pruning, which
by definition can't be done at plan time.  So I doubt it's going
to play nice with what we are trying to accomplish in this thread.

Moreover, while "replace a bunch of small pallocs with one big one"
would save some palloc effort, what are you going to do to ensure
that that memory has the right initial contents?  I think this idea is
likely to make the executor a great deal more notationally complex
without actually buying all that much.  Maybe Andres can make it work,
but I don't want to contort other parts of the system design on the
purely hypothetical basis that this might happen.

> I think what Amit had before your objection was starting to turn into

The reason I posted this idea was that I didn't think the previously
existing patch looked promising at all.

			regards, tom lane

---

From: David Rowley <[email-masked]>
Date: Sun, 19 May 2024 13:51:51 +1200

> problems that I'm aware of, because these locks are fairly weak.

It may not bite many people, it's just that if it does, I don't see
what we could do to help those people. At the moment we could tell
them to adjust their DDL script to obtain the locks in the same order
as their query.  With your idea that cannot be done as the order could
change when the planner switches the join order.

> But an executor-driven lock order could preserve that property too.

I think you'd have to lock the parent before the child. That would
remain true and consistent anyway when taking locks during a
breadth-first plan traversal.

> to play nice with what we are trying to accomplish in this thread.

I think we could have both, providing there was a way to still
traverse the executor state tree in EXPLAIN. We'd need a way to skip
portions of the plan that are not relevant or could be invalid for the
current execution. e.g can't show Index Scan because index has been
dropped.

> existing patch looked promising at all.

Ok.  It would be good if you could expand on that so we could
determine if there's some fundamental reason it can't work or if
that's because you were blinded by your epiphany and didn't give that
any thought after thinking of the alternative idea.

I've gone to effort to point out things that I think are concerning
with your idea. It would be good if you could do the same for the
previous patch other than "it didn't look promising". It's pretty hard
for me to argue with that level of detail.

David

---

From: Amit Langote <[email-masked]>
Date: Mon, 20 May 2024 21:13:21 +0900

> explains about the deadlocking issue.

Thought to chime in on this.

A deadlock may occur with the execution-time locking proposed in the
patch if the DDL script makes assumptions about how a cached plan's
execution determines the locking order for children of multiple parent
relations. Specifically, the deadlock can happen if the script tries
to lock the child relations directly, instead of locking them through
their respective parent relations.  The patch doesn't change the order
of locking of relations mentioned in the query, because that's defined
in AcquirePlannerLocks().

--
Thanks, Amit Langote