plan shape work

On 5/19/25 20:01, Robert Haas wrote:

Hi,

A couple of people at pgconf.dev seemed to want to know more about my
ongoing plan shape work, so here are the patches I have currently.
This is a long way from something that actually looks like a usable
feature, but these are bits of infrastructure that I think will be
necessary to get to a usable feature. As a recap, my overall goal here
is to make it so that you can examine a finished plan, figure out what
decisions the planner made, and then somehow get the planner to make
those same decisions over again in a future planning cycle. Since
doing this for all types of planner decisions seems too difficult for
an initial goal, I'm focusing on scans and joins for now. A further
goal is that I want it to be possible for extensions to use this
infrastructure to implement a variety of different policies that they
might feel to be beneficial, so I'm looking to minimize the amount of
stuff that has to be done in core PostgreSQL or can only be used by
core PostgreSQL.

...

Thanks for the overview. I don't have any immediate feedback, but it
sounds like it might be related to the "making planner decisions clear"
session from the unconference ...

The basic premise of that session was about how to give users better
info about the planner decisions - why paths were selected/rejected,
etc. A simple example would be "why was the index not used", and the
possible answers include "dominated by cost by another path" or "does
not match the index keys" etc.

I wonder if this work might be useful for something like that.

regards

--
Tomas Vondra

+1, this seems like it could be very useful. A somewhat related issue
is being able to tie plan nodes back to the query text: it can be hard
to understand the planner's decisions if it's not even clear what part
of the query it's making decisions about. I'm sure this is not an easy
problem in general, but I wonder if you think that could be improved
in the course of this work, or if you have other thoughts about it.

Thanks,
Maciek

On Tue, May 20, 2025 at 2:45 PM Tomas Vondra <tomas@vondra.me> wrote:

Thanks for the overview. I don't have any immediate feedback, but it
sounds like it might be related to the "making planner decisions clear"
session from the unconference ...

The basic premise of that session was about how to give users better
info about the planner decisions - why paths were selected/rejected,
etc. A simple example would be "why was the index not used", and the
possible answers include "dominated by cost by another path" or "does
not match the index keys" etc.

I wonder if this work might be useful for something like that.

I've been wondering that, too. There's definitely some indirect ways
in which that might be the case. For example, I think this work would
lend itself to saying "hey, try planning this query, but for that
table over there, use an index scan on this table." Then, it either
still doesn't -- meaning the index isn't usable for some reason -- or
it does and you can see the resulting plan with presumably higher cost
and maybe infer why it didn't happen. That's better than today, where
we have only very crude tools that let us do things like disable an
entire scan type for the entire query, and I think it would make it a
lot easier and less frustrating for a knowledgeable user to figure out
why things are happening.

But even though I think that would be better than today, I'm not sure
it rises to the level of actually being good, because I think it still
requires a fairly knowledgeable operator to figure things out, and you
probably have to experiment a bunch to understand the situation
instead of, say, being able to just look at the EXPLAIN plan and see
the answer. I think being able to look at the EXPLAIN plan and see the
answer, without needing a bunch of poking around, would be the ideal
scenario here.

But in some sense this is the same problem as understanding how an AI
neural network is reasoning. The answer to "why did the planner pick
plan X" is always "X was the cheapest possible plan". Ideas like "we
chose a merge join because both tables are large enough that neither
would fit into a hash table conveniently" are human explanations of
why the math had the effect that it did; they are not how the planner
actually reasons. So it's not just a matter of exposing the actual
reasoning process to the user, because the computer is not reasoning
in a way that a human would. It would have to be a matter of exposing
some kind of other information that would allow the human being to
comprehend easily what led the machine's algorithm to a certain
conclusion; and it is not obvious how to get there.

