Обсуждение: pg_plan_advice

Поиск

Список

Период

Сортировка

pg_plan_advice

От

Robert Haas

Дата:

30 октября 2025 г., 17:00:05

As I have mentioned on previous threads, for the past while I have
been working on planner extensibility. I've posted some extensibility
patches previously, and got a few of them committed in
Sepember/October with Tom's help, but I think the time has come a
patch which actually makes use of that infrastructure as well as some
further infrastructure that I'm also including in this posting.[1] The
final patch in this series adds a new contrib module called
pg_plan_advice. Very briefly, what pg_plan_advice knows how to do is
process a plan and emits a (potentially long) long text string in a
special-purpose mini-language that describes a bunch of key planning
decisions, such as the join order, selected join methods, types of
scans used to access individual tables, and where and how
partitionwise join and parallelism were used. You can then set
pg_plan_advice.advice to that string to get a future attempt to plan
the same query to reproduce those decisions, or (maybe a better idea)
you can trim that string down to constrain some decisions (e.g. the
join order) but not others (e.g. the join methods), or (if you want to
make your life more exciting) you can edit that advice string and
thereby attempt to coerce the planner into planning the query the way
you think best. There is a README that explains the design philosophy
and thinking in a lot more detail, which is a good place to start if
you're curious, and I implore you to read it if you're interested, and
*especially* if you're thinking of flaming me.

But that doesn't mean that you *shouldn't* flame me. There are a
remarkable number of things that someone could legitimately be unhappy
about in this patch set. First, any form of user control over the
planner tends to be a lightning rod for criticism around here. I've
come to believe that's the wrong way of thinking about it: we can want
to improve the planner over the long term and *also* want to have
tools available to work around problems with it in the short term.
Further, we should not imagine that we're going to solve problems that
have stumped other successful database projects any time in the
foreseeable future; no product will ever get 100% of cases right, and
you don't need to get to very obscure cases before other products
throw up their hands just as we do. But, second, even if you're OK
with the idea of some kind of user control over the planner, you could
very well be of the opinion that what I've implemented here is utter
crap. I've certainly had to make a ton of very opinionated decisions
to get to this point, and you are entitled to hate them. Of course, I
did have *reasons* for making the decisions, so if your operating
theory as to why I did something is that I'm a stupid moron, perhaps
consider an alternative explanation or two as well. Finally, even if
you're OK with the concept and feel that I've made some basically
reasonable design decisions, you might notice that the code is full of
bugs, needs a lot of cleanup, is missing features, lacks
documentation, and a bunch of other stuff. In that judgement, you
would be absolutely correct. I'm not posting it here because I'm
hoping to get it committed in November -- or at least, not THIS
November.  What I would like to do is getting some design feedback on
the preliminary patches, which I think will be more possible if
reviewers also have the main pg_plan_advice to look at as a way of
understanding why the exist, and also some feedback on the
pg_plan_advice patch itself.

Now I do want to caveat the statement that I am looking for feedback
just a little bit. I imagine that there will be some people reading
this who are already imagining how great life will be when they put
this into production, and begin complaining about either (1) features
that it's missing or (2) things that they don't like about the design
of the advice mini-language. What I'd ask you to keep in mind is that
you will not be able to put this into production unless and until
something gets committed, and getting this committed is probably going
to be super-hard even if you don't creep the scope, so maybe don't do
that, especially if you haven't read the README yet to understand what
the scope is actually intended to be. The details of the advice
mini-language are certainly open to negotiation; of everything, that
would be one of the easier things to change. However, keep in mind
that there are probably LOTS AND LOTS of people who all have their own
opinions about what decisions I should have made when designing that
mini-language, and an outcome where you personally get everything you
want and everyone who disagrees is out of luck is unlikely. In other
words, constructive suggestions for improvement are welcome, but
please think twice before turning this into a bikeshedding nightmare.
Now is the time to talk about whether I've got the overall design
somewhat correct moreso than whether I've spelled everything the way
you happen to prefer.[2]

I want to mention that, beyond the fact that I'm sure some people will
want to use something like this (with more feature and a lot fewer
bugs) in production, it seems to be super-useful for testing. We have
a lot of regression test cases that try to coerce the planner to do a
particular thing by manipulating enable_* GUCs, and I've spent a lot
of time trying to do similar things by hand, either for regression
test coverage or just private testing. This facility, even with all of
the bugs and limitations that it currently has, is exponentially more
powerful than frobbing enable_* GUCs. Once you get the hang of the
advice mini-language, you can very quickly experiment with all sorts
of plan shapes in ways that are currently very hard to do, and thereby
find out how expensive the planner thinks those things are and which
ones it thinks are even legal. So I see this as not only something
that people might find useful for in production deployments, but also
something that can potentially be really useful to advance PostgreSQL
development.

Which brings me to the question of where this code ought to go if it
goes anywhere at all. I decided to propose pg_plan_advice as a contrib
module rather than a part of core because I had to make a WHOLE lot of
opinionated design decisions just to get to the point of having
something that I could post and hopefully get feedback on. I figured
that all of those opinionated decisions would be a bit less
unpalatable if they were mostly encapsulated in a contrib module, with
the potential for some future patch author to write a different
contrib module that adopted different solutions to all of those
problems. But what I've also come to realize is that there's so much
infrastructure here that leaving the next person to reinvent it may
not be all that appealing. Query jumbling is a previous case where we
initially thought that different people might want to do different
things, but eventually realized that most people really just wanted
some solution that they didn't have to think too hard about. Likewise,
in this patch, the relation identifier system described in the README
is the only thing of its kind, to my knowledge, and any system that
wants to accomplish something similar to what pg_plan_advice does
would need a system like that. pg_hint_plan doesn't have something
like that, because pg_hint_plan is just trying to do hints. This is
trying to do round-trip-safe plan stability, where the system will
tell you how to refer unambiguously to a certain part of the query in
a way that will work correctly on every single query regardless of how
it's structured or how many times it refers to the same tables or to
different tables using the same aliases. If we say that we're never
going to put any of that infrastructure in core, then anyone who wants
to write a module to control the planner is going to need to start by
either (a) reinventing something similar, (b) cloning all the relevant
code, or (c) just giving up on the idea of unambiguous references to
parts of a query. None of those seem like great options, so now I'm
less sure whether contrib is actually the right place for this code,
but that's where I have put it for now. Feedback welcome, on this and
everything else.

Perhaps more than any other patch I've ever written, I know I'm
playing with fire here just by putting this out on the list, but I'm
nevertheless hopeful that something good can come of it, and I hope we
can have a constructive discussion about what that thing should be. I
think there is unquestionably is a lot of demand for the ability to
influence the planner in some form, but there is a lot of room for
debate about what exactly that should mean in practice. While I
personally am pretty happy with the direction of the code I've
written, modulo the large amount of not-yet-completed bug fixing and
cleanup, there's certainly plenty of room for other people to feel
differently, and finding out what other people think is, of course,
the whole point of posting things publicly before committing them --
or in this case, before even finishing them.[3] If you're interested
it contributing to the conversation, I urge you to start with the
following things: (1) the README in the final patch; (2) the
regression test examples in the final patch, which give a good sense
of what it actually looks like to use this; and (3) the earlier
patches, which show the minimum amount of core infrastructure that I
think we need in order to make something like this workable (ideas on
how to further reduce that footprint are very welcome).

Thanks,

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

[1] All of the earlier patches have been posted previously in some
form, but the commit messages have been rewritten for clarity, and the
"Allow for plugin control over path generation strategies" patch has
been heavily rewritten since it was last posted; the earlier versions
turned out to have substantial inadequacies.

[2] This is not to say that proposal to modify or improve the syntax
are unwelcome, but the bigger obstacle to getting something committed
here is probably reaching some agreement on the internal details. Any
changes to src/include/optimizer or src/backend/optimizer need careful
scrutiny from a design perspective. Also, keep in mind that the syntax
needs to fit what we can actually do: a proposal to change the syntax
to something that implies semantics we can't implement is a dead
letter.

[3] Note, however, that a proposal to achieve the same or similar
goals by different means is more welcome than a proposal that I should
have done some other project entirely. I've already put a lot of work
into these goals and hope to achieve them, at least to some degree,
before I start working toward something else.

Вложения

Re: pg_plan_advice

От

Jakub Wartak

Дата:

31 октября 2025 г., 12:58:59

On Thu, Oct 30, 2025 at 3:00 PM Robert Haas <robertmhaas@gmail.com> wrote:

[..over 400kB of attachments, yay]

Thank You for working on this!

My gcc-13 was nitpicking a little bit (see
compilation_warnings_v1.txt), so attached is just a tiny diff to fix
some of those issues. After that, clang-20 run was clean too.

> First, any form of user control over the planner tends to be a lightning rod for criticism around here.

I do not know where this is coming from, but everybody I've talked to
was saying this is needed to handle real enterprise databases and
applications. I just really love it, how one could precisely adjust
the plan with this even with the presence of heavy aliasing:

postgres=# explain (plan_advice, costs off) SELECT * FROM (select *
from t1 a join t2 b using (id)) a, t2 b, t3 c WHERE a.id = b.id and
b.id = c.id;
                     QUERY PLAN