I have a sense - possibly an incorrect one - that the core of the
problem here is that the planner considers lots of very similar
alternatives. A hypothetical feature that showed the second-cheapest
plan would be all but useless, because the second-cheapest plan would
just be a very minor variation of the cheapest plan in almost all
cases. One idea that crossed my mind was to display information in
EXPLAIN about what would have happened if we'd done something really
different. For instance, suppose that at a certain level of the plan
tree we actually chose a merge join, but we also show the estimated
cost of the cheapest hash join (if any) and the cheapest nested loop
(if any) that we considered at that level. The user might be able to
draw useful conclusions based on whether those numbers were altogether
absent (i.e. that join type was not viable at all) or whether the cost
was a little higher or a lot higher than that of the path actually
chosen. For scans, you could list which indexes were believed to be
usable and perhaps what the cost would have been for the cheapest one
not actually selected; and what the cost of a sequential scan would
have been if you hadn't picked one.

I'm not sure how useful this would be, so the whole idea might
actually suck, or maybe it's sort of the right idea but needs a bunch
of refinement to really be useful. I don't have a better idea right
now, though.

If there are any notes that were taken during that unconference
session, please point me in the right direction; I was in another
session at that time but would read any available notes with interest.

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

On Tue, May 20, 2025 at 3:09 PM Maciek Sakrejda <maciek@pganalyze.com> wrote:

+1, this seems like it could be very useful. A somewhat related issue
is being able to tie plan nodes back to the query text: it can be hard
to understand the planner's decisions if it's not even clear what part
of the query it's making decisions about. I'm sure this is not an easy
problem in general, but I wonder if you think that could be improved
in the course of this work, or if you have other thoughts about it.

Thanks. I don't really have any ideas about the problem you mention,
perhaps partly because I haven't experienced it too much. I mean, I
have sometimes been confused about which parts of the query go with
which parts of the EXPLAIN, but I think in my experience so far that
is mostly because either (1) both the query and the EXPLAIN output are
super long and maybe also super-wide and therefore it's hard to
correlate things by eye or (2) somebody wrote a query where they use
the same table and/or table alias over and over again in different
parts of the query and so it's hard to tell which reference goes with
which. Neither of those problems seems all that exciting to me from a
dev perspective: if you're calling everything a or x or orders or
something, maybe don't do that, and if your query is 1500 characters
long, I guess you need to budget some time to align that with the
query plan. I don't really know how much we can do here. But maybe
there are cases that I haven't seen where something better is
possible, or perhaps you have some good idea that I haven't
considered.

(If I'm honest, I do have an idea that I think might very
significantly improve the readability of EXPLAIN output. I think it
would make it much less wide in normal cases without making it much
longer. This has been percolating in my brain for a few years now and
I have the vague intention of proposing it at some point, but not
until I'm good and ready to be flamed to a well-done crisp, because
I'm quite sure there will be more than one opinion on the merits.)

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

On Wed, May 21, 2025 at 7:29 AM Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, May 20, 2025 at 3:09 PM Maciek Sakrejda <maciek@pganalyze.com> wrote:

+1, this seems like it could be very useful. A somewhat related issue
is being able to tie plan nodes back to the query text: it can be hard
to understand the planner's decisions if it's not even clear what part
of the query it's making decisions about. I'm sure this is not an easy
problem in general, but I wonder if you think that could be improved
in the course of this work, or if you have other thoughts about it.

Thanks. I don't really have any ideas about the problem you mention,
perhaps partly because I haven't experienced it too much.

That may be due to your extensive experience with Postgres and EXPLAIN plans.

I mean, I
have sometimes been confused about which parts of the query go with
which parts of the EXPLAIN, but I think in my experience so far that
is mostly because either (1) both the query and the EXPLAIN output are
super long and maybe also super-wide and therefore it's hard to
correlate things by eye or (2) somebody wrote a query where they use
the same table and/or table alias over and over again in different
parts of the query and so it's hard to tell which reference goes with
which. Neither of those problems seems all that exciting to me from a
dev perspective: if you're calling everything a or x or orders or
something, maybe don't do that, and if your query is 1500 characters
long, I guess you need to budget some time to align that with the
query plan.

Fair enough, although the people trying to make sense of EXPLAIN plans
are sometimes not the same ones who are writing the queries. And
sometimes the queries are not written by people at all but by ORMs
(or—heaven help us—vibe coded). "Don't do X" is a reasonable response
to "It hurts when I do X," but it doesn't really solve the user's
problem. That said, it's hard to argue with "We don't have any good
ideas on how to improve this right now, and it's not a total dumpster
fire, so we'll focus on other work."

I don't really know how much we can do here. But maybe
there are cases that I haven't seen where something better is
possible, or perhaps you have some good idea that I haven't
considered.

No great ideas here. I thought initially that a good solution would be
to have structured EXPLAIN output include something like "Query Text
Start Index" and "Query Text End Index" fields for each node, but I
realized that this doesn't really work for multiple joins (and
probably other cases). Maybe "Query Text Indices", as a list of pairs?
But from the little I know about the planner, that seems like any sort
of tracking back to the source would be hard to implement. And it only
really solves the problem for external EXPLAIN viewers, and only ones
that put in the work to support this. I'm not sure if the problem can
be meaningfully addressed for text format, but maybe that's another
reason not to spend time on it in core.

(If I'm honest, I do have an idea that I think might very
significantly improve the readability of EXPLAIN output. I think it
would make it much less wide in normal cases without making it much
longer. This has been percolating in my brain for a few years now and
I have the vague intention of proposing it at some point, but not
until I'm good and ready to be flamed to a well-done crisp, because
I'm quite sure there will be more than one opinion on the merits.)

I'm intrigued, and happy to stand by with an extinguisher. The road to
great ideas is paved with bad ideas.

Thanks,
Maciek

On Wed, May 21, 2025 at 12:03 PM Maciek Sakrejda <m.sakrejda@gmail.com> wrote:

That may be due to your extensive experience with Postgres and EXPLAIN plans.

Yes, that is very possible. All things being equal, it helps to have
done something a lot of times.

Fair enough, although the people trying to make sense of EXPLAIN plans
are sometimes not the same ones who are writing the queries. And
sometimes the queries are not written by people at all but by ORMs
(or—heaven help us—vibe coded). "Don't do X" is a reasonable response
to "It hurts when I do X," but it doesn't really solve the user's
problem. That said, it's hard to argue with "We don't have any good
ideas on how to improve this right now, and it's not a total dumpster
fire, so we'll focus on other work."

+1 to all of that.

No great ideas here. I thought initially that a good solution would be
to have structured EXPLAIN output include something like "Query Text
Start Index" and "Query Text End Index" fields for each node, but I
realized that this doesn't really work for multiple joins (and
probably other cases). Maybe "Query Text Indices", as a list of pairs?
But from the little I know about the planner, that seems like any sort
of tracking back to the source would be hard to implement. And it only
really solves the problem for external EXPLAIN viewers, and only ones
that put in the work to support this. I'm not sure if the problem can
be meaningfully addressed for text format, but maybe that's another
reason not to spend time on it in core.

I'm not gonna say you couldn't make something like that work, but it
sounds like a lot of effort for a hypothetical piece of external
visualization software that might or might not produce satisfying
results. My advice to anyone wanting to pursue this idea would be:
make a totally fake POC first. Get a sample query with at least a
moderately complex plan, get the EXPLAIN output, manually generate
whatever data you think PostgreSQL ought to be able to spit out, and
do a mock-up of an external viewer. When you're happy with the
results, show it to some other people and see if they also like it. We
can have the discussion about whether to include anything in core and
what it should be after that. I definitely would not rule out the
possibility that something like this could turn out to be really cool
-- maybe hovering over stuff and having the corresponding part of the
plan get highlighted will turn out to be awesome. But I think it might
also turn out that there are things where it's not quite clear what
you can or should usefully highlight, like target-list items, or for
example a case where the query says that a.x = b.x and b.x = c.x but
in the actual plan we use evaluate a.x = c.x, an expression not
appearing anywhere in the query text. The legwork of sorting some of
that kind of stuff out should really happen before making a feature
proposal.