-----------------------------------------------------
 Merge Join
   Merge Cond: (a.id = c.id)
   ->  Merge Join
         Merge Cond: (a.id = b.id)
         ->  Index Scan using t1_pkey on t1 a
         ->  Index Scan using t2_pkey on t2 b
   ->  Sort
         Sort Key: c.id
         ->  Seq Scan on t3 c
 Supplied Plan Advice:
   SEQ_SCAN(ble5) /* not matched */
 Generated Plan Advice:
   JOIN_ORDER(a#2 b#2 c)
   MERGE_JOIN_PLAIN(b#2 c)
   SEQ_SCAN(c)
   INDEX_SCAN(a#2 public.t1_pkey )
   NO_GATHER(c a#2 b#2)
(17 rows)

postgres=# set pg_plan_advice.advice = 'SEQ_SCAN(b#2)';
SET
postgres=# explain (plan_advice, costs off) SELECT * FROM (select *
from t1 a join t2 b using (id)) a, t2 b, t3 c WHERE a.id = b.id and
b.id = c.id;
                     QUERY PLAN
----------------------------------------------------
 Hash Join
   Hash Cond: (b.id = a.id)
   ->  Seq Scan on t2 b
   ->  Hash
         ->  Merge Join
               Merge Cond: (a.id = c.id)
               ->  Index Scan using t1_pkey on t1 a
               ->  Sort
                     Sort Key: c.id
                     ->  Seq Scan on t3 c
 Supplied Plan Advice:
   SEQ_SCAN(b#2) /* matched */
 Generated Plan Advice:
   JOIN_ORDER(b#2 (a#2 c))
   MERGE_JOIN_PLAIN(c)
   HASH_JOIN(c)
   SEQ_SCAN(b#2 c)
   INDEX_SCAN(a#2 public.t1_pkey)
   NO_GATHER(c a#2 b#2)

To attract a little attention to the $thread, the only bigger design
(usability) question that keeps ringing in my head is how we are going
to bind it to specific queries without even issuing any SETs(or ALTER
USER) in the far future in the grand scheme of things. The discussed
query id (hash), full query text comparison, maybe even strstr(query ,
"partial hit") or regex all seem to be kind too limited in terms of
what crazy ORMs can come up with (each query will be potentially
slightly different, but if optimizer reference points are stable that
should nail it good enough, but just enabling it for the very specific
set of queries and not the others [with same aliases] is some major
challenge).

Due to this, at some point I was even thinking about some hashes for
every plan node (including hashes of subplans), e.g.:
 Merge Join // hash(MERGE_JOIN_PLAIN(b#2) + ';' somehashval1 + ';'+
somehahsval2 ) => somehashval3
   Merge Cond: (a.id = c.id)
   ->  Merge Join
         Merge Cond: (a.id = b.id)
         ->  Index Scan using t1_pkey on t1 a // hash(INDEX_SCAN(a#2
public.t1_pkey)) => somehashval1
         ->  Index Scan using t2_pkey on t2 b // hash(INDEX_SCAN(b#2
public.t2_pkey)) => somehashval2

and then having a way to use `somehashval3` (let's say it's SHA1) as a
way to activate the necessary advice. Something like having a way to
express it using plan_advice.on_subplanhashes_plan_advice =
'somehashval3: SEQ_SCAN(b#2)'. This would have the benefit of being
able to override multiple similiar SQL queries in one go rather than
collecting all possible query_ids, but probably it's stupid,
heavyweight, but that would be my dream ;)

-J.

Вложения

Re: pg_plan_advice

От

Robert Haas

Дата:

31 октября 2025 г., 15:51:34

On Fri, Oct 31, 2025 at 5:59 AM Jakub Wartak
<jakub.wartak@enterprisedb.com> wrote:
> > First, any form of user control over the planner tends to be a lightning rod for criticism around here.
>
> I do not know where this is coming from, but everybody I've talked to
> was saying this is needed to handle real enterprise databases and
> applications. I just really love it, how one could precisely adjust
> the plan with this even with the presence of heavy aliasing:

Thanks for the kind words.

I'll respond to the points about compiler warnings later.

> To attract a little attention to the $thread, the only bigger design
> (usability) question that keeps ringing in my head is how we are going
> to bind it to specific queries without even issuing any SETs(or ALTER
> USER) in the far future in the grand scheme of things. The discussed
> query id (hash), full query text comparison, maybe even strstr(query ,
> "partial hit") or regex all seem to be kind too limited in terms of
> what crazy ORMs can come up with (each query will be potentially
> slightly different, but if optimizer reference points are stable that
> should nail it good enough, but just enabling it for the very specific
> set of queries and not the others [with same aliases] is some major
> challenge).

Yeah, I haven't really dealt with this problem yet.

> Due to this, at some point I was even thinking about some hashes for
> every plan node (including hashes of subplans),
[...]
>
> and then having a way to use `somehashval3` (let's say it's SHA1) as a
> way to activate the necessary advice. Something like having a way to

This doesn't make sense to me, because it seems circular. We can't use
anything in the plan to choose which advice string to use, because the
purpose of the advice string is to influence the choice of plan. In
other words, our choice of what advice string to use has to be based
on the properties of the query, not the plan. We can implement
anything we want to do in terms of exactly how that works: we can use
the query ID, or the query text, or the query node tree.
Hypothetically, we could call out to a user-defined function and pass
the query text or the query node tree as an argument and let it do
whatever it wants to decide on an advice string. The practical problem
here is computational cost -- any computation that gets performed for
every single query is going to have to be pretty cheap to avoid
creating a performance problem. That's why I thought matching on query
ID or exact matching on query text would likely be the most practical
approaches, aside from the obvious alternative of setting and
resetting pg_plan_advice.advice manually. But I haven't really
explored this area too much yet, because I need to get all the basics
working first.

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

Re: pg_plan_advice

От

Alastair Turner

Дата:

01 ноября 2025 г., 00:17:41

On Fri, 31 Oct 2025, 12:51 Robert Haas, <robertmhaas@gmail.com> wrote:

On Fri, Oct 31, 2025 at 5:59 AM Jakub Wartak
<jakub.wartak@enterprisedb.com> wrote:
> > First, any form of user control over the planner tends to be a lightning rod for criticism around here.
>
> I do not know where this is coming from, but everybody I've talked to
> was saying this is needed to handle real enterprise databases and
> applications. I just really love it, how one could precisely adjust
> the plan with this even with the presence of heavy aliasing:

I really like the functionality of the current patch as well, even though I am suspicious of user control over the planner. By giving concise, precise control over a plan, this allows people who believe they can out-plan the planner to test their alternative, and possibly fail.

Whatever other UIs and integrations you build as you develop this towards you goal, please keep what's currently there user accessible. Not only for testing code, but also for testing users' belief that they know better.

Alastair

Re: pg_plan_advice

От

Hannu Krosing

Дата:

01 ноября 2025 г., 19:10:08

This weas recently shared in LinkedIn
https://www.vldb.org/pvldb/vol18/p5126-bress.pdf

For example it says that 31% of all queries are metadata queries, 78%
have LIMIT, 20% of queries have 10+ joins, with 0.52% exceeding 100
joins. , 12% of expressions have depths between 11-100 levels, some
exceeding 100. These deeply nested conditions create optimization
challenges benchmarks don't capture.etc.

This reinforces my belief thet we either should have some kind of
two-level optimization, where most queries are handled quickly but
with something to trigger a more elaborate optimisation and
investigation workflow.

Or alternatively we could just have an extra layer before the query is
sent to the database which deals with unwinding the product of
excessively stupid query generators (usually, but not always, some BI
tools :) )


On Fri, Oct 31, 2025 at 10:18 PM Alastair Turner <minion@decodable.me> wrote:
>
>
> On Fri, 31 Oct 2025, 12:51 Robert Haas, <robertmhaas@gmail.com> wrote:
>>
>> On Fri, Oct 31, 2025 at 5:59 AM Jakub Wartak
>> <jakub.wartak@enterprisedb.com> wrote:
>> > > First, any form of user control over the planner tends to be a lightning rod for criticism around here.
>> >
>> > I do not know where this is coming from, but everybody I've talked to
>> > was saying this is needed to handle real enterprise databases and
>> > applications. I just really love it, how one could precisely adjust
>> > the plan with this even with the presence of heavy aliasing:
>
>
> I really like the functionality of the current patch as well, even though I am suspicious of user control over the
planner.By giving concise, precise control over a plan, this allows people who believe they can out-plan the planner to
testtheir alternative, and possibly fail. 
>
> Whatever other UIs and integrations you build as you develop this towards you goal, please keep what's currently
thereuser accessible. Not only for testing code, but also for testing users' belief that they know better. 
>
> Alastair

Re: pg_plan_advice

От

Robert Haas

Дата:

03 ноября 2025 г., 19:18:08

On Fri, Oct 31, 2025 at 5:17 PM Alastair Turner <minion@decodable.me> wrote:
> I really like the functionality of the current patch as well, even though I am suspicious of user control over the
planner.By giving concise, precise control over a plan, this allows people who believe they can out-plan the planner to
testtheir alternative, and possibly fail.

Indeed. The downside of letting people control anything is that they
may leverage that control to do something bad. However, I think it is
unlikely that very many people would prefer to write an entire query
plan by hand. If you wanted to do that, why would you being using
PostgreSQL in the first place? Furthermore, if somebody does try to do
that, I expect that they will find it frustrating and difficult: the
planner considers a large number of options even for simple queries
and an absolutely vast number of options for more difficult queries,
and a human being trying possibilities one by one is only ever going
to consider a tiny fraction of those possibilities. The ideal
possibility often won't be in that small subset of the search space,
and the user will be wasting their time. If that were the design goal
of this feature, I don't think it would be worth having.

But it isn't. As I say in the README, what I consider the principal
use case is reproducing plans that you know to have worked well in the
past. Sometimes, the planner is correct for a while and then it's
wrong later. We don't need to accept the proposition that users can
out-plan the planner. We only need to accept that they can tell good
plans from bad plans better than the planner. That is a low bar to
clear. The planner never finds out what happens when the plans that it
generates are actually executed, but users do. If they are
sufficiently experienced, they can make reasonable judgements about
whether the plan they're currently getting is one they'd like to
continue getting. Of course, they may make wrong judgements even then,
because they lack knowledge or experience or just make a mistake, but
it's not a farcically unreasonable thing to do. I've basically never
wanted to write my own query plan from scratch, but I've certainly
looked at many plans over the years and judged them to be great, or
terrible, or good for now but risky in the long-term; and I'm probably
not the only human being on the planet capable of making such
judgements with some degree of competence.

> Whatever other UIs and integrations you build as you develop this towards you goal, please keep what's currently
thereuser accessible. Not only for testing code, but also for testing users' belief that they know better.

And this is also a good point. Knowledgeable and experienced users can
look at a plan that the planner generated, feel like it's bad, and
wonder why the planner picked it. You can try to figure that out by,
for example, setting enable_SOMETHING = false and re-running EXPLAIN,
but since there aren't that many such knobs relevant to any given
query, and since changing any of those knobs can affect large swathes
of the query and not just the part you're trying to understand better,
it can actually be really difficult to understand why the planner
thought that something was the best option. Sometimes you can't even
tell whether the planner thinks that the plan you expected to be
chosen is *impossible* or just *more expensive*, which is always one
of the things that I'm keen to find out when something weird is
happening. This can make answering that question a great deal easier.
If some important index is not getting used, you can say "no, really,
I want to see what happens with this query when you plan it with that
index" -- and then it either gives you a plan that does use that
index, and you can see how much more expensive it is and why, or it
still doesn't give you a plan using that index, and you know that the
index is inapplicable to the query or unusable in general for some
reason. You don't necessarily have it as a goal to coerce the planner
in production; your goal may very well be to find out why your belief
that you know better is incorrect.

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

Re: pg_plan_advice

От

Robert Haas

Дата:

03 ноября 2025 г., 19:41:35

On Sat, Nov 1, 2025 at 12:10 PM Hannu Krosing <hannuk@google.com> wrote:
> This reinforces my belief thet we either should have some kind of
> two-level optimization, where most queries are handled quickly but
> with something to trigger a more elaborate optimisation and
> investigation workflow.
>
> Or alternatively we could just have an extra layer before the query is
> sent to the database which deals with unwinding the product of
> excessively stupid query generators (usually, but not always, some BI
> tools :) )

I'd like to keep the focus of this thread on the patches that I'm
proposing, rather than other ideas for improving the planner. I
actually agree with you that at least the first of these things might
be a very good idea, but that would be an entirely separate project
from these patches, and I feel a lot more qualified to do this project
than that one.

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

Re: pg_plan_advice

От

John Naylor

Дата:

04 ноября 2025 г., 14:47:48

On Thu, Oct 30, 2025 at 9:00 PM Robert Haas <robertmhaas@gmail.com> wrote:
> First, any form of user control over the
> planner tends to be a lightning rod for criticism around here. I've
> come to believe that's the wrong way of thinking about it: we can want
> to improve the planner over the long term and *also* want to have
> tools available to work around problems with it in the short term.