I'm intrigued, and happy to stand by with an extinguisher. The road to
great ideas is paved with bad ideas.

Thanks. That proposal is a task for another day, but I appreciate the sentiment.

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

Robert Haas <robertmhaas@gmail.com> writes:

Hi,

... As a recap, my overall goal here
is to make it so that you can examine a finished plan, figure out what
decisions the planner made, and then somehow get the planner to make
those same decisions over again in a future planning cycle.

I am feeling that this is similar with Oracle's outline feature, where
the final plan is examined and then a series of hints are stored and
then during the replanning of the the same query, these hints will be
applied to planner. If one of the hints is not appliable any more, like
the index is unusable, it is just ignored.

This list of elided nodes is stored in the PlannedStmt

I did a quick check on the attached patches and I can see some more
information is added into PlannedStmt. then my question are the
PlannedStmt is not avaiable during the future planning cycle, then how
does these information would be helpful on the feture planning? and I'm
strange that there are little changes on the optimizer part. Does this
patchset just a preparation work for reconstructing a same plan in
future?

outside of the portion of the tree that actually gets executed, so
that code that is doing plan tree inspection can look at it but
execution doesn't get any slower.

Thank you for sharing this!

--
Best Regards
Andy Fan

Andy Fan <zhihuifan1213@163.com> writes:

This list of elided nodes is stored in the PlannedStmt

I did a quick check on the attached patches and I can see some more
information is added into PlannedStmt. then my question are the
PlannedStmt is not avaiable during the future planning cycle, then how
does these information would be helpful on the feture planning? and I'm
strange that there are little changes on the optimizer part. Does this
patchset just a preparation work for reconstructing a same plan in
future?

I'm sure it is a preliminary work for reconstructing a same plan in
future, sorry for this noise.

--
Best Regards
Andy Fan

On Mon, May 19, 2025 at 2:01 PM Robert Haas <robertmhaas@gmail.com> wrote:

A couple of people at pgconf.dev seemed to want to know more about my
ongoing plan shape work, so here are the patches I have currently.

Here's an updated patch set. My goal for the September CommitFest is
to get patches 0001-0004 committed. Of course, if there are too many
objections or too little review, that might not happen, but that's my
goal.

This patch set is basically unchanged from the previous patch set,
except that I've added one new patch. 0007 records information about
Append node consolidation into the final plan tree. Without this, when
we build an AppendPath or MergeAppendPath and pull up the subpaths
from a similar underlying node, we can lose the RTIs from the
subordinate node, making it very difficult to analyze the plan after
the fact.

Just to remark a bit further on the structure of the patch set,
0001-0003 are closely related. The only one I really need committed in
order to move forward is 0001, but I think the others are a good idea.
There is probably room for some bikeshedding on the output produced by
0002. Then after that, 0004 stands alone as an incredibly important
and foundational patch: without it, there's no way to know what the
name of a subplan will be until after it's already been planned. I am
fairly confident in the approach that I've taken here, but it does
cause user-visible changes in EXPLAIN output about which people might
conceivably have strong opinions. Getting agreement either on what
I've done here or some variant of the approach is essential for me to
be able to move forward. Then, 0005-0007 all have to do with
preserving in the final plan various details that today would be
discarded at the end of planning. While I'm happy to have comments on
these now, I'm still not completely confident that I've found all
issues in this area or handled them perfectly; hence, I'm not in a
hurry to move forward with those just yet.

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

On Tue, Aug 26, 2025 at 10:58:33AM -0400, Robert Haas wrote:

During planning, there is one range table per subquery; at the end if
planning, those separate range tables are flattened into a single
range table. Prior to this change, it was impractical for code
examining the final plan to understand which parts of the flattened
range table came from which subquery's range table.

If the only consumer of the final plan is the executor, that is
completely fine. However, if some code wants to examine the final
plan, or what happens when we execute it, and extract information from
it that be used in future planning cycles, it's inconvenient.

I am very interested in how plans can be used for future planning.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

Do not let urgent matters crowd out time for investment in the future.