The most frustrating real-world incidents I've had were in the course
of customers planning a major version upgrade, or worse, after
upgrading and finding that a 5 minute query now takes 5 hours. I
mention this to emphasize that workarounds will be needed also to deal
with rare unintended effects that arise from our very attempts to
improve the planner.

> Further, we should not imagine that we're going to solve problems that
> have stumped other successful database projects any time in the
> foreseeable future; no product will ever get 100% of cases right, and
> you don't need to get to very obscure cases before other products
> throw up their hands just as we do.

Right.

> it seems to be super-useful for testing. We have
> a lot of regression test cases that try to coerce the planner to do a
> particular thing by manipulating enable_* GUCs, and I've spent a lot
> of time trying to do similar things by hand, either for regression
> test coverage or just private testing. This facility, even with all of
> the bugs and limitations that it currently has, is exponentially more
> powerful than frobbing enable_* GUCs. Once you get the hang of the
> advice mini-language, you can very quickly experiment with all sorts
> of plan shapes in ways that are currently very hard to do, and thereby
> find out how expensive the planner thinks those things are and which
> ones it thinks are even legal. So I see this as not only something
> that people might find useful for in production deployments, but also
> something that can potentially be really useful to advance PostgreSQL
> development.

That sounds very useful as well.

--
John Naylor
Amazon Web Services

Re: pg_plan_advice

От

Robert Haas

Дата:

04 ноября 2025 г., 22:54:44

On Fri, Oct 31, 2025 at 5:59 AM Jakub Wartak
<jakub.wartak@enterprisedb.com> wrote:
> My gcc-13 was nitpicking a little bit (see
> compilation_warnings_v1.txt), so attached is just a tiny diff to fix
> some of those issues. After that, clang-20 run was clean too.

Here's v2. Change log:

- Attempted to fix the compiler warnings. I didn't add elog() before
pg_unreachable() as you suggested; instead, I added a dummy return
afterwards. Let's see if that works. Also, I decided after reading the
comment for list_truncate() that what I'd done there was not going to
be acceptable, so I rewrote the code slightly. It now copies the list
when adding to it, instead of relying on the ability to use
list_truncate() to recreate the prior tstate.

- Deleted the SQL-callable pg_parse_advice function and related code.
That was useful to me early in development but I don't think anyone
will need it at this point; if you want to test whether an advice
string can be parsed, just try setting pg_plan_advice.advice.

- Fixed a couple of dumb bugs in pgpa_trove.c.

- Added a few more regression test scenarios.

- Fixed a couple of typos/thinkos.

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

Вложения

Re: pg_plan_advice

От

Robert Haas

Дата:

06 ноября 2025 г., 19:45:47

Here's v3. I've attempted to fix some more things that cfbot didn't
like, one of which was an actual bug in 0005, and I also fixed a
stupid few bugs in pgpa_collector.c and added a few more tests.

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