On 19/5/2025 20:01, Robert Haas wrote:

Hope you find this interesting. If you do, let me know what you think.Thanks for looking in this direction!

Since 2017, we designed features that should 'memorise' the experience
of previous executions, checking the PlanState tree and instrumentation
after execution.
The first dumb prototype you should know - AQO. The following, more
robust code is 'replan' [1]https://postgrespro.com/docs/enterprise/16/realtime-query-replanning. These two features utilise the core patch,
and I hope this patch can be removed after a few adjustments to the core.
In fact, 'replan' is used to stop execution in the middle (if the
time/memory usage limit is achieved), pass through the state, collect
valuable data and execute again. That's why using data for the
subsequent execution needs a matching query tree, and that data may be
irrelevant for a different set of constants.

The lessons learned during design and after some usage in real life:
1. Subplans should be uniquely identified during the planning. Like you
mentioned, two similar subplans on the same query level may be executed
differently and provide different row numbers to the 'knowledge base'. I
used the subquery_planner hack that generated a unique ID for each subplan.
2. Each node should be identified - I used a kind of signature at each
RelOptInfo node - just a hash generated by restrictinfos and underlying
signatures. This approach needs a create_plan hook to copy the signature
to the Plan nodes.
3. A great source of fluctuations is 'never executed' nodes - because
depending on the constant, the subtree may never be executed or produce
tons of tuples - I resolved it by just ignoring 'never executed'
results, no in-core changes needed.
4. A tree of paths may implement a single RelOptInfo. I have saved the
signature at the top of the Plan node for the corresponding RelOptInfo
and have never encountered any problems yet. It limits the use of
gathered data on cardinality/group number/peak memory consumption
somewhat, but not significantly.

I think the node extension fields and hooks, debated in the thread [2]/messages/by-id/CA+TgmoYxfg90rw13+JcYwn4dwSC+agw7o8-A+fA3M0fh96pg8w@mail.gmail.com,
may be enough to enable extensions to implement such a feature.

What's more, I personally prefer to have a hook that allows extension
checking of a condition during execution - it may be done inside the
ExecProcNode() by calling the node-specific hook, which an extension may
initialise in the PlanState structure before the start of execution.
Additionally, it would be beneficial to have a hook for error processing
at the top of the portal execution code - this is a key point for
managing query resources. If we need to stop and re-execute the query,
it is the only place where we can release all the resources assigned.
One more helpful thing - an option to postpone receiver sending data out
of the instance-side, even result headings. We may want to decide on the
query plan after some tuples are produced, and if something has already
been sent to the user, we can't just stop and rebuild the plan.

[1]: https://postgrespro.com/docs/enterprise/16/realtime-query-replanning
[2]: /messages/by-id/CA+TgmoYxfg90rw13+JcYwn4dwSC+agw7o8-A+fA3M0fh96pg8w@mail.gmail.com
/messages/by-id/CA+TgmoYxfg90rw13+JcYwn4dwSC+agw7o8-A+fA3M0fh96pg8w@mail.gmail.com

--
regards, Andrei Lepikhov

Hi Robert,

On Tue, Aug 26, 2025 at 7:59 AM Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, May 19, 2025 at 2:01 PM Robert Haas <robertmhaas@gmail.com> wrote:

A couple of people at pgconf.dev seemed to want to know more about my
ongoing plan shape work, so here are the patches I have currently.

Here's an updated patch set. My goal for the September CommitFest is
to get patches 0001-0004 committed. Of course, if there are too many
objections or too little review, that might not happen, but that's my
goal.

Thanks for the patches!

I don’t know enough about the history in this area to object to your
approach or suggest an alternative design. That said, I’ve reviewed
patches 0001-0004, and as individual patches they make sense to me.

Below are some more detailed comments, which would only be relevant if
you decide to proceed in this direction.

0002:

                         QUERY PLAN
 ----------------------------------------------------------
  Nested Loop Left Join
-   Output: t1.i, (1), t2.i2, i3, t4.i4
+   Output: t1.i, (1), t2.i2, t3.i3, t4.i4
    ->  Nested Loop Left Join