Here's v4. This version has some bug fixes and test case changes to
0005 and 0006, with the goal of getting CI to pass cleanly (which it
now does for me, but let's see if cfbot agrees).

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

Вложения

Re: pg_plan_advice

От

"Dian Fay"

Дата:

23 ноября 2025 г., 03:43:40

On Tue Nov 18, 2025 at 11:19 AM EST, Robert Haas wrote:
> Here's v4. This version has some bug fixes and test case changes to
> 0005 and 0006, with the goal of getting CI to pass cleanly (which it
> now does for me, but let's see if cfbot agrees).

Thanks for working on this, Robert! I think the design seems solid (and
very powerful) from a user perspective. I was curious what would happen
with row-level security interactions so I tried it out on a toy example
I put together a while back. I found one case where scan advice fails on
an intentionally naive/bad policy implementation, but I'm not sure why
and it seems like the kind of weird corner case that might be useful to
reason about. See attached for the setup script, then:

set pg_plan_advice.advice = 'BITMAP_HEAP_SCAN(item public.item_tags_idx)';
set item_reader.allowed_tags = '{alpha,beta}';
set role item_reader;

explain (plan_advice, analyze, verbose, costs, timing)
select * from item
where value ilike 'a%' and tags && array[1];

Seq Scan on public.item  (cost=0.00..41777312.00 rows=54961 width=67) (actual time=2.947..8603.333 rows=6762.00
loops=1)
 Disabled: true
 Output: item.id, item.value, item.tags
 Filter: (EXISTS(SubPlan exists_1) AND (item.value ~~* 'a%'::text) AND (item.tags && '{1}'::integer[]))
 Rows Removed by Filter: 993238
 Buffers: shared hit=1012312
 SubPlan exists_1
   ->  Seq Scan on public.tag  (cost=0.00..41.75 rows=1 width=0) (actual time=0.008..0.008 rows=0.21 loops=1000000)
         Filter: ((current_setting('item_reader.allowed_tags'::text) IS NOT NULL) AND
((current_setting('item_reader.allowed_tags'::text))::text[]@> ARRAY[tag.name]) AND (item.tags @> ARRAY[tag.id])) 
         Rows Removed by Filter: 18
         Buffers: shared hit=1000000
Planning Time: 1.168 ms
Supplied Plan Advice:
 BITMAP_HEAP_SCAN(item public.item_tags_idx) /* matched, failed */
Generated Plan Advice:
 SEQ_SCAN(item tag@exists_1)
 NO_GATHER(item tag@exists_1)
Execution Time: 8603.615 ms

Since the policies don't contain any execution boundaries, all the quals
should be going into a single bucket for planning if I understand the
process correctly. The bitmap heap scan should be a candidate given the
`tags &&` predicate (and indeed if I switch to a privileged role, the
advice matches successfully without any policies in the mix), but gdb
shows the walker bouncing out of pgpa_walker_contains_scan without any
candidate scans for the BITMAP_HEAP_SCAN strategy.

I do want to avoid getting bikesheddy about the advice language so I'll
forbear from syntax discussion, but one design thought with lower-level
implications did occur to me as I was playing with this: it might be
useful in some situations to influence the planner _away_ from known
worse paths while leaving it room to decide on the best other option. I
think the work you did in path management should make this pretty
straightforward for join and scan strategies, since it looks like you've
basically made the enable_* gucs a runtime-configurable bitmask (which
seems like a perfectly reasonable approach to my "have done some source
diving but not an internals hacker" eyes), and could disable one as
easily as forcing one.

"Don't use this one index" sounds more fiddly to implement, but also
less valuable since in that case you probably already know which other
index it should be using.

Вложения

rls-demo-item-tags.sql

Re: pg_plan_advice

От

Robert Haas

Дата:

24 ноября 2025 г., 19:14:27

On Sat, Nov 22, 2025 at 7:43 PM Dian Fay <di@nmfay.com> wrote:
> Thanks for working on this, Robert!

Thanks for looking at it! I was hoping for a bit more in the way of
responses by now, honestly.

> Since the policies don't contain any execution boundaries, all the quals
> should be going into a single bucket for planning if I understand the
> process correctly. The bitmap heap scan should be a candidate given the
> `tags &&` predicate (and indeed if I switch to a privileged role, the
> advice matches successfully without any policies in the mix), but gdb
> shows the walker bouncing out of pgpa_walker_contains_scan without any
> candidate scans for the BITMAP_HEAP_SCAN strategy.

I can understand why it seems that way, but when I try setting
enable_seqscan=false instead of using pg_plan_advice, I get exactly
the same result. I think this is actually a great example both of why
this is actually a very powerful tool and also why it has the
potential to be really confusing. The power comes from the fact that
you can find out whether the planner thinks that the thing you want to
do is even possible. In this case, that's easy anyway because the
example is simple enough, but sometimes you can't set
enable_seqscan=false or similar because it would change too many other
things in the plan at that same time and you wouldn't be able to
compare. In those situations, this figures to be useful. However, all
this can do is tell you that the answer to the question "is this a
possible plan shape?" is "no". It cannot tell you why, and you may
easily find the result counterintuitive.

And honestly, this is one of the things I'm worried about if we go
forward with this, that we'll get a ton of people who think it doesn't
work because it doesn't force the planner to do things which the
planner rejects on non-cost considerations. We're going to need really
good documentation to explain to people that if you use this to try to
force a plan and you can't, that's not a bug, that's the planner
telling you that that plan shape is not able to be considered for some
reason. That won't keep people from complaining about things that
aren't really bugs, but at least it will mean that there's a link we
can give them to explain why the way they're thinking about it is
incorrect. However, that will just beg the next question of WHY the
planner doesn't think a certain plan can be considered, and honestly,
I've found over the years that I often need to resort to the source
code to answer those kinds of questions. People who are not good at
reading C source code are not going to like that answer very much, but
I still think it's better if they know THAT the planner thinks the
plan shape is impossible even if we can't tell them WHY the planner
thinks that the plan shape is impossible. We probably will want to
document at least some of the common reasons why this happens, to cut
down on getting the same questions over and over again.

In this particular case, I think the problem is that the user-supplied
qual item.tags @> ARRAY[id] is not leakproof and therefore must be
tested after the security qual. There's no way to use a Bitmap Heap
Scan without reversing the order of those tests.

> I do want to avoid getting bikesheddy about the advice language so I'll
> forbear from syntax discussion, but one design thought with lower-level
> implications did occur to me as I was playing with this: it might be
> useful in some situations to influence the planner _away_ from known
> worse paths while leaving it room to decide on the best other option. I
> think the work you did in path management should make this pretty
> straightforward for join and scan strategies, since it looks like you've
> basically made the enable_* gucs a runtime-configurable bitmask (which
> seems like a perfectly reasonable approach to my "have done some source
> diving but not an internals hacker" eyes), and could disable one as
> easily as forcing one.

I mostly agree. Saying not to use a sequential scan on a certain
table, or not to use a particular index, or not to use a particular
join method seem like things that would be potentially useful, and
they would be straightforward generalizations of what the code already
does. For me, that would principally be a way to understand better why
the planner chose what it did. I often wonder what the planner's
second choice would have been, but I don't just want the plan with the
second-cheapest overall cost, because that will be something just
trivially different. I want the cheapest plan that excludes some key
element of the current plan, so I can see a meaningfully different
alternative.

That said, I don't see this being a general thing that would make
sense across all of the tags that pg_plan_advice supports. For
example, NO_JOIN_ORDER() sounds hard to implement and largely useless.

The main reason I haven't done this is that I want to keep the focus
on plan stability, or said differently, on things that can properly
round-trip. You should be able to run a query with EXPLAIN
(PLAN_ADVICE), then set pg_plan_advice.advice to the resulting string,
rerun the query, and get the same plan with all of the advice
successfully matching. Since EXPLAIN (PLAN_ADVICE) would never emit
these proposed negative tags, we'd need to think a little bit harder
about how that stuff should be tested. That's not necessarily a big
deal or anything, but I didn't think it was an essential element of
the initial scope, so I left it out. I'm happy to add it in at some
point, or for someone else to do so, but not until this much is
working well.

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

Re: pg_plan_advice

От

"Dian Fay"

Дата:

30 ноября 2025 г., 06:16:44

On Mon Nov 24, 2025 at 11:14 AM EST, Robert Haas wrote:
> On Sat, Nov 22, 2025 at 7:43 PM Dian Fay <di@nmfay.com> wrote:
>> Since the policies don't contain any execution boundaries, all the quals
>> should be going into a single bucket for planning if I understand the
>> process correctly. The bitmap heap scan should be a candidate given the
>> `tags &&` predicate (and indeed if I switch to a privileged role, the
>> advice matches successfully without any policies in the mix), but gdb
>> shows the walker bouncing out of pgpa_walker_contains_scan without any
>> candidate scans for the BITMAP_HEAP_SCAN strategy.
>
> In this particular case, I think the problem is that the user-supplied
> qual item.tags @> ARRAY[id] is not leakproof and therefore must be
> tested after the security qual. There's no way to use a Bitmap Heap
> Scan without reversing the order of those tests.

Right, I keep forgetting the functions underneath those array operators
aren't leakproof. Thanks for digging.

> And honestly, this is one of the things I'm worried about if we go
> forward with this, that we'll get a ton of people who think it doesn't
> work because it doesn't force the planner to do things which the
> planner rejects on non-cost considerations. We're going to need really
> good documentation to explain to people that if you use this to try to
> force a plan and you can't, that's not a bug, that's the planner
> telling you that that plan shape is not able to be considered for some
> reason.

Once we're closer to consensus on pg_plan_advice or something like it
landing, I'm interested in helping out on this end of things!

Re: pg_plan_advice

От

Robert Haas

Дата:

05 декабря 2025 г., 22:57:09

On Sat, Nov 29, 2025 at 10:17 PM Dian Fay <di@nmfay.com> wrote:
> Once we're closer to consensus on pg_plan_advice or something like it
> landing, I'm interested in helping out on this end of things!

Thanks!

014f9a831a320666bf2195949f41710f970c54ad removes the need for what was
previously 0004, so here is a new patch series with that dropped, to
avoid confusing cfbot or human reviewers.

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

On Wed, Dec 10, 2025 at 9:54 AM Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Dec 10, 2025 at 6:20 AM Amit Langote <amitlangote09@gmail.com> wrote:
> These are just high-level comments after browsing the patches and
> reading some bits like pgpa_identifier to get myself familiarized with
> the project. I like that the key concept here is plan stability
> rather than plan control, because that framing makes it easier to
> treat this as infrastructure instead of policy.

Thanks, I agree. I'm sure people will use this for plan control, but
if you start with that, then it's really unclear what things you
should allow to be controlled and what things not. Defining the focus
as plan stability makes round-trip safety a priority and the scope of
what you can request is what the planner could have generated had the
costing come out just so. There's still some definitional questions at
the margin, but IMHO it's much less fuzzy.

I couldn't have said this any better than Amit did. In my experience, lack of a plan stability feature is far and away the most cited reason for not porting to PostgreSQL. They want query plan stability first and foremost. The amount of plan tweaking they do is actually pretty minimal, once they get good-enough performance during user acceptance they want to encase those query plans in amber because that's what the customer signed-off on. After that, they're happy to scan the performance trendlines, and only make tweaks when it's worth a change request.

But that's not to say I disagree with you categorically. Suppose we
decided (and I'm not saying we should) to start showing relation
identifiers in EXPLAIN output instead of identifying things in EXPLAIN
output as we do today. Maybe we even decide to show elided subqueries
and similar as first-class parts of the EXPLAIN output, also using
relation identifier syntax. That would be a pretty significant change,
and would destabilize a WHOLE LOT of regression test outputs, but then
relation identifiers become a first-class PostgreSQL concept that
everyone who looks at EXPLAIN output will encounter and, probably,
come to understand.

I think the change would be worth the destabilization, because it makes it so much easier to talk about complex query plans. Additionally, it would make it reasonable to programmatically extract portions of a plan, allowing for much more fine-grained regression tests regarding plans.

Showing the elided subqueries would be a huge benefit, outlining the benefits that the planner is giving you "for free".

> On the infrastructure patches (0001-0005): these look sensible. The
> range table flattening info, elided node tracking, and append node

One thing I am curious about is that by tracking the elided nodes, would it make more sense in the long run to have the initial post-naming plan tree be immutable, and generate a separate copy minus the elided parts?

Re: pg_plan_advice

От

Robert Haas

Дата:

11 декабря 2025 г., 00:29:50

On Wed, Dec 10, 2025 at 4:09 PM Corey Huinker <corey.huinker@gmail.com> wrote:
> I think the change would be worth the destabilization, because it makes it so much easier to talk about complex query
plans.Additionally, it would make it reasonable to programmatically extract portions of a plan, allowing for much more
fine-grainedregression tests regarding plans. 

I'll wait for more votes before thinking about doing anything about
this, because I have my doubts about whether the consensus will
actually go in favor of such a large change. Or maybe someone else
would like to try mocking it up (even if somewhat imperfectly) so we
can all see just how large an impact it makes.

>> > On the infrastructure patches (0001-0005): these look sensible. The
>> > range table flattening info, elided node tracking, and append node
>
> One thing I am curious about is that by tracking the elided nodes, would it make more sense in the long run to have
theinitial post-naming plan tree be immutable, and generate a separate copy minus the elided parts? 

Probably not. Having two entire copies of the plan tree would be
pretty expensive. I think that we've bet on the right idea, namely,
that the primary consumer of plan trees should be the executor, and
the primary goal should be to create plan trees that make the executor
run fast. I believe the right approach is basically what we do today:
you're allowed to put things into the plan that aren't technically
necessary for execution, if they're useful for instrumentation and
observability purposes and they don't add an unreasonable amount of
overhead. These patches basically just extend that existing principle
to a few new things.

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

Re: pg_plan_advice

От

Robert Haas

Дата:

11 декабря 2025 г., 18:09:47

On Fri, Dec 5, 2025 at 2:57 PM Robert Haas <robertmhaas@gmail.com> wrote:
> 014f9a831a320666bf2195949f41710f970c54ad removes the need for what was
> previously 0004, so here is a new patch series with that dropped, to
> avoid confusing cfbot or human reviewers.

Here's v6, with minor improvements over v5.

0001: Unchanged.

0002, 0003: Unchanged except for typo fixes pointed out by reviewers.

0004: I've improved the hook placement, which was previously such as
to make correct unique-semijoin handling impossible, and I improved
the associated comment about how to use the hook, based on experience
trying to actually do so.

0005: Fixed a small bug related to unique-semijoin handling (other
problems remain). Tidied things up to avoid producing non-actionable
NO_GATHER() advice in a number of cases, per some off-list feedback
from Ajaykumar Pal.

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

On Fri, Dec 12, 2025 at 10:09 AM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> Next bit of fuzzer feedback:

And another bit, but this time I was able to minimize into a
regression case, attached.

This comment in pgpa_identifier_matches_target() seems to be incorrect:

>     /*
>      * The identifier must specify a schema, but the target may leave the
>      * schema NULL to match anything.
>      */

But I don't know whether that's because the assumption itself is
wrong, or because a layer above hasn't filtered something out before
getting to this point.

--Jacob

Вложения

regress-crash.diff.txt

Re: pg_plan_advice

От

Robert Haas

Дата:

15 декабря 2025 г., 23:06:08

Here's v7.

In 0001, I removed "const" from a node's struct declaration, because
Tom gave me some feedback to avoid that on another recent patch, and I
noticed I had done it here also. 0002, 0003, and 0004 are unchanged.

In 0005:

- Refactored the code to avoid issuing SEMIJOIN_NON_UNIQUE() advice in
cases where uniqueness wasn't actually considered.
- Adjusted the code not to issue NO_GATHER() advice for non-relation
RTEs. (This is the issue reported by Ajay Pal in a recent message to
this thread, which was also mentioned in an XXX in the code.)
- Reject zero-length delimited identifiers, per Jacob's email.
- Properly initialize element->indexes in pgpa_trove_add_to_hash, per
Jacob'e email.
- Add gather((d d/d.d)) test case, per Jacob, and fix the related bug
in pgpa_identifier_matches_target, per Jacob's email.
- Add EXPLAIN SELECT 1 after various test cases in syntax.sql, to
improve test coverage, per analysis of why the existing test case
didn't catch a bug previously reported by Jacob.
- Added a dummy initialization to pgpa_collector.c to placate nervous
compilers, per discussion with Jacob.

I think this mostly catches me up on responding to issues reported
here, although there is one thing reported to me off-list that I
haven't dealt with yet. If there's anything reported on thread that
isn't addressed here, let me know.

Thanks,

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

Hi Robert,

Thank you very much for your work on the pg_plan_advice patch series. It is an impressive and substantial contribution, and it seems like a meaningful step forward toward addressing long-standing query plan stability issues in PostgreSQL.

While reviewing the v7 patches, I noticed a few points that I wanted to raise for discussion:
1. GEQO interaction (patch 4):
Since GEQO relies on randomized search, is there a risk that the optimizer may fail to explore the specific join order or path that is being enforced by the advice mask? In that case, could this lead to failures such as inability to construct the required join relation or excessive planning time if the desired path is not sampled?
2. Parallel query serialization (patches 1–3):
Several new fields (subrtinfos, elidedNodes, child_append_relid_sets) are added to PlannedStmt, but I did not see corresponding changes in outfuncs.c / readfuncs.c. Without serialization support, parallel workers executing subplans or Append nodes may not receive this metadata. Is this handled elsewhere, or is it something still pending?
3. Alias handling when generating advice (patch 5):
In pgpa_output_relation_name, the advice string is generated using get_rel_name(relid), which resolves to the underlying table name rather than the RTE alias. In self-join cases this could be ambiguous (e.g., my_table vs my_table). Would it be more appropriate to use the RTE alias when available?
4. Minor typo (patch 4):
In src/include/nodes/relation.h, parititonwise appears to be a typo and should likely be partitionwise.

I hope these comments are helpful, and I apologize in advance if any of this is already addressed elsewhere in the series.

Best regards,
Haibo

On Mon, Dec 15, 2025 at 12:06 PM Robert Haas <robertmhaas@gmail.com> wrote:

Here's v7.

In 0001, I removed "const" from a node's struct declaration, because
Tom gave me some feedback to avoid that on another recent patch, and I
noticed I had done it here also. 0002, 0003, and 0004 are unchanged.

In 0005:

- Refactored the code to avoid issuing SEMIJOIN_NON_UNIQUE() advice in
cases where uniqueness wasn't actually considered.
- Adjusted the code not to issue NO_GATHER() advice for non-relation
RTEs. (This is the issue reported by Ajay Pal in a recent message to
this thread, which was also mentioned in an XXX in the code.)
- Reject zero-length delimited identifiers, per Jacob's email.
- Properly initialize element->indexes in pgpa_trove_add_to_hash, per
Jacob'e email.
- Add gather((d d/d.d)) test case, per Jacob, and fix the related bug
in pgpa_identifier_matches_target, per Jacob's email.
- Add EXPLAIN SELECT 1 after various test cases in syntax.sql, to
improve test coverage, per analysis of why the existing test case
didn't catch a bug previously reported by Jacob.
- Added a dummy initialization to pgpa_collector.c to placate nervous
compilers, per discussion with Jacob.

I think this mostly catches me up on responding to issues reported
here, although there is one thing reported to me off-list that I
haven't dealt with yet. If there's anything reported on thread that
isn't addressed here, let me know.

Thanks,

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

Re: pg_plan_advice

От

Lukas Fittl

Дата:

30 декабря 2025 г., 04:15:18

On Mon, Dec 15, 2025 at 12:06 PM Robert Haas <robertmhaas@gmail.com> wrote:
> Here's v7.

I'm excited about this patch series, and in an effort to help land the infrastructure, here is a review of 0001 - 0003 to start:

For 0001, I'm not sure the following comment is correct:

> /* When recursing = true, it's an unplanned or dummy subquery. */
> rtinfo->dummy = recursing;

Later in that function we only recurse if its a dummy subquery - in the case of an unplanned subquery (rel->subroot == NULL)

add_rtes_to_flat_rtable won't be called again (instead the relation RTEs are directly added to the finalrtable). Maybe we can

clarify that comment as "When recursing = true, it's a dummy subquery or its children.".

From my medium-level understanding of the planner, I don't think the lack of tracking unplanned subqueries

in subrtinfos is a problem, at least for the pg_plan_advice type use cases.

---

For 0002:

It might be helpful to clarify in a comment that ElidedNode's plan_node_id represents the surviving node, not that of the elided node.

I also noticed that this currently doesn't support cases where multiple nodes are elided, e.g. with multi-level table partitioning:

CREATE TABLE pt (l1 date, l2 text) PARTITION BY RANGE (l1);
CREATE TABLE pt_202512 PARTITION OF pt FOR VALUES FROM ('2025-12-01') TO ('2026-01-01') PARTITION BY LIST (l2);
CREATE TABLE pt_202512_TEST PARTITION OF pt_202512 FOR VALUES IN ('TEST');

EXPLAIN (RANGE_TABLE) SELECT * FROM pt WHERE l1 = '2025-12-15' AND l2 = 'TEST';

QUERY PLAN
-------------------------------------------------------------------
Seq Scan on pt_202512_test pt (cost=0.00..29.05 rows=1 width=36)
Filter: ((l1 = '2025-12-15'::date) AND (l2 = 'TEST'::text))
Scan RTI: 3
Elided Node Type: Append
Elided Node RTIs: 1 <=== This is missing RTI 2
RTI 1 (relation, inherited, in-from-clause):
Relation: pt
RTI 2 (relation, inherited, in-from-clause):
Relation: pt_202512
RTI 3 (relation, in-from-clause):
Relation: pt_202512_test
Unprunable RTIs: 1 2 3

In a quick test, adding child_append_relid_sets (from 0003) to the relids being passed to record_elided_node fixes

that. Presumably the case of partitionwise join relids doesn't matter, because that would prevent it being elided.

---

For 0003:

I also find the "cars" variable suffix a bit hard to understand, but not sure a comment next to the variables is that useful.

Separately, the noise generated by all the additional "_cars" variables isn't great.

I wonder a little bit if we couldn't introduce a better abstraction here, e.g. a struct "AppendPathInput" that contains the

two related lists, and gets populated by accumulate_append_subpath/get_singleton_append_subpath and then

passed to create_append_path as a single argument.

---

Note that 0005 needs a rebase, since 48d4a1423d2e92d10077365532d92e059ba2eb2e changed the GetNamedDSMSegment API.

You may also want to move the CF entry to the PG19-4 commitfest so CFbot runs again.

Thanks,

Lukas

Lukas Fittl

Re: pg_plan_advice

От

Robert Haas

Дата:

06 января, 22:36:03

On Mon, Dec 29, 2025 at 8:15 PM Lukas Fittl <lukas@fittl.com> wrote:
> For 0001, I'm not sure the following comment is correct:
>
> > /* When recursing = true, it's an unplanned or dummy subquery. */
> > rtinfo->dummy = recursing;
>
> Later in that function we only recurse if its a dummy subquery - in the case of an unplanned subquery (rel->subroot
==NULL) 
> add_rtes_to_flat_rtable won't be called again (instead the relation RTEs are directly added to the finalrtable).
Maybewe can 
> clarify that comment as "When recursing = true, it's a dummy subquery or its children.".

Presumably, a child of an unplanned or dummy subquery will also be
unplanned or dummy, so I'm not sure I understand the need to clarify
here.

> From my medium-level understanding of the planner, I don't think the lack of tracking unplanned subqueries
> in subrtinfos is a problem, at least for the pg_plan_advice type use cases.

I don't think so, either. I believe that anything that falls into this
category is something that is not actually going to be reflected in
the final plan tree, but we can't lose track of it completely because
it can matter for purposes like locking or invalidation. Since plan
advice only targets things that appear in the final plan tree, it
shouldn't care. If we did want to care, e.g. to emit advice like
WE_ARE_EXPECTING_THIS_TO_BE_DUMMY(whatever_table), we'd need more than
an rtoffset per subquery; we'd have to map each RTI individually,
because some RTIs are tossed completely for in the "dummy" case,
meaning that the rtoffset isn't constant for the whole subquery.
AFAICT, this is not an issue because we need not care about the dummy
subqueries at all. The only reason I included the SubPlanRTInfo at all
for this case is that the previous SubPlanRTInfo might be for a
non-dummy subquery, and some code might want to look at the next entry
in the list to see where the portion of the range table belonging to
that previous subqueries ends. This lets you do that.

> For 0002:
>
> It might be helpful to clarify in a comment that ElidedNode's plan_node_id represents the surviving node, not that of
theelided node. 

Good point. I'll add this comment:

+ *
+ * plan_node_id is that of the surviving plan node, the sole child of the
+ * one which was elided.

> I also noticed that this currently doesn't support cases where multiple nodes are elided, e.g. with multi-level table
partitioning:
>
> CREATE TABLE pt (l1 date, l2 text) PARTITION BY RANGE (l1);
> CREATE TABLE pt_202512 PARTITION OF pt FOR VALUES FROM ('2025-12-01') TO ('2026-01-01') PARTITION BY LIST (l2);
> CREATE TABLE pt_202512_TEST PARTITION OF pt_202512 FOR VALUES IN ('TEST');
>
> EXPLAIN (RANGE_TABLE) SELECT * FROM pt WHERE l1 = '2025-12-15' AND l2 = 'TEST';
>
>                             QUERY PLAN
> -------------------------------------------------------------------
>  Seq Scan on pt_202512_test pt  (cost=0.00..29.05 rows=1 width=36)
>    Filter: ((l1 = '2025-12-15'::date) AND (l2 = 'TEST'::text))
>    Scan RTI: 3
>    Elided Node Type: Append
>    Elided Node RTIs: 1    <=== This is missing RTI 2
>  RTI 1 (relation, inherited, in-from-clause):
>    Relation: pt
>  RTI 2 (relation, inherited, in-from-clause):
>    Relation: pt_202512
>  RTI 3 (relation, in-from-clause):
>    Relation: pt_202512_test
>  Unprunable RTIs: 1 2 3
>
> In a quick test, adding child_append_relid_sets (from 0003) to the relids being passed to record_elided_node fixes
> that. Presumably the case of partitionwise join relids doesn't matter, because that would prevent it being elided.

I'm not really sure there's a problem here. We definitely do not want
to end up with something like "Elided Node RTIs: 1 2". What I've found
experimentally is that it's often important to preserve relid sets,
but you need to preserve them as sets, not individually. So there
could be an argument that we somehow want to preserve both {1} and {2}
here, but that's not equivalent to {1,2}, which looks like a
partitionwise join between relid 1 and relid 2. But it isn't
especially clear to me that we actually need to preserve RTI 2 here.
One reason why preserving RTIs is important is so that as we descend a
join tree, we can find the RTIs that the optimizer thought it was
joining, but that only requires finding RTI 1, not RTI 2. Another
reason why preserving RTIs is important is so that we can use relation
identifiers to describe planning decisions made with respect to those
RTIs, but that doesn't apply here because partition expansion just
always happens.

Of course, it's quite possible that there are reasons unrelated to
this patch set why this information would be good to preserve, but if
we want to do it, we're going to have to adjust the data
representation somehow. We'd either need to give the ElidedNode a
"cars" representation instead of a single RTI set, or we'd need to
have some separate way of representing this. I hesitate a little bit
to design something without a use case in mind, but maybe you have
one?

> For 0003:
>
> I also find the "cars" variable suffix a bit hard to understand, but not sure a comment next to the variables is that
useful.
> Separately, the noise generated by all the additional "_cars" variables isn't great.
>
> I wonder a little bit if we couldn't introduce a better abstraction here, e.g. a struct "AppendPathInput" that
containsthe 
> two related lists, and gets populated by accumulate_append_subpath/get_singleton_append_subpath and then
> passed to create_append_path as a single argument.

I spent some time thinking about this day and haven't been quite able
to come up with something that I like. The problem is that
pa_partial_subpaths and pa_nonpartial_subpaths share a single
child_append_relid_sets variable, namely pa_subpath_cars, and
accumulate_append_subpaths gets called with that as the last argument
and different things for the previous two. One thing I tried was
making the AppendPathInput struct contain three lists rather than two,
but then accumulate_append_subpath() needs an argument that makes it
work in one of three different modes:

Mode 1: normal -- add everything to the "normal" list
Mode 2: building parallel-aware append with partial path -- add things
to the "normal" list except for parallel-aware appends which need to
be split between the normal and special lists
Mode 3: building parallel-aware append with non-partial path -- add
things to the "special" list

I also tried splitting up accumulate_append_subpath() into two
functions, thinking that maybe I could segregate the parallel-append
handling from the more normal cases. This seems somewhat appealing in
the sense that having accumulate_append_subpath() hold a bunch of
extra logic that only one call site needs isn't very nice, but
changing it doesn't really seem to help with the problem that we have
two subpath lists sharing one cars list in this case. I'll try to find
some more time to think about this, but if you have any ideas
meanwhile, I'd be happy to hear them.

> Note that 0005 needs a rebase, since 48d4a1423d2e92d10077365532d92e059ba2eb2e changed the GetNamedDSMSegment API.

I'll fix this. I don't love the way that commit made the callback and
the callback arg non-consecutive arguments.

> You may also want to move the CF entry to the PG19-4 commitfest so CFbot runs again.

It seems that I cannot move it to 19-4. I moved it to 19-Final.

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

Re: pg_plan_advice

От

Robert Haas

Дата:

06 января, 22:50:46

On Mon, Dec 29, 2025 at 6:34 PM Haibo Yan <tristan.yim@gmail.com> wrote:
> 1. GEQO interaction (patch 4):
> Since GEQO relies on randomized search, is there a risk that the optimizer may fail to explore the specific join
orderor path that is being enforced by the advice mask? In that case, could this lead to failures such as inability to
constructthe required join relation or excessive planning time if the desired path is not sampled? 

The interaction of this feature with GEQO definitely needs more study.
If you have some time to work on this, I think testing and reporting
results would be quite useful. However, I don't think we should ever
get planner failure, and I'm doubtful about excessive planning time as
well. The effect of plan advice is to disable some paths just as if
enable_<whatever> were set to false, so if you provide very specific
advice while planning with GEQO, I think you might just end up with a
disabled path that doesn't account for the advice. However, this
should be checked, and I haven't gotten there yet. I'll add an XXX to
the README to make sure this doesn't get forgotten.

> 2. Parallel query serialization (patches 1–3):
> Several new fields (subrtinfos, elidedNodes, child_append_relid_sets) are added to PlannedStmt, but I did not see
correspondingchanges in outfuncs.c / readfuncs.c. Without serialization support, parallel workers executing subplans or
Appendnodes may not receive this metadata. Is this handled elsewhere, or is it something still pending? 

I believe that gen_node_support.pl should take care of this
automatically unless the node type is flagged as
pg_node_attr(custom_read_write).

> 3. Alias handling when generating advice (patch 5):
> In pgpa_output_relation_name, the advice string is generated using get_rel_name(relid), which resolves to the
underlyingtable name rather than the RTE alias. In self-join cases this could be ambiguous (e.g., my_table vs
my_table).Would it be more appropriate to use the RTE alias when available? 

No. That function is only used for indexes.

> 4. Minor typo (patch 4):
> In src/include/nodes/relation.h, parititonwise appears to be a typo and should likely be partitionwise.

Will fix, thanks.

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

Re: pg_plan_advice

От

Lukas Fittl

Дата:

07 января, 10:04:19

On Mon, Dec 29, 2025 at 5:15 PM Lukas Fittl <lukas@fittl.com> wrote:

On Mon, Dec 15, 2025 at 12:06 PM Robert Haas <robertmhaas@gmail.com> wrote:
> Here's v7.

I'm excited about this patch series, and in an effort to help land the infrastructure, here is a review of 0001 - 0003 to start:

For the review of 0004, I decided to spend a few days to test if the plan generation strategy logic will

work for pg_hint_plan, as an existing extension in the ecosystem that is widely used, but functions today

by virtue of modifying planner GUCs whilst the planner is running.

First of all, as is expected, the extension completely stops working with 0004 in place - because we now

only read the GUCs at planner start, the mechanism of modifying them in the middle doesn't work. I don't

think we can avoid this.

That said, good news: After a bunch of iterations, I get a clean pass on the pg_hint_plan regression tests,

whilst completely dropping its copying of core code and hackish re-run of set_plain_rel_pathlist. See [0]

for a draft PR (on my own fork of pg_hint_plan) with individual patches that explain some regression test

differences.

Adding Michael in CC, since he's been thankfully maintaining pg_hint_plan over the years, and I think if

0004 gets merged that should significantly reduce the maintenance burden, independently of what happens

with pg_plan_advice - so his input would be useful here.

The biggest change in the regression test output was due to how the "Parallel" hint worked in pg_hint_plan

(basically it was setting parallel_*_cost to zero, and then messed with the gucs that factor into

compute_parallel_worker) -- I think the only sensible thing to do is to change that in pg_hint_plan, and

instead rely on rejecting non-partial paths with PGS_CONSIDER_NONPARTIAL if "hard" enforcement of

parallelism is requested. That caused some minor plan changes, but I think they can still be argued to be

matching the user's intent of "make a scan involving this relation parallel".

There were two bugs in 0004 that I had to fix to make this work:

In cost_index, we are checking "path->path.parallel_workers == 0", but parallel_workers only gets

set later in the function, causing the PGS_CONSIDER_NONPARTIAL mask to not be applied. Replacing

this with checking the "partial_path" argument instead makes it work.

In cost_samplescan, we set the PGS_CONSIDER_NONPARTIAL mask if its a non-partial path, but that

causes Sample Scans to always be disabled when setting PGS_CONSIDER_NONPARTIAL on the

relation. I think we could simply drop that check, since we never generate partial sample scan paths.

Otherwise 0004 looks good to me, and the mechanism of working with mask values felt natural to me,

especially in contrast with existing ways to achieve something similar. I did not test partition wise joins,

since pg_hint_plan doesn't cover them today.

[0]: https://github.com/lfittl/pg_hint_plan/pull/1

Thanks,

Lukas

Lukas Fittl

Re: pg_plan_advice

От

Haibo Yan

Дата:

08 января, 01:46:54

>> 1. GEQO interaction (patch 4):
>> Since GEQO relies on randomized search, is there a risk that the optimizer may fail to explore the specific join order or path that is being enforced by the advice mask? In that case, could this lead to failures such as inability to construct the required join relation or excessive planning time if the desired path is not sampled?

> The interaction of this feature with GEQO definitely needs more study.
> If you have some time to work on this, I think testing and reporting
> results would be quite useful. However, I don't think we should ever
> get planner failure, and I'm doubtful about excessive planning time as
> well. The effect of plan advice is to disable some paths just as if
> enable_<whatever> were set to false, so if you provide very specific
> advice while planning with GEQO, I think you might just end up with a
> disabled path that doesn't account for the advice. However, this
> should be checked, and I haven't gotten there yet. I'll add an XXX to
> the README to make sure this doesn't get forgotten.

I conducted extensive tests today using randomized advice strings to challenge pg_plan_advice under GEQO pressure. The results strongly support your hypothesis: for standard Left-Deep trees (which GEQO natively supports), the interaction is stable and efficient.

I executed a stress test involving 100,000 iterations (100 random join structures x 1000 random seeds). The planning time remained low, and no planning failures occurred for valid topology advice.

Observation on Bushy Plans: I did identify one anomaly regarding "Bushy Plans" (e.g., ((t1 t2) (t3 t4))). Since PostgreSQL's GEQO implementation is strictly Left-Deep and cannot generate Bushy trees, if a user manually forges a Bushy Plan advice:

It does not cause a planner crash (e.g., "failed to construct join relation").

Instead, the planner seems to silently ignore the structural constraint of the advice and falls back to a path GEQO can actually find.

I believe this behavior is acceptable because pg_plan_advice is intended to stabilize plans that the optimizer can generate. Since GEQO cannot generate Bushy plans, users should not be supplying them.

Script
-----------------------------------------------------------------------------------
/* * GEQO Stress Test for pg_plan_advice
* -----------------------------------
* Methodology:
* 1. Generates 100 random "Left-Deep" join topologies (t1 joining t2..t100 in random orders).
* 2. This simulates valid advice that GEQO is capable of producing.
* 3. For each topology, runs 1000 iterations with random GEQO seeds.
* 4. Measures success rate and planning time overhead.
*/
DO $$
DECLARE
v_jo TEXT;
v_jo_rest TEXT;
v_nl TEXT;
v_scan TEXT;
v_ng TEXT;
v_adv TEXT;
v_sql TEXT;
v_seed FLOAT;
v_ok INT := 0;
v_err INT := 0;
v_msg TEXT;
k INT;
i INT;
j INT;
v_ts1 timestamp;
v_ts2 timestamp;
v_cur_ms numeric;
v_total_ms numeric := 0;
v_max_ms numeric := 0;
BEGIN
-- Pre-generate static parts of the advice to save time
SELECT string_agg('t'||n, ' ' ORDER BY n) INTO v_nl FROM generate_series(2,100) n;
SELECT string_agg('t'||n, ' ' ORDER BY n) INTO v_scan FROM generate_series(1,100) n;
SELECT string_agg('t'||n, ' ' ORDER BY n) INTO v_ng FROM generate_series(1,100) n;

-- Construct the SQL: t1 JOIN t2 JOIN t3 ... JOIN t100
v_sql := 'EXPLAIN (COSTS OFF) SELECT count(*) FROM t1';
FOR j IN 2..100 LOOP
v_sql := v_sql || ' JOIN t' || j || ' ON t1.id=t' || j || '.id';
END LOOP;

-- Configure GEQO for stress testing (force it ON, low effort/pool)
PERFORM set_config('geqo', 'on', false);
PERFORM set_config('geqo_threshold', '12', false);
PERFORM set_config('geqo_effort', '1', false);
PERFORM set_config('geqo_pool_size', '0', false);

RAISE NOTICE 'Starting Stress Test: 100 Outer Loops (Random Plans) x 1000 Inner Loops (Random Seeds)...';

-- Outer Loop: Generate 100 different valid Advice structures
FOR k IN 1..100 LOOP
-- Randomize the join order of t2..t100 to simulate different Left-Deep trees
SELECT string_agg('t'||n, ' ' ORDER BY random()) INTO v_jo_rest FROM generate_series(2,100) n;
v_jo := 't1 ' || v_jo_rest;

v_adv := 'JOIN_ORDER(' || v_jo || ') ' ||
'NESTED_LOOP_PLAIN(' || v_nl || ') ' ||
'SEQ_SCAN(' || v_scan || ') ' ||
'NO_GATHER(' || v_ng || ')';

PERFORM set_config('pg_plan_advice.advice', v_adv, false);

-- Inner Loop: Test the specific advice against 1000 random GEQO seeds
FOR i IN 1..1000 LOOP
v_seed := random();
PERFORM set_config('geqo_seed', v_seed::text, false);

BEGIN
v_ts1 := clock_timestamp();

EXECUTE v_sql;

v_ts2 := clock_timestamp();

v_cur_ms := EXTRACT(EPOCH FROM (v_ts2 - v_ts1)) * 1000;
v_total_ms := v_total_ms + v_cur_ms;

IF v_cur_ms > v_max_ms THEN
v_max_ms := v_cur_ms;
END IF;

v_ok := v_ok + 1;

EXCEPTION WHEN OTHERS THEN
GET STACKED DIAGNOSTICS v_msg = MESSAGE_TEXT;
v_err := v_err + 1;
RAISE WARNING 'Outer % / Inner % Crashed! Seed: %, Err: %', k, i, v_seed, v_msg;
END;
END LOOP;

RAISE NOTICE 'Batch %/100 completed.', k;
END LOOP;

RAISE NOTICE '---------------------------';
RAISE NOTICE 'Total Scenarios: 100,000';
RAISE NOTICE 'Success: %', v_ok;
RAISE NOTICE 'Failed: %', v_err;
RAISE NOTICE 'Total Time: % ms', round(v_total_ms, 2);
RAISE NOTICE 'Avg Time: % ms', round(v_total_ms / (v_ok + v_err + 0.0001), 2);
RAISE NOTICE 'Max Time: % ms', round(v_max_ms, 2);
RAISE NOTICE '---------------------------';

IF v_err > 0 THEN
RAISE NOTICE 'CONCLUSION: Conflict found.';
ELSE
RAISE NOTICE 'CONCLUSION: No errors found.';
END IF;
END $$;
-----------------------------------------------------------------------------------

On Tue, Jan 6, 2026 at 11:50 AM Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Dec 29, 2025 at 6:34 PM Haibo Yan <tristan.yim@gmail.com> wrote:
> 1. GEQO interaction (patch 4):
> Since GEQO relies on randomized search, is there a risk that the optimizer may fail to explore the specific join order or path that is being enforced by the advice mask? In that case, could this lead to failures such as inability to construct the required join relation or excessive planning time if the desired path is not sampled?

The interaction of this feature with GEQO definitely needs more study.
If you have some time to work on this, I think testing and reporting
results would be quite useful. However, I don't think we should ever
get planner failure, and I'm doubtful about excessive planning time as
well. The effect of plan advice is to disable some paths just as if
enable_<whatever> were set to false, so if you provide very specific
advice while planning with GEQO, I think you might just end up with a
disabled path that doesn't account for the advice. However, this
should be checked, and I haven't gotten there yet. I'll add an XXX to
the README to make sure this doesn't get forgotten.

> 2. Parallel query serialization (patches 1–3):
> Several new fields (subrtinfos, elidedNodes, child_append_relid_sets) are added to PlannedStmt, but I did not see corresponding changes in outfuncs.c / readfuncs.c. Without serialization support, parallel workers executing subplans or Append nodes may not receive this metadata. Is this handled elsewhere, or is it something still pending?

I believe that gen_node_support.pl should take care of this
automatically unless the node type is flagged as
pg_node_attr(custom_read_write).

> 3. Alias handling when generating advice (patch 5):
> In pgpa_output_relation_name, the advice string is generated using get_rel_name(relid), which resolves to the underlying table name rather than the RTE alias. In self-join cases this could be ambiguous (e.g., my_table vs my_table). Would it be more appropriate to use the RTE alias when available?

No. That function is only used for indexes.

> 4. Minor typo (patch 4):
> In src/include/nodes/relation.h, parititonwise appears to be a typo and should likely be partitionwise.

Will fix, thanks.

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

Re: pg_plan_advice

От

Jacob Champion

Дата:

08 января, 02:11:24

On Mon, Dec 15, 2025 at 12:06 PM Robert Haas <robertmhaas@gmail.com> wrote:
> - Add gather((d d/d.d)) test case, per Jacob, and fix the related bug
> in pgpa_identifier_matches_target, per Jacob's email.

I think this fix affected the ability to omit the partition schema in
advice strings. Attached is a quick test that shows it on my machine,
plus an attempted fix that mashes together the v6 and v7 approaches.
(I have diffs in my generated plan advice compared to what's in
v7-0005, so I'm not sure if my .out file is correct.)

--Jacob

Вложения

partnsp.diff.txt

Re: pg_plan_advice

От

Robert Haas

Дата:

08 января, 19:07:31

On Wed, Jan 7, 2026 at 2:04 AM Lukas Fittl <lukas@fittl.com> wrote:
> That said, good news: After a bunch of iterations, I get a clean pass on the pg_hint_plan regression tests,
> whilst completely dropping its copying of core code and hackish re-run of set_plain_rel_pathlist. See [0]
> for a draft PR (on my own fork of pg_hint_plan) with individual patches that explain some regression test
> differences.

That sounds AWESOME.

> The biggest change in the regression test output was due to how the "Parallel" hint worked in pg_hint_plan
> (basically it was setting parallel_*_cost to zero, and then messed with the gucs that factor into
> compute_parallel_worker) -- I think the only sensible thing to do is to change that in pg_hint_plan, and
> instead rely on rejecting non-partial paths with PGS_CONSIDER_NONPARTIAL if "hard" enforcement of
> parallelism is requested. That caused some minor plan changes, but I think they can still be argued to be
> matching the user's intent of "make a scan involving this relation parallel".

Cool. I'm sort of curious what changed, but maybe it's not important
enough to spend time discussing right now.

> There were two bugs in 0004 that I had to fix to make this work:
>
> In cost_index, we are checking "path->path.parallel_workers == 0", but parallel_workers only gets
> set later in the function, causing the PGS_CONSIDER_NONPARTIAL mask to not be applied. Replacing
> this with checking the "partial_path" argument instead makes it work.

I agree that this is a bug. I'm thinking this might be the appropriate fix:

     enable_mask = (indexonly ? PGS_INDEXONLYSCAN : PGS_INDEXSCAN)
-        | (path->path.parallel_workers == 0 ? PGS_CONSIDER_NONPARTIAL : 0);
+        | (partial_path ? 0 : PGS_CONSIDER_NONPARTIAL);

> In cost_samplescan, we set the PGS_CONSIDER_NONPARTIAL mask if its a non-partial path, but that
> causes Sample Scans to always be disabled when setting PGS_CONSIDER_NONPARTIAL on the
> relation. I think we could simply drop that check, since we never generate partial sample scan paths.

This one is less obvious to me. I mean, if PGS_CONSIDER_NONPARTIAL
lets us consider non-partial plans, and a sample scan is a non-partial
plan, then shouldn't the flag need to be set in order for us to
consider it? If not, maybe we need to rethink the name or the
semantics of that bit in some way.

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

Re: pg_plan_advice

От

Lukas Fittl

Дата:

08 января, 19:30:42

On Thu, Jan 8, 2026 at 8:07 AM Robert Haas <robertmhaas@gmail.com> wrote:
> > The biggest change in the regression test output was due to how the "Parallel" hint worked in pg_hint_plan
> > (basically it was setting parallel_*_cost to zero, and then messed with the gucs that factor into
> > compute_parallel_worker) -- I think the only sensible thing to do is to change that in pg_hint_plan, and
> > instead rely on rejecting non-partial paths with PGS_CONSIDER_NONPARTIAL if "hard" enforcement of
> > parallelism is requested. That caused some minor plan changes, but I think they can still be argued to be
> > matching the user's intent of "make a scan involving this relation parallel".
>
> Cool. I'm sort of curious what changed, but maybe it's not important
> enough to spend time discussing right now.

In my assessment there were roughly four kinds of changes on the
regression test outputs for pg_hint_plan:

1) The order of operations (e.g. whats the inner/outer rel in a
parallel plan) - I think that's probably caused by the previous zero
cost confusing the planner
2) The placement of the Gather node in the case of joins (its now on
top of the join in more cases, vs below it) - I think that's also
caused by the previous zero cost logic
3) Parallelism wasn't enforced before, but is enforced now (e.g.
sequential scans on empty relations didn't get a Gather node
previously)
4) Worker count changed because rel_parallel_workers is still limited
by max_parallel_workers_per_gather, and so my patched version now sets
that for the whole query to the highest of the workers requested in
Parallel hints (vs before it was able to keep that to just what a plan
node needed)

> > There were two bugs in 0004 that I had to fix to make this work:
> >
> > In cost_index, we are checking "path->path.parallel_workers == 0", but parallel_workers only gets
> > set later in the function, causing the PGS_CONSIDER_NONPARTIAL mask to not be applied. Replacing
> > this with checking the "partial_path" argument instead makes it work.
>
> I agree that this is a bug. I'm thinking this might be the appropriate fix:
>
>      enable_mask = (indexonly ? PGS_INDEXONLYSCAN : PGS_INDEXSCAN)
> -        | (path->path.parallel_workers == 0 ? PGS_CONSIDER_NONPARTIAL : 0);
> +        | (partial_path ? 0 : PGS_CONSIDER_NONPARTIAL);

Yup, that's exactly the change I had locally that worked as expected.

> > In cost_samplescan, we set the PGS_CONSIDER_NONPARTIAL mask if its a non-partial path, but that
> > causes Sample Scans to always be disabled when setting PGS_CONSIDER_NONPARTIAL on the
> > relation. I think we could simply drop that check, since we never generate partial sample scan paths.
>
> This one is less obvious to me. I mean, if PGS_CONSIDER_NONPARTIAL
> lets us consider non-partial plans, and a sample scan is a non-partial
> plan, then shouldn't the flag need to be set in order for us to
> consider it? If not, maybe we need to rethink the name or the
> semantics of that bit in some way.

Yeah, I would agree with you that is inconsistent with the flag's name
- but on the flip side, its difficult for the caller to conditionally
set the flag (which you'd have to do to avoid a "Disabled" showing in
the plan), since we're setting it on the RelOptInfo (do we know if the
scan is a sample scan at that point?).