-         Output: t1.i, t2.i2, (1), i3
+         Output: t1.i, t2.i2, (1), t3.i3
          Join Filter: false
          ->  Hash Left Join
                Output: t1.i, t2.i2, (1)

These plan changes introduced by 0002, which adds schema qualifiers,
make me very happy. I think it’s a nice improvement on its own.

In reply to 0002's commit message:

XXX. I have broken this out as a separate commit for now; however,
it could be merged with the commit to add 'relids' to 'Result'; or
the patch series could even be rejiggered to present this as the
primary benefit of that change, leaving the EXPLAIN changes as a
secondary benefit, instead of the current organization, which does
the reverse.

I’m fine with the current organization. I agree that if we just
compare the EXPLAIN changes in 0001, which add additional “Replaces:”
information for the simple Result node, with the EXPLAIN changes in
0002, the changes in 0002 are arguably more attractive. However, I
think the EXPLAIN changes in 0001 are a more direct reflection of what
the rest of 0001 is trying to achieve: keeping track of the RTIs a
Result node is scanning. The changes in 0002 feel more like a side
benefit.

With that said, this is just my personal preference. If other
reviewers feel differently, I won’t object.

0003:

In get_scanned_rtindexes():
+       case T_NestLoop:
+           {
+               Bitmapset  *outer_scanrelids;
+               Bitmapset  *inner_scanrelids;
+               Bitmapset  *combined_scanrelids;
+
+               outer_scanrelids =
+                   get_scanned_rtindexes(root, plan->lefttree);
+               inner_scanrelids =
+                   get_scanned_rtindexes(root, plan->righttree);
+               combined_scanrelids =
+                   bms_union(outer_scanrelids, inner_scanrelids);
+               inner_scanrelids = remove_join_rtis(root, inner_scanrelids);
+
+               return remove_join_rtis(root, combined_scanrelids);
+               break;
+           }

It looks like there is an redundant assignment to inner_scanrelids:
+ inner_scanrelids = remove_join_rtis(root, inner_scanrelids);

0004:

There is a compiler warning reported in the CommitFest build:
https://cirrus-ci.com/task/6248981396717568

[23:03:57.811] subselect.c: In function ‘sublinktype_to_string’:
[23:03:57.811] subselect.c:3232:1: error: control reaches end of non-void
function [-Werror=return-type]
[23:03:57.811] 3232 | }
[23:03:57.811] | ^
[23:03:57.811] cc1: all warnings being treated as errors
[23:03:57.812] make[4]: *** [<builtin>: subselect.o] Error 1
[23:03:57.812] make[3]: *** [../../../src/backend/common.mk:37:
plan-recursive] Error 2
[23:03:57.812] make[2]: *** [common.mk:37: optimizer-recursive] Error 2
[23:03:57.812] make[2]: *** Waiting for unfinished jobs....
[23:04:05.513] make[1]: *** [Makefile:42: all-backend-recurse] Error 2
[23:04:05.514] make: *** [GNUmakefile:21: world-bin-src-recurse] Error 2

You might want to add a return to get rid of the warning.