I wonder if its worth considering to invert the flag, i.e.
"PGS_PREFER_PARTIAL" - that way its clear that in case of paths that
can't be partial, we use the non-partial version without a penalty.

PS: I realized my prior mails were not plain text and had bad word
wrap (thanks Gmail-based workflow!) - hopefully this one is better :)

Thanks,
Lukas

--
Lukas Fittl

Re: pg_plan_advice

От

Robert Haas

Дата:

08 января, 19:37:51

On Thu, Jan 8, 2026 at 11:31 AM Lukas Fittl <lukas@fittl.com> wrote:
> Yeah, I would agree with you that is inconsistent with the flag's name
> - but on the flip side, its difficult for the caller to conditionally
> set the flag (which you'd have to do to avoid a "Disabled" showing in
> the plan), since we're setting it on the RelOptInfo (do we know if the
> scan is a sample scan at that point?).

How about checking rte->tablesample, as set_rel_pathlist does?

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

Re: pg_plan_advice

От

Robert Haas

Дата:

08 января, 21:21:35

On Wed, Jan 7, 2026 at 6:11 PM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> On Mon, Dec 15, 2025 at 12:06 PM Robert Haas <robertmhaas@gmail.com> wrote:
> > - Add gather((d d/d.d)) test case, per Jacob, and fix the related bug
> > in pgpa_identifier_matches_target, per Jacob's email.
>
> I think this fix affected the ability to omit the partition schema in
> advice strings. Attached is a quick test that shows it on my machine,
> plus an attempted fix that mashes together the v6 and v7 approaches.
> (I have diffs in my generated plan advice compared to what's in
> v7-0005, so I'm not sure if my .out file is correct.)

Right, so I also am seeing some instability in the plans for some of
the test cases. I've attempted to address that in v8 by making the
three partitioned tables have unequal row counts. I also adapted your
test case and adopted your code fix. Unfortunately, I haven't been
able to respond to everyone's reports about v7 yet, partly because of
Christmas vacation, partly because of the number of reports, and
partly because, uh, I'm slow I guess. But here's the list of things
that have changed:

0001, 0003: No changes.

0002: Comment update.

0004: Comment update, bug fix to cost_index() per comment from Lukas.

- Added an XXX to the README to highlight the need for more GEQO investigation.
- Regression test expected output changes.
- Make partitionwise_join test tables different sizes in the hopes of
stabilizing test results.
- Add test case for forcing partitionwise join order with and without
a schema specification.
- Add new GUC pg_plan_advice.trace_mask because I've written similar
debugging code too many times already.
- Replace reference to inner_beneath_any_gather[i] with
inner_beneath_any_gather[k] per comment from Jakub.
- Add logic to pgpa_planner_apply_join_path_advice taking into account
that PARTITIONWISE() implicitly constrains the join order.
- Rebased.

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

On Wed, Jan 7, 2026 at 5:47 PM Haibo Yan <tristan.yim@gmail.com> wrote:
> Instead, the planner seems to silently ignore the structural constraint of the advice and falls back to a path GEQO
canactually find.

Does the plan end up disabled in that case?

> I believe this behavior is acceptable because pg_plan_advice is intended to stabilize plans that the optimizer can
generate.Since GEQO cannot generate Bushy plans, users should not be supplying them.

Right, I agree. A core principal here is that you can only nudge the
planner towards a plan it would have considered anyway. In the case of
GEQO, there is some randomness to which plans are considered. Your
advice will only be reliably take into account if it applies to
elements that must be part of the final plan. For instance, if you
advise the use of a sequential scan or an index scan, that should
work, because that relation has to be scanned somehow. Advice on a
join method should almost always work, since it can apply to any
non-leading table. Of course, you also won't be able to advise an
infeasible join method, but that would be true without GEQO, too.
Advice on the join order is going to be iffy when using GEQO -- if a
compatible join order is highly likely to be considered, e.g. because
you specify something like JOIN_ORDER(just_one) that only sets the
driving table -- then it'll probably work, but if you give a complete
join order specification, it probably won't. If you want to avoid
that, you can adjust geqo_threshold.

Thanks for looking into this.

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

Re: pg_plan_advice

От

Lukas Fittl

Дата:

08 января, 23:47:05

On Thu, Jan 8, 2026 at 11:37 AM Robert Haas <robertmhaas@gmail.com> wrote:
> Ideally, if you say "hey, I want to use parallel query here," you
> would get the best plan that the planner knows how to construct that
> happens to use parallelism. #2 sounds to me like you weren't really
> getting that, because you were making parallelism on cost rather than
> disabling non-parallel paths. #3 also sounds that way, because you
> asked for parallelism and didn't get it. So I would tend to view those
> changes as improvements.

Agreed from my perspective.

> The others are a little trickier. I don't think I quite understand
> what this patch changed with respect to #4. I'm guessing that maybe
> what's happening here is that this isn't really due to anything in the
> patch set, but is rather due to relying on pgs_mask rather than
> copy-pasting lots of code, which maybe incidentally removed the
> ability to tweak something that pg_hint_plan was previously tweaking.
> Maybe you want to think about writing a patch to go with 0004 that
> addresses that gap specifically?

Yeah, I don't think this needs to be reviewed in detail here now, but
I do think there is a potential to improve worker count overrides in
core code. I agree this can be done as a follow-up to 0004 - and I
don't have a good sense yet for how infrastructure for this could look
like.

pg_hint_plan has some existing logic to copy parallel worker counts
into other paths (e.g. for joins), and I kept that for now in that
draft patch [0] to reduce further test output differences.

> I'm actually kind of surprised that you didn't run into a similar
> problem with the Rows() hint, for which 0004 also doesn't provide
> infrastructure. If there's no problem, cool, but if there's a problem
> there you haven't detected yet, maybe we should try to plug that gap,
> too.

Yeah, 0004 doesn't provide infrastructure for that directly. However,
it does add joinrel_setup_hook which enables modifying joinrel->rows
at the right time without copying core code. Previously pg_hint_plan
modified the row estimates for joins through copied core code [1], but
0004 lets it affect join rels through the new hook [2].

> I don't quite know what to say about #1. If your theory about it being
> due to the zero costs is correct, that's another example of the
> current implementation not really being able to achieve the ideal
> behavior, and the patch making it better. If there's something else
> going on there, then I don't know.

Yeah, I don't think we need to worry about this in the context of
applying 0004, and its something that Michael or other pg_hint_plan
maintainers can assess when updating it.

Its worth noting for clarity, whilst 0004 requires extensions that
modify certain GUCs during the planning process to instead use the PGS
mask, max_parallel_workers_per_gather is not one of these GUCs. I was
driven to also do the parallel hint changes because I wanted to prove
that pg_hint_plan can now function without any copying of core code,
but technically all the parallel regression test changes can be
avoided when keeping the prior zero parallel_*_cost mechanism +
modifying max_parallel_workers_per_gather during planning as before,
whilst still updating the scan/join hints to use the PGS mask logic.

Thanks,
Lukas

[0]: https://github.com/lfittl/pg_hint_plan/blob/postgres-19-with-plan-generation-strategies/pg_hint_plan.c#L4602
[1]: https://github.com/ossc-db/pg_hint_plan/blob/master/make_join_rel.c#L126
[2]: https://github.com/lfittl/pg_hint_plan/blob/postgres-19-with-plan-generation-strategies/pg_hint_plan.c#L4372

--
Lukas Fittl

Re: pg_plan_advice

От

Robert Haas

Дата:

08 января, 23:58:54

On Thu, Dec 18, 2025 at 8:36 AM Robert Haas <robertmhaas@gmail.com> wrote:
> What must be happening here is that either pgpa_join.c (maybe with
> complicity from pgpa_walker.c) is not populating the
> pgpa_plan_walker_context's join_strategies[JSTRAT_NESTED_LOOP_PLAIN]
> member correctly, or else pgpa_output.c is not serializing it to text
> correctly. I suspect the former is a more likely but I'm not sure
> exactly what's happening.

I think I see the problem: pgpa_process_unrolled_join() returns a set
called "all_relids" but it only returns the union of the inner relid
sets, not including the outer relid set. In your example, we want to
get:

NESTED_LOOP_PLAIN((part partsupp) (supplier part partsupp))

But the join order is:

JOIN_ORDER(nation (supplier (part partsupp)))

So every table is the outer table of some unrolled join, except for
the innermost table, which is partsupp. So all the others get omitted
from the output, and we get the output you saw:

NESTED_LOOP_PLAIN(partsupp partsupp)

Proposed fix attached.

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

Вложения

unrolled_join.diff.txt

Re: pg_plan_advice

От

Haibo Yan

Дата:

09 января, 01:52:29

> Does the plan end up disabled in that case?

I understand that except for join order advice which might not be honored(due to GEQO's randomness), other forms of advice—like scan types or join methods—remain effective since those operations are inevitable parts of the plan regardless of the specific tree structure.

Thanks for the explanation regarding the design philosophy. It clarifies that the primary goal is stabilizing plans within the optimizer's valid search space rather than forcing impossible paths. I will keep the geqo_threshold adjustment in mind if strict structure enforcement is ever needed.

Regards

Haibo

On Thu, Jan 8, 2026 at 11:53 AM Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Jan 7, 2026 at 5:47 PM Haibo Yan <tristan.yim@gmail.com> wrote:
> Instead, the planner seems to silently ignore the structural constraint of the advice and falls back to a path GEQO can actually find.

Does the plan end up disabled in that case?

> I believe this behavior is acceptable because pg_plan_advice is intended to stabilize plans that the optimizer can generate. Since GEQO cannot generate Bushy plans, users should not be supplying them.

Right, I agree. A core principal here is that you can only nudge the
planner towards a plan it would have considered anyway. In the case of
GEQO, there is some randomness to which plans are considered. Your
advice will only be reliably take into account if it applies to
elements that must be part of the final plan. For instance, if you
advise the use of a sequential scan or an index scan, that should
work, because that relation has to be scanned somehow. Advice on a
join method should almost always work, since it can apply to any
non-leading table. Of course, you also won't be able to advise an
infeasible join method, but that would be true without GEQO, too.
Advice on the join order is going to be iffy when using GEQO -- if a
compatible join order is highly likely to be considered, e.g. because
you specify something like JOIN_ORDER(just_one) that only sets the
driving table -- then it'll probably work, but if you give a complete
join order specification, it probably won't. If you want to avoid
that, you can adjust geqo_threshold.

Thanks for looking into this.

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

Re: pg_plan_advice

От

Michael Paquier

Дата:

09 января, 02:29:38

On Thu, Jan 08, 2026 at 11:07:31AM -0500, Robert Haas wrote:
> On Wed, Jan 7, 2026 at 2:04 AM Lukas Fittl <lukas@fittl.com> wrote:
>> That said, good news: After a bunch of iterations, I get a clean
>> pass on the pg_hint_plan regression tests, whilst completely
>> dropping its copying of core code and hackish re-run of
>> set_plain_rel_pathlist. See [0] for a draft PR (on my own fork of
>> pg_hint_plan) with individual patches that explain some regression
>> test differences.
>
> That sounds AWESOME.

So you are telling me that I can commit code that deletes code.  Count
me in.  The project has some merge requests that I've been holding on
a bit due to what's happening here and because I did not really look
at the internals that have changed.  It's great to see that you have
begun an investigation, Lukas.

>> The biggest change in the regression test output was due to how the
>> "Parallel" hint worked in pg_hint_plan (basically it was setting
>> parallel_*_cost to zero, and then messed with the gucs that factor
>> into compute_parallel_worker) -- I think the only sensible thing to
>> do is to change that in pg_hint_plan, and instead rely on rejecting
>> non-partial paths with PGS_CONSIDER_NONPARTIAL if "hard"
>> enforcement of parallelism is requested. That caused some minor
>> plan changes, but I think they can still be argued to be matching
>> the user's intent of "make a scan involving this relation
>> parallel".
>
> Cool. I'm sort of curious what changed, but maybe it's not important
> enough to spend time discussing right now.

I suspect that this is going to be an incremental integration process,
and it smells to me that it is going to require more than one major
release before being able to remove the whole set of hacks that
pg_hint_plan has been using, particularly with the GUCs, the costing
and the forced update of the backend routines which is a ugly
historical hack.  Saying that, I would need to look at the plan
outputs to be sure, perhaps we would be OK even with slight changes.
These happen every year, because the plans tested are complex enough
that some of the sub-paths are changed, but the hints still work
properly.  This year for v19 we have at least the changes in the
expression names.
--
Michael

Вложения

signature.asc

Re: pg_plan_advice

От

Jakub Wartak

Дата:

09 января, 16:52:59

On Thu, Jan 8, 2026 at 7:21 PM Robert Haas <robertmhaas@gmail.com> wrote:
[..]
> I've attempted to address that in v8 [..snip]
[..]

The full TPC-H queries set still reported some issues with v8 (for
q20/with wrong plan after using advice and two "partially matched" for
q9 and q2). However after applying that tiny patch from [1] it makes
all of the problems go away and for the TPC-H query set there are no
failures anymore (yay!). By failure I mean a different plan when using
advice and/or any partial match or failure to apply advice.

So, I've switched to more realistic test for each TPC-H query:
1) ensure we have valid stats, gather plan, save advices
2) clear stats using pg_clear_relation_stats() , apply advice and
check if we have exact same plan