Still in 0004:
--- a/src/backend/optimizer/plan/planagg.c
+++ b/src/backend/optimizer/plan/planagg.c
@@ -339,6 +340,8 @@ build_minmax_path(PlannerInfo *root, MinMaxAggInfo
*mminfo,
    memcpy(subroot, root, sizeof(PlannerInfo));
    subroot->query_level++;
    subroot->parent_root = root;
+   subroot->plan_name = choose_plan_name(root->glob, "minmax", true);
+
    /* reset subplan-related stuff */
    subroot->plan_params = NIL;
    subroot->outer_params = NULL;
@@ -359,6 +362,9 @@ build_minmax_path(PlannerInfo *root, MinMaxAggInfo
*mminfo,
    /* and we haven't created PlaceHolderInfos, either */
    Assert(subroot->placeholder_list == NIL);

+   /* Add this to list of all PlannerInfo objects. */
+   root->glob->allroots = lappend(root->glob->allroots, root);
+

In the last diff, it should add "subroot" instead of "root" to the
list of all PlannerInfos. Currently, if there are multiple minmax
expressions, we end up with the following plan showing duplicate
names:

test=# explain (costs off) SELECT MIN(value), MAX(value) FROM test_minmax;
QUERY PLAN
-------------------------------------------------------------------------------------------------
Result
Replaces: Aggregate
InitPlan minmax_1
-> Limit
-> Index Only Scan using test_minmax_value_idx on test_minmax
Index Cond: (value IS NOT NULL)
InitPlan minmax_1
-> Limit
-> Index Only Scan Backward using test_minmax_value_idx on
test_minmax test_minmax_1
Index Cond: (value IS NOT NULL)
(10 rows)

Best,
Alex

On Thu, Aug 28, 2025 at 1:22 PM Alexandra Wang
<alexandra.wang.oss@gmail.com> wrote:

Thanks for the patches!

Thanks for the review. Responding just briefly to avoid quoting too much text:

- I'm glad to hear that you like 0002 and consider it an improvement
independent of what follows.
- I'm glad to hear that you're OK with the current split between 0001 and 0002.
- I would like opinions on those topics from more people.
- I have attempted to fix all of the other mistakes that you pointed
out in the attached v3, which is also rebased.

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

On Wed, Sep 3, 2025 at 5:07 AM Robert Haas <robertmhaas@gmail.com> wrote:

Thanks for the review. Responding just briefly to avoid quoting too much text:

- I'm glad to hear that you like 0002 and consider it an improvement
independent of what follows.
- I'm glad to hear that you're OK with the current split between 0001 and 0002.
- I would like opinions on those topics from more people.
- I have attempted to fix all of the other mistakes that you pointed
out in the attached v3, which is also rebased.

I've reviewed 0001 to 0003, and they all make sense to me. The
changes to the EXPLAIN output in 0001 and 0002 are very nice
improvements.

I found some issues with 0003 though. It seems get_scanned_rtindexes
is intended to return RTI sets with outer join relids excluded. For
some node types, such as Append and MergeAppend, it fails to do so,
which can cause the assertion in assert_join_preserves_scan_rtis to
fail. For example:

create table p (a int, b int) partition by range(a);
create table p1 partition of p for values from (0) to (10);
create table p2 partition of p for values from (10) to (20);

set enable_partitionwise_join to on;

explain (costs off)
select * from p t1
left join p t2 on t1.a = t2.a
left join p t3 on t2.b = t3.b;
server closed the connection unexpectedly

Besides, to exclude outer join relids, it iterates over the RTI sets,
checks each RTE for type RTE_JOIN, and bms_del_member it if found (cf.
remove_join_rtis). I think a simpler approach would be to leverage
PlannerInfo.outer_join_rels:

scanrelids = bms_difference(scanrelids, root->outer_join_rels);

Therefore, I suggest that we don't try to remove the join RTIs in
get_scanned_rtindexes. Instead, we do that in
assert_join_preserves_scan_rtis -- before comparing the RTIs from the
outer and inner subplans with the join's RTIs -- by leveraging
PlannerInfo.outer_join_rels. And remove_join_rtis can be retired.

- Richard

On Thu, Sep 4, 2025 at 4:21 AM Richard Guo <guofenglinux@gmail.com> wrote:

I found some issues with 0003 though. It seems get_scanned_rtindexes
is intended to return RTI sets with outer join relids excluded. For
some node types, such as Append and MergeAppend, it fails to do so,
which can cause the assertion in assert_join_preserves_scan_rtis to
fail. For example:

create table p (a int, b int) partition by range(a);
create table p1 partition of p for values from (0) to (10);
create table p2 partition of p for values from (10) to (20);

set enable_partitionwise_join to on;

explain (costs off)
select * from p t1
left join p t2 on t1.a = t2.a
left join p t3 on t2.b = t3.b;
server closed the connection unexpectedly

Ouch. Good catch.

Besides, to exclude outer join relids, it iterates over the RTI sets,
checks each RTE for type RTE_JOIN, and bms_del_member it if found (cf.
remove_join_rtis). I think a simpler approach would be to leverage
PlannerInfo.outer_join_rels:

scanrelids = bms_difference(scanrelids, root->outer_join_rels);

I was not aware of outer_join_rels, so thank you for pointing it out.
However, consider this query:

select 1 from pg_class a inner join pg_class b on a.relfilenode = b.relfilenode;

Here, we end up with a three-item range table: one for a, one for b,
and one for the join. But the join is not an outer join, and does not
appear in root->outer_join_rels. Therefore, I'm not sure we can rely
on outer_join_rels in this scenario.

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

Robert Haas <robertmhaas@gmail.com> writes:

I was not aware of outer_join_rels, so thank you for pointing it out.
However, consider this query:

select 1 from pg_class a inner join pg_class b on a.relfilenode = b.relfilenode;

Here, we end up with a three-item range table: one for a, one for b,
and one for the join. But the join is not an outer join, and does not
appear in root->outer_join_rels. Therefore, I'm not sure we can rely
on outer_join_rels in this scenario.

Plain (not-outer) joins will never be included in a relid set in the
first place.

regards, tom lane

On Fri, Sep 5, 2025 at 12:00 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Plain (not-outer) joins will never be included in a relid set in the
first place.

Ah, well then Richard's idea might work! Let me try it and see what happens...

Thanks,

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

On Fri, Sep 5, 2025 at 12:40 PM Robert Haas <robertmhaas@gmail.com> wrote:

Ah, well then Richard's idea might work! Let me try it and see what happens...

Here's the result. Seem to work OK.

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

On Wed, May 21, 2025 at 7:29 PM Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, May 20, 2025 at 2:45 PM Tomas Vondra <tomas@vondra.me> wrote:

I have a sense - possibly an incorrect one - that the core of the
problem here is that the planner considers lots of very similar
alternatives. A hypothetical feature that showed the second-cheapest
plan would be all but useless, because the second-cheapest plan would
just be a very minor variation of the cheapest plan in almost all
cases. One idea that crossed my mind was to display information in
EXPLAIN about what would have happened if we'd done something really
different. For instance, suppose that at a certain level of the plan
tree we actually chose a merge join, but we also show the estimated
cost of the cheapest hash join (if any) and the cheapest nested loop
(if any) that we considered at that level. The user might be able to
draw useful conclusions based on whether those numbers were altogether
absent (i.e. that join type was not viable at all) or whether the cost
was a little higher or a lot higher than that of the path actually
chosen. For scans, you could list which indexes were believed to be
usable and perhaps what the cost would have been for the cheapest one
not actually selected; and what the cost of a sequential scan would
have been if you hadn't picked one.

I'm not sure how useful this would be, so the whole idea might
actually suck, or maybe it's sort of the right idea but needs a bunch
of refinement to really be useful. I don't have a better idea right
now, though.

Having detailed information on the costs of alternative join
methods/scan method, even when a different method is chosen, would be
valuable information. For example, if a merge join is selected for
tables t1 and t2 in a subquery, showing the estimated costs for both a
hash join and a nested loop join would provide a more complete picture
of the planner's decision-making process.

And I believe, this information would be particularly useful if the
cost of a non-selected plan, such as a nested loop join, is very close
to the cost of the chosen merge join. In such cases, a database
administrator or query optimizer could use this insight to manually
override the planner's choice and opt for the nested loop join for
specific tables in a subquery. This level of detail would empower
users to fine-tune query performance and explore alternative execution
strategies.

IIUC, one of the goal of this work is where operator can say I want to
use this scan method while scanning a particular table in a particular
subquery, that means if the planner can give the information about non
selected paths as well then it would be really helpful in making this
process more smooth otherwise without much information on what path
got rejected its very hard to provide hints.

--
Regards,
Dilip Kumar
Google