The only thing this hav revealed is what appears to be some tiny
problem with placing GroupAggregates or am I wrong or is that known
limitation? (The original plan shows "Partial GroupAggregate" while
the one using advice is not aware of the need to use it; yet
contrib/pg_plan_advices/README in "Future Work" indicates it is out of
scope for now, right?)

--- /tmp/plan
+++ /tmp/planadviced
Sort
   Sort Key: (sum((lineitem.l_extendedprice * ('1'::numeric -
lineitem.l_discount)))) DESC
-  ->  Finalize GroupAggregate
+  ->  GroupAggregate
         Group Key: nation.n_name
         ->  Gather Merge
               Workers Planned: 2
-              ->  Partial GroupAggregate
-                    Group Key: nation.n_name
-                    ->  Sort
-                          Sort Key: nation.n_name
-                          ->  Hash Join
-                                Hash Cond: ((lineitem.l_suppkey =
supplier.s_suppkey) AND (customer.c_nationkey = supplier.s_nationkey))
+              ->  Sort
+                    Sort Key: nation.n_name
+                    ->  Hash Join
+                          Hash Cond: ((lineitem.l_suppkey =
supplier.s_suppkey) AND (customer.c_nationkey = supplier.s_nationkey))

-J.

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

Вход в личный кабинет

Восстановление пароля

Подтверждение аккаунта

Изменение пароля

Обсуждение: pg_plan_advice

Вложения

Вложения

Вложения

Вложения

Вложения

Вложения

Вложения

Вложения

Вложения

Вложения

Вложения

Вложения

Вложения

Вложения