Subject: pg_plan_advice

From: Robert Haas <[email-masked]>
Date: Thu, 30 Oct 2025 10:00:05 -0400

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.

---

From: Jakub Wartak <[email-masked]>
Date: Fri, 31 Oct 2025 10:58:59 +0100

On Thu, Oct 30, 2025 at 3:00 PM Robert Haas <[email-masked]> 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.
From 06038e237420b7054912118076e8b981def4f545 Mon Sep 17 00:00:00 2001
From: Jakub Wartak <[email-masked]>
Date: Fri, 31 Oct 2025 09:35:59 +0100
Subject: [PATCH] fixup: supress some gcc warnings

---
 contrib/pg_plan_advice/pgpa_ast.c    |  3 ++-
 contrib/pg_plan_advice/pgpa_output.c | 12 ++++++++----
 contrib/pg_plan_advice/pgpa_walker.c |  4 +++-
 3 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/contrib/pg_plan_advice/pgpa_ast.c b/contrib/pg_plan_advice/pgpa_ast.c
index ed18950af18..be598874c48 100644
--- a/contrib/pg_plan_advice/pgpa_ast.c
+++ b/contrib/pg_plan_advice/pgpa_ast.c
@@ -138,7 +138,8 @@ pgpa_cstring_advice_tag(pgpa_advice_tag_type advice_tag)
 			return "TID_SCAN";
 	}
 
-	Assert(false);
+	elog(ERROR, "unrecognized advice type: %d", advice_tag);
+	pg_unreachable();
 }
 
 /*
diff --git a/contrib/pg_plan_advice/pgpa_output.c b/contrib/pg_plan_advice/pgpa_output.c
index 2175278b580..5aae5071990 100644
--- a/contrib/pg_plan_advice/pgpa_output.c
+++ b/contrib/pg_plan_advice/pgpa_output.c
@@ -10,6 +10,7 @@
  *-------------------------------------------------------------------------
  */
 
+#include "c.h"
 #include "postgres.h"
 
 #include "pgpa_output.h"
@@ -507,7 +508,8 @@ pgpa_cstring_join_strategy(pgpa_join_strategy strategy)
 			return "HASH_JOIN";
 	}
 
-	Assert(false);
+	elog(ERROR, "unrecognized join strategy: %d", strategy);
+	pg_unreachable();
 }
 
 /*
@@ -536,11 +538,12 @@ pgpa_cstring_scan_strategy(pgpa_scan_strategy strategy)
 			return "TID_SCAN";
 	}
 
-	Assert(false);
+	elog(ERROR, "unrecognized scan strategy: %d", strategy);
+	pg_unreachable();
 }
 
 /*
- * Get a C string that corresponds to the specified scan strategy.
+ * Get a C string that corresponds to the specified query feature type.
  */
 static char *
 pgpa_cstring_query_feature_type(pgpa_qf_type type)
@@ -557,7 +560,8 @@ pgpa_cstring_query_feature_type(pgpa_qf_type type)
 			return "SEMIJOIN_UNIQUE";
 	}
 
-	Assert(false);
+	elog(ERROR, "unrecognized query feature type: %d", type);
+	pg_unreachable();
 }
 
 /*
diff --git a/contrib/pg_plan_advice/pgpa_walker.c b/contrib/pg_plan_advice/pgpa_walker.c
index d22ac11bf91..44adeb4511b 100644
--- a/contrib/pg_plan_advice/pgpa_walker.c
+++ b/contrib/pg_plan_advice/pgpa_walker.c
@@ -359,7 +359,7 @@ pgpa_walk_recursively(pgpa_plan_walker_context *walker, Plan *plan,
 	{
 		int			num_aqf = list_length(active_query_features);
 
-		(void) list_truncate(active_query_features, num_aqf - 1);
+		active_query_features = list_truncate(active_query_features, num_aqf - 1);
 	}
 }
 
@@ -768,6 +768,7 @@ pgpa_walker_join_order_matches_member(pgpa_join_member *member,
 					relids = bms_add_member(relids, rti);
 				}
 			}
+			break;
 
 		case PGPA_TARGET_IDENTIFIER:
 			{
@@ -777,6 +778,7 @@ pgpa_walker_join_order_matches_member(pgpa_join_member *member,
 										  &target->rid);
 				relids = bms_make_singleton(rti);
 			}
+			break;
 	}
 
 	return bms_equal(member->scan->relids, relids);
-- 
2.43.0

From 4fdb5d6b047fc7fd188be23c64ae7f352b014f21 Mon Sep 17 00:00:00 2001
From: Jakub Wartak <[email-masked]>
Date: Fri, 31 Oct 2025 09:34:58 +0100
Subject: [PATCH 1/2] fixup: not sure

---
 contrib/pg_plan_advice/pgpa_planner.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/pg_plan_advice/pgpa_planner.c b/contrib/pg_plan_advice/pgpa_planner.c
index c4859c23020..5b7d2cbd9f4 100644
--- a/contrib/pg_plan_advice/pgpa_planner.c
+++ b/contrib/pg_plan_advice/pgpa_planner.c
@@ -1411,7 +1411,7 @@ pgpa_planner_apply_scan_advice(RelOptInfo *rel,
 		 * Note that setting PGS_CONSIDER_NONPARTIAL in my_gather_mask is
 		 * equivalent to allowing the non-use of either form of Gather here.
 		 */
-		if (my_entry->tag == PGPA_TAG_GATHER |
+		if (my_entry->tag == PGPA_TAG_GATHER ||
 			my_entry->tag == PGPA_TAG_GATHER_MERGE)
 		{
 			if (!just_one_rel)
-- 
2.43.0

---

From: Robert Haas <[email-masked]>
Date: Fri, 31 Oct 2025 08:51:34 -0400

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

> set of queries and not the others [with same aliases] is some major

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

> every plan node (including hashes of subplans),
[...]
> 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.

---

From: Alastair Turner <[email-masked]>
Date: Fri, 31 Oct 2025 21:17:41 +0000

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

---

From: Hannu Krosing <[email-masked]>
Date: Sat, 1 Nov 2025 17:10:08 +0100

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 <[email-masked]> 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 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

---

From: Robert Haas <[email-masked]>
Date: Mon, 3 Nov 2025 11:18:08 -0500

> 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 be…

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 there user accessible. Not only for testing code, but also for testing users' belief…

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.

---

From: Robert Haas <[email-masked]>
Date: Mon, 3 Nov 2025 11:41:35 -0500

> excessively stupid query generators (usually, but not always, some BI

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.

---

From: John Naylor <[email-masked]>
Date: Tue, 4 Nov 2025 18:47:48 +0700

On Thu, Oct 30, 2025 at 9:00 PM Robert Haas <[email-masked]> wrote:
> First, any form of user control over the

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.

> throw up their hands just as we do.

Right.

> it seems to be super-useful for testing. We have

That sounds very useful as well.

--
John Naylor
Amazon Web Services

---

From: Robert Haas <[email-masked]>
Date: Tue, 4 Nov 2025 14:54:44 -0500

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

---

From: Robert Haas <[email-masked]>
Date: Thu, 6 Nov 2025 11:45:47 -0500

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.

---

From: "Matheus Alcantara" <[email-masked]>
Date: Mon, 17 Nov 2025 11:42:45 -0300

Hi

> stupid few bugs in pgpa_collector.c and added a few more tests.
I've spent some time playing with these patches. I still don't have to
much comments on the syntax yet but I've noticed a small bug or perhaps
I'm missing something?

When I run CREATE EXTENSION pg_plan_advice I'm able to use the
EXPLAIN(plan_advice) but if try to open another connection, with the
extension already previously created, I'm unable to use once I drop and
re-create the extension.

tpch=# create extension pg_plan_advice;
ERROR:  extension "pg_plan_advice" already exists
tpch=# explain(plan_advice) select 1;
ERROR:  unrecognized EXPLAIN option "plan_advice"
LINE 1: explain(plan_advice) select 1;
                ^
tpch=# drop extension pg_plan_advice ;
DROP EXTENSION
tpch=# create extension pg_plan_advice;
CREATE EXTENSION
tpch=# explain(plan_advice) select 1;
                QUERY PLAN
------------------------------------------
 Result  (cost=0.00..0.01 rows=1 width=4)
 Generated Plan Advice:
   NO_GATHER("*RESULT*")

And thanks for working on this. I think that this can be a very useful
feature for both users and for postgres hackers, +1 for the idea.

-- 
Matheus Alcantara

---

From: Robert Haas <[email-masked]>
Date: Mon, 17 Nov 2025 10:09:36 -0500

> much comments on the syntax yet but I've noticed a small bug or perhaps

Cool, thanks for looking. I am guessing that the paucity of feedback
thus far is partly because there's a lot of stuff to absorb -- though
the main point at this stage is really to get some opinions on the
planner infrastructure/hooks, which don't necessarily require full
understanding of (never mind agreement with) the design of
pg_plan_advice itself.

> extension already previously created, I'm unable to use once I drop and

This is just an idiosyncrasy of PostgreSQL's extension framework.
Whether or not EXPLAIN (PLAN_ADVICE) works depends on whether the
shared module has been loaded, not whether the extension has been
created. The purpose of CREATE EXTENSION is to put SQL objects, such
as function definitions, into the database, but there's no SQL
required to enable EXPLAIN (PLAN_ADVICE) -- or for setting the
pg_plan_advice.advice GUC. However, running CREATE EXTENSION to
establish the function definitions will incidentally load the shared
module into that particular session.

Therefore, the best way to use this module is to add pg_plan_advice to
shared_preload_libraries. Alternatively, you can use
session_preload_libraries or run LOAD in an individual session. If you
don't care about the collector interface, that's really all you need.
If you do care about the collector interface, then in addition you
will need to run CREATE EXTENSION, so that the SQL functions needed to
access it are available.

---

From: Robert Haas <[email-masked]>
Date: Tue, 18 Nov 2025 11:19:24 -0500

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

---

From: "Dian Fay" <[email-masked]>
Date: Sat, 22 Nov 2025 19:43:40 -0500

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

---

From: Robert Haas <[email-masked]>
Date: Mon, 24 Nov 2025 11:14:27 -0500

On Sat, Nov 22, 2025 at 7:43 PM Dian Fay <[email-masked]> 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.

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

> diving but not an internals hacker" eyes), and could disable one as

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.

---

From: "Dian Fay" <[email-masked]>
Date: Sat, 29 Nov 2025 22:16:44 -0500

> Scan without reversing the order of those tests.

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

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

---

From: Robert Haas <[email-masked]>
Date: Fri, 5 Dec 2025 14:57:09 -0500

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

---

From: "Greg Burd" <[email-masked]>
Date: Mon, 08 Dec 2025 15:39:25 -0500

> avoid confusing cfbot or human reviewers.

Hey Robert,

Thanks for working on this!  I think the idea has merit, I hope it lands sometime soon.

I've worked on extending PostgreSQL's planner for a JSONB-like document system. The new query shapes frequently caused mysterious planner issues. Having the ability to:

1. Dump detailed planner decisions for comparison during development
2. Record planner choices from customer databases to reproduce their issues
3. Apply fixed plans to specific queries as a quick customer workaround while addressing root causes in later releases

...would have been invaluable.

Yes, there's danger here ("with great power comes great responsibility"), but I see this as providing more information to make better decisions when working with the black art of planner logic.

ORM Query Variation Challenge

Jakob's point about "crazy ORMs" is important. ORMs generate queries with minor variations that should ideally match the same plan advice. I need to study the plan advice matching logic more deeply to understand how it handles query variations.

This reminded me of Erlang/Elixir's "parse transforms" - compiled code forms an AST that registered transforms can modify via pattern matching. The concept might be relevant here: pattern-matching portions of query ASTs to apply advice despite syntactic variations. I'd need to think more about whether this intersects well with the current design or if it's impractical, but it's worth exploring.

> Attachments:
> * v5-0001-Store-information-about-range-table-flattening-in.patch

contrib/pg_overexplain/pg_overexplain.c

+               /* Advance to next SubRTInfo, if it's time. */
+               if (lc_subrtinfo != NULL)
+               {
+                       next_rtinfo = lfirst(lc_subrtinfo);
+                       if (rti > next_rtinfo->rtoffset)

Should the test be >= not >? Unless I am I reading this wrong, when rti == rtoffset, that's the first entry of the new subplan's range table. That would mean that the current logic skips displaying the subplan name for the first RTE of each subplan.

in src/include/nodes/plannodes.h there is:

+typedef struct SubPlanRTInfo
+{
+       NodeTag         type;
+       const char *plan_name;
+       Index           rtoffset;
+       bool            dummy;
+} SubPlanRTInfo;

This is where I get confused, if rtoffset is an Index, then the comparison (above) in pg_overexplain uses rti > next_rtinfo->rtoffset where rti starts at 1. If rtoffset is 0 for the first subplan, the logic might be off-by-one, no?

> This commit teaches pg_overexplain'e RANGE_TABLE option to make use
Minor nit in the commit message, "pg_overexplain'e" should be "pg_overexplain's"

> * v5-0002-Store-information-about-elided-nodes-in-the-final.patch

+/*
+ * Record some details about a node removed from the plan during setrefs
+ * procesing, for the benefit of code trying to reconstruct planner decisions
+ * from examination of the final plan tree.
+ */

Nit, "procesing" should be "processing"

> * v5-0003-Store-information-about-Append-node-consolidation.patch

src/backend/optimizer/path/allpaths.c

/* Now consider each interesting sort ordering */
foreach(lcp, all_child_pathkeys)
{
        List       *subpaths = NIL;
        bool            subpaths_valid = true;
+       List       *subpath_cars = NIL;
        List       *startup_subpaths = NIL;
        bool            startup_subpaths_valid = true;
+       List       *startup_subpath_cars = NIL;
        List       *partial_subpaths = NIL;
+       List       *partial_subpath_cars = NIL;
        List       *pa_partial_subpaths = NIL;
        List       *pa_nonpartial_subpaths = NIL;
+       List       *pa_subpath_cars = NIL;

I find "cars" a bit cryptic (albeit clever), I think I've decoded it properly and it stands for "child_append_relid_sets", correct?  Could you add a comment or use a clearer name like subpath_child_relids or consolidated_relid_sets?

+accumulate_append_subpath(Path *path, List **subpaths, List **special_subpaths,
+                                                 List **child_append_relid_sets)
 {
        if (IsA(path, AppendPath))
        {
@@ -2219,6 +2256,8 @@ accumulate_append_subpath(Path *path, List **subpaths, List **special_subpaths)
                if (!apath->path.parallel_aware || apath->first_partial_path == 0)
                {
                        *subpaths = list_concat(*subpaths, apath->subpaths);
+                       *child_append_relid_sets =
+                               lappend(*child_append_relid_sets, path->parent->relids);

Is it possible that when pulling up multiple subpaths from an AppendPath, only ONE relid set is added to child_append_relid_sets, but MULTIPLE paths are added to subpaths?  If so, that would break the correspondence between the lists which would be bad, right?

src/include/nodes/pathnodes.h
+ * Whenever accumulate_append_subpath() allows us to consolidate multiple
+ * levels of Append paths are consolidated down to one, we store the RTI
+ * sets for the omitted paths in child_append_relid_sets. This is not necessary
+ * for planning or execution; we do it for the benefit of code that wants
+ * to inspect the final plan and understand how it came to be.

Minor: "paths are consolidated" is redundant, should be "paths consolidated" or "allows us to consolidate".

> * v5-0004-Allow-for-plugin-control-over-path-generation-str.patch

src/backend/optimizer/path/costsize.c
+       else
+               enable_mask |= PGS_CONSIDER_NONPARTIAL;

-       path->disabled_nodes = enable_seqscan ? 0 : 1;
+       path->disabled_nodes =
+               (baserel->pgs_mask & enable_mask) == enable_mask ? 0 : 1;

When parallel_workers > 0 the path is partial and doesn't need PGS_CONSIDER_NONPARTIAL. But if parallel_workers == 0, it's non-partial and DOES need it, right?  Would this mean that non-partial paths can be disabled even when the scan type itself (e.g., PGS_SEQSCAN) is enabled?  Intentional?

> * v5-0005-WIP-Add-pg_plan_advice-contrib-module.patch

It seems this is still WIP with a solid start, I'm not going to dig too much into it. :)

Keep it up, best.

-greg

---

From: Jacob Champion <[email-masked]>
Date: Mon, 8 Dec 2025 17:18:50 -0800

Hello,

> avoid confusing cfbot or human reviewers.

I really like this idea! Telling the planner, "if you need to make a
decision for [this thing], choose [this way]," seems to be a really
nice way of sidestepping many of the concerns with "user control".

I've started an attempt to throw a fuzzer at this, because I'm pretty
useless when it comes to planner/optimizer review. I don't really know
what the overall fuzzing strategy is going to be, given the multiple
complicated inputs that have to be constructed and somehow correlated
with each other, but I'll try to start small and expand:

a) fuzz the parser first, because it's easy and we can get interesting inputs
b) fuzz the AST utilities, seeded with "successful" corpus members from a)
c) stare really hard at the corpus of b) and figure out how to
usefully mutate a PlannedStmt with it
d) use c) to fuzz pgpa_plan_walker, then pgpa_output_advice, then...?

I'm in the middle of an implementation of b) now, and it noticed the
following code (which probably bodes well for the fuzzer itself!):

>        if (rid->partnsp == NULL)
>            result = psprintf("%s/%s", result,
>                              quote_identifier(rid->partnsp));

I assume that should be quote_identifier(rid->partrel)?

= Other Notes =

GCC 11 complains about the following code in pgpa_collect_advice():

>        dsa_area   *area = pg_plan_advice_dsa_area();
>        dsa_pointer ca_pointer;
>
>        pgpa_make_collected_advice(userid, dbid, queryId, now,
>                                   query_string, advice_string, area,
>                                   &ca_pointer);
>        pgpa_store_shared_advice(ca_pointer);

It doesn't know that area is guaranteed to be non-NULL, so it can't
prove that ca_pointer is initialized.

(GCC also complains about unique_nonjoin_rtekind() not initializing
the rtekind, but I think that's because of a bug [1].)

--Jacob

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107838

---

From: Robert Haas <[email-masked]>
Date: Tue, 9 Dec 2025 14:34:43 -0500

> Thanks for working on this!  I think the idea has merit, I hope it lands sometime soon.

Thanks. I think it needs a good deal more review first, but I
appreciate the support.

> Should the test be >= not >? Unless I am I reading this wrong, when rti == rtoffset, that's the first entry of the new subplan's range table. That would mean that the current logic skips displaying…

I don't think so. I think I actually had it that way at one point, and
I believe I found that it was wrong. RTIs are 1-based, so the smallest
per-subquery RTI is 1. rtoffset is the amount that must be added to
the per-subquery RTI to get a "flat" RTI that can be used to index
into the final range table. But if you find that theoretical argument
unconvincing, by all means please test it and see what happens!

> Minor nit in the commit message, "pg_overexplain'e" should be "pg_overexplain's"

Thanks, fixed in my local branch.

> Nit, "procesing" should be "processing"

Thanks, fixed in my local branch.

> I find "cars" a bit cryptic (albeit clever), I think I've decoded it properly and it stands for "child_append_relid_sets", correct?  Could you add a comment or use a clearer name like subpath_child…

I certainly admit that this is a bit too clever. I am not entirely
sure how to make it less clever. There needs to be a
child-append-relid-sets list corresponding to every current and future
subpath list, and the names of some of those subpath lists are already
quite long, so whatever naming convention we choose for the "cars"
lists had better not add too much more length to the variable name. I
felt like someone looking at this might initially be confused by what
"cars" meant, but then I thought that they would probably look at how
the variable was used and see that it was for example being passed as
the second argument to get_singleton_append_subpath(), which is named
child_append_relid_sets, or being passed to create_append_path or
create_merge_append_path, which also use that naming. I figured that
this would clear up the confusion pretty quickly. I could certainly
add a comment above this block of variable assignments saying
something like "for each list of paths, we must also maintain a list
of child append relid sets, etc. etc." but I worried that this would
create as much confusion as it solved, i.e. somebody reading the code
would be going: why is this comment here? Is it trying to tell me that
there's something weirder going on than what is anyway obvious?

If I get more opinions that some clarification is needed here, I'm
happy to change it, especially if those opinions agree with each other
on exactly what to change, but I think for now I'll leave it as it is.

> Is it possible that when pulling up multiple subpaths from an AppendPath, only ONE relid set is added to child_append_relid_sets, but MULTIPLE paths are added to subpaths?  If so, that would break…

That would indeed be bad, but I'm not clear on how you think it could
happen. Can you clarify?

> Minor: "paths are consolidated" is redundant, should be "paths consolidated" or "allows us to consolidate".

Thanks, fixed in my local branch.

> When parallel_workers > 0 the path is partial and doesn't need PGS_CONSIDER_NONPARTIAL. But if parallel_workers == 0, it's non-partial and DOES need it, right?  Would this mean that non-partial pat…

See this comment:

 * Finally, unsetting PGS_CONSIDER_NONPARTIAL disables all non-partial paths
 * except those that use Gather or Gather Merge. In most other cases, a
 * plugin can nudge the planner toward a particular strategy by disabling
 * all of the others, but that doesn't work here: unsetting PGS_SEQSCAN,
 * for instance, would disable both partial and non-partial sequential scans.

> It seems this is still WIP with a solid start, I'm not going to dig too much into it. :)

Thanks for the review so far!

---

From: Robert Haas <[email-masked]>
Date: Tue, 9 Dec 2025 14:45:47 -0500

> d) use c) to fuzz pgpa_plan_walker, then pgpa_output_advice, then...?

Cool. I'm bad at fuzzing, but I think fuzzing by someone who is good
at it is very promising for this kind of patch.

> I assume that should be quote_identifier(rid->partrel)?

Yes, thanks. Fixed locally. By the way, if your fuzzer can also
produces some things to add contrib/pg_plan_advice/sql for cases like
this, that would be quite helpful. Ideally I would have caught this
with a manually-written test case, but obviously that didn't happen.

> prove that ca_pointer is initialized.

I don't know what to do about that. I can understand why it might be
unable to prove that, but I don't see an obvious way to change the
code that would make life easier. I could add Assert(area != NULL)
before the call to pgpa_make_collected_advice() if that helps.

> the rtekind, but I think that's because of a bug [1].)

This one could be fixed with a dummy initialization, if needed.

---

From: Amit Langote <[email-masked]>
Date: Wed, 10 Dec 2025 20:20:38 +0900

Hi Robert,

> *especially* if you're thinking of flaming me.

Thanks for posting this.  Looks very interesting to me.

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.

> something that can potentially be really useful to advance PostgreSQL

+1, the testing benefits make this worthwhile.

> but that's where I have put it for now. Feedback welcome, on this and

On the relation identifier system: IMHO this part doesn't seem as
opinionated as the advice mini-language. The requirements pretty much
dictate the design -- you need alias names and occurrence counters to
handle self-joins, partition fields for partitioned tables, and a
string representation to survive dump/restore. There doesn't seem to
be much flexibility in that.

Given that, it seems more practical to put this in core from the
start. Extensions that might want to build plan-advice-like
functionality shouldn’t have to clone this logic and wait another
release for something that’s already well-defined and deterministic.
The mini-language is opinionated and belongs in contrib, but the
identifier infrastructure just solves a fundamental problem cleanly.

On the infrastructure patches (0001-0005): these look sensible. The
range table flattening info, elided node tracking, and append node
consolidation preserve information that's currently lost -- there's
some additional overhead to track this, but it's fixed per-relation
per-subquery, which seems reasonable.  The path generation hooks
(0005) are a clear improvement: moving from global enable_* GUCs to
per-RelOptInfo pgs_mask gives extensions the granularity they need for
relation-specific and join-specific decisions. Yes, you need C code to
use them, but you'd need to write C code to do something of value in
this area anyway, and the hooks give you control that GUCs can't
provide.

Overall, I'm supportive of getting these committed once they're ready.
contrib/pg_plan_advice is a compelling proof-of-concept for why these
hooks are needed.

I'll try to post more specific comments once I've read this some more.

--
Thanks, Amit Langote

---

From: Jakub Wartak <[email-masked]>
Date: Wed, 10 Dec 2025 12:43:39 +0100

On Fri, Dec 5, 2025 at 8:57 PM Robert Haas <[email-masked]> wrote:
[..]
> avoid confusing cfbot or human reviewers.

Quick-question regarding cross-interactions of the extensions: would
it be possible for auto_explain to have something like
auto_explain.log_custom_options='PLAN_ADVICES' so that it could be
dumping the advice of the queries involved . I can see there is
ApplyExtensionExplainOption() and that would have to probably be used
by auto_explain(?) Or is there any other better way or perhaps it
somehow is against some design or it's just outside of initial scope?
This would solve two problems:
a) sometimes explaining manually (psql) is simply not realistic as it
is being run by app only
b) auto_explain could log nested queries and could print plan advices
along the way, which can be very painful process otherwise
(reverse-engineering how the optimizer would name things  in more
complex queries run from inside PLPGSQL functions)

BTW, some feedback: the plan advices (plan fixing) seems to work fine
for nested queries inside PLPGSQL, and also I've discovered (?) that
one can do even today with patchset the following:
   alter function blah(bigint) set pg_plan_advice.advice =
'NESTED_LOOP_MATERIALIZE(b)';
which seems to be pretty cool, because it allows more targeted fixes
without even having capability of fixing plans for specific query_id
(as discussed earlier).

For the generation part, the only remaining thing is how it integrates
with partitions (especially the ones being dynamically created/dropped
over time). Right now one needs to keep the advice(s) in sync after
altering the partitions, but it could be expected that some form of
regexp/partition-templating would be built into pg_plan_advices
instead. Anyway, I think this one should go into documentation just as
known-limitations for now.

While scratching my head on how to prove that this is not crashing
I've also checked below ones (TLDR all ok):
1. PG_TEST_INITDB_EXTRA_OPTS="-c
shared_preload_libraries='pg_plan_advice'"  meson test  # It was clean
2. PG_TEST_INITDB_EXTRA_OPTS="-c
shared_preload_libraries='pg_plan_advice'" PGOPTIONS="-c
pg_plan_advice.advice=NESTED_LOOP_MATERIALIZE(certainlynotused)" meson
test # This had several failures, but all is OK: it's just some of
them had to additional (expected) text inside regression.diffs:
NESTED_LOOP_MATERIALIZE(certainlynotused) /* not matched */
3. PG_TEST_INITDB_EXTRA_OPTS="-c
shared_preload_libraries='pg_plan_advice' -c
pg_plan_advice.shared_collection_limit=42"  meson test # It was clean
too

-J.

---

From: Robert Haas <[email-masked]>
Date: Wed, 10 Dec 2025 08:33:11 -0500

On Wed, Dec 10, 2025 at 6:43 AM Jakub Wartak
<[email-masked]> wrote:
> dumping the advice of the queries involved

Yes, I had the same idea. I think the tricky part here is that an
option can have an argument. Most options will probably have Boolean
arguments, but there are existing in-core counterexamples, such as
FORMAT. We should try to figure out the GUC in such a way that it can
be used either to set a Boolean option by just specifying it, or that
it can be used to set an option to a value by writing both. Maybe it's
fine if the GUC value is just a comma-separated list of entries, and
each entry can either be an option name or an option name followed by
a space followed by an option value, i.e. if FORMAT were custom, then
you could write auto_explain.log_custom_options='format xml,
plan_advice' or auto_explain.log_custom_options='plan_advice true,
range_table false' and have sensible things happen. In fact, very
possibly the GUC should just accept any options whether in-core or
out-of-core and not distinguish, so it would be more like
auto_explain.log_options.

> without even having capability of fixing plans for specific query_id

Yes, this is a big advantage of reusing the GUC machinery for this
purpose (but see the thread on "[PATCH] Allow complex data for GUC
extra").

> known-limitations for now.

Right. I don't think trying to address this at this stage makes sense.
To maintain my sanity, I want to focus for now only on things that
round-trip: that is, we can generate it, and then we can accept that
same stuff. If we're using a parallel plan for every partition e.g.
they are all sequential scans or all index scans, we could generate
SEQ_SCAN(foo/*) or similar and then we could accept that. But figuring
that out would take a bunch of additional infrastructure that I don't
have the time or energy to create right this minute, and I don't see
it as anywhere close to essential for v1. Some other problems here:

1. What happens when a small number of partitions are different? The
code puts quite a bit of energy into detecting conflicting advice, and
honestly probably should put even more, and you might say, well, if
there's just one partition that used an index scan, then I still want
the advice to read SEQ_SCAN(foo/*) INDEX_SCAN(foo/foo23 foo23_a_idx)
and not signal a conflict, but that's slightly unprincipled.

2. INDEX_SCAN() specifications and similar will tend not to be
different for every partition because the index names will be
different for every partition. You might want something that says "for
each partition of foo, use the index on that partition that is a child
of this index on the parent".

Long run, there's a lot of things that can be added to this to make it
more concise (and more expressive, too). Another similar idea is to
have something like NO_GATHER_UNLESS_I_SAID_SO() so that a
non-parallel query doesn't have to do NO_GATHER(every single relation
including all the partitions). I'm pretty sure this is a valuable
idea, but, again, it's not essential for v1.

> pg_plan_advice.shared_collection_limit=42"  meson test # It was clean

You can set pg_plan_advice.always_explain_supplied_advice=false to
clean up some of the noise here. This kind of testing is why I
invented that option. I think that in production, we REALLY REALLY
want any supplied advice to show up in the EXPLAIN plan even if the
user did not specify the PLAN_ADVICE option to EXPLAIN. Otherwise,
understanding what is going on with an EXPLAIN plan that a
hypothetical customer sends to a hypothetical PostgreSQL expert who
has to support said hypothetical customer will be a miserable
experience. But for testing purposes, it's nice to be able to shut it
off so you don't get random regression diffs.

---

From: Robert Haas <[email-masked]>
Date: Wed, 10 Dec 2025 09:54:13 -0500

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

> be much flexibility in that.

Right. There's some flexibility. For instance, you could handle
partitions using occurrence numbers, which would actually save a bunch
of code, but that seems obviously worse in terms of user experience.
Also, you could if you wanted key it off of the name of the table
rather than the relation alias used for the table. I think that's also
worse but possibly it's debatable. You could change the order of the
pieces in the representation; e.g. maybe plan_name should come first
rather than last; or you could change the separator characters. But,
honestly, none of that strikes me as sufficient grounds to want
multiple implementations. If the choices I've made don't seem good to
other people, then we should just change them and hopefully find
something everybody can live with. It's a bit like the way that
extension SQL scripts use "--" as a separator: maybe not everybody
agrees that this is the absolutely most elegant choice, but nobody's
proposing a a second version of the extension mechanism just to do
something different.

> identifier infrastructure just solves a fundamental problem cleanly.

It's not quite as easy to make a sharp distinction between these
things as someone might hope. Note that the lexer and parser handle
the whole mini-language, which includes parsing the relation
identifiers. That doesn't of course mean that the code to *generate*
relation identifiers couldn't be in core, and I actually had it that
way at one point, but it's not very much code and I wasn't too
impressed with how that turned out. It seemed to couple the core code
to the extension more tightly than necessary for not much real
benefit.

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. Then obviously the relation identifier generation
code needs to be in core, and that makes total sense because we're
actually using it for something, and arguably we've made life easier
for everyone who wants to use pg_plan_advice in the future because
they're already familiar with the identifier convention. The downside
is everyone has to get used to the new EXPLAIN output even if they
don't care about pg_plan_advice or hate it with a fiery passion.

So my point here is that there are things we can decide to do to make
some or all of this "core," but IMHO it's not just as simple as saying
"this is in, that's out". It's more about deciding what the end state
ought to look like, and how integrated this stuff ought to be into the
fabric of PostgreSQL. I started with the minimal level of integration:
little pieces of core infrastructure, all used by a giant extension.
Now we need to either decide that's where we want to settle, or decide
to push to some greater or lesser degree toward more integration.

> contrib/pg_plan_advice is a compelling proof-of-concept for why these

Great. I don't think there's anything terribly controversial in
0001-0004. I think the comments and so on might need improving and
there could be little mini-bugs or whatever, but basically I think
they work and I don't anticipate any major problems. However, I'd want
at least one other person to do a detailed review before committing
anything. 0005 might be a little more controversial. There's some
design choices to dislike (though I believe I've made them for good
reason) and there's a question of whether it's as complete as we want.
It might be fine to commit it the way it is and just adjust it later
if we find that something ought to be different, but it's also
possible that we should think harder about some of the choices or hold
off for a bit while other parts of this effort move forward. I'm happy
to hear opinions on the best strategy here.

> I'll try to post more specific comments once I've read this some more.

Thanks for the review so far, and that sounds great!

---

From: Corey Huinker <[email-masked]>
Date: Wed, 10 Dec 2025 16:09:24 -0500

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

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

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

---

From: Robert Haas <[email-masked]>
Date: Wed, 10 Dec 2025 16:29:50 -0500

On Wed, Dec 10, 2025 at 4:09 PM Corey Huinker <[email-masked]> 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-grained regression 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.

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

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.

---

From: Robert Haas <[email-masked]>
Date: Thu, 11 Dec 2025 10:09:47 -0500

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

---

From: Jacob Champion <[email-masked]>
Date: Thu, 11 Dec 2025 17:11:09 -0800

On Tue, Dec 9, 2025 at 11:46 AM Robert Haas <[email-masked]> wrote:
> By the way, if your fuzzer can also

Sure! (They'll need to be golfed down.) Here are three entries that
hit the crash, each on its own line:

> join_order(qoe((nested_l oindex_scanp_plain))se(nested_loop_plain)nested_loo/_pseq_scanlain)
> join_order(qoe((nested_loop_plain))se(nested_loop_plain)nesemij/insted_loop_plain)
> gather(gather(gar(g/ther0))gtaher(gathethga))

Something the fuzzer really likes is zero-length identifiers ("").
Maybe that's by design, but I thought I'd mention it since the
standard lexer doesn't allow that and syntax.sql doesn't exercise it.

> before the call to pgpa_make_collected_advice() if that helps.

With USE_ASSERT_CHECKING, that should help, but I'm not sure if it
does without. (I could have sworn there was a conversation about that
at some point but I can't remember any of the keywords.) Could also
just make a dummy assignment. Or tag pg_plan_advice_dsa_area() with
__attribute__((returns_nonnull)), but that's more portability work.

--Jacob

---

From: Robert Haas <[email-masked]>
Date: Fri, 12 Dec 2025 12:36:17 -0500

> > gather(gather(gar(g/ther0))gtaher(gathethga))

At least for me, setting pg_plan_advice.advice to any of these strings
does not provoke a crash. What I discovered after a bit of
experimentation is that you get the crash if you (a) set the string to
something like this and then (b) run an EXPLAIN. Turns out, I already
had a test in syntax.sql that is sufficient to provoke the crash, so,
locally, I've added 'EXPLAIN SELECT 1' after each test case in
syntax.sql that is expected to successfully alter the value of the
GUC.

> standard lexer doesn't allow that and syntax.sql doesn't exercise it.

That's not by design. I've added a matching error check locally.

> __attribute__((returns_nonnull)), but that's more portability work.

As in initialize ca_pointer to InvalidDsaPointer?

---

From: Jacob Champion <[email-masked]>
Date: Fri, 12 Dec 2025 10:09:44 -0800

On Fri, Dec 12, 2025 at 9:36 AM Robert Haas <[email-masked]> wrote:
> something like this and then (b) run an EXPLAIN.

Makes sense (this fuzzer was exercising pgpa_format_advice_target()).

> As in initialize ca_pointer to InvalidDsaPointer?

Yeah.

Next bit of fuzzer feedback: I need the following diff in
pgpa_trove_add_to_hash() to avoid a crash when the hashtable starts to
fill up:

>     element = pgpa_trove_entry_insert(hash, key, &found);
> +   if (!found)
> +       element->indexes = NULL;
>     element->indexes = bms_add_member(element->indexes, index);

The advice string that triggered this is horrific, but I can send it
to you offline if you're morbidly curious. (I can spend time to
minimize it or I can get more fuzzer coverage, and I'd rather do the
latter right now :D)

--Jacob

---

From: Ajay Pal <[email-masked]>
Date: Mon, 15 Dec 2025 12:00:43 +0530

During further testing of the plan_advice patch's latest version, I
observed that the following query is generating a no_gather plan. This
specific plan structure is not being accepted by the query planner.

postgres=*# set local pg_plan_advice.advice='NO_GATHER("*RESULT*")';
SET
postgres=*# explain ( plan_advice) SELECT
CAST('99999999999999999999999999999999999999999999999999' AS NUMERIC);
                QUERY PLAN
-------------------------------------------
 Result  (cost=0.00..0.01 rows=1 width=32)
 Supplied Plan Advice:
   NO_GATHER("*RESULT*") /* not matched */
 Generated Plan Advice:
   NO_GATHER("*RESULT*")
(5 rows)

Thanks
Ajay

---

From: Jacob Champion <[email-masked]>
Date: Mon, 15 Dec 2025 08:37:43 -0800

On Fri, Dec 12, 2025 at 10:09 AM Jacob Champion
<[email-masked]> 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
diff --git a/contrib/pg_plan_advice/sql/gather.sql b/contrib/pg_plan_advice/sql/gather.sql
index 58280043913..cb04ed5cf30 100644
--- a/contrib/pg_plan_advice/sql/gather.sql
+++ b/contrib/pg_plan_advice/sql/gather.sql
@@ -38,6 +38,9 @@ SET LOCAL pg_plan_advice.advice = 'gather_merge(f d)';
 EXPLAIN (COSTS OFF, PLAN_ADVICE)
 	SELECT * FROM gt_fact f JOIN gt_dim d ON f.dim_id = d.id ORDER BY d.id;
 SET LOCAL pg_plan_advice.advice = 'gather(f d)';
+EXPLAIN (COSTS OFF, PLAN_ADVICE)
+	SELECT * FROM gt_fact f JOIN gt_dim d ON f.dim_id = d.id ORDER BY d.id;
+SET LOCAL pg_plan_advice.advice = 'gather((d d/d.d))';
 EXPLAIN (COSTS OFF, PLAN_ADVICE)
 	SELECT * FROM gt_fact f JOIN gt_dim d ON f.dim_id = d.id ORDER BY d.id;
 COMMIT;

---

From: Robert Haas <[email-masked]>
Date: Mon, 15 Dec 2025 15:06:08 -0500

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,

---

From: Jakub Wartak <[email-masked]>
Date: Wed, 17 Dec 2025 11:12:40 +0100

[..]

OK, so I've tested today from Your's branch directly, so I hope that
was also v7. Given the following q20 query:

SELECT s_name, s_address
FROM supplier, nation
WHERE s_suppkey in
    (SELECT ps_suppkey
     FROM partsupp
     WHERE ps_partkey in
         (SELECT p_partkey
          FROM part
          WHERE p_name LIKE 'forest%' )
       AND ps_availqty >
         (SELECT 0.5 * sum(l_quantity)
          FROM lineitem
          WHERE l_partkey = ps_partkey
            AND l_suppkey = ps_suppkey
            AND l_shipdate >= DATE '1994-01-01'
            AND l_shipdate < DATE '1994-01-01' + INTERVAL '1' year ) )
  AND s_nationkey = n_nationkey
  AND n_name = 'CANADA'
ORDER BY s_name;

in normal conditions (w/o advice) the above query generates:

 Sort  (cost=1010985030.44..1010985030.59 rows=61 width=51)
   Sort Key: supplier.s_name
   ->  Nested Loop  (cost=0.42..1010985028.63 rows=61 width=51)
         Join Filter: (nation.n_nationkey = supplier.s_nationkey)
         ->  Seq Scan on nation  (cost=0.00..1.31 rows=1 width=4)
               Filter: (n_name = 'CANADA'::bpchar)
         ->  Nested Loop Semi Join  (cost=0.42..1010985008.29
rows=1522 width=55)
               Join Filter: (partsupp.ps_suppkey = supplier.s_suppkey)
               ->  Seq Scan on supplier  (cost=0.00..249.30 rows=7730 width=59)
               ->  Materialize  (cost=0.42..1010755994.57 rows=1973 width=4)
                     ->  Nested Loop  (cost=0.42..1010755984.71
rows=1973 width=4)
                           ->  Seq Scan on part  (cost=0.00..4842.25
rows=1469 width=4)
                                 Filter: ((p_name)::text ~~ 'forest%'::text)
                           ->  Index Scan using pk_partsupp on
partsupp  (cost=0.42..688053.87 rows=1 width=8)
                                 Index Cond: (ps_partkey = part.p_partkey)
                                 Filter: ((ps_availqty)::numeric >
(SubPlan expr_1))
                                 SubPlan expr_1
                                   ->  Aggregate
(cost=172009.42..172009.44 rows=1 width=32)
                                         ->  Seq Scan on lineitem
(cost=0.00..172009.42 rows=1 width=5)
                                               Filter: ((l_shipdate >=
'1994-01-01'::date) AND (l_shipdate < '1995-01-01 00:00:00'::timestamp
without time zone) AND (l_partkey = partsupp.ps_partkey) AND
(l_suppkey = partsupp.ps_suppkey))

 Generated Plan Advice:
   JOIN_ORDER(nation (supplier (part partsupp)))
   NESTED_LOOP_PLAIN(partsupp partsupp) <--- [X]
   NESTED_LOOP_MATERIALIZE(partsupp)
   SEQ_SCAN(nation supplier part lineitem@expr_1)
   INDEX_SCAN(partsupp public.pk_partsupp)
   SEMIJOIN_NON_UNIQUE((partsupp part))
   NO_GATHER(supplier nation partsupp part lineitem@expr_1)

Please see the - I think it's confusing? -
NESTED_LOOP_MATERIALIZE(partsupp partsupp) - that's 2x the same
string? This causes it to turn into below plan -- I've marked the
problem with [X]

 Sort  (cost=50035755.50..50035755.66 rows=61 width=51)
   Sort Key: supplier.s_name
   ->  Nested Loop  (cost=12562154.32..50035753.70 rows=61 width=51)
         Join Filter: (nation.n_nationkey = supplier.s_nationkey)
         ->  Seq Scan on nation  (cost=0.00..1.31 rows=1 width=4)
               Filter: (n_name = 'CANADA'::bpchar)
         ->  Nested Loop Semi Join  (cost=12562154.32..50035733.36
rows=1522 width=55)
             [X] -- missing Join Filter here
               ->  Seq Scan on supplier  (cost=0.00..249.30 rows=7730 width=59)
               [X] -- HJ instead of Materialize+Nested Loop below:
               ->  Hash Join  (cost=12562154.32..12567002.09 rows=1 width=4)
                     Hash Cond: (part.p_partkey = partsupp.ps_partkey)
                     ->  Seq Scan on part  (cost=0.00..4842.25
rows=1469 width=4)
                           Filter: ((p_name)::text ~~ 'forest%'::text)
                     ->  Hash  (cost=12562154.02..12562154.02 rows=24 width=8)
                           ->  Index Scan using pk_partsupp on
partsupp  (cost=0.42..12562154.02 rows=24 width=8)
                                 [X] -- wrong Index Cond below
(suppkey instead of partkey)
                                 Index Cond: (ps_suppkey = supplier.s_suppkey)
                                 Filter: ((ps_availqty)::numeric >
(SubPlan expr_1))
                                 SubPlan expr_1
                                   ->  Aggregate
(cost=172009.42..172009.44 rows=1 width=32)
                                         ->  Seq Scan on lineitem
(cost=0.00..172009.42 rows=1 width=5)
                                               Filter: ((l_shipdate >=
'1994-01-01'::date) AND (l_shipdate < '1995-01-01 00:00:00'::timestamp
without time zone) AND (l_partkey = partsupp.ps_partkey) AND
(l_suppkey = partsupp.ps_suppkey))

Supplied Plan Advice:
   SEQ_SCAN(nation) /* matched */
   SEQ_SCAN(supplier) /* matched */
   SEQ_SCAN(part) /* matched */
   SEQ_SCAN(lineitem@expr_1) /* matched */
   INDEX_SCAN(partsupp public.pk_partsupp) /* matched */
   JOIN_ORDER(nation (supplier (part partsupp))) /* matched, conflicting */
   NESTED_LOOP_PLAIN(partsupp) /* matched, conflicting */
   NESTED_LOOP_PLAIN(partsupp) /* matched, conflicting */
   NESTED_LOOP_MATERIALIZE(partsupp) /* matched, conflicting, failed */
   SEMIJOIN_NON_UNIQUE((partsupp part)) /* matched, conflicting */
   NO_GATHER(supplier) /* matched */
   NO_GATHER(nation) /* matched */
   NO_GATHER(partsupp) /* matched */
   NO_GATHER(part) /* matched */
   NO_GATHER(lineitem@expr_1) /* matched */

So the difference is basically between:
    set pg_plan_advice.advice = '[..] NESTED_LOOP_PLAIN(partsupp
partsupp) NESTED_LOOP_MATERIALIZE(partsupp) [..]';
which causes wrong plan and outcome:
    NESTED_LOOP_MATERIALIZE(partsupp) /* matched, conflicting, failed */

and apparently proper advice like below which has better yield:
    set pg_plan_advice.advice = '[..] NESTED_LOOP_PLAIN(part partsupp)
NESTED_LOOP_MATERIALIZE(partsupp) [..]';
which is not generated , but caused good plan, however it also prints:
   NESTED_LOOP_PLAIN(part) /* matched, conflicting, failed */
   NESTED_LOOP_MATERIALIZE(partsupp) /* matched, conflicting */
but that seems "failed" there, seems to be untrue?

Another idea is perhaps, we could have some elog(WARNING) - but not
Asserts() - in assert-only enabled build that could alert us in case
of duplicated entries being detected for the same ops in
pg_plan_advice_explain_feedback()?

-J.

---

From: Jakub Wartak <[email-masked]>
Date: Wed, 17 Dec 2025 14:44:02 +0100

On Wed, Dec 17, 2025 at 11:12 AM Jakub Wartak
<[email-masked]> wrote:
>[..q20..]

OK, now for the q10:

 Sort
   Sort Key: (sum((lineitem.l_extendedprice * ('1'::numeric -
lineitem.l_discount)))) DESC
   ->  Finalize GroupAggregate
         Group Key: customer.c_custkey, nation.n_name
         ->  Gather Merge
               Workers Planned: 2
               ->  Partial GroupAggregate
                     Group Key: customer.c_custkey, nation.n_name
                     ->  Sort
                           Sort Key: customer.c_custkey, nation.n_name
                           ->  Hash Join
                                 Hash Cond: (customer.c_nationkey =
nation.n_nationkey)
                                 ->  Parallel Hash Join
                                       Hash Cond: (orders.o_custkey =
customer.c_custkey)
                                       ->  Nested Loop
                                             ->  Parallel Seq Scan on orders
                                                   Filter:
((o_orderdate >= '1993-10-01'::date) AND (o_orderdate < '1994-01-01
00:00:00'::timestamp without time zone))
                                             ->  Index Scan using
lineitem_l_orderkey_idx_l_returnflag on lineitem
                                                   Index Cond:
(l_orderkey = orders.o_orderkey)
                                       ->  Parallel Hash
                                             ->  Parallel Seq Scan on customer
                                 ->  Hash
                                       ->  Seq Scan on nation
 Generated Plan Advice:
   JOIN_ORDER(orders lineitem customer nation)
   NESTED_LOOP_PLAIN(lineitem)
   HASH_JOIN(customer nation)
   SEQ_SCAN(orders customer nation)
   INDEX_SCAN(lineitem public.lineitem_l_orderkey_idx_l_returnflag)
   GATHER_MERGE((customer orders lineitem nation))

but when set the advice it generates wrong NL instead of expected
Parallel HJ (so another way to fix is to simply disable PQ, yuck),
but:

 Sort
   Sort Key: (sum((lineitem.l_extendedprice * ('1'::numeric -
lineitem.l_discount)))) DESC
   ->  Finalize GroupAggregate
         Group Key: customer.c_custkey, nation.n_name
         ->  Gather Merge
               Workers Planned: 2
               ->  Partial GroupAggregate
                     Group Key: customer.c_custkey, nation.n_name
                     ->  Sort
                           Sort Key: customer.c_custkey, nation.n_name
                           ->  Nested Loop
                                 ->  Hash Join
                                       Hash Cond:
(customer.c_nationkey = nation.n_nationkey)
                                       ->  Parallel Hash Join
                                             Hash Cond:
(orders.o_custkey = customer.c_custkey)
                                             ->  Parallel Seq Scan on orders
                                                   Filter:
((o_orderdate >= '1993-10-01'::date) AND (o_orderdate < '1994-01-01
00:00:00'::timestamp without time zone))
                                             ->  Parallel Hash
                                                   ->  Parallel Seq
Scan on customer
                                       ->  Hash
                                             ->  Seq Scan on nation
                                 ->  Index Scan using
lineitem_l_orderkey_idx_l_returnflag on lineitem
                                       Index Cond: (l_orderkey =
orders.o_orderkey)
 Supplied Plan Advice:
   SEQ_SCAN(orders) /* matched */
   SEQ_SCAN(customer) /* matched */
   SEQ_SCAN(nation) /* matched */
   INDEX_SCAN(lineitem public.lineitem_l_orderkey_idx_l_returnflag) /*
matched */
   JOIN_ORDER(orders lineitem customer nation) /* matched,
conflicting, failed */
   NESTED_LOOP_PLAIN(lineitem) /* matched, conflicting */
   HASH_JOIN(customer) /* matched, conflicting */
   HASH_JOIN(nation) /* matched, conflicting */
   GATHER_MERGE((customer orders lineitem nation)) /* matched */

So to me it looks like in Generated Plan Advice we:
- have proper HASH_JOIN(customer nation)
- but it somehow forgot to include "HASH_JOIN(orders)" to cover for
that Parallel Hash Join on (orders.o_custkey = customer.c_custkey)
with input from NL. After adding that manually, it achieves the same
input plan properly.

Please let me know if I'm wrong, I was kind of thinking Parallel is
not fully supported, but README/tests seem to state otherwise.

-J.

---

From: Jakub Wartak <[email-masked]>
Date: Thu, 18 Dec 2025 13:27:27 +0100

> > On Mon, Dec 15, 2025 at 9:06 PM Robert Haas <[email-masked]> wrote:

Hi, this is a follow-up just to the q10.

[..]

Well, it's quite a ride with the Q10 and I partially wrong with above:

0. The reported earlier wrong missing "HASH_JOIN(orders customer)" -
that part was okay
1. The Incremental Sort is being used in the original plan, but is
still IS not reflected in the generated advice.
2a. I've noticed Memoize/Index Scan was not being respected for "nation"
2b. Seq scan for nation was being done for "nation"

So total modification list, I've ended up doing (+ for adding , - for removing):

+ HASH_JOIN(orders customer) -- from earlier reply
+ NESTED_LOOP_MEMOIZE(nation)
+ INDEX_SCAN(nation public.pk_nation)
- HASH_JOIN(customer nation) -- as it was we were having NL() in org plan
SEQ_SCAN(orders customer nation) ==> SEQ_SCAN(orders customer)

In full the best shape seems to be Q10 with pg_plan_advice.advice =
'HASH_JOIN(orders customer) JOIN_ORDER(orders lineitem customer
nation)    NESTED_LOOP_PLAIN(lineitem)    SEQ_SCAN(orders customer)
INDEX_SCAN(lineitem public.lineitem_l_orderkey_idx_l_returnflag)
GATHER_MERGE((customer orders lineitem nation))
NESTED_LOOP_MEMOIZE(nation)';

which yields:
 Sort
   Sort Key: (sum((lineitem.l_extendedprice * ('1'::numeric -
lineitem.l_discount)))) DESC
   ->  GroupAggregate
         Group Key: customer.c_custkey, nation.n_name
         ->  Gather Merge
               Workers Planned: 2
               ->  Sort
                     Sort Key: customer.c_custkey, nation.n_name
                     ->  Nested Loop
                           ->  Parallel Hash Join
                                 Hash Cond: (orders.o_custkey =
customer.c_custkey)
                                 ->  Nested Loop
                                       ->  Parallel Seq Scan on orders
                                             Filter: ((o_orderdate >=
'1993-10-01'::date) AND (o_orderdate < '1994-01-01
00:00:00'::timestamp without time zone))
                                       ->  Index Scan using
lineitem_l_orderkey_idx_l_returnflag on lineitem
                                             Index Cond: (l_orderkey =
orders.o_orderkey)
                                 ->  Parallel Hash
                                       ->  Parallel Seq Scan on customer
                           ->  Memoize
                                 Cache Key: customer.c_nationkey
                                 Cache Mode: logical
                                 ->  Index Scan using pk_nation on nation
                                       Index Cond: (n_nationkey =
customer.c_nationkey)

but that Incremental Sort *is* still missing. In original plan we are doing
   Incremental Sort (Sort Key: customer.c_custkey, nation.n_name,
Presorted Key: customer.c_custkey)
   <-- .... Sort(Sort Key: customer.c_custkey)

However, even with my overrides I haven't found an immediately obvious
way to force it to use Incremental Sort on a specific field, so it
just sorts on two at once. Maybe it's something that should be
expressed through GATHER_MERGE()?, but that's not obvious how and
where. In terms of raw performance , it seems to be very similiar
(98ms +/- 8ms even between those two).

-J.

---

From: Robert Haas <[email-masked]>
Date: Thu, 18 Dec 2025 08:36:44 -0500

>    NO_GATHER(supplier nation partsupp part lineitem@expr_1)

Yeah, that's not right. There are three nested loops here, so we
should have three pieces of nested loop advice.
NESTED_LOOP_MATERIALIZE(partsupp) covers the innermost nested loop.
The other two are NESTED_LOOP_PLAIN, but the advice should cover all
the tables on the inner side of the join. I think it should read:

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

Ordering isn't significant here, so NESTED_LOOP_PLAIN((part supplier
partsupp) (partsupp part)) would be logically equivalent. Doesn't
matter exactly what we output here, but it shouldn't be just partsupp.

>     set pg_plan_advice.advice = '[..] NESTED_LOOP_PLAIN(part partsupp)

This isn't quite what you want, because this says that part should be
on the outer side of a NESTED_LOOP_PLAIN by itself and partsupp should
also be on the outer side of a NESTED_LOOP_PLAIN by itself. You need
the extra set of parentheses to indicate that the join product of
those two tables should be on the outer side of a NESTED_LOOP_PLAIN,
rather than each table individually.

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.

---

From: Robert Haas <[email-masked]>
Date: Thu, 18 Dec 2025 15:39:13 -0500

>    GATHER_MERGE((customer orders lineitem nation))

This looks correct to me.

> Parallel HJ (so another way to fix is to simply disable PQ, yuck),

This is obviously bad. I'm not quite sure what happened here, but my
guess is that something prevented the JOIN_ORDER advice from being
applied cleanly and then everything went downhill from there. I wonder
if JOIN_ORDER doesn't interact properly with incremental sorts --
that's a situation for which I don't think I have existing test
coverage.

> with input from NL. After adding that manually, it achieves the same

The first table in the JOIN_ORDER() specification isn't supposed to
have a join method specification, because the join method specifier
says what appears on the inner, i.e. second, arm of the join.

---

From: Haibo Yan <[email-masked]>
Date: Mon, 29 Dec 2025 15:33:53 -0800

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

---

From: Lukas Fittl <[email-masked]>
Date: Mon, 29 Dec 2025 17:15:18 -0800

> 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: "David G. Johnston" <[email-masked]>
Date: Sun, 1 Mar 2026 21:10:52 -0700

On Fri, Feb 27, 2026 at 6:16 PM David G. Johnston <
[email-masked]> wrote:

> On Fri, Feb 27, 2026 at 3:46 PM Robert Haas <[email-masked]> wrote:
>
>> On Thu, Feb 26, 2026 at 8:55 AM Robert Haas <[email-masked]>
>> wrote:
>> > Thanks, Alex, for the review.
>>
>> Here's v18. In addition to fixing the problems pointed out by Alex,
>> there are a couple of significant changes in this version.
>>
>>
> I have a mind to walk through the readmes and sgmls but its going to be in
> chunks.  Here's one for the readme for pg_plan_advice with a couple of
> preliminary sgml changes.
>
>
0003 sgml focus with some readme.

There is an inconsistency between readme and sgml regarding the "join
(strategy|method) advice" label.

The wording for partitionwise is better in the readme than the sgml.

I did make some bulkier suggestions - they do not contain proper markup.

There may be some repeated suggestions from my previous review - I didn't
try to match up what you did and did not take in.

I re-ordered semijoin to be alphabetical - which also had the benefit of
matching the layout of the paragraph.  Flipping the order of "former" and
"latter" is quite intentional.

I defined what "successfully enforced" means in the emit warning GUC.  That
was my unresearched guess after reading how "failed" behaves.

I found "negative join order constraint" challenging to parse.  I tried to
word it more like what is done in the readme.

I don't know if this conflicts with my previous diff of the same patch.  A
couple of overlap spots possibly but they were largely independent (readme
then, sgml now).

David J.
diff --git a/contrib/pg_plan_advice/README b/contrib/pg_plan_advice/README
index 0b888fd82f2..441bd79da50 100644
--- a/contrib/pg_plan_advice/README
+++ b/contrib/pg_plan_advice/README
@@ -141,7 +141,7 @@ side and some kind of join between t2 and t3 on the inner side, but without
 saying how that join must be performed or anything about which relation should
 appear on which side of the join, or even whether this kind of join has sides.
 
-Join Strategy Advice
+Join Method Advice
 ====================
 
 Tags such as NESTED_LOOP_PLAIN specify the method that should be used to
@@ -151,10 +151,10 @@ side of a plain nested loop (one without materialization or memoization)
 and that it should also put a join between the relation whose identifier is
 "y" and the relation whose identifier is "z" on the inner side of a nested
 loop. Hence, for an N-table join problem, there will be N-1 pieces of join
-strategy advice; no join strategy advice is required for the outermost
+method advice; no join method advice is required for the outermost
 table in the join problem.
 
-Considering that we have both join order advice and join strategy advice,
+Considering that we have both join order advice and join method advice,
 it might seem natural to say that NESTED_LOOP_PLAIN(x) should be redefined
 to mean that x should appear by itself on one side or the other of a nested
 loop, rather than specifically on the inner side, but this definition appears
@@ -166,13 +166,13 @@ join, the two sides are treated very differently and saying that a certain
 relation should be involved in one of those operations without saying which
 role it should take isn't saying much.
 
-This choice of definition implies that join strategy advice also imposes some
+This choice of definition implies that join method advice also imposes some
 join order constraints. For example, given a join between foo and bar,
 HASH_JOIN(bar) implies that foo is the driving table. Otherwise, it would
 be impossible to put bar beneath the inner side of a Hash Join.
 
 Note that, given this definition, it's reasonable to consider deleting the
-join order advice but applying the join strategy advice. For example,
+join order advice but applying the join method advice. For example,
 consider a star schema with tables fact, dim1, dim2, dim3, dim4, and dim5.
 The automatically generated advice might specify JOIN_ORDER(fact dim1 dim3
 dim4 dim2 dim5) HASH_JOIN(dim2 dim4) NESTED_LOOP_PLAIN(dim1 dim3 dim5).
@@ -218,6 +218,9 @@ in the query, specifying just PARTITIONWISE((t1 t2)) would have the same
 effect, since there would be no other rels to which t3 could be joined in
 a partitionwise fashion.
 
+Note that, regardless of what advice is specified, no partitionwise joins
+will be possible if enable_partitionwise_join = off.
+
 Parallel Query (Gather, etc.)
 =============================
 
diff --git a/doc/src/sgml/pgplanadvice.sgml b/doc/src/sgml/pgplanadvice.sgml
index 817bafa21c6..305f3890ed8 100644
--- a/doc/src/sgml/pgplanadvice.sgml
+++ b/doc/src/sgml/pgplanadvice.sgml
@@ -12,12 +12,12 @@
   to be described, reproduced, and altered using a special-purpose "plan
   advice" mini-language. It is intended to allow stabilization of plan choices
   that the user believes to be good, as well as experimentation with plans that
-  the planner believes to be non-optimal.
+  the planner believes to be suboptimal.
  </para>
 
  <para>
   Note that, since the planner often makes good decisions, overriding its
-  judgement can easily backfire. For example, if the distribution of the
+  judgment can easily backfire. For example, if the distribution of the
   underlying data changes, the planner normally has the option to adjust the
   plan in an attempt to preserve good performance. If the plan advice prevents
   this, a very poor plan may be chosen. It is important to use plan advice
@@ -29,9 +29,8 @@
   <title>Getting Started</title>
 
   <para>
-   In order to use this module, the <literal>pg_plan_advice</literal> module
-   must be loaded. You can do this on a system-wide basis by adding
-   <literal>pg_plan_advice</literal> to
+   This module must first be loaded by adding
+   <literal>pg_plan_advice</literal> either to
    <xref linkend="guc-shared-preload-libraries"/> and restarting the
    server, or by adding it to
    <xref linkend="guc-session-preload-libraries"/> and starting a new session,
@@ -43,7 +42,7 @@
    Once the <literal>pg_plan_advice</literal> module is loaded,
    <link linkend="sql-explain"><literal>EXPLAIN</literal></link> will support
    a <literal>PLAN_ADVICE</literal> option. You can use this option to see
-   a plan advice string for the chosen plan. For example:
+   the plan advice string for the chosen plan. For example:
   </para>
 
 <programlisting>
@@ -133,15 +132,15 @@ EXPLAIN (COSTS OFF)
   <para>
    Plan advice is written imperatively; that is, it specifies what should be
    done. However, at an implementation level,
-   <literal>pg_plan_advice</literal> works by telling the core planner what
+   <literal>pg_plan_advice</literal> works by telling the planner what
    should not be done. In other words, it operates by constraining the
    planner's choices, not by replacing it. Therefore, no matter what advice
-   you provide, you will only ever get a plan that the core planner would have
+   you provide, you will only ever get a plan that the planner would have
    considered for the query in question. If you attempt to force what you
    believe to be the correct plan by supplying an advice string, and the
    planner still fails to produce the desired plan, this means that either
    there is a bug in your advice string, or the plan in question was not
-   considered viable by the core planner. This commonly happens for one of two
+   considered viable by the planner. This commonly happens for one of two
    reasons. First, it might be that the planner believes that the plan you're
    trying to force would be semantically incorrect - that is, it would produce
    the wrong results - and for that reason it wasn't considered. Second, it
@@ -200,7 +199,7 @@ EXPLAIN (COSTS OFF)
 
   <para>
    An <firstterm>advice target</firstterm> uniquely identifies a particular
-   instance of a particular relation involved in a particular query. In simple
+   instance of a particular relation involved in a particular (sub)plan. In simple
    cases, such as the examples shown above, the advice target is simply the
    relation alias. However, a more complex syntax is required when subqueries
    are used, when tables are partitioned, or when the same relation alias is
@@ -237,8 +236,8 @@ alias_name#occurrence_number/partition_schema.partition_name@plan_name
    and uses the parent query's subplan name (or no subplan name, if pulled up
    to the top level). Furthermore, the correct subquery name is sometimes not
    obvious. For example, when two queries are joined using an operation such as
-   <literal>UNION</literal> or <literal>INTERSECT</literal>, no name for the
-   subqueries is present in the SQL syntax; instead, a system-generated name is
+   <literal>UNION</literal> or <literal>INTERSECT</literal>, no names for the
+   subqueries are present in the SQL syntax; instead, a system-generated name is
    assigned to each branch. The easiest way to discover the proper advice
    targets is to use <literal>EXPLAIN (PLAN_ADVICE)</literal> and examine the
    generated advice.
@@ -362,7 +361,7 @@ Join
 
    <para>
     Parenthesized sublists can be arbitrarily nested, but sublists surrounded
-    by curly braces cannot themselves contain sublists.
+    by curly braces cannot contain sublists.
    </para>
 
    <para>
@@ -371,7 +370,7 @@ Join
     are multiple join problems that are optimized separately by the planner.
     This can happen due to the presence of subqueries, or because there is a
     partitionwise join. In the latter case, each branch of the partitionwise
-    join can have its own join order, independent of every other branch.
+    join can have its own join order.
    </para>
 
   </sect3>
@@ -405,10 +404,11 @@ join_method_name(<replaceable>join_method_item</replaceable> [ ... ])
    </para>
 
    <para>
-    Note that join method advice implies a negative join order constraint.
+    Note that join method advice necessarily constrains the available join orders.
     Since the named relation or relations must be on the inner side of a join
     using the specified method, none of them can be the driving table for the
-    entire join problem. Moreover, no relation inside the set should be joined
+    entire join problem (i.e., JOIN_ORDER(a b) and HASH_JOIN(a) are conflicting).
+    Moreover, no relation inside the set should be joined
     to any relation outside the set until all relations within the set have
     been joined to each other. For example, if the advice specifies
     <literal>HASH_JOIN((a b))</literal> and the system begins by joining either
@@ -416,7 +416,7 @@ join_method_name(<replaceable>join_method_item</replaceable> [ ... ])
     plan could never be compliant with the request to put exactly those two
     relations on the inner side of a hash join. When using both join order
     advice and join method advice for the same query, it is a good idea to make
-    sure that they do not mandate incompatible join orders.
+    sure that they do not mandate conflicting join orders.
    </para>
 
   </sect3>
@@ -432,13 +432,16 @@ PARTITIONWISE(<replaceable>partitionwise_item</replaceable> [ ... ])
 ( <replaceable>advice_target</replaceable> [ ... ] ) }</synopsis>
 
    <para>
-    When applied to a single target, <literal>PARTITIONWISE</literal>
-    specifies that the specified table should not be part of any partitionwise
-    join. When applied to a list of targets, <literal>PARTITIONWISE</literal>
-    specifies that exactly that set of relations should be joined in
-    partitionwise fashion. Note that, regardless of what advice is specified,
-    no partitionwise joins will be possible if
-    <literal>enable_partitionwise_join = off</literal>.
+    PARTITIONWISE() advise can be used to specify both those partitionwise joins
+    which should be performed and those which should not be performed; the idea
+    is that each argument to PARTITIONWISE specifies a set of relations that
+    should be scanned partitionwise after being joined to each other and nothing
+    else. Hence, for example, PARTITIONWISE((t1 t2) t3) specifies that the
+    query should contain a partitionwise join between t1 and t2 and that t3
+    should not be part of any partitionwise join. If there are no other rels
+    in the query, specifying just PARTITIONWISE((t1 t2)) would have the same
+    effect, since there would be no other rels to which t3 could be joined in
+    a partitionwise fashion.
    </para>
 
   </sect3>
@@ -446,8 +449,8 @@ PARTITIONWISE(<replaceable>partitionwise_item</replaceable> [ ... ])
   <sect3 id="pgplanadvice-semijoin-unique">
   <title>Semijoin Uniqueness Advice</title>
    <synopsis>
-SEMIJOIN_UNIQUE(<replaceable>sj_unique_item</replaceable> [ ... ])
 SEMIJOIN_NON_UNIQUE(<replaceable>sj_unique_item</replaceable> [ ... ])
+SEMIJOIN_UNIQUE(<replaceable>sj_unique_item</replaceable> [ ... ])
 
 <phrase>where <replaceable>sj_unique_item</replaceable> is:</phrase>
 
@@ -457,9 +460,9 @@ SEMIJOIN_NON_UNIQUE(<replaceable>sj_unique_item</replaceable> [ ... ])
    <para>
     The planner sometimes has a choice between implementing a semijoin
     directly and implementing a semijoin by making the nullable side unique
-    and then performing an inner join. <literal>SEMIJOIN_UNIQUE</literal>
-    specifies the latter strategy, while <literal>SEMIJOIN_NON_UNIQUE</literal>
-    specifies the former strategy. In either case, the argument is the single
+    and then performing an inner join. <literal>SEMIJOIN_NON_UNIQUE</literal>
+    specifies the former strategy, while <literal>SEMIJOIN_UNIQUE</literal>
+    specifies the latter strategy. In either case, the argument is the single
     relation or list of relations that appear beneath the nullable side of the
     join.
    </para>
@@ -545,11 +548,11 @@ NO_GATHER(<replaceable>advice_target</replaceable> [ ... ])
 
 <programlisting>
 SET pg_plan_advice.advice = 'hash_join(f g) join_order(f g) index_scan(f no_such_index)';
-SET
-rhaas=# EXPLAIN (COSTS OFF)                                                     SELECT * FROM jo_fact f
-        LEFT JOIN jo_dim1 d1 ON f.dim1_id = d1.id
-        LEFT JOIN jo_dim2 d2 ON f.dim2_id = d2.id
-        WHERE val1 = 1 AND val2 = 1;
+EXPLAIN (COSTS OFF) 
+        SELECT * FROM jo_fact f
+            LEFT JOIN jo_dim1 d1 ON f.dim1_id = d1.id
+            LEFT JOIN jo_dim2 d2 ON f.dim2_id = d2.id
+            WHERE val1 = 1 AND val2 = 1;
                             QUERY PLAN
 -------------------------------------------------------------------
  Hash Join
@@ -577,12 +580,16 @@ rhaas=# EXPLAIN (COSTS OFF)
    on the inner side of a hash join is listed as
    <literal>not matched</literal>. The <literal>JOIN_ORDER</literal> advice
    tag involves one valid target and one invalid target, and so is listed as
-   <literal>partially matched</literal>. Note that
-   <literal>HASH_JOIN(f g)</literal> is actually a request for two logically
-   separate behaviors, whereas <literal>JOIN_ORDER(f g)</literal> is a single
-   request. When providing advice feedback, <literal>EXPLAIN</literal> shows
-   each logical request separately, together with all the feedback applicable
-   to that request type.
+   <literal>partially matched</literal>.
+  </para>
+
+  <para> 
+   Feedback advice is normalized into logical directives. Note, in the example,
+   that the <literal>HASH_JOIN(f g)</literal> join method advice was decomposed
+   into its two logically separate behaviors, <literal>HASH_JOIN(f)</literal>
+   and <literal>HASH_JOIN(g)</literal>, in the feedback. Since
+   <literal>JOIN_ORDER(f g)</literal> is a single logical request it appears
+   as-is in the feedback.
   </para>
 
   <para>
@@ -594,17 +601,17 @@ rhaas=# EXPLAIN (COSTS OFF)
    <listitem>
     <para>
      <literal>matched</literal> means that all of the specified advice targets
-     were observed together during query planning, at a time at which the
+     were observed together during query planning, at times when the
      advice could be enforced.
     </para>
    </listitem>
 
    <listitem>
     <para>
-     <literal>partially matched</literal> means that some but not all of the
-     specified advice targets were observed during query planning, or all
-     of the advice targets were observed but not together. For example, this
-     may happen if all the targets of <literal>JOIN_ORDER</literal> advice
+     <literal>partially matched</literal> means either that only some of the
+     advice targets were observed during query planning, or all
+     of the advice targets were observed but not at the right times. For example,
+     this may happen if all the targets of <literal>JOIN_ORDER</literal> advice
      individually match the query, but the proposed join order is not legal.
     </para>
    </listitem>
@@ -663,6 +670,15 @@ rhaas=# EXPLAIN (COSTS OFF)
    <literal>partially matched</literal>, or <literal>not matched</literal>.
   </para>
 
+  <para>
+   Thus, in the example above, INDEX_SCAN(f no_such_index) matched because the
+   only named target in the advice, f, is present in the query. The attempt to
+   locate no_such_index failed, marking the advice inapplicable. Since the plan
+   did not include an index scan, but matched the target, the additional failed
+   marking is applied.  Both partially matched and not matched imply a failed
+   outcome so the additional marking is omitted.
+  </para>
+
  </sect2>
 
  <sect2 id="pgplanadvice-config-params">
@@ -719,8 +735,8 @@ rhaas=# EXPLAIN (COSTS OFF)
      <para>
       <varname>pg_plan_advice.always_store_advice_details</varname> allows
       <literal>EXPLAIN</literal> to show details related to plan advice even
-      when prepared queries are used.  The default value is
-      <literal>false</literal>.  When planning a prepared query, it is not
+      when prepared queries are used. The default value is
+      <literal>false</literal>. When planning a prepared query, it is not
       possible to know whether <literal>EXPLAIN</literal> will later be used,
       so by default, to reduce overhead, <literal>pg_plan_advice</literal>
       will not generate plan advice or feedback on supplied advice. This means
@@ -747,6 +763,9 @@ rhaas=# EXPLAIN (COSTS OFF)
       emits a warning whenever supplied plan advice is not successfully
       enforced. The default value is <literal>false</literal>.
      </para>
+     <para>
+      Success means that all advice feedback is marked as matched, and not failed.
+     </para>
     </listitem>
    </varlistentry>

---

From: "David G. Johnston" <[email-masked]>
Date: Mon, 2 Mar 2026 15:09:12 -0700

On Sun, Mar 1, 2026 at 9:10 PM David G. Johnston <[email-masked]>
wrote:

>> I have a mind to walk through the readmes and sgmls but its going to be
>> in chunks.  Here's one for the readme for pg_plan_advice with a couple of
And now 0004 sgml (no readme):

My OCD wants these named pg_advice_{plan,collect,stash} so they sort
together.

Strongly thinking using "entries" throughout makes more sense than "query
texts and advice string" - it is shorter and more inclusive since the
actual stored info covers IDs and timestamp.

I made one swap where shared was being mentioned before local.

I added some unresearched answers to open questions I had at the end of the
main section.  Namely, pertaining to advice feedback output and capturing
explain plans themselves.

David J.
diff --git a/doc/src/sgml/pgcollectadvice.sgml b/doc/src/sgml/pgcollectadvice.sgml
index fd7d879d816..ba2837c3982 100644
--- a/doc/src/sgml/pgcollectadvice.sgml
+++ b/doc/src/sgml/pgcollectadvice.sgml
@@ -11,11 +11,16 @@
   The <filename>pg_collect_advice</filename> extension allows you to
   automatically generate plan advice each time a query is planned and store
   the query and the generated advice string either in local or shared memory.
+  Collection enablement and entries are described below, but importantly,
+  there is no deduplication involved; each planning event produces one entry,
+  and a mandatory cap on the number of entries allowed must be set before
+  enabling collection.
   Note that this extension requires the <xref linkend="pgplanadvice" /> module,
-  which performs the actual plan advice generation; this module only knows
-  how to store the generated advice for later examination. Whenever
-  <literal>pg_collect_advice</literal> is loaded, it will automatically load
-  <literal>pg_plan_advice</literal>.
+  which performs the actual plan advice generation; this module just handles
+  enabling automatic generation and performing the storage of the generated
+  advice for later examination through the extension's views.
+  Whenever <literal>pg_collect_advice</literal> is loaded, it will automatically
+  load <literal>pg_plan_advice</literal>.
  </para>
 
  <para>
@@ -30,12 +35,14 @@
  </para>
 
  <para>
-  <literal>pg_collect_advice</literal> includes both a shared advice
-  collector and a local advice collector. The local advice collector makes
-  queries and their advice strings visible only to the session where those
-  queries were planned, while the shared advice collector collects data
-  on a system-wide basis, and authorized users can examine data from all
-  sessions.
+  <literal>pg_collect_advice</literal> includes both a local advice
+  collector and a shared advice collector.
+  The local advice collector stores entries locally to the session where those
+  queries were planned and requires the extension to be installed within the
+  database the session is connected to.
+  The shared advice collector stores entries for all databases in the cluster,
+  and authorized users can examine entries from any session connected to a
+  database where the extension is installed.
  </para>
 
  <para>
@@ -67,14 +74,26 @@
  </para>
 
  <para>
-  In addition to the query texts and advice strings, the advice collectors
-  will also store the OID of the role that caused the query to be planned,
+  For each entry in a collector, in addition to the query texts and advice
+  strings, the advice collector will also store the OID of the role that caused the query to be planned,
   the OID of the database in which the query was planned, the query ID,
   and the time at which the collection occurred. This module does not
   automatically enable query ID computation; therefore, if you want the
-  query ID value to be populated in collected advice, be sure to configure
-  <literal>compute_query_id = on</literal>. Otherwise, the query ID may
-  always show as <literal>0</literal>.
+  query ID value to be populated in collected advice, be sure the setting
+  <literal>compute_query_id</literal> is set to <literal>on</literal>,
+  otherwise the query ID will appear as <literal>0</literal>.
+ </para>
+
+ <para>
+  The pg_plan_advice module also produces feedback; the collector
+  is unable to capture such feedback as it relies on running explain
+  with PLAN_ADVICE which does not happen during normal execution.
+ </para>
+
+ <para>
+  Plan advice is not a substitute for seeing a query plan. The auto_explain
+  module can be used (imprecisely) in conjunction with this module to add explain
+  output to the corpus of planner data automatically captured for executed queries.
  </para>
 
  <sect2 id="pgcollectadvice-functions">
@@ -92,8 +111,7 @@
 
     <listitem>
      <para>
-      Removes all collected query texts and advice strings from backend-local
-      memory.
+      Removes all collected entries from backend-local memory.
      </para>
     </listitem>
    </varlistentry>
@@ -110,8 +128,7 @@
 
     <listitem>
      <para>
-      Returns all query texts and advice strings stored in the local
-      advice collector.
+      Returns all entries stored in the local advice collector.
      </para>
     </listitem>
    </varlistentry>
@@ -126,8 +143,7 @@
 
     <listitem>
      <para>
-      Removes all collected query texts and advice strings from shared
-      memory.
+      Removes all collected entires from shared memory.
      </para>
     </listitem>
    </varlistentry>
@@ -144,8 +160,7 @@
 
     <listitem>
      <para>
-      Returns all query texts and advice strings stored in the shared
-      advice collector.
+      Returns all entries stored in the shared advice collector.
      </para>
     </listitem>
    </varlistentry>
@@ -170,7 +185,7 @@
     <listitem>
      <para>
       <varname>pg_collect_advice.local_collector</varname> enables the
-      local advice collector.  The default value is <literal>false</literal>.
+      local advice collector. The default value is <literal>false</literal>.
      </para>
     </listitem>
    </varlistentry>
@@ -186,8 +201,8 @@
     <listitem>
      <para>
       <varname>pg_collect_advice.local_collection_limit</varname> sets the
-      maximum number of query texts and advice strings retained by the
-      local advice collector.  The default value is <literal>0</literal>.
+      maximum number of entries retained by the local advice collector.
+      The default value is <literal>0</literal>.
      </para>
     </listitem>
    </varlistentry>
@@ -203,7 +218,7 @@
     <listitem>
      <para>
       <varname>pg_collect_advice.shared_collector</varname> enables the
-      shared advice collector.  The default value is <literal>false</literal>.
+      shared advice collector. The default value is <literal>false</literal>.
       Only superusers and users with the appropriate <literal>SET</literal>
       privilege can change this setting.
      </para>
@@ -221,8 +236,8 @@
     <listitem>
      <para>
       <varname>pg_collect_advice.shared_collection_limit</varname> sets the
-      maximum number of query texts and advice strings retained by the
-      shared advice collector.  The default value is <literal>0</literal>.
+      maximum number of entries retained by the shared advice collector.
+      The default value is <literal>0</literal>.
       Only superusers and users with the appropriate <literal>SET</literal>
       privilege can change this setting.
      </para>

---

From: "David G. Johnston" <[email-masked]>
Date: Mon, 2 Mar 2026 16:11:17 -0700

On Mon, Mar 2, 2026 at 3:09 PM David G. Johnston <[email-masked]>
wrote:

> On Sun, Mar 1, 2026 at 9:10 PM David G. Johnston <
Lastly, 0007 sgml (stash)

Placed entry in correct position on contrib page.

Expanded a bit on the security aspect comment.  I suppose there is some
indirect exposure via decisions being made implying table sizes or
records-per-FK...I left that unmentioned.

Added some explicit limitations to user-supplied values.

I would personally like to see "pg_set_stashed_advice" returning something
besides void.  I would usually go for text, but maybe in the interest of
i18n an integer would suffice.  +1 if a row was added, -1 if a row is
removed, or 0 for an update.  That would necessitate any other no-op being
an error.  Presently, supplying NULL for the query_id is not an error -
that seems like an oversight.  Same goes for supplying NULL for
stash_name.  If both of those cases produce errors (leaving NULL
advice_string being a remove indicator) the integer return seems like it
should work just fine.

Sorta feels like this module would appreciate advice strings having a
comment feature - so instead of just leaving an empty string behind saying
"we know, no advice needed" - it could contain actual content that isn't
applied advice.  Given the complexity of planning, comments seem warranted
in the advice themselves in any case.

Feels like this module needs export and import functions, especially given
the intro paragraph about the contents being volatile.  Or maybe a psql
script example producing a dynamic script file involving \gexec.

David J.
diff --git a/doc/src/sgml/contrib.sgml b/doc/src/sgml/contrib.sgml
index 62af2b21fd7..8f09d728698 100644
--- a/doc/src/sgml/contrib.sgml
+++ b/doc/src/sgml/contrib.sgml
@@ -151,7 +151,6 @@ CREATE EXTENSION <replaceable>extension_name</replaceable>;
  &ltree;
  &pageinspect;
  &passwordcheck;
- &pgstashadvice;
  &pgbuffercache;
  &pgcollectadvice;
  &pgcrypto;
@@ -161,6 +160,7 @@ CREATE EXTENSION <replaceable>extension_name</replaceable>;
  &pgplanadvice;
  &pgprewarm;
  &pgrowlocks;
+ &pgstashadvice;
  &pgstatstatements;
  &pgstattuple;
  &pgsurgery;
diff --git a/doc/src/sgml/pgstashadvice.sgml b/doc/src/sgml/pgstashadvice.sgml
index d1878f4f7c1..79b89beba38 100644
--- a/doc/src/sgml/pgstashadvice.sgml
+++ b/doc/src/sgml/pgstashadvice.sgml
@@ -10,7 +10,7 @@
  <para>
   The <filename>pg_stash_advice</filename> extension allows you to stash
   <link linkend="pgplanadvice">plan advice</link> strings in dynamic
-  shared memory where they can be automatically applied.  An
+  shared memory where they can be automatically applied. An
   <literal>advice stash</literal> is a mapping from
   <link linkend="guc-compute-query-id">query identifiers</link> to plan advice
   strings. Whenever a session is asked to plan a query whose query ID appears
@@ -25,7 +25,7 @@
   In order to use this module, you will need to execute
   <literal>CREATE EXTENSION pg_stash_advice</literal> in at least
   one database, so that you have access to the SQL functions to manage
-  advice stashes.  You will also need the <literal>pg_stash_advice</literal>
+  advice stashes. You will also need the <literal>pg_stash_advice</literal>
   module to be loaded in all sessions where you want this module to
   automatically apply advice. It will usually be best to do this by adding
   <literal>pg_stash_advice</literal> to
@@ -54,12 +54,12 @@
   string that you wish to store for each query. One way to do this is to use
   <literal>EXPLAIN</literal>: the <literal>VERBOSE</literal> option will
   show the query ID, and the <literal>PLAN_ADVICE</literal> option will
-  show plan advice.  <xref linkend="pgcollectadvice" /> can be used to
+  show plan advice. <xref linkend="pgcollectadvice" /> can be used to
   obtain this information for an entire workload, although care must be
   taken since it can use up a lot of memory very quickly. Query identifiers can
   also be obtained through tools such as <xref linkend="pgstatstatements" />
   or <xref linkend="monitoring-pg-stat-activity-view" />, but these tools
-  will not provide plan advice strings.  Note that
+  will not provide plan advice strings. Note that
   <xref linkend="guc-compute-query-id" /> must be enabled for query
   identifiers to be computed; if set to <literal>auto</literal>, loading
   <literal>pg_stash_advice</literal> will enable it automatically.
@@ -84,7 +84,9 @@
   <literal>pg_stash_advice.stash_name</literal> for their session, and this
   may reveal the contents of any advice stash with that name. Users should
   assume that information embedded in stashed advice strings may become visible
-  to nonprivileged users.
+  to nonprivileged users. However, advice targets are the only explicit
+  userspace data present in advice strings and most catalog contents are
+  considered non-privileged in PostgreSQL.
  </para>
 
  <sect2 id="pgstashadvice-functions">
@@ -102,7 +104,8 @@
 
     <listitem>
      <para>
-      Creates a new, empty advice stash with the given name.
+      Creates a new, empty advice stash with the given name, which must not be
+      zero length.
      </para>
     </listitem>
    </varlistentry>
@@ -134,10 +137,13 @@
     <listitem>
      <para>
       Stores an advice string in the named advice stash, associated with
-      the given query identifier.  If an entry for that query identifier
-      already exists in the stash, it is replaced.  If
+      the given query identifier. If an entry for that query identifier
+      already exists in the stash, it is replaced. If
       <parameter>advice_string</parameter> is <literal>NULL</literal>,
-      any existing entry for that query identifier is removed.
+      any existing entry for that query identifier is removed. A zero length advice
+      string is stored in the stash. An error is raised if stash_name does
+      not exist. Passing NULL for the query identifier results in a silent no-op,
+      and passing 0 is not permitted, but otherwise, any bigint value is accepted.
      </para>
     </listitem>
    </varlistentry>
@@ -170,7 +176,7 @@
 
     <listitem>
      <para>
-      Returns one row for each entry in the named advice stash.  If
+      Returns one row for each entry in the named advice stash. If
       <parameter>stash_name</parameter> is <literal>NULL</literal>, returns
       entries from all stashes.
      </para>
@@ -197,7 +203,7 @@
     <listitem>
      <para>
       Specifies the name of the advice stash to consult during query
-      planning.  The default value is the empty string, which disables
+      planning. The default value is the empty string, which disables
       this module.
      </para>
     </listitem>

---

From: Robert Haas <[email-masked]>
Date: Wed, 4 Mar 2026 10:17:37 -0500

On Fri, Feb 27, 2026 at 8:16 PM David G. Johnston
<[email-masked]> wrote:
> I have a mind to walk through the readmes and sgmls but its going to be in chunks.  Here's one for the readme for pg_plan_advice with a couple of preliminary sgml changes.

While I'm grateful for the feedback, I feel like you tend to suggest a
lot of edits that seem like they're just substituting your
idiosyncratic preferences for mine e.g. writing "types of scan" vs.
"scan types," or writing "additional, separate join problems" vs.
"independent join problems" or "judiciously" vs. "conservatively". I
don't really consider these to be improvements, nor do I necessarily
think they're worse, but I just don't see the point in litigating this
kind of stuff. If I've written something that is legitimately unclear,
or factually incorrect, or there's a spelling or punctuation mistake,
I'm happy to correct that kind of stuff, but I don't really want to go
through and replace a bunch of words that I liked with a bunch of
synonyms that you picked.

Also, when you just provide a diff like this, it's not that clear to
me why you're suggesting particular changes, which makes it hard to
decide whether I agree with them. And in a lot of cases I don't.
Looking at some particular examples:

+isn't going to work any more. That's expected. It should be resilient to
+changes in the statistics, including any CREATE STATISTICS related changes.

This is broadly true but seems a bit obvious to mention in a README.
If we were going to mention it, I'd think it would go in the
user-facing documentation. But I don't quite see why we should mention
it at all. If plan advice couldn't override changes caused by
statistics, what would even be the point of it? Also, it's not
categorically true in all situations, because as discussed elsewhere,
we have limitations like lack of control over aggregation strategy.

 Tags such as NESTED_LOOP_PLAIN specify the method that should be used to
-perform a certain join. More specifically, NESTED_LOOP_PLAIN(x (y z)) says
+perform a certain join - with the target appearing directly on the inner side
+of the join list first. Thus, NESTED_LOOP_PLAIN(x (y z)) says
 that the plan should put the relation whose identifier is "x" on the inner
 side of a plain nested loop (one without materialization or memoization)
 and that it should also put a join between the relation whose identifier is

This seems like you're adding a second explanation of what the
paragraph already goes onto say, except that the existing explanation
is more precise and detailed.

-useless in practice. It gives the planner too much freedom to do things that
+problematic in practice. It gives the planner too much freedom to do
things that

I mean, I stand by the word I picked. I don't want to weaken it.

-This means that if advice can say that a certain optimization or technique
-should be used, it should also be able to say that the optimization or
-technique should not be used. We should never assume that the absence of an
-instruction to do a certain thing means that it should not be done; all
-instructions must be explicit.
+In other words, advice tags must define whether they encourage or discourage
+certain optimizations or techniques. (NO_GATHER is an example of the latter.
+There is no generic "NOT" syntax, e.g., NOT(HASH_JOIN(dim2 dim4).))

My text explains an important design principle that future hackers
must keep in mind when modifying this system to avoid breaking
everything. Your replacement text just describes how it works today.
Considering that this is a README for hackers, I think that's much
worse.

-  advice" mini-language. It is intended to allow stabilization of plan choices
+  advice" domain specific language (DSL). It is intended to allow
stabilization of plan choices

There's a debate to be had about whether it's better to say
mini-language or domain specific language here, but it's hard for me
to decide which is better if all you provide is a diff replacing A
with B. I definitely think it's worse to write (DSL) here. There is no
point in defining an acronym if we're never going to use it anywhere.

---

From: "David G. Johnston" <[email-masked]>
Date: Wed, 4 Mar 2026 08:44:42 -0700

On Wed, Mar 4, 2026 at 8:17 AM Robert Haas <[email-masked]> wrote:

> idiosyncratic preferences for mine

Yeah, some of these end up being mostly stylistic.  Though I do try to
limit them to ones where I see inconsistency or the style I'm reading just
doesn't resonate with me.  I usually point out the ones that are IMO
material, versus just something that tripped me up while I was reading, but
failed to do so here.

I do need to work in a way to better annotate/comment on the why of these.
Any suggestions for a better flow or feedback format?  Inline comments
wrapped in sgml comments?  Or just copy the diff into the email body and
inline comment there - leaving the original diff attachment as-is?

> point in defining an acronym if we're never going to use it anywhere.
This was truly just a "have you considered using this terminology instead"
kind of prompt.  The acronym would have been useful when going an replacing
the other uses of mini-language that I left alone since I hadn't myself
decided which one was better.

I didn't do my usual email recap on this first patch which is my bad.  I
corrected that with the others.

David J.

---

From: Robert Haas <[email-masked]>
Date: Wed, 4 Mar 2026 11:20:45 -0500

On Wed, Mar 4, 2026 at 10:45 AM David G. Johnston
<[email-masked]> wrote:
> I do need to work in a way to better annotate/comment on the why of these.  Any suggestions for a better flow or feedback format?  Inline comments wrapped in sgml comments?  Or just copy the diff into the email body and inline comment there - leaving the original diff attachment as-is?

My suggestion is to break these fixes up into three categories: clear
errors, stylistic suggestions, substantive concerns.

Clear errors can be handled by just sending a patch that fixes all the
clear errors without changing anything else. If I intended to write
"applied" and I actually typed "aplied," it's fine to bundle that with
10 other mistakes and submit them without commentary.

For substantive concerns, I think it's most helpful to just quote the
patch hunk in the body of your email and say what your concern is. If
you have suggested wording feel free to suggest that as well, but I'd
focus more on saying what the problem is rather than jumping to the
solution. At least some of these are cases where what I wrote wasn't
sufficient for you to understand the patch, which a very fair issue to
raise, but if you try to write your own wording, the fact that you
don't understand the patch makes it hard for you to write quality
documentation for it. If you say "I read where you said X and I tried
Y and it seemed like the wrong thing happened, so either the
documentation sucks or the code is buggy," now we're having a
worthwhile conversation. If you just change the documentation based on
your understanding of the results of an undisclosed experiment, I feel
like the chances of the result being an improvement are not great.

Stylistic concerns are the most complicated. If your concern is
something like "this sentence is hard to understand," I'd class that
as a substantive concern and treat it the same way. Beyond that, I'm
not really sure. Honestly, I think we may just have different
stylistic preferences, because my experience so far reading your
proposed documentation patches is that I tend to agree with relatively
little of what you want to do. I believe, though, that other
committers feel differently about it and find your proposed changes
quite helpful. So I'm not sure exactly what to recommend here, but
perhaps take a lighter touch when it's my patch? I'm OK with some
friendly suggestions that I can accept or reject, but going through a
huge list of minor wording tweaks is as much work as going through a
substantial review of the code itself, but for a lot less benefit,
especially if they all look like random changes that I don't
understand why you're proposing.

Hope that makes sense and isn't too harsh.

Thanks,

---

From: "David G. Johnston" <[email-masked]>
Date: Fri, 6 Mar 2026 07:46:24 -0700

On Wed, Mar 4, 2026 at 9:20 AM Robert Haas <[email-masked]> wrote:

> > I do need to work in a way to better annotate/comment on the why of
> these.  Any suggestions for a better flow or feedback format?  Inline
> comments wrapped in sgml comments?  Or just copy the diff into the email
> body and inline comment there - leaving the original diff attachment as-is?
>
Thank you for putting in the time to respond.  That was quite helpful.
I've tweaked my tooling to help me remember to do this going forward.

David J.

---

From: Robert Haas <[email-masked]>
Date: Tue, 10 Mar 2026 10:55:26 -0400

On Fri, Mar 6, 2026 at 9:47 AM David G. Johnston
<[email-masked]> wrote:
> Thank you for putting in the time to respond.  That was quite helpful.  I've tweaked my tooling to help me remember to do this going forward.

Thanks. Here's v19. I've incorporated a bunch of your changes, some
that were fixes for clear errors and a few of the more stylistic cases
where I decided that I agreed with you, and I've also fixed some
similar things upon which you did not remark. Separately, I've also
committed the patches that were previously 0001, 0002, and 0005, so
all the preparatory stuff is done now, and this version just contains
the substantive commits, for which I am still in need of review,
especially for 0004. I have also made a minor code adjustment:
pg_plan_advice.trace_mask now prints the subplan name except for the
toplevel "subplan".

Here are a few comments on some of the changes you made that I did not adopt:

- In the README, "we" are the PostgreSQL developers and "the user" is
the person using the module. In the documentation, "you" is the user.
If there are deviations from this idea, we should fix them, but it
seems OK that the two documents have different rules, inasmuch as they
address different audiences.

- For the same reason, I chose not to copy the note about
enable_partitionwise_join=off into the README, as that is not a core
concept for developers but a likely error for users.  We also don't
really want to start duplicating content too much; arguably, we have
too much of that already.

- I don't really think we need to document the exact rules for
argument to, e.g., the pg_stash_advice functions. I think that makes
the documentation ponderous without adding any real utility. At most,
I would mention in some centralized place what the rules for stash
names are. Separately, there is the question of whether the current
naming rules are the right ones.

- In a lot of places, especially in the README, I just disagreed with
your choice of what to emphasize. For instance, I thought my longer
explanation of how we must not infer guidance from the absence of
advice was better than your shorter one, because it's such a critical
point for future developers touching this code to understand. On the
other hand, saying that advice should be stable in the face of
statistics changes was redundant: if it doesn't even do that much,
then what would even be the point?

- I chose to retain the use of the term "core planner" in the SGML
documentation rather than your suggestion of deleting the word "core".
I do not love the wording I've chosen here, but I don't love your
change, either. It seems to me that there is a risk of people being
confused about the distinction between the planning-related logic in
src/backend/optimizer and the planning-related object in
pg_plan_advice itself. I included the word in core for emphasis.
Whether this is the right idea is debatable, of course.

Thanks,

---

From: Robert Haas <[email-masked]>
Date: Thu, 12 Mar 2026 13:15:39 -0400

On Tue, Mar 10, 2026 at 10:55 AM Robert Haas <[email-masked]> wrote:
> Thanks. Here's v19.

I've now committed the main patch. I think that I'm not likely to get
too much more feedback on that in this release cycle, and I judge that
more people will be sad if it doesn't get shipped this cycle than if
it does. That might turn out to be catastrophically wrong, but if many
problem reports quickly begin to emerge, it can always be reverted.
I'm hopeful my testing has been thorough enough to avoid that, but
there's a big difference between lab-testing and real world usage, so
we'll see.

I'm still hoping to get some more feedback on the remaining patches,
which are much smaller and conceptually simpler. While there is no
time to redesign them at this point in the release cycle, there is
still the opportunity to fix bugs, or decide that they're too
half-baked to ship. So here is v20 with just those patches. Of course,
post-commit review of the main patch is also very welcome.

Thanks,

---

From: Alexandra Wang <[email-masked]>
Date: Thu, 12 Mar 2026 16:45:49 -0700

Hi Robert,

> post-commit review of the main patch is also very welcome.

Thanks for the patches!

I've run the meson tests for all three modules, and they all pass on
my laptop.

For the make tests, I had to add

+EXTRA_INSTALL = contrib/pg_plan_advice

in contrib/pg_collect_advice/Makefile in order for "make check" for
that module to run.

I don't really have a sense of how others feel about including these
modules, so I can't speak to that. Personally, though, I very much
like the test_plan_advice module because it gives me peace of mind,
and I feel it should accompany the already committed pg_plan_advice
module.

I reviewed the code and have a few minor comments:

0001:
The "make check" issue mentioned above.

0002:
Looks good to me.

0003:
There is a typo in the commit message:
"pg_stash.advice_stash" should be "pg_stash_advice.stash_name".

> stash->pgsa_stash_id = pgsa_state->next_stash_id++;

I doubt there will be more than 2 billion stashes in the system, but
if in any case we reach that number, we don't handle int overflow.
Should we set a limit on how many stashes can be stored?

Nit:
find_defelem_by_defname() is defined in all three modules, and also in
pg_plan_advice. Knowing it is very small, would it make sense to
extern the one in pg_plan_advice and reuse it?

Best,
Alex

-- 
Alexandra Wang
EDB: https://www.enterprisedb.com

---

From: Lukas Fittl <[email-masked]>
Date: Fri, 13 Mar 2026 01:38:25 -0700

Hi Robert,

> problem reports quickly begin to emerge, it can always be reverted.

I think its good to get this in, and not do it in the last minute, but
just to be clear on my part, I've reviewed earlier versions, but I
have not reviewed the latest code to the extent I would have liked
before it was committed, due to competing priorities on my end. That
said, its a good forcing function, so let me do my part to help shake
out any bugs that remain.

---

From: Alexander Lakhin <[email-masked]>
Date: Sat, 14 Mar 2026 14:00:00 +0200

Hello Robert,

> there's a big difference between lab-testing and real world usage, so

I've found a crash inside pgpa_join_path_setup(), reproduced with:
echo "geqo_threshold = 2" >/tmp/extra.config
TEMP_CONFIG=/tmp/extra.config make -s check -C contrib/pg_plan_advice/

Program terminated with signal SIGSEGV, Segmentation fault.
(gdb) bt
#0  0x000070820b96d215 in pgpa_join_path_setup (root=0x5ef5a84682b8, joinrel=0x5ef5a8497f10, outerrel=0x5ef5a8472b48,
     innerrel=0x5ef5a84baeb8, jointype=JOIN_UNIQUE_INNER, extra=0x7fff08823490) at pgpa_planner.c:602
#1  0x00005ef57df9742b in add_paths_to_joinrel (root=0x5ef5a84682b8, joinrel=0x5ef5a8497f10, outerrel=0x5ef5a8472b48,
     innerrel=0x5ef5a84baeb8, jointype=JOIN_UNIQUE_INNER, sjinfo=0x5ef5a8473c00, restrictlist=0x5ef5a8498450) at 
joinpath.c:178
#2  0x00005ef57df9d178 in populate_joinrel_with_paths (root=0x5ef5a84682b8, rel1=0x5ef5a8472b48, rel2=0x5ef5a84731f0,
     joinrel=0x5ef5a8497f10, sjinfo=0x5ef5a8473c00, restrictlist=0x5ef5a8498450) at joinrels.c:1197
#3  0x00005ef57df9c6ab in make_join_rel (root=0x5ef5a84682b8, rel1=0x5ef5a8472b48, rel2=0x5ef5a84731f0) at joinrels.c:774
#4  0x00005ef57df700be in merge_clump (root=0x5ef5a84682b8, clumps=0x5ef5a8497e60, new_clump=0x5ef5a8497eb0, num_gene=2,
     force=false) at geqo_eval.c:259
#5  0x00005ef57df6ff3e in gimme_tree (root=0x5ef5a84682b8, tour=0x5ef5a84ba688, num_gene=2) at geqo_eval.c:198
#6  0x00005ef57df6fe12 in geqo_eval (root=0x5ef5a84682b8, tour=0x5ef5a84ba688, num_gene=2) at geqo_eval.c:101
#7  0x00005ef57df708fa in random_init_pool (root=0x5ef5a84682b8, pool=0x5ef5a84c3988) at geqo_pool.c:108
#8  0x00005ef57df70439 in geqo (root=0x5ef5a84682b8, number_of_rels=2, initial_rels=0x5ef5a84ba1f8) at geqo_main.c:127
#9  0x00005ef57df77aa3 in make_rel_from_joinlist (root=0x5ef5a84682b8, joinlist=0x5ef5a8473b40) at allpaths.c:3902
#10 0x00005ef57df715e2 in make_one_rel (root=0x5ef5a84682b8, joinlist=0x5ef5a8473b40) at allpaths.c:240
#11 0x00005ef57dfbede4 in query_planner (root=0x5ef5a84682b8, qp_callback=0x5ef57dfc5ae9 <standard_qp_callback>,
     qp_extra=0x7fff08823af0) at planmain.c:297
...

Could please look at this?

Best regards,
Alexander

---

From: Zsolt Parragi <[email-masked]>
Date: Sat, 14 Mar 2026 15:06:04 +0000

Hello

I noticed a difference between installed headers with make and meson,
which is caused by pg_plan_advice.h. It is completely missing from the
make build, and installs to the wrong location with meson.

Please see the attached patch that fixes this.

---

From: Robert Haas <[email-masked]>
Date: Mon, 16 Mar 2026 12:52:55 -0400

> Please see the attached patch that fixes this.

Thanks. The changes to the Makefile seem to mirror what is done in
contrib/isn/Makefile, but I'm not so sure about the meson.build
changes. sepgsql uses dir_data / 'contrib' rather than
dir_include_server. src/pl/pl{perl,pgsql,python} use
dir_include_server, but they also live in src/pl, not contrib. I don't
think I understand what the underlying principal is supposed to be
here. If you or anyone else knows, please enlighten me.

---

From: Robert Haas <[email-masked]>
Date: Mon, 16 Mar 2026 13:11:16 -0400

> Could please look at this?

Thanks. Proposed fix attached.

---

From: Robert Haas <[email-masked]>
Date: Mon, 16 Mar 2026 13:34:59 -0400

> +EXTRA_INSTALL = contrib/pg_plan_advice

Thanks, will fix.

> "pg_stash.advice_stash" should be "pg_stash_advice.stash_name".

Thanks, will fix.

> Should we set a limit on how many stashes can be stored?

I think the thing to do here is just change the stash ID to a 64-bit
value. I'll do that in the next version. We assume in numerous places
that a 64-bit integer never overflows (e.g. LSNs) which is pretty fair
considering how long it would take for that to happen.

> extern the one in pg_plan_advice and reuse it?

I think if we want to avoid duplicating this function, we should
actually put it someplace in core, e.g. expose it via defrem.h and put
the code in commands/define.c. Any user of extendplan.h is likely to
need a function like this, but we shouldn't put it there because there
could be other use cases as well.

---

From: Robert Haas <[email-masked]>
Date: Mon, 16 Mar 2026 16:51:09 -0400

On Fri, Mar 13, 2026 at 4:39 AM Lukas Fittl <[email-masked]> wrote:
> From a code review perspective, I found some minor issues:
>
> - Identifiers that are named like advice types cause an error (so I
> can't name my table "hash_join")

Thanks, I extracted and committed your fix for that issue.

> - pgpa_destroy_join_unroller does not free the inner_unrollers values
> (which I think goes against the intent of cleaning up the allocations
> right away)

Yeah, that makes sense.

> - pgpa_parser uses pgpa_yyerror, but that function doesn't use
> elog(ERROR) like the main parser does - I think it'd be more
> consistent to use YYABORT explicitly, e.g. how we do it in cubeparse

Perhaps, but I think this needs more thought, because you haven't done
anything about the calls in pgpa_scanner.l. I think right now, the
operating principle is that parsing continues after an error and that
we mustn't crash as a result, though the resulting tree will probably
be invalid for practical use. If we're going to change that to stop on
the spot, I think we should probably do it for both lexing and
parsing, and think about whether that leads to any other changes or
simplificatons.

> - pgpa_scanner accepts integers with underscores, but incorrectly uses
> a simple strtoint on them (which would fail), instead of pg_strtoint32
> / pg_strtoint32_safe

OK.

> - pgpa_walk_recursively uses "0" for the boolean value being passed to
> a recursive call in one case (should be "false")

OK.

> From a usability perspective, I do wonder about two things when it
> comes to users specifying advice directly (which will happen in
> practice, e.g. when debugging plan problems):
>
> 1) The lack of being able to target scan advice (e.g. SEQ_SCAN) to a
> partition parent is frustrating - I believe we discussed partitioning
> earlier in the thread in terms of gathering+applying it, but I do
> wonder if we shouldn't at least allow users to specify a partitioned
> table/partitioned index, instead of only the children. See attached
> nocfbot-0002 for an idea what would be enough, I think.

I'm not on board with this without a lot more study. I've been down
this road before, and it can easily end in tears. Examining the
partition structure on the fly can have a performance cost, and it
might even have security ramifications or at least bugs if there are
concurrent modifications to the partitioning structure happening.
Also, the test_plan_advice framework doesn't do much to tell us
whether this actually works. Also, I understand the frustration and
I'm sure we'll want to introduce various forms of wildcards, but I
think there will be a lot of opinions about that should actually look
like. One can, as you've done here, follow index links from child to
parent. One can do a wildcard match on the index name. One could want
to specify an index on a particular column rather than a specific
name, to survive index renamings. I wouldn't be surprised if there are
other ideas, too. Three weeks before feature freeze is not the time to
be taking an opinionated position on what the best answers will
ultimately turn out to be. It's easy to write a tool that will spit
out matching index advice for all indexes involving in a partitioning
hierarchy, and I think that's what people should do for now. Or,
perhaps they should use the generated advice from actual plans instead
of writing hand-crafted advice.

We always have the option to add more to this in the future, but
taking things out is not half so easy.

> 2) I find the join strategy advice hard to use - pg_hint_plan has
> hints (e.g. HashJoin) that allow saying "use a hash join when joining
> these two rels in any order", vs pg_plan_advice requires setting a
> specific outer rel, which only makes sense when you want to fully
> specify every aspect of the plan. I suspect users who directly write
> advice will struggle with specifying join strategy advice as it is
> right now. We could consider having a different syntax for saying "I
> want a hash join when these two rels are joined, but I don't care
> about the order", e.g. "USE_HASH_JOIN(a b)". If you think that's
> worthwhile I'd be happy to take a stab at a patch.

I'd be inclined to classify that as a design error in pg_hint_plan,
but maybe I'm just not understanding something. Under what
circumstances would you know that you wanted two tables to be joined
via a hash join but not care which one was on which side of the join?
The rels on the two sides are treated very differently. One of them
needs to be small enough to fit in the hash table and should be one
where repeated index lookups aren't better (else, you should be
advising a nested loop and an index scan). The other can be basically
anything. What I know (in a situation where I might write some advice
manually) is that I want a certain table to be the one that goes into
the hash table, not that there should be a hash join someplace in the
general vicinity of one of the tables.

Also, there's a definitional question here. What exactly does
USE_HASH_JOIN(a b) mean? Possible definitions:

1. "a" must be joined directly to "b" without any other tables on
either side, and a hash join must be used to perform that join.
2. either "a" must appear on the inner side of a hash join with "b"
somewhere on the other side, possibly accompanied by other tables, or
the reverse
3. the plan must contain at least one hash join where "a" and "b"
appear on opposite sides of the join

Suppose I have a fact table f and three dimension tables d1, d2, and
d3. If I wrote USE_HASH_JOIN(f d1) USE_HASH_JOIN(f d2) USE_HASH_JOIN(f
d3), what happens? Under definition 1, the advice fails, because f
must be first joined to either d1, d2, or d3, and whichever one is
chosen, the other advice can't now be satisfied. Under definition 2, I
will definitely end up with hash joins all the way through, but it's
possible that the driving table will be one of the dimension tables,
which can be first joined to f and then to the other tables. Under
definition 3, I'm not even guaranteed to end up with all hash joins: I
can join d1, d2, and d3 to each other in any way I like, and then hash
join the result of that to f in either direction, and the rule is
satisfied for all three advice items.

None of those possibilities sound right. I argue that definitions 1
and 3 produce such absurd results in this scenario that they're not
even worth any further consideration, but I can see someone arguing
that definition 2 doesn't sound so bad. After all, you could still fix
it to achieve the probably-expected outcome by also writing
JOIN_ORDER(f). But that only works here because we know what we want
the driving table to me. Suppose alternatively that we have two large
tables B1 and B2 and a small table S, and we've figured out that the
planner tends to like to use the index on table S when it really ought
to be using a hash join, but we trust its judgement as to how to join
B1 and B2. With HASH_JOIN() as I've implemented it, that's easy: just
write HASH_JOIN(S). With your USE_HASH_JOIN, how do I do that exactly?
If I write USE_HASH_JOIN(B1 S), definition 2 permits a plan like this:

Hash Join
  -> Nested Loop
    -> Seq Scan on B2
    -> Index Scan on S
  -> Hash
    -> Seq Scan on B1

To me, that looks a heck of a lot like my USE_HASH_JOIN() locution
just didn't do anything. It certainly didn't do what I intended it to
do, and the only way I can make sure that something like this doesn't
happen is to *also* constrain the join order. If I'm willing to decide
which of B1 and B2 should be the driving table, then I can write
JOIN_ORDER(B1 B2 S) or JOIN_ORDER(B2 B1 S) and now everything is
fixed. But this seems 100% backwards given your stated goal: you want
to be able to constraint the join method without constraining the join
order. As I see it, the problem here is that a symmetric USE_HASH_JOIN
directive either uses something like definition 1 which is too tight a
constraint, or definition 2 or 3 which are extremely weak constraints
that essentially allow the planner to satisfy the constraint using
some other part of the plan tree.

Now, of course, I got to pick these examples, so I picked examples
that prove my point. Maybe there are examples where a "one side or the
other" constraint actually works better. But I don't know what those
examples are. When I've experimented this kind of thing, I've found
that I never get the results that I want because the planner just does
something stupid that technically satisfies the constraint but is
nothing like what I actually meant. If you know of examples where my
definitions suck and the "one side or the other" definition produces
great results, I'd love to hear about them ... although I would have
loved to hear about them even more 4.5 months ago when I first posted
this patch set and already had the phrase "useless in practice" in the
README on exactly this topic. This is exactly why I put the patches up
for design review before they were fully baked.

> For v20-0001, from a quick conceptual review:
>
> I find the two separate GUC mechanisms for local backend vs shared
> memory a bit confusing as a user (which one should I be using?).
> Enabling the shared memory mechanism on a system-wide basis seems like
> it'd likely have too high overhead anyway for production systems?
> (specifically worried about the advice capturing for each plan, though
> I haven't benchmarked it)
>
> I wonder if we shouldn't keep this simpler for now, and e.g. only do
> the backend local version to start - we could iterate a bit on
> system-wide collection out-of-core, e.g. I'm considering teaching
> pg_stat_plans to optionally collect plan advice the first time it sees
> a plan ID (plan advice is roughly a superset of what we consider a
> plan ID there), and then we could come back to this for PG20.

The shared version is rather useful for testing, though. That's
actually why I created it initially: turn on the shared collector, run
the regression tests, and then use SQL to look through the collector
results for interesting things. You can't do that with the local
collector.

> To help assess impact, I did a quick test run and looked at three
> not-yet-committed patches in the commitfest that affect planner logic
> ([0], [1] and [2]), to see if they'd require pg_plan_advice changes
> (master with v20-0002 applied). Maybe I picked the wrong patches, but
> at least with those no pg_plan_advice changes were needed with the
> test_plan_advice test enabled.

Nice.

> On the code itself:
> - Is there a reason you're setting
> "pg_plan_advice.always_store_advice_details" to true, instead of using
> pg_plan_advice_request_advice_generation?
> - I wonder if we could somehow detect advice not matching? Right now
> that'd be silently ignored, i.e. you'd only get a test failure when we
> generate the wrong advice that causes a plan change in the regression
> tests.

I think it already does more than what you seem to be thinking:
test_plan_advice checks the advice feedback, too. However, it's also
true that even more could be done. The code proposed here checks that
all advice is /* matched */ without being /* failed */ or /*
inapplicable */, but I have code locally that checks the reverse,
namely that every decision that, during the re-plan, every decision
could have been constrained by advice actually was. I felt it was a
bit too late to add that to what I was submitting for v19, but that
decision could certainly be revisited. Taking it even further, we
could do structural comparisons of the before-and-after plans, or we
could search for disabled nodes aside from what default_pgs_mask
should imply. I'm not very confident that those things are worth the
code they would take to implement, though. The existing checks found a
lot of bugs, but I think whatever is still wrong is probably not
super-likely to be found by additional cross-checks. That could be
wrong; it's just a hunch.

> For v20-0003, initial thoughts:
>
> I think getting at least a basic version of this in would be good, as
> a server-wide way to set advice for queries can help people get out of
> a problem when Postgres behaves badly - and we know from pg_hint_plan
> (which has a hint table) that this can be useful even without doing
> any kind of parameter sniffing/etc to be smart about different
> parameters for the same query.

Yep.

> The name "stash" feels a bit confusing as an end-user facing term.
> Maybe something like "pg_apply_advice", or "pg_query_advice" would be
> better? (though I kind of wish we could tie it more closely to "plan
> advice", but e.g. "pg_plan_advice_apply" feels too lengthy)

I've heard that other people also find "stash" not as intuitive as it
could be, and I'm open to changing it. However, whatever we call this
has to make its relationship to pg_plan_advice clear. If we have
pg_plan_advice and pg_query_advice, which one is the core module and
which one is automatically supplying advice? You can't tell from the
name, which I think will be confusing. In the long run, I suspect we
will end up with a moderately-large pile of tools for either capturing
advice or automatically supplying it, and we should think about how
we'd like all those to get named. Right now, I suppose we might end up
with something like this:

pg_plan_advice: The core. There can only be one.

different ways of capturing advice strings: pg_collect_advice,
pg_capture_advice, pg_save_advice, pg_baseline_advice, ....

different ways of supplying advice strings: pg_stash_advice,
pg_auto_advice, pg_lookup_advice, pg_store_advice,
pg_advice_from_query_comment, ...

I don't love this, because the pattern I see developing here is that
module names will either be very long or they'll just take a word from
an existing module name (like "collect") and replace it with a synonym
(like "capture"). That's not going to produce anything very mnemonic,
so it would be nice to do better. But I think the route to doing
better has to be to get more specific with the naming, not less. Like,
if we renamed pg_stash_advice to
pg_lookup_in_memory_advice_by_query_id, then the name tells you
EXACTLY what it does, which is great. Unfortunately, the name is also
very long, which is why I didn't go that route.

---

From: Lukas Fittl <[email-masked]>
Date: Mon, 16 Mar 2026 16:25:05 -0700

> parsing, and think about whether that leads to any other changes or

Fair - I don't think there is a practical impact from not doing the
YYABORT calls, since as you say care is taken to not crash in that
case.

> hierarchy, and I think that's what people should do for now.

That's fair. I would like us to do something about this in the PG20
release cycle - for my part, I think its reasonable to follow the
declarative partitioning parent-child relationship for indexes if
present - assuming we can sort out the performance/etc. aspects of
that.

For 19 I think we might want to consider calling this out more
explicitly in the documentation under the "Scan Method Advice"
paragraph, i.e. that one cannot specify partition parent table names
(at least not ones that have no data of their own) and instead one has
to specify the partitions individually. Otherwise I think users will
just be confused by the Append node that says "Disabled: true" and the
advice that didn't match.

> via a hash join but not care which one was on which side of the join?

I think the common case would be someone sees the planner picked a
Nested Loop, and instead wants to see the plan that prefers a Hash
Join (or Merge Join), e.g. to understand costing differences. That's
how I usually use pg_hint_plan, to understand what the alternate plan
looked like that the planner didn't pick, but where costs were close.
The top-level "enable_nestloop = off" often tends to not work that
well for complex plans, hence my historic use of pg_hint_plan's
HASHJOIN/MERGEJOIN (or NO_NESTLOOP) hints for this purpose.

> great results, I'd love to hear about them ...

Thanks for the detailed work through - I think I see your
implementation choice for this more clearly now. I've also re-read the
documentation section on join methods and I think that is clear enough
in terms of how it works.

I don't think a change here is necessary. I think for the use case I
described I will just resort to testing both variants (i.e. being more
specific which shape of plan I want), which I think aligns with the
goals of pg_plan_advice as compared to pg_hint_plan.

Later in the release cycle I'll see if I can put together a community
resource that compares pg_hint_plan to pg_plan_advice, and where they
differ. I suspect many end users will have similar questions, and
whilst I don't think explaining the differences belongs in the regular
Postgres docs, it could fit the wiki as a cheatsheet of sorts.

> although I would have

Understood - I'll admit I mainly looked at the high-level join logic
before (and the join hooks in detail when doing the pg_hint_plan
testing) but had not fully understood how you dealt with join
hierarchies / specifying them in the advice. I had previously looked
at examples where multiple tables were listed assuming it worked like
hint plan, but that's not the case.

> results for interesting things. You can't do that with the local

Right - I can see the usefulness for testing, but I worry that people
use it on production systems and then experience unexpected
performance issues. That said, we could address that with a warning in
the docs noting its not intended for production use.

Out of time for today to think through naming for 0003 more, but I'll
see that I find more time this week.

Thanks,
Lukas

-- 
Lukas Fittl

---

From: Zsolt Parragi <[email-masked]>
Date: Tue, 17 Mar 2026 05:06:08 +0000

> here. If you or anyone else knows, please enlighten me.

PGXS defines it as:

#   HEADERS_$(MODULE) -- files to install into
#     $(includedir_server)/$MODULEDIR/$MODULE; the value of $MODULE must be
#     listed in MODULES or MODULE_big

where

#   MODULEDIR -- subdirectory of $PREFIX/share into which DATA and DOCS files
#     should be installed (if not set, default is "extension" if EXTENSION
#     is set, or "contrib" if not)

And I mirrored that in meson.

Data seems to be wrong for headers, as that's

#   DATA -- random files to install into $PREFIX/share/$MODULEDIR

> sepgsql uses dir_data / 'contrib'

Also, sepgsql installs an sql file, not an include.

---

From: Robert Haas <[email-masked]>
Date: Tue, 17 Mar 2026 09:44:34 -0400

> present - assuming we can sort out the performance/etc. aspects of

I wonder if instead of doing this as you did it, we should try to make
partition expansion happen around expand_inherited_rtentry time. So
maybe you can write something like PARTITIONED_SEQ_SCAN(x) or
PARTITIONED_INDEX_SCAN(x y) and, internally, that gets replaced with
one SEQ_SCAN(..) or INDEX_SCAN(...) entry per child before the core
planning logic engages. And, during the plan walk, we could do the
reverse transformation: if we computed matching advice items for every
partition, consolidate them down to just one before constructing the
final advice string. If we did that, the whole thing would be
symmetric and we'd have a certain amount of automatic test coverage,
plus we'd shorten a lot of automatically written advice strings
considerably. Maybe this is not better than the more on-the-fly way
that you did it, but I think it's worth some study.

While I'm on the subject, there are some other opportunities for
brevity that I have not pursued for this release. In particular,
NO_GATHER(...) often seems quite tedious to me. We could introduce a
wildcard, like "*", that just means everything, so you could write
NO_GATHER(*) for non-parallel queries. However, that seems like it's
actually not great, because as soon as you have a single Gather or
Gather Merge node in the plan, you're back to needing to write all the
other ones out. So another idea is to let you write something like
ONLY_EXPLICIT_GATHER() as a no-argument incantation that means that
everything that isn't mentioned as a GATHER() or GATHER_MERGE() target
should be considered NO_GATHER(). Or you could call that something
like NO_GATHER_OTHERS() or whatever. Or maybe there could be some
general default facility that lets you say what should happen when
nothing is specified, like DEFAULTS(NO_GATHER), but right now the
number of things that you could apply that to would be quite limited.

> advice that didn't match.

We can consider that, but I think the bigger picture here is that
writing advice strings by hand is hard, and that's why EXPLAIN
(PLAN_ADVICE) and pg_collect_advice exist -- to give you a starting
point. I fear that if we try to enumerate a lot of specific examples
of ways in which people might be confused in the documentation, they
won't read it, and they'll still be confused. I think the primary
focus of the documentation should be to get people to use the advice
generation facilities as their main way to discover how to use the
system, and then pointing out specific things that may still be
confusing where it makes sense is also good to do. For example, in a
case like this, if you sit down and write INDEX_SCAN(partitioned_table
partitioned_index), yeah that's not going to work, and you're going to
be confused (potentially). But that's not really what you're supposed
to do. You're supposed to start by running EXPLAIN (PLAN_ADVICE) on
the query whose plan you're trying to manipulate, and if you do that,
you'll see that the generated advice shows a separate INDEX_SCAN() or
SEQ_SCAN() item for each child table, and then it's sorta obvious what
you're supposed to be doing. You may very well not like that -- I
think many, many people are going to complain about it -- but you'll
understand what is possible with the system that we have. Now that is
not to say that I think you're wrong about documenting this, and I've
certainly tried to document some other instances of things that I
found confusing even as the author of the system. However, there's
also a lot of cases that I haven't tried to document because it's just
too much useless, abstract information. On this particular point, if
there's a nice plan to fit this into the documentation that doesn't
feel like a jarring topic shift or a long detour into minor details,
I'm fine with it, but I don't think it's worth a lot of contortions to
fit this specific thing in considering how many other things there
are.

> HASHJOIN/MERGEJOIN (or NO_NESTLOOP) hints for this purpose.

It's interesting to me that this works well for you even in complex
plans. For example, let's say I have something like this (omitting
sorts):

Merge Join
-> Nested Loop
  -> Hash Join
    -> Seq Scan on A
    -> Hash
      -> Seq Scan on B
  -> Index Scan on C
-> Nested Loop
  -> Seq Scan on D
  -> Index Scan on E

Now, what do you write here to get rid of the nested loop involving C?
Your example before of USE_HASH_JOIN(x y) seemed to imply that you
mentioned two tables, but this is a 3-table join, so are you
mentioning all three tables in this case, like USE_HASH_JOIN(A B C)?
Or are you mentioning just C, or A and C, or what? In pg_plan_advice,
it's just HASH_JOIN(C). I'm unclear what the right answer is in
pg_hint_plan. The documentation says that HashJoin(table table [...])
forces hash join for the joins on the tables specified, but I don't
know whether that means that the whole thing functions as a single
constraint (i.e. the N-way join product of all mentioned tables should
be computed using a HashJoin at the uppermost level) or as a
constraint per table (every mentioned table should be involved in a
hash join). If it's the former, then you could write HashJoin(A B C)
in this case, but that wouldn't preclude switching the join order so
that the A-C join is done first using a Nested Loop and then the join
to B is done afterward as a HashJoin, which is probably not what you
wanted. If it's the latter, then HashJoin(C) is probably good enough,
although not necessarily. If there are join clauses connecting B and
C, the planner could try to cheat by doing a hash join between B and C
first and then doing a Nested Loop join to A afterwards, which is also
probably not what you wanted, but typically there won't be such join
clauses so it will work out. Also, if the HashJoin(C) just means there
has to be a hash join somewhere above C, rather than that C has to be
on onside or the other of a hash join by itself, then the planner
could also cheat by switching the outer merge join to a hash join, but
I'm guessing that probably isn't what it means. But if that's the
case, then what would you write you did want to replace the outer
merge join with a hash join? Both sides have more than one table, so
if HashJoin(x) means x has to appear alone on one side of the join,
there would be no way to get what you want here. I wish the actual
behavior of pg_hint_plan were better-documented here, and I'd
appreciate an explanation of how you use it in a case like this and
what actually happens.

But all that having been said, I do think there's space for softer
constraints than what pg_plan_advice currently offers. For example,
there's currently no way to say "I'd like an index scan but I don't
care which index you use". I've been thinking we could invent
ANY_INDEX_SCAN() and ANY_INDEX_ONLY_SCAN() for that purpose at some
point, or maybe people would prefer negative constraints instead, like
NO_SEQ_SCAN(). And maybe your idea of either-way join method
constraints falls in that category too. It's easiest for me to imagine
someone wanting that for merge join, where the two sides are treated
more nearly symmetrically. But I think we would need to nail down
exactly what the semantics of that are. Given that we've got a
sublists available as a tool, we could define a "symmetric join method
request" to take two arguments where the first is the relation
identifiers, or a list of all the relation identifiers, that appear on
one side of the join, and the same for the other side of the join. So
in the example above, if you wanted to replace the nested loop with a
hash join in one direction or the other, you could write
FLIPFLOPPY_HASH_JOIN((A B) C), and if you wanted to replace the outer
merge join, you could write FLIPFLOPPY_HASH_JOIN((A B C) (D E)). I'm
not altogether convinced that's better than just writing HASH_JOIN(C)
or HASH_JOIN((D E)), and there's some definitely user and code
complexity to supporting both methods, but maybe it will turn out to
be the right thing.

(It also occurs to me that the proposed semantics of
FLIPFLOPPY_HASH_JOIN are actually both stronger and weaker than the
existing HASH_JOIN directive. It's weaker in that the sides of the
join can be switched, but it's stronger in that it constrains what has
to be on both sides of the join, whereas HASH_JOIN does not do that.
Of course, as is hopefully clear by now, this is not the only possible
set of semantics that we could choose to implement here: things that
seem simple when you think about 2-table cases are often not simple at
all when scaled up to more complex situations. More than anything
else, I want whatever we implement to be extremely well-defined, with
absolutely no room for debate about what a given advice tag does or
whether a certain plan complies with a certain advice item.)

> Postgres docs, it could fit the wiki as a cheatsheet of sorts.

Yeah, that sounds nice.

> the docs noting its not intended for production use.

I think it depends a lot on what you mean by "production use". I think
it's definitely true that pg_collect_advice is not intended for
continuous advice collection. If you try to use it that way, you'll
either start throwing away entries you wanted to keep (if the
collection limit is low) or you'll blow out memory (if the collection
limit is high). But it's completely reasonable to enable it on a
production server in a controlled way, to collect all the queries and
advice strings for one representative transaction of each type (or
even 10 or 50 representative transactions of each type). So I feel
like this is about understanding how it's intended to be used, and the
answer is definitely not "just like pg_stat_statements!".

BTW, I wonder if it would be worth considering, obviously for next
release cycle rather than this one, extending pg_stat_statements to
have the ability to grab plan advice as well, rather than building the
query normalization and deduplication features that pg_stat_statements
already has into pg_collect_advice.

---

From: Robert Haas <[email-masked]>
Date: Tue, 17 Mar 2026 13:44:06 -0400

> And I mirrored that in meson.

Right, OK. This all seems rather confusing and a bit under-documented,
but after looking it over I think you've got it correct, so committed.

---

From: Robert Haas <[email-masked]>
Date: Tue, 17 Mar 2026 17:45:52 -0400

> post-commit review of the main patch is also very welcome.

I've now committed test_plan_advice, since it seems crazy to me to
have pg_plan_advice in the tree without it and reviewers evidently
agree at least with the concept test_plan_advice is something we
should have. Here are the remaining two patches, as v21.

Separately, I also committed a fix for the GEQO crash that Alexander
Lakhin found. The patch I proposed on list was missing a bms_copy() --
as proposed, it fixed the crash but was still wrong. I added that and
committed it.

---

From: Robert Haas <[email-masked]>
Date: Wed, 18 Mar 2026 13:07:54 -0400

Hi,

Coverity complained about the fact that pgpa_planner_feedback_warning
does not do anything to free the memory used by flagbuf or detailbuf,
and Tom asked whether that failure could amount to a significant leak.
The best way to use a lot of memory here is to have a very long advice
string that doesn't do anything useful, so I created this test case:

load 'pg_plan_advice';
select set_config('pg_plan_advice.advice', v.v, false) from (select
string_agg('SEQ_SCAN(hogehogehoge' || g || ')', ' ') v from
generate_series(1,10000) g) v;
set pg_plan_advice.feedback_warnings = true;
select 1;

While this is not the worst case imaginable, it's pretty bad. There's
probably no use case for having 10000 advice items in your advice
string even if they were all valid, and here none of them are valid
for the final query (select 1). So this produces "WARNING:  supplied
plan advice was not enforced" with a 10000 line detail message where
all the lines look like this, but with different numbers:

advice SEQ_SCAN(hogehogehoge7348) feedback is "not matched"

Failure to free the buffers costs us just over 1MB of memory, which is
not wildly disproportionate given that the message itself is over half
a MB long, so I'm not sure that freeing it is all that useful here. I
don't think the context we use for planning normally sticks around for
all that long. If I'm wrong about that and it does, then we should
probably wrap our own shorter-lived context around the stuff this
module is doing rather than trying to free allocations individually.

But this does raise two points which are perhaps worthy of some
further consideration:

1. Maybe we should limit the length of the detail message. In some
other cases, like reportDependentObjects, we limit the detail message
to the first 100 problems found, and then say at the end of that
detail message how many more problems there were afterwards. We could
do that here, too. I'm not 100% sure it's worth the code, but then
again it's not much code.

2. The way the current code works is to transform the advice feedback
into a Node-tree representation which is stashed in the PlannedStmt's
extension_state, and then also passes that Node-tree representation to
pgpa_planner_feedback_warning, which generates the actual warning.
That Node-tree representation is currently used by EXPLAIN to display
the advice feedback, which I think for most people will be a nicer
interface than enabling pg_plan_advice.feedback_warnings, and it can
also be used by other extensions. For instance, we could extend
pg_stash_advice so that it looks at the advice feedback and updates
the shared store, so that users can monitor whether their stashed
advice is doing what they hoped it would.

However, in a case like this, that Node tree is actually quite large:
about 16MB. I guess that's because pgpa_planner_append_feedback() has
to do multiple allocations for every item of feedback: a C string,
DefElem, an Integer, plus whatever lappend() charges us to add to a
List. We could save that memory by adding code here to optimize for
the case where we need to generate warnings but we don't need the Node
tree for anything else. I'm inclined not to do that, because (A) I
don't think temporarily using 16MB when the user specifies 10,000
items of bogus advice is really that bad and (B) complicating the code
has its own costs, such as maybe introducing more bugs. However, maybe
someone else sees it differently.

Another idea is to try to find a more economic Node representation.
For instance, we could jam the flags into the DefElem's location
field, instead of building a separate Integer node, or we could build
the advice feedback as a giant binary blob and wrap it in a varlena
and a Const node and leave consumers to make sense of that as best
they're able, or we could invent a new Node type that's just to the
perfect thing to hold a C string and an integer. I'm inclined to think
that the first two are too hacky and the third is too special-purpose,
but again someone else might see it otherwise.

Thoughts?

---

From: Lukas Fittl <[email-masked]>
Date: Wed, 18 Mar 2026 11:59:46 -0700

> again it's not much code.

I think we have similar problems elsewhere in Postgres where a large
user input causes an even larger log output - e.g. a case I'm familiar
with are complicated queries with long IN list inputs and their
associated EXPLAIN plans being logged by auto_explain - I recently had
a case where someone reported an OOM due to auto_explain trying to log
a > 100 MB sized query plan.

I think its actually less a problem with plan advice, since presumably
we won't have ORMs generating plan advice, and even if we do it'd be
per-table - so I think its unlikely a genuine use case would use 1000s
of advice tags.

That said, I also don't think super long long messages are actually
helpful. I do wonder if we should have a more coarse GUC that limits
DETAIL lines of any kind to a maximum length (e.g. 100 kB) across the
board instead of special casing every emitter.

> advice is doing what they hoped it would.

Yeah, I think the ability for other extensions to retrieve this is
pretty important - whether in pg_stash_advice, or any other kind of
plan management extension that wants to know the outcome of applied
advice.

> but again someone else might see it otherwise.

Yeah, I feel like we're definitely constrained here by the fact that
advice tags are defined by a contrib module vs in-core - if they were
in-core we could just add a dedicated node type for them. I don't
think inventing a specialized binary format only defined in the
contrib module makes sense.

Two other ideas:

1) What if we return the utilized advice string as a separate DefElem
with a list of strings, and then the feedback just has to reference an
index into that list? (though I suppose that doesn't actually save
memory, now that I think that through -- unless we assume the caller
already has the advice string, but I don't think we can rely on that)

2) We could consider having separate DefElems for the different flag
types (i.e. "feedback_failed", "feedback_match_full", etc), and then a
list of strings attached to each - that'd save the nested DefElem and
the Integer node

Thanks,
Lukas

-- 
Lukas Fittl

---

From: Robert Haas <[email-masked]>
Date: Wed, 18 Mar 2026 18:19:31 -0400

On Wed, Mar 18, 2026 at 4:44 PM Tom Lane <[email-masked]> wrote:
> Don't know if you noticed yet, but avocet has shown [1] that one
> pg_plan_advice test case is unstable under debug_discard_caches = 1:

I had not. Thanks for the pointer.

> It looks like the appearance of "Supplied Plan Advice:" depends
> on whether the prepared query's plan got regenerated or not.
> I'm not sure if this represents a bug (ie undesirable behavior) or
> it's just that the test is being insufficiently careful about
> being reproducible.

Well, that's embarrassing: it's a copy-and-paste error. The test
prepares and executes pt1, then prepares and executes pt2, then
prepares pt3 and executes pt1, then prepares pt4 and execute p2. pt3
and pt4 are never used for anything. Also there's a related typo in a
comment. See attached.

---

From: Robert Haas <[email-masked]>
Date: Wed, 18 Mar 2026 18:34:29 -0400

On Wed, Mar 18, 2026 at 6:26 PM Tom Lane <[email-masked]> wrote:
> Ah ... so the observed behavior is because if pt2 does get replanned,
> that happens with a different always_store_advice_details setting
> than it used originally?

Close, but not quite. always_store_advice_details is set for the
even-numbered tests, so it's the same for pt2 and pt4. But
pg_plan_advice.advice is empty for the the first half of the file and
set for the second half of the file, so it's empty when pt2 is
prepared and contains SEQ_SCAN(ptab) when pt4 is prepared. That
accounts for the difference. avocet is actually doing the right thing,
and the rest of the buildfarm is doing the wrong thing for lack of
replanning.

---

From: Robert Haas <[email-masked]>
Date: Wed, 18 Mar 2026 18:57:59 -0400

On Wed, Mar 18, 2026 at 6:43 PM Tom Lane <[email-masked]> wrote:
> Well, avocet is producing the output you want, but I think the rest
> are behaving correctly given the way the script is written.

Yeah, this goes back to my "that's embarrassing" comment: obviously, I
really did not proofread this well at all.

> Anyway, I confirm that the patched output is stable with
> debug_discard_caches = 1, so LGTM.

Yeah, I also tested that here. Glad it works that way for you also.

---

From: Robert Haas <[email-masked]>
Date: Thu, 19 Mar 2026 12:54:43 -0400

> board instead of special casing every emitter.

I think it would be difficult for generic code to do something
sensible when the message is really long. I mean, it could just cut it
off after N lines, but then you have no idea how much more there would
have been, and you probably want to tell the user something about
that. You could add a completely generic message to the end like "plus
10525 more lines of output that were truncated for display," but
that's pretty unsatisfying. If you want to show something contextually
appropriate, the implementation needs to be separate for each case
even if the limit is common. Anyway, the question here is not about
such a generic mechanism, but about whether somebody wants to argue
for sticking a limit of 100 on feedback messages on the theory that
log spam is bad, or whether it's fine as-is either because (a) the
likelihood of a significant number of people hitting that limit is
thought to be too low to worry about or (b) the likelihood of someone
wanting all of those messages (e.g. for machine-parsing) is thought to
be high enough that a limit is actually worse than no limit. I do not
really have a horse in the race, so if nobody else has a strong
opinion, I'm going to leave it alone for now and consider changing it
if a strong opinion materializes later.

> already has the advice string, but I don't think we can rely on that)

I think that if we're using Integer nodes, it's just never going to be
very economical. We could use an IntList, which I believe would be
better, but there are a few complications that make me not like this
idea very much. One, what the feedback is actually about is a
reconstruction of a particular advice item into string form, not the
original string, e.g. if you input SEQ_SCAN(foo bar), the feedback
will be on SEQ_SCAN(foo) and SEQ_SCAN(bar). Two, even if you ignore
that, this would leave consumers of the data with the problem of
finding the end of the advice item unless you stored both starting and
ending indexes, which would have its own costs.

> list of strings attached to each - that'd save the nested DefElem and

But it would also very often duplicate a bunch of the strings, which
seems likely to work out to a loss more often than not. You could
avoid that by have a list of strings per unique flag combination, but
that would be extra work to compute and I think it would be less
convenient for consumers. One user-visible consequence would be that
the advice feedback in EXPLAIN output would have much less to do with
the original order of the advice string.

---

From: Robert Haas <[email-masked]>
Date: Thu, 19 Mar 2026 13:17:04 -0400

On Thu, Mar 19, 2026 at 12:46 PM Tom Lane <[email-masked]> wrote:
> I just realized that this new test module has added yet another
> execution of the entire core regression test suite ... and to add
> insult to injury, it runs the planner twice for each plannable
> query therein.  I think this is an outrageous abuse of our buildfarm
> and urgently request that you find some less climate-destroying
> way of getting reasonable test coverage of pg_plan_advice.

I don't think just nuking this is a reasonable option. During the
development of pg_plan_advice, test_plan_advice was the single most
valuable testing tool. It was not close. Manual review, both by me and
by others, found bugs; fuzz testing by Jacob Champion found bugs; AI
code review found bugs. test_plan_advice was an order of magnitude
more effective than all of those things put together. In fact, I'd say
maybe two orders of magnitude. Pretty much every significant logical
error was something that required a very specific query shape to find,
and the regression tests are by far the best repository of weird query
shapes that we have.

I think without something like test_plan_advice, the chances of us
noticing if future planner changes break pg_plan_advice are near zero,
and with it, they're quite good -- because we're not only testing the
queries that exist in the regression test suite now, but we're testing
future queries that are added to the regression test suite by people
who are hacking on the planner. Those added queries presumably create
plan shapes that are relevant to the code that they're changing,
whereas a fixed pg_plan_advice-only works if people think to add
something to it, which they very likely won't, and even if they do, I
have no confidence that they'll know what things to add and what not.
I certainly didn't know which queries in the regression tests were
going to break under test_plan_advice until I tried it.

One thing we might be able to do here to save on cycles is combine
test_plan_advice with some existing run of the regression tests. We
seem to run them to test sepgsql, to test pg_upgrade, and in
027_stream_regress.pl. I don't think we can piggyback on sepgsql here
because there are too many cases where it just won't be built or
tested, but we could possibly piggyback on one of the other runs, or
on the main regression tests. If that's not viable, another option
would be to have it run on only some buildfarm members rather than all
of them.  But if the tests aren't run by default, then I fear people
will experience quite a bit frustration the first time something
breaks only after they commit.

Before I forget, another idea that might help is to see if we can
tweak meson.build to start running this particular test earlier. I
thought about that during development, but I didn't actually do it. If
the issue is that test being last to finish, that could help. If the
issue is total resource consumption, it won't help with that.

> I would dig into why grison and schnauzer are failing this test,
> except that I don't agree that we should be running it in the
> first place.

I'll go have a look.

---

From: Robert Haas <[email-masked]>
Date: Thu, 19 Mar 2026 16:38:23 -0400

> > except that I don't agree that we should be running it in the

grison failed like this:

 CREATE INDEX ON vaccluster(wrap_do_analyze(i));
+ERROR:  indexes on virtual generated columns are not supported

This is a surprising diff, because either that command is trying to
create an index on a virtual generated column, or it's not, and it
either is or isn't for every machine in the buildfarm and under
test_plan_advice or not. The problem might be that the first
TupleDescAttr(...) == ATTRIBUTE_GENERATED_VIRTUAL test in DefineIndex
can be reached with attno == 0, which seems like it will then access
memory that is not part of the TupleDesc. If that memory happens to
contain a 'v' in the right spot, this will happen. (Is it not viable
to have TupleDescAttr() assert that the second argument is >= 0?)

skink has a failure that looks like this:

+WARNING:  supplied plan advice was not enforced
+DETAIL:  advice NESTED_LOOP_MEMOIZE(nt) feedback is "matched, failed"

I think this is caused by a minor bug in the pgs_mask infrastructure.
get_memoize_path() exits quickly when outer_path->parent->rows < 2, on
the theory that the resulting path must lose on cost. But that
presumes that we could do a plain nested loop instead, i.e. that
PGS_NESTLOOP_PLAIN is set. And it might not be. Before the pgs_mask
stuff, that case couldn't happen: enable_nestloop=off disabled all
nested loops, and enable_memoize=off disabled only the memoized
version, but there wasn't any way to disable only the non-memoized
version (which, of course, was part of the point of the whole thing).
I think the fix is as attached.

Unfortunately, the other failures look like they are pointing to a
rather more serious problem. schnauzer's got a failure that looks like
this:

+WARNING:  supplied plan advice was not enforced
+DETAIL:  advice SEQ_SCAN(pg_trigger@exists_1) feedback is "matched, failed"
+advice NO_GATHER(pg_trigger@exists_1) feedback is "matched, failed"

And an older run on skink has a failure that looks like this:

+WARNING:  supplied plan advice was not enforced
+DETAIL:  advice SEQ_SCAN(pg_trigger@exists_6) feedback is "matched, failed"
+advice NO_GATHER(pg_trigger@exists_6) feedback is "matched, failed"

What these failures have in common is that both of them involve
selecting from information_schema.views. What "matched, failed" means
is that we saw the advice target during planning and tried to
influence the plan, but then observed that the final plan doesn't
respect the supplied advice. The query for information_schema.views
involves three subplans, so the fact that exists_6 is mentioned here
is a strong hint that the AlternativeSubPlan machinery is in play
here. The query is planned once, and one of the two alternative
subplans is chosen, generating advice for that plan. Upon replanning,
the other plan is chosen, so the subplan for which we have plan advice
doesn't appear in the query at all, leading to this. Now, you might
wonder how that's possible, considering that we're planning the same
query twice in a row with advice that isn't supposed to change
anything. My guess is that it's possible because these machines are
slow and other tests are running concurrently. If those other sessions
execute DDL, they can send sinval messages, which can cause the second
planning cycle to see different statistics than the first one. That
then means the plan can change in any way except for what the advice
system already knows how to control, and choice of AlternativeSubPlan
is not in that set of things. I think I actually saw a failure similar
to this once or twice locally during development, but that was back
when the code had a lot of bugs, and I assumed that the failure was
caused by some transient bug in whatever changes I was hacking on at
the time, or some other bug that I fixed later, rather than being a
real issue. I think the reason it doesn't happen very often is because
the statistics have to change enough at just the right moment and even
on slower buildfarm machines, most of the time, they don't.

It's not really clear to me exactly where to place the blame for this
category of failure. One view is that tests are being run in parallel
and I didn't think about that, and therefore this is a defect in the
test methodology that needs to be rectified somehow (hopefully not by
throwing it out). We might be able to fix that by running the test
suite serially rather than in parallel, although I expect that since
you (Tom) are already unhappy with the time this takes, that will
probably go over like a lead balloon. Another angle is to blame this
on the decision to assign different plan names to the different
subroots that we use for the alternative subplans. If we used
exists_1, exists_2, and exists_3 twice each instead of
exists_1..exists_6, this wouldn't happen, though that idea seems
questionable on theoretical grounds. A third possible take is that not
including choice-of-alternative-subplan in the initial scope was an
error.

As of this moment, I'm not really sure which way to jump. I need to
think further about what to do about this one. We can continue the
discussion about reducing the cost at the same time; again, I am
definitely not saying that it isn't legitimate to be concerned about
the CPU cycles expended running these tests, but those CPU cycles have
found three separate problems in two days, which is not nothing.

Separately, I am now 100% convinced that I need to go revise the
pg_collect_advice patch, because that adds yet another run of the core
regression tests, but for much less possibility of any real gain. I'll
go get rid of that and figure out what, if anything, to replace it
with.

---

From: Robert Haas <[email-masked]>
Date: Thu, 19 Mar 2026 17:33:37 -0400

On Thu, Mar 19, 2026 at 4:12 PM Matheus Alcantara
<[email-masked]> wrote:
> We can add a 'priority' for the test:
>
>    'tap': {
>      'tests': [
>        't/001_replan_regress.pl',
>      ],
> +   'test_kwargs': {'priority': 50},
>    },
>
> Even if it's not help with resource consumption I think that it still
> worth adding. It reduces from ~5m to ~4m on my machine.

I tested this out here. Without this change, 'meson test' takes
2:53-2:55 for me. After making the change, I got times from 2:53-2:56,
so basically no change. But I suspect your proposal here is still the
right thing to do. I wondered if it should actually do what
src/test/regress/meson.build does:

    'test_kwargs': {
      'priority': 50,
      'timeout': 1000,
    },

 ...but it seems as though the timeout for TAP tests is already 1000s,
so maybe we don't need to change anything. Or maybe the recent
"timedout" errors in the buildfarm are a sign that 1000s isn't long
enough for this more-intensive run:

https://buildfarm.postgresql.org/cgi-bin/show_failures.pl?max_days=3&stage=timedout&filter=Submit

If so, that would be sad. On my local machine, which is a ~3 yo
MacBook, running just the "regress" test suite takes ~11.1 s, and
running just the "test_plan_advice" suite takes ~12s, so I admit that
I'm slightly confused about why this is having such a big impact for
you and Tom. Obviously I'm not running with expensive options like
debug_discard_caches or Valgrind enabled, but presumably you're not
doing that locally either and it still shaves a minute off the runtime
for you. What exactly is different, I'm not entirely sure.

Anyway, I think I should still go make your suggested change, unless
somebody objects. We may change more later, but if this provides some
relief to some people for now, it seems worth doing.

---

From: Robert Haas <[email-masked]>
Date: Thu, 19 Mar 2026 18:19:17 -0400

On Thu, Mar 19, 2026 at 6:03 PM Tom Lane <[email-masked]> wrote:
> Please note that I was citing the runtime of a much slower machine
> (longfin is a 2018 mac mini).  But in any case, what I was griping
> about was the additional cost added to a buildfarm run; I don't see
> that test_plan_advice is a lot slower than the main regression tests.
> It's just that those are already a significant investment, and we
> just iterated them another time.

Right, and there's definitely plenty of worthless crap in there that
isn't adding any value. For example, every \dWHATEVER command in the
regression test is running basically the same queries every time, and
after the first time we're probably not learning anything new. And all
the DDL commands that are part of the regression tests are fairly
useless here. The grison failure is actually triggered by a DDL
command, but I think that might just be luck rather than the
test_plan_advice module is doing anything to systematically increase
the likelihood of such findings. But I don't know how we can separate
the wheat from the chaff. Obviously a lot of the DDL and \d commands
in the tests are either setup for queries that we should care about
testing, or verification that those queries did what they were
supposed to do. If we split the main regression test suite up, so that
we had one test suite for planner-related stuff, another for DDL, and
another for data types, or something like that, then test_plan_advice
would probably mostly just need to run on the first of those. But that
kind of split seems like a lot of work.

Do you think the idea of piggybacking the test_plan_advice run onto
another run that we're already doing has any potential? That would
reduce the incremental cost quite a lot, I think.

---

From: Robert Haas <[email-masked]>
Date: Thu, 19 Mar 2026 19:11:18 -0400

On Thu, Mar 19, 2026 at 6:43 PM Tom Lane <[email-masked]> wrote:
> It would, but it's conceptually ugly and it might make it much harder
> to detangle the cause of a failure, so I don't care for it much.

It does carry that risk. *Typically* failures are going to be a
WARNING message complaining about something related to advice, so the
chance of confusion is perhaps not as high as it would be in some
other cases -- but the grison failure is a counterexample. I'm
somewhat inclined to discount that particular counterexample because
the bug is entirely unrelated to test_plan_advice or pg_plan_advice,
so I am not sure it really would have mattered if we hadn't known that
test_plan_advice was what precipitated it. But there might be other
cases where that isn't so.

> I don't have any great ideas here.  Your point about the test having
> helped to find a lot of bugs is compelling, and so is the fact that
> it's seemingly exposing more issues we've not understood yet.
> Maybe we can eventually buy back the cycles by not running it by
> default, but clearly now is not the time for that.

OK, thanks. To be honest, my biggest fear here is not that the test
doesn't have enough value, but that it has a little too much value,
i.e. that we're going to find that future planner improvements require
pg_plan_advice adjustments more often than we're all comfortable with.
Hopefully that fear is unjustified, but we're not going to know for a
while.

---

From: Robert Haas <[email-masked]>
Date: Fri, 20 Mar 2026 12:06:40 -0400

On Thu, Mar 19, 2026 at 1:02 PM Tom Lane <[email-masked]> wrote:
> FWIW, I'm inclined to think that people won't actually have this
> turned on unless they want to see that output, and then they'll
> probably want to see all of it.  So I'm inclined to leave it as-is
> for now.  If field experience teaches differently, we can always
> improve it later.

Thanks for chiming in. I'll leave it as-is for the time being. My
guess is that this will be fairly low on the list of things people
complain about, but I'll be very happy if proven wrong, since this one
is easily changed.

---

From: Robert Haas <[email-masked]>
Date: Sat, 21 Mar 2026 09:13:59 -0400

On Thu, Mar 19, 2026 at 4:38 PM Robert Haas <[email-masked]> wrote:
> schnauzer's got a failure that looks like this:
>

I spent a bunch of time looking into this. I don't have a definitive
answer to the question of what to do about it yet, but I wanted to
write down some initial thoughts.

First, I think that I broke fix_alternative_subplan when I added
disabled_nodes. Before, when disabling things just affected the cost,
the logic here would have taken what was disabled into account in
picking between alternatives. Now it doesn't, because I didn't add a
disabled_nodes field to SubPlan. You can see that it breaking with
this test case:

CREATE TABLE t1 (a int);
CREATE TABLE t2 (a int);
CREATE INDEX ON t2(a);
INSERT INTO t1 SELECT generate_series(1, 1000);
INSERT INTO t2 SELECT generate_series(1, 100000);
ANALYZE;

EXPLAIN (VERBOSE, COSTS ON)
SELECT * FROM t1
WHERE EXISTS (SELECT 1 FROM t2 WHERE t2.a = t1.a) OR t1.a < 0;

SET enable_seqscan = off;
SET enable_indexonlyscan = off;

EXPLAIN (VERBOSE, COSTS ON)
SELECT * FROM t1
WHERE EXISTS (SELECT 1 FROM t2 WHERE t2.a = t1.a) OR t1.a < 0;

With the currently-committed code, you get a hashed SubPlan both
times, and it's just disabled in the second case. But there's a
perfectly good non-hashed variant that is not disabled: scan t2 using
a parameterized Index Scan. So that sucks. I have a small patch to fix
this, which I'll post later. I don't think we can or should do
anything about this in released branches, but we should fix it in
master regardless of what happens to pg_plan_advice or
test_plan_advice.

Second, in terms of actually fixing the problem, I think the issue
here is that the scope I chose for pg_plan_advice doesn't quite fit
the goal of making test_plan_advice the canonical way of testing this
code. In order to keep this project manageable in size, I decided
that, for the first version, pg_plan_advice wasn't going to try to
control anything above the level of scan/join planning, so for example
we don't care what kind of aggregation the planner chooses. That
should be OK for test_plan_advice, because test_plan_advice works by
checking if all the advice applied cleanly, and since we didn't emit
any advice about the aggregation method in the first place, there
can't be any problem with applying it later. In other words, if the
aggregation method chosen does flip between consecutive planning
cycles, it should not cause a test_plan_advice test failure. This same
general principle applies to a bunch of other cases too, like set
operation planning: if there's more than one way to do it, the planner
can change its mind and test_plan_advice should not care. However,
there's an important exception: if something changes above the level
of scan/join planning that affects what rels are involved in scan/join
planning, then a plan change will cause a test_plan_advice failure.

The failures above are of that type: the way the AlternativeSubPlan
machinery works is that the query gets copied and replanned, and each
plan has a separate plan name. So the final plan has either
pg_trigger@exists_5 or pg_trigger@exists_6 in it, but never both.
There's one other case that I think is similar, which is the
MinMaxAggPath stuff: if we choose the MinMaxAggPath, then the final
plan will have something like t1@minmax_1 where it otherwise would
have had just t1, or perhaps t1@minmax_1 instead of t1@something_else.
These cases have something in common, which is that they are the only
cases where we make a new PlannerInfo to try replanning part of the
query in a second way. That's the pattern that causes breakage here. I
would be remiss if I did not mention that Jakub Wartak was poking at
me about the MinMaxAggPath case a while back, but I dismissed it as
out of scope, which was accurate, but that's because I didn't think at
the time that it would destabilize test_plan_advice. Now, I think it
could, although I don't think we have seen any such failures in the
buildfarm yet. Perhaps a concurrent statistics change is more likely
to flip the hashed/non-hashed SubPlan decision than the choice of
whether to use MinMaxAggPath.

One approach that I considered here is to try to unify the "sibling"
relations. If the final plan is bound to contain either
pg_trigger@exists_5 or pg_trigger@exists_6, maybe the advice shouldn't
actually think there are two separate things. Maybe instead of
subqueries exists_1 ... exists_6 we should end up with subqueries
exists_1 ... exists_3, with each name used twice. That amounts to
deciding that the patch to give each subplan a unique name got it
wrong. While this idea has some appeal, ultimately I think it's a
loser, because addressing the problem this way wouldn't actually fix
the test_plan_advice instability we're currently seeing, or at least
not necessarily. For example, in the test case earlier in this email,
INDEX_SCAN(t2@whatever) can only be applied if the non-hashed subplan
is chosen, because we won't consider a plan index scan to even be a
possibility for a full table scan. An index-only scan is considered,
but not a plain index scan. This means that even if we flattened the
rels in each pair of siblings together and called each pair by the
same name, we could still see failures to apply advice cleanly.
Moreover, that's assuming that optimizations like self-join
elimination, left join removal, and partition pruning always apply in
the same way to both copies, which doesn't sound like a safe
assumption at all.

A second approach is to try to control the initial conditions of the
two planning cycles better. Possibly just running the tests serially
instead of in parallel would get that done, but that seems too slow to
consider and, even if we did it anyway, I'm not all that sure that an
autoanalyze or autovacuum in the background couldn't mess us up. Or,
alternatively, if we could keep the second planning cycle from seeing
different statistics than the first, I think that would do it, but I
don't think there's a feasible way of doing that.

So I'm left with the idea that to get test_plan_advice to be fully
stable on these slower machines, it will probably be necessary to make
it control which AlternativeSubPlan is chosen and whether a
MinMaxAggPath is chosen or not. I have some ideas about how to
accomplish that in a reasonably elegant fashion without adding too
much new machinery, but I need to spend some more time validating
those ideas before committing to a precise course of action. More
soon.

---

From: Robert Haas <[email-masked]>
Date: Tue, 24 Mar 2026 16:47:12 -0400

> I think the fix is as attached.

The new test in that version was exactly backwards. I have corrected
that issue and committed this.

---

From: Robert Haas <[email-masked]>
Date: Tue, 24 Mar 2026 17:09:51 -0400

> those ideas before committing to a precise course of action. More

Here is v22. There are four new patches.

0001 adds a disabled_nodes fields to SubPlan, to fix the bug that I
identified in the email to which this is a reply.

0002-0004 are an attempt to fix the remaining buildfarm failures not
already addressed (or attempted to be addressed, anyway) by other
commits. The basic idea, implemented by 0004, is to add a
DO_NOT_SCAN() advice tag. This advice is generated when we consider a
MinMaxAggPath or a hashed SubPlan. In either case, all relations which
are part of the non-selected alternative are marked DO_NOT_SCAN(),
which works like scan type advice but disables every possible scan
type rather than still allowing exactly one of them. Unless I've
missed something, this should be sufficient to make pg_plan_advice
stabilize which of two alternative SubPlans we pick and whether or not
a min/max aggregate is chosen. 0002 does some preliminary refactoring
to provide a more centralized way of tracking per-PlannerInfo details
within pg_plan_advice. 0003 makes the necessary change to
src/backend/optimizer, which consists of adding an alternative_root
field to each PlannerInfo and setting it appropriately. 0004 then
updates pg_plan_advice to implement DO_NOT_SCAN().

0005 is the pg_collect_advice module from previous versions of the
patch set. The main change here is that I completely rewrote the TAP
test, which previously was running the entire regression test suite
yet another time. That's been replaced with something that is much
faster and much better targeted at properly testing the shared advice
collector. Aside from that, I added one more check for
InvalidDsaPointer where the code was previously lacking one.

0006 is the pg_stash_advice module from previous versions of the patch
set. I have adjusted this to be much safer against permanent DSA
leaks. It now uses dshash_find_or_insert_extended instead of relying
on the ability to dshash_find a just-inserted entry without error. It
now also holds an LWLock while inserting or updating an entry in the
dshash table, for reasons explained in the comments. On the other
hand, it no longer unnecessarily holds the LWLock in exclusive mode
when looking up advice strings for automatic application, which was a
rather silly mistake in the previous version. A few additional tests
have been added. Alphabetization in contrib/Makefile has been fixed.

---

From: Lukas Fittl <[email-masked]>
Date: Wed, 25 Mar 2026 16:59:06 -0700

Hi Robert,

> identified in the email to which this is a reply.

This looks good, and is consistent with how it would have worked
before the introduction of disabled nodes (since costs would have just
been very high and thus discourage a subplan with many disabled
nodes).

Only nit is that the commit hash reference in the commit message
doesn't seem right, I think you probably meant
e22253467942fdb100087787c3e1e3a8620c54b2

> updates pg_plan_advice to implement DO_NOT_SCAN().

For 0002:

I think that overall looks like a good refactoring, with two minor notes:

> index fee88904760..70139ff42be 100644
> @@ -2017,34 +1949,64 @@ pgpa_planner_feedback_warning(List *feedback)
>                  errdetail("%s", detailbuf.data));
>  }
>
> -#ifdef USE_ASSERT_CHECKING
> -
>  /*
> - * Fast hash function for a key consisting of an RTI and plan name.
> + * Get or create the pgpa_planner_info for the subroot with the given
> + * plan_name.
>  */
> -static uint32
> -pgpa_ri_checker_hash_key(pgpa_ri_checker_key key)
> +static pgpa_planner_info *
> +pgpa_planner_get_proot(pgpa_planner_state *pps, PlannerInfo *root)
>  {

I'd word that as "Get or create the pgpa_planner_info for the given
PlannerInfo and its associated plan_name", since you're not passing a
plan_name as the argument.

> @@ -2053,19 +2015,34 @@ pgpa_ri_checker_save(pgpa_planner_state *pps, PlannerInfo *root,
>                      RelOptInfo *rel)
> {
> ...
> +   if (proot->rid_array_size <= rel->relid)
> +   {
> +           int                     new_size = Max(proot->rid_array_size, 8);
> +
> +           while (new_size < rel->relid)
> +                   new_size *= 2;

This could use pg_nextpower2_32 on the rel->relid instead of the
manual while loop.

---

For 0003:

I wonder if "original_root" wouldn't be more correct here as a name
(instead of "alternative_root"), since if I follow the implementation
correctly, you are adding a pointer on each alternative root, back to
the original root that the alternative was copied from.

I also wonder if maybe we should be more narrow in what we keep here.
It seems 0004 mainly needs the original plan name, so maybe its better
if we just keep that for targeting purposes, vs a full pointer to the
PlannerInfo. The planner makes an effort to zap unused subplans at the
end of set_plan_references, and I think this new field would then be
the only pointer to those unused subplans. If we decided to add an
early free there at some point (instead of just making them a NULL
entry in the list), that'd break pg_plan_advice.

---

For 0004:

> +    /*
> +     * If the corresponding PlannerInfo has an alternative_root, then this is
> +     * the plan name from that PlannerInfo; otherwise, it is the same as
> +     * plan_name.
> +     *
> +     * is_alternative_plan is set to true for every pgpa_planner_info that
> +     * shares an alternative_plan_name with at least one other, and to false
> +     * otherwise.
> +     */
> +    char       *alternative_plan_name;
> +    bool        is_alternative_plan;

Per the earlier note, I think using "original_plan_name" would make
more sense here, because it'll be the name the alternatives are based
on. I also think "has_alternative_plan" is more clear for the boolean,
since it'll be set on the info for the original info as well, if I
understand correctly.

Otherwise 0004 looks like a reasonable compromise for now. I feel like
we can find better ways of doing this over time, and there are parts
I'm not excited about (e.g. the targeting feels a bit brittle when it
comes to anything that'd cause generated alternative subplan names to
change), but I think it works for now.

---

From: Robert Haas <[email-masked]>
Date: Thu, 26 Mar 2026 09:55:53 -0400

> e22253467942fdb100087787c3e1e3a8620c54b2

Whoops. Obviously got the wrong thing stuck in my cut and paste buffer
when I was writing that. Thanks for checking it. I'm going to go ahead
and commit this, because I'm pretty confident that it's correct, and
the rest of these patches are not going to fix the buildfarm
instability without it, and I'm pretty sure multiple committers are
pretty tired of seeing these test_plan_advice failures already.

> plan_name as the argument.

Right, the comment isn't quite correct. I don't think your rewording
is quite right either, though, because there's really no reason to
mention plan_name here at all. I'll adjust it.

> This could use pg_nextpower2_32 on the rel->relid instead of the

OK, will change that also, and then also commit this one.

> the original root that the alternative was copied from.

To me, that seems less clear. Someone might think that the original
root means the toplevel PlannerInfo, or that whatever PlannerInfo we
had around when we created the current PlannerInfo will be the
original_root. But in fact we are only using this in a much more
narrow situation, namely, when we're creating a new PlannerInfo as a
way to consider an alternative implementation of the same portion of
the query. That is, I think alternative conveys a sibling
relationship, and original doesn't necessarily do so.

> entry in the list), that'd break pg_plan_advice.

The dangling pointers are a good point; I agree that's bad. However,
I'd be more inclined to fix it by nulling out the alternative_root
pointers at the end of set_plan_references. I think that would just be
the case where root->isAltSubplan[ndx] && root->isUsedSubplan[ndx].
The reason I'm reluctant to just store the name is that there's not an
easy way to find a PlannerInfo by name. I originally proposed an
"allroots" list in PlannerGlobal, but we went with subplanNames on
Tom's suggestion. I subsequently realized that this kind of stinks for
code that is trying to use this infrastructure for anything, for
exactly this reason, but Tom never responded and I never pressed the
issue. But I think we're boxing ourselves into a corner if we just
keep storing names that can't be looked up everywhere. It doesn't
matter for the issue before us, so maybe doing as you say here is the
right idea just so we can move forward, but I think we're probably
kidding ourselves a little bit.

> From a design perspective, I'm -1 on storing of full query text
> strings in shared memory when the shared collector mode. With large
> query texts and without an aggregate MB size limit that's an
> expressway into OOM land, even if you used a low value like 100
> entries max, because ORMs are just really good at creating large query
> texts unexpectedly. I'm also skeptical whether that's a good idea for
> the local collection mode, but it'd be less problematic there.
> Overall, I think this needs to rely on queryid instead and not store
> query texts. I would not make queryid optional, but instead enable it
> automatically - which fits together with pg_stash_advice taking it as
> input.
>
> I realize not having query texts reduces its effectiveness (since you
> don't see which parameters produced which plan advice), but it still
> helps surface which different advice strings where seen for which
> query IDs, letting you identify if you're getting a mix of bad and
> good plans. And I'm just really worried people will enable this on
> production in shared collection mode and take down their system.

I fully admit that pg_collect_advice is crude, but I don't think
ripping out some portion of the limited functionality that it has is
going to get us where we want to be. If it hadn't collected the query
strings, it would have been useless for the purpose for which I
originally wrote it. We could add a GUC for a length limit, perhaps,
but I think the real feature that this needs to be used in the way
that you seem to want to use it is deduplication, and as I said
earlier, I think we should consider adding the advice collection logic
to pg_stat_statements rather than building an alternative version of
that module with overlapping functionality. If you think this is a
sufficiently large foot-gun that we shouldn't ship it, then I'll just
drop this patch for this cycle and we can revisit what to do for next
cycle. It's not critical, and with more time we can talk through
possible approaches and have time to code something up in a
responsible way. There's just no time to make big design changes right
now. If some small adjustment (like adding a GUC to limit the length
of the query string we're willing to collect) elevates it to
acceptability then I'm happy to do that, but otherwise we should just
revisit the topic for next cycle.

> From a design perspective, I'm worried about the fact that we lose the
> stashed advice on a restart. e.g. imagine a DBA using pg_stash_advice
> to pin a query that sometimes picks a bad plan to the good plan, but
> then their cloud provider applies a security update overnight.
> Suddenly the database is slow because the bad plans are being picked
> again.
>
> pg_hint_plan's solution to this (the "hints" table [0]) uses an actual
> table managed by the extension - but I suspect that doesn't fit the
> picture, since it'd be per database, etc. It does have the benefit of
> being restart safe though, and being copied to replicas.
>
> I wonder if we could find a way to dump and restore the advice stash
> information via a file, so its at least crash and restart safe?

I think if we want to ship this extension in this release, then the
only alternative is to tell users they have to put in place a manual
process for this. That is not great, but the original version of
pg_prewarm had the exact same issue, and I don't think anyone thought
that made it useless. Indeed, pginfcore still has nothing comparable
to autoprewarm, and I'm not saying that as a way of throwing shade on
pgfincore.

What I'm concerned about with this module is completely different: I'm
wondering whether the approach it takes to using DSA is OK, whether
the security model is adequate, and that sort of thing. Expanding the
scope is 100% off the table in my book. Feature freeze is in less than
two weeks.

---

From: Matheus Alcantara <[email-masked]>
Date: Thu, 26 Mar 2026 11:30:40 -0300

On 26/03/26 10:55, Robert Haas wrote:
> that module with overlapping functionality.
>

I also think that we should consider adding the advice string on 
pg_stat_statements. It seems to make more sense to me IMHO.

Adding support for auto_explain to explain(plan_advice, ...) (or any 
other custom explain option from loadable modules) would help or make 
sense here? I have been thinking about this for a while.

---

From: Robert Haas <[email-masked]>
Date: Thu, 26 Mar 2026 10:37:15 -0400

> sense here? I have been thinking about this for a while.

I think that some generic support for custom explain options in
auto_explain is a good idea, but if you use that method to collect
advice strings, you're going to have quite a bit of log-filtering work
to do to get anything useful out of it. That might be fine for some
people, and it's certainly better than nothing, but I think eventually
we want a cleaner way. But still, +many for upgrading auto_explain
with this feature.

---

From: Robert Haas <[email-masked]>
Date: Thu, 26 Mar 2026 13:20:21 -0400

> pretty tired of seeing these test_plan_advice failures already.

Done.

> mention plan_name here at all. I'll adjust it.

Done and committed, after also adjusting the memory context handling
to avoid re-breaking GEQO.

> kidding ourselves a little bit.

Here's a new version, where I've replaced alternative_root by
alternative_plan_name, serving the same function.

---

From: Lukas Fittl <[email-masked]>
Date: Thu, 26 Mar 2026 12:51:29 -0700

Hi Robert,

> alternative_plan_name, serving the same function.

Great, I think that's better for now, and if we have a broader use
case in the future we can always adjust this to be the full
PlannerInfo.

That said, reflecting on the change, I wonder if its odd that we're
copying a string pointer instead of making an actual string copy. I
think that's probably okay in practice?

I'm still 50/50 on the naming here, since we have the alternative sub
plan that has an "alternative plan name" that's not that of the
alternative itself, but rather the base plan that was utilized. But I
see your concern regarding the naming being confusing in terms of what
the "original" or "base" would actually refer to. I've also considered
whether something like "alternative_plan_group" could make sense
(since all alternative sub plans will have the same value), but maybe
that conveys too much intent on what this is used for.

That said, I think for now, to get the buildfarm happy again, v23/0001
seems good.

v23/0002 also looks good.

Thanks,
Lukas

--
Lukas Fittl

---

From: Mark Dilger <[email-masked]>
Date: Thu, 26 Mar 2026 16:20:49 -0700

Hi Robert,

Thanks for the pg_plan_advice patches!  I've tried hard to attack them, to
get
them to fail in some catastrophic way.  You seem to have hardened the code
well.  I found only one concern for you to consider, a kind of memory leak
ratchet:

Once the system reaches memory pressure where:
  - The 8192-byte DSA size class is exhausted (needs a new DSM segment, OS
refuses)
  - Smaller size classes still have free space in existing superblocks

Then every single query that triggers advice collection will:
  1. Successfully allocate an advice entry from existing free space
  2. Enter store_shared_advice, hit the same chunk boundary
  3. Fail to allocate the chunk
  4. Leak the advice entry
  5. Reduce remaining free space in the small size classes

This continues until the small size classes are also exhausted, at which
point
make_collected_advice itself fails (the DSA area has been consumed by leaked
entries). The ratchet is self-reinforcing: each failure guarantees the next
failure while consuming more resources, assuming nobody else is freeing
memory simultaneously.

I looked for situations where something inside postgres would keep retrying
after the OOM, but the most likely I think is just an application that
treats
OOM as a transient error and keeps retrying.

See the make_collected_advice() call in pg_collect_advice_save(); the point
where DSA memory is allocated but not yet linked into any data structure.
Everything downstream from here (the four dsa_allocate0 calls inside
store_shared_advice) can fail and leak it.

-- 

*Mark Dilger*

---

From: Robert Haas <[email-masked]>
Date: Thu, 26 Mar 2026 19:25:57 -0400

> think that's probably okay in practice?

Normally, all of planning happens in the same memory context. Under
GEQO, joins are planned in shorter-lived contexts that are reset, but
I don't think a new PlannerInfo can get created in one of those
short-lived contexts. At any rate, there's no point in having two
copies of the same string in the same memory context.

> that conveys too much intent on what this is used for.

Let's go with this for now, and if a consensus emerges that I got it
wrong, we can always change it.

> v23/0002 also looks good.

Thanks. Committed. Nothing has obviously broken so far, BUT even
machines like skink that were failing weren't failing on every run, so
it may be a while before we get a clear view of the situation --
unless of course this didn't fix it or even made things worse, in
which case we might find out a lot faster. Hopefully not, but then I
thought this was going to work the first time.

In the meanwhile, we should try to make a decision on what to do about
pg_collect_advice and pg_stash_advice.

---

From: Jakub Wartak <[email-masked]>
Date: Fri, 27 Mar 2026 09:00:03 +0100

On Thu, Mar 26, 2026 at 6:20 PM Robert Haas <[email-masked]> wrote:
>[..v23]

0003: please be the judge here, as I'm not sure. Isn't there some too high
concurrency hit in pg_get_collected_shared_advice? If I do
pgbench -M extended -c 12 -j 12 -P 1 -S:

    progress: 59.0 s, 191008.4 tps, lat 0.063 ms stddev 0.200, 0 failed
    progress: 60.0 s, 197571.2 tps, lat 0.061 ms stddev 0.026, 0 failed
    progress: 61.0 s, 189825.5 tps, lat 0.063 ms stddev 0.208, 0 failed
    progress: 62.0 s, 197082.4 tps, lat 0.061 ms stddev 0.027, 0 failed
    progress: 63.0 s, 69345.9 tps, lat 0.173 ms stddev 1.651, 0 failed
    progress: 64.0 s, 47243.6 tps, lat 0.251 ms stddev 2.128, 0 failed
    progress: 65.0 s, 48211.6 tps, lat 0.247 ms stddev 2.156, 0 failed

there is visible collapse from 190k to 48k tps was due to constant flood
of artificial calls of: select count(*) from pg_get_collected_shared_advice();

The code does LW_SHARED there over potentially lots of of tuplestore_putvalues()
calls. However any other backend does pgca_planner_shutdown()->
pg_collect_advice_save()->store_shared_advice() which is trying to grab
LW_EXCLUSIVE lock, so everything might be be blocked across whole cluster? (I
mean for the duration of tuplestore entry and that seems to even talk about
"tape"/"disk", so to me it looks like prolonged I/O operations for temp might
impact CPU-only planning stuff?)

Maybe it is possible to buffer those reads under LW_SHARED into
backend-only (private)
memory and later just fill tuplestore later to avoid such hazard? (but the
obvious problem is how much memory we can have and how big shared area can
become). Or maybe after some time simply release it and sleep and re-take it?

0004: question, why in the pg_get_advice_stashes() the second call to
dshash_seq_init() nearby "Emit results" is done with exclusive=true , but
apparently only reads it?

-J.

---

From: Lukas Fittl <[email-masked]>
Date: Fri, 27 Mar 2026 01:31:11 -0700

> concurrency hit in pg_get_collected_shared_advice? If I do

I've been thinking more about 0003 (pg_collect_advice) today, and I'm
getting increasingly skeptical that we should try to get that into 19.
It just feels like there is more design work to be done here, and I
don't see the pressing need to have this in place.

Instead, I wonder if we should just add a "debug_log_plan_advice"
setting to pg_plan_advice, that always logs the plan advice when
enabled. Basically like "always_store_advice_details", but emit a log
line in addition to doing the work. That could either be enabled on a
single session with a sufficiently high client_min_messages to consume
it directly, or written to the log for a few minutes when trying to
capture production activity (on small production systems where the
logging overhead is acceptable).

I don't see a log-based approach be less useful than the shared memory
approach, because I think our aggregation design here is not right yet
(and doesn't scale to production traffic), and so we might as well
have the community try out some things with the log output instead.

For the other one 0004 (pg_stash_advice) I feel its worth trying to
get it in, if we can figure out the remaining issues. I'll try to do
another pass on that tomorrow after some sleep.

Thanks,
Lukas

-- 
Lukas Fittl

---

From: Robert Haas <[email-masked]>
Date: Fri, 27 Mar 2026 08:55:41 -0400

On Fri, Mar 27, 2026 at 2:00 AM Alexander Lakhin <[email-masked]> wrote:
> I could not reproduce recent failures from skink and morepork so far, but
> I found that changing some GUCs can trigger similar warnings, e.g.:

Thanks for the reports.

> echo "geqo_threshold = 8" >/tmp/extra.config

Failures here are expected. When planning is done via GEQO, not all
join orders are explored, and pg_plan_advice can only constrain the
join order from among the options considered by the planner. So, with
GEQO + test_plan_advice, any given test is going to pass if the second
round of planning considers the join order chosen by the first, and
fail otherwise.

This could be improved at some point in the future. For example,
somebody could add hooks to GEQO so that pg_plan_advice can cause it
to generate only candidates which are consistent with the supplied
advice. In practice, I'm not sure this is going to be a good use of
time. I suspect the energy would be better invested in improving GEQO
or coming up with a more useful replacement. The gap that exists here
doesn't mean that you can't use pg_plan_advice with GEQO; it only
means that you are going to have a bad time using them together if you
provide *complete* (or nearly-complete) plan advice.

> echo "join_collapse_limit = 1000" >/tmp/extra.config

The cause here actually seems to be GEQO once again. Raising the
join_collapse_limit causes some join problems to get bigger, which has
the result that they then use GEQO. At least for me, if I also bump up
geqo_threshold, the failures go away.

> and an assertion failure with:
> enable_parallel_append = off
> enable_partitionwise_aggregate = on
> cpu_tuple_cost = 1000
>
> TRAP: failed Assert("relids != NULL"), File: "pgpa_scan.c", Line: 248, PID: 1956762

Obviously, this one's a bug. I think the attached should fix it.

---

From: Robert Haas <[email-masked]>
Date: Fri, 27 Mar 2026 09:08:30 -0400

> impact CPU-only planning stuff?)

Yeah ... I mean, I don't know what you want here.  If you fetch very
large quantities of data under a shared lock while concurrent activity
is trying to add data under an exclusive lock, that's going to be
slow. Now, as you say, there are ways to improve this. However, I
don't feel like running pg_get_collected_shared_advice() in a tight
loop is a normal use case. Normally you would turn it on, run a bunch
of queries, and then run that once at the end. Even that could hit
some issues because every session will be fighting to insert into the
hash table, but here you've made it much worse in a way that I would
say is artificial.

> apparently only reads it?

Good question. Actually, couldn't both of those loops use a shared lock only?

---

From: Robert Haas <[email-masked]>
Date: Fri, 27 Mar 2026 10:20:30 -0400

>   5. Reduce remaining free space in the small size classes

Yeah, that's a leak. I just got through trying to harden
pg_stash_advice so that kind of thing can't happen, but I failed to
realize that pg_collect_advice has a version of the same issue.

Thanks,

---

From: Robert Haas <[email-masked]>
Date: Fri, 27 Mar 2026 11:44:15 -0400

> logging overhead is acceptable).

Sure, we could do something like that. It means that people have to do
log parsing to get the advice out cleanly, but it avoids the concerns
about memory utilization, and it's simple to code.

> another pass on that tomorrow after some sleep.

Let me just talk a little about the way that I see the space of
possible designs here. Let's set aside what we do or do not have time
to code for a moment and just think about what makes some kind of
sense in theory. I believe we can divide this up along a few different
axes:

1. Where is advice stored for on-the-fly retrieval? Possible answers
include: in shared memory, in local memory, in a table, on disk. But
realistically, I doubt that "in a table" is a realistic option. Even
if we hard-coded a direct index lookup i.e. without going through the
query planner, I think this would be a fairly expensive thing to do
for every query, and if this is going to be usable on production
systems, it has to be fast. I am absolutely certain that "on disk" is
not a realistic option. Local memory is an option. It has the
advantage of making server-lifespan memory leaks impossible, and the
further advantage of avoiding contention between different backends,
since each backend would have its own copy of the data. The big
disadvantage is memory consumption. If we think the advice store is
going to be small, then that might be fine, but somebody is
potentially going to have thousands of advice strings stored,
duplicating that across hundreds (or, gulp, thousands) of backends is
pretty bad.

2. What provision do we have for durability? Possible answers include:
in a table, on disk, nothing. I went with nothing, partly on the
theory that it gives users more flexibility. We don't really care
where they store their query IDs and advice strings, as long as they
have a way to feed them into the mechanism. But of course I was also
trying to save myself implementation complexity. There are some tricky
things about a table: as implemented, the advice stores are
cluster-wide objects, but tables are database objects. If we're
supposed to automatically load advice strings from a table that might
be in another database into an in-memory store, well, we can't. That
could be fixed by scoping stores to a specific database, which would
be inconvenient only for users who have the same schema in multiple
databases and would want to share stashes across DBs, which is
probably not common. A disk file is also an option. It requires
inventing some kind of custom format that we can generate and parse,
which has some complexity, but reading from a table has some
complexity too; I'm not sure which is messier.

3. How do we limit memory usage? One possible approach involves
limiting the size of the hash table by entries or by memory
consumption, but I don't think that's too valuable if that's all we
do, because presumably all that's going to do is annoy people who hit
the limit. It could make sense if we switched to a design where the
superuser creates the stash, assigns it a memory limit, and then
there's a way to give permission to use that stash to some particular
user who is bound by the memory limit. In that kind of design, the
person setting aside the memory has different and superior privileges
to the person using it, so the cap serves a real purpose. A
complicating factor here is that dshash doesn't seem to be well set up
to enforce memory limits. You can do it if each dshash uses a separate
DSA, but that's clunky, too. A completely different direction is to
treat the in-memory component of the system as a cache, backed by a
table or file from which cold entries are retrieved as needed. The
problem with this - and I think it's a pretty big problem - is that
performance will probably fall off a cliff as soon as you overflow the
cache. I mean, it might not, if most of the requests can be satisfied
from cache and a small percentage get fetched from cold storage, but
if it's a case where a uniformly-accessed working set is 10% larger
than the cache, the cache hit ratio will go down the tubes and so will
performance.

4. How do we match advice strings to queries? The query ID is the
obvious answer, but you could also think of having an option to match
on query text, for cases when query IDs collide. You could do
something like store a tag in the query string or in a GUC and look
that up, instead of the query ID, but then I'm not sure why you don't
just store the advice string instead of the tag, and then you don't
need a hash table lookup anymore. You could do some kind of pattern
matching on the query string rather than using the query ID, but that
feels like it would be hard to get right, and also probably more
expensive. There are probably other options here but I don't know what
they are. I doubt that it makes sense from a performance standpoint to
delegate out to a function written in a high-level language, and if
you want to delegate to C code then you can just forget about
pg_stash_advice and just use the add_advisor hook directly. I really
feel like I'm probably missing some possible techniques, here. I
wonder if other people will come up with some clever ideas.

5. What should the security model be? Right now it's as simple as can
be: the superuser gets to decide who can use the mechanism. But also,
not to be overlooked, an individual session can always opt out of
automatic advice application by clearing the stash_name GUC. It
shouldn't be possible to force wrong answers on another session even
if you can impose arbitrary plan_advice, but there is a good chance
that you could find a way to make their session catastrophically slow,
so it's good that users can opt themselves out of the mechanism.
Nonetheless, if we really want to have mutually untrusting users be
able to use this facility, then stashes should have owners, and you
should only be able to access a stash whose owner has authorized you
to do so. This is all a bit awkward because there's no way for an
extension to integrate with the built-in permissions mechanisms for
handling e.g. dropped users, and in-memory objects are a poor fit
anyway. Also, all of this is bound to have a performance cost: if
every access to a stash involves permissions checking, that's going to
add a possibly-significant amount of overhead to a code path that
might be very hot. And, in many environments, that permissions check
would be a complete waste of cycles. Things could maybe be simplified
by deciding that stash access can't be delegated: a stash has one and
only one owner, and it can affect only that user and not any other.
That is unlike what we do for built-in objects, but it simplifies the
permissions check to strcmp(), which is super-cheap compared to
catalog lookups. All in all, I don't really know which way to jump
here: what I've got right now looks almost stupidly primitive, and I
suspect it is, but adding complexity along what seem to be the obvious
lines isn't an obvious win, either.

6. How do we tell users when there's a problem? Right now, the only
available answer is to set pg_plan_advice.feedback_warnings, which I
don't think is unreasonable. If you're using advice as intended, i.e.
only for particular queries that really need it, then it really
shouldn't be generating any output unless something's gone wrong, but
I also don't think everyone is going to want this information going to
the main log file. Somehow percolating the advice feedback back to the
advisor that provided it -- pg_stash_advice or whatever -- feels like
a thing that is probably worth doing. pg_stash_advice could then
summarize the feedback and provide reports: hey, look, of the 100
queries for which you stashed advice, 97 of them always showed /*
matched */ for every item of advice, but the last 3 had varying
numbers of other results. Being able to query that via SQL seems like
it would be quite useful. I tend to think that it's almost a given
that we're going to eventually want something like this, but the
details aren't entirely clear to me. It could for example be developed
as a separate extension that can work for any advisor, rather than
being coupled to pg_stash_advice specifically -- but I'm also not
saying that's better, and I think it might be worse. Figuring out
exactly what we want to do here is a lot less critical than the items
mentioned above because, no matter what we do about any of those
things, this can always be added afterwards if and when desired. But,
I'm mentioning it for completeness.

If there are other design axes we should be talking about, I'm keen to
hear them. This is just what came to mind off-hand. Of course, I'm
also interested in everyone's views on what the right decisions are
from among the available options (or other options they may have in
mind).

Thanks,

---

From: Lukas Fittl <[email-masked]>
Date: Sun, 29 Mar 2026 11:58:39 -0700

Hi Robert,

> duplicating that across hundreds (or, gulp, thousands) of backends is

I've been pondering this particular question for the last few days
(because pg_hint_plan uses a table, so in a sense the question is, why
not do that), and I've come to the conclusion that I think your choice
of shared memory seems reasonable, especially in combination with the
stash GUC being a way to control which stash gets used.

I don't think local memory makes as much sense, because realistically
one would want their advice applied to all backends, and whilst we
could invent some synchronization mechanism, that seems more brittle
than managing shared memory.

The other problem of using a table (if we were to go that route),
besides performance overhead, is that advice would have to be tied to
individual databases where the extension was created - and for
multi-tenant use cases where you have a one-customer-per-database
setup, it'd basically be unusable. I think the combination of shared
memory and the GUC mechanism would work really well for that since you
could pick the advice stash to use for a given database using ALTER
DATABASE, without repeating the advice definitions.

It is worth noting in such a use case, one could still have a problem
with queryids being different between databases, but as of 787514b30bb
("Use relation name instead of OID in query jumbling for
RangeTblEntry") that should no longer be the case, if the
schemas/queries match.

> complexity too; I'm not sure which is messier.

I think a simple disk file is the way to go, similar to how
autoprewarm works with its "autoprewarm.blocks" file. Its a bit
awkward that that just sits in the main data directory, but since
pg_prewarm already does it today, I think its okay to have another
contrib module do the same. As noted I'm mainly worried about restarts
that the user didn't control, causing advice that was set to be lost.

I've attached a patch of how that could look like on top of your v23,
that copies the modified stash information to a
"pg_stash_advice.entries" file, and loads it after restarts.

> than the cache, the cache hit ratio will go down the tubes and so will

Because the number of entries here is controlled by the user (i.e. its
not a function of the workload, but a function of how much advice you
as a user have set), I'm much less worried about memory usage, as long
as we document it clearly how to measure the amount of memory used.

We could also consider having a parameter that sets a maximum
size/number of entries, and require you to remove entries if that is
exceeded.

I agree on your concerns that a hybrid design (i.e. one that falls
back to on-disk) has a performance cliff that will be unexpected. I
don't think we need to solve for this use case of having that many
advice strings for now, as I think the main utility of pg_stash_advice
is to fix a handful of badly behaving queries through manual
intervention, not to automatically fix thousands of them.

> wonder if other people will come up with some clever ideas.

I think for what makes sense in tree at this point, simple queryid
matching is the way to go.

I'm sure there are better ways to do matching of queries (i.e. make it
dependent on parameters in some way, be smart about significant table
size changes, etc), but we can let the community iterate on that out
of tree, since they can just build an extension like pg_stash_advice
that use the functions provided by pg_plan_advice.

> lines isn't an obvious win, either.

I think the current trade-off is probably okay in terms of requiring a
superuser or its equivalent to grant access to create stash entries,
and allow unprivileged users to opt out of applied advice.

In practice for a good amount of our user base these days the question
will be "Does my cloud provider give me access to create stash
entries", so its maybe worth thinking about if we could also allow
pg_maintain to manage entries by default?

> I'm mentioning it for completeness.

I think figuring out a feedback mechanism would be a good thing, but
we could definitely add that in a later release without a major design
change of pg_stash_advice, I think. And as you note,
pg_plan_advice.feedback_warnings exists as a mechanism today to
validate whether advice was applied as expected.

> from among the available options (or other options they may have in

I can't think of other angles at this point, at least not in the
context of what it makes sense to do in-tree for this release.

Thanks,
Lukas

-- 
Lukas Fittl

---

From: Robert Haas <[email-masked]>
Date: Mon, 30 Mar 2026 10:53:30 -0400

Hi,

Thanks for taking the time to respond. My reading of your comments is
that we are in overall agreement on the design, with the possible
exception of persisting data cross restarts. I will write more about
that topic below; but I think if that's the only design disagreement
we have, it makes sense to go forward with committing the patch that I
have, and we can continue to discuss whether we want to add something
related to persistence. The only reason not to do that would be if
there were a consensus that the lack of a persistence framework was
such a critical defect that we shouldn't ship this at all without
that, but I don't agree with that idea and I think it would be a
pretty strong position for someone to take.

> "pg_stash_advice.entries" file, and loads it after restarts.

I'll be honest: I don't like this design much at all, but I do see the
practical advantages of it, and we have done similar things elsewhere,
in particular in autoprewarm. Before I get to the specifics of your
patch, let me complain about some things that I don't like at the
design level. We lose a lot by directing data through a bespoke
mechanism rather than handling it as table data. There are no
checksums, so we have less protection against corruption. There is no
write-ahead logging, so data does not make it to standbys, which is
more of a potential issue for pg_stash_advice than it is for
autoprewarm. All the code to read and write the file is specific to
this contrib module, so it can have its own bugs separate from every
similar module's bugs. The data can't easily be examined and
manipulated from SQL as table data can. It's just a messy one-off that
solves a practical problem but is otherwise not very nice. Of course,
sometimes such messy one-offs are the right answer.

In terms of the patch itself, the concurrency situation here seems
noticeably worse than with autoprewarm. In that case, there's only one
authorized writer at a time, tracked via pid_using_dumpfile. But in
this case, it seems like multiple backends could be writing to the
temporary dump file at the same time, which could result in a
corrupted file that doesn't reload properly. Your code also has a race
condition when reloading the data: the first arriving backend tries to
reload the flat file, but any other backends that arrive while that's
in progress see no stashed advice, and if the load fails for some
reason, it's never retried, and the first modification to the
in-memory state will clobber the file. autoprewarm has this issue to
some extent as well, but that's more OK there because recreating the
contents of shared buffers is only an approximate good, but people
probably don't want their stashed advice to disappear out from under
them if it was billed as persistent. That said, I'm not entirely
opposed to a design where there's a small window where the advice
stash is empty after a restart, because avoiding that means that it
has to be safe to do the reload of saved advice from the middle of a
query planning cycle, which is probably true with a flat-file design
but wouldn't be true with a table. Still, I don't know whether the
current behavior is deliberate or accidental.

I also feel a bit uncomfortable with the idea of rewriting the entire
file on every single change. If the hypothesis that this is only for
adjusting the behavior of a small number of critical queries is
correct, then it won't matter, but if people start using this for lots
of queries, it's potentially painful. Neither autoprewarm nor
pg_stat_statements does that. pg_stat_statements reads data only at
postmaster startup and writes data only at postmaster shutdown, so it
simply accepts loss of incremental changes in case of a crash, but
that also means it doesn't read and write the file repeatedly.
autoprewarm writes the file periodically from a background worker so
that the on-disk state doesn't drift too far out of sync with what's
in memory, without promising perfect durability. Both of those
placements have the further advantage that the reading and writing of
the file is not being done "in medias res," which does seem to have
certain advantages from a robustness perspective. For example, without
necessarily endorsing this design, suppose you added a background
worker and there are GUCs to configure the database that it connects
to and the query it executes to restore advice stashes. Or,
alternatively, a background worker that still uses a flat file. Either
way, that opens up design ideas like: when you see that the in-memory
stashes are not yet reloaded, you can decide to wait up to X seconds
for that to happen and then proceed anyway if it hasn't happened by
then. I'm not saying that is the right idea here necessarily, but it's
an option, whereas what you've done doesn't lend itself to that sort
of idea.

One other note is that fscanf() ending in a newline could eat up
whitespace at the start of the following line. Since a stash name can
begin with whitespace, that could be an issue.

> as we document it clearly how to measure the amount of memory used.

The module doesn't have a built-in way to do that right now. Are you
thinking we would document that pg_get_dsm_registry_allocations() can
be used?

> pg_maintain to manage entries by default?

Wouldn't it make more sense for the cloud provider to grant execute
permissions on these functions to pg_maintain if they are so inclined?
This is a brand-new facility, so I think we had better be conservative
in terms of default permissions.

---

From: Lukas Fittl <[email-masked]>
Date: Tue, 31 Mar 2026 19:25:04 -0700

> pretty strong position for someone to take.

Yeah, I think my position is that having a solution to persistence
would be very good, but if that's not doable for this release, I think
we have a potential way forward in future releases, at least when it
comes to being restart-safe.

That said, I still think it'll make a big difference in practice to be
restart safe right away, if we can make it happen.

> sometimes such messy one-offs are the right answer.

I think if we wanted a table, we should make it a table - but I think
the fact that we want this to be low-overhead for running queries to
examine whether there is anything for them to apply, would require
some kind of cache in front of it, and that gets complicated pretty
quickly.

For the file-based direction, just for reference, I'm attaching an
updated version of that (on top of Robert's earlier v23), that
utilizes a background worker to write out the dump file as needed, at
most every 60 seconds. It also reworks some of the output logic to do
better memory management, and uses a TSV file format that can be
easily read again.

To be clear, I think its okay to go ahead with merging pg_stash_advice
without that and make it a best effort to get the file saving in too -
but I think with the current design in this patch represents a
reasonable solution to what we can do in terms of persistence across
restarts in either 19 or 20.

> thinking we would document that pg_get_dsm_registry_allocations() can

Yeah, for example. Alternatively we could provide a function/view that
lists all advice across all stashes, so you can more easily see the
result size of that and estimate what the in-memory use is. But
pointing to pg_get_dsm_registry_allocations seems easier.

> in terms of default permissions.

I guess. I'm always worried that providers get that wrong and forget
to give end users the permissions - but I suppose end users can
complain to their providers if that's the case.

I've done another look over pg_set_stashed_advice and I think its in
good shape. The only trailing thought I have is that we could consider
running a fuzzer against the pg_set_advice function in particular,
just to see if anything pops up (beyond having the ability to make a
very large memory allocation through a large advice string, which is
maybe a problem?).

Thanks,
Lukas

-- 
Lukas Fittl

---

From: Lukas Fittl <[email-masked]>
Date: Tue, 31 Mar 2026 23:33:50 -0700

> pointing to pg_get_dsm_registry_allocations seems easier.

Actually, that won't work in practice with the code as of v23 -
pg_get_dsm_registry_allocations() always returns the fixed 64 byte
allocation from GetNamedDSMSegment, but is oblivious to the individual
DSA allocations (even after adding hundreds of entries):

SELECT * FROM pg_get_dsm_registry_allocations();

      name       |  type   | size
-----------------+---------+------
 pg_stash_advice | segment |   64
(1 row)

Is there a reason you didn't use GetNamedDSA / GetNamedDSHash for the
other allocations? (which we have as of fe07100e82b09)

With the adjustments in the attached patch, this gets reported as expected:

SELECT * FROM pg_get_dsm_registry_allocations();

         name          |  type   |   size
-----------------------+---------+-----------
 pg_stash_advice       | segment |        24
 pg_stash_advice_stash | hash    |   1048576
 pg_stash_advice_dsa   | area    | 803209216
 pg_stash_advice_entry | hash    |   1048576
(4 rows)

> very large memory allocation through a large advice string, which is

Obviously I meant "I've done another look over pg_stash_advice and I
think its in good shape".

I've done some basic fuzzing with the pg_set_stashed_advice function,
including concurrently setting advice, and that didn't yield any
surprises.

Thanks,
Lukas

-- 
Lukas Fittl

---

From: Robert Haas <[email-masked]>
Date: Thu, 2 Apr 2026 12:15:09 -0400

> restarts in either 19 or 20.

Somewhat against my better judgement, I have attempted to put your
patch into committable shape. In the process, I rewrote pretty much
the whole thing.  So here's v24, also dropping pg_collect_advice.

0001 is the pg_stash_advice patch from v23, but with a number of
changes motivated by your desire to add persistence. I split the code
into two files, because I felt that file was starting to get a little
bit large, and I didn't want to just add a whole bunch more stuff to
it. For the most part, this is just code movement, but I did make a
couple of substantive changes. First, I restricted stash names to
alphanumeric characters and underscores, basically looking like
identifier names. This is partly because I got thinking about the
escaping requirements for the persistence file, but it's also because
I realized that letting somebody name their stashes with spaces or
non-printable characters in the name or a bunch of random punctuation
was probably more confusing than useful. Second,
pgsa_set_advice_string() was previously taking a lock itself, but most
of its sister functions require the caller to do that; I changed it to
match. Third, lock is also now held when calling
pgsa_clear_advice_string(), which may not be entirely necessary, but
it seems safer and shouldn't cost anything meaningful.

0002 adds persistence. Here's a list of changes from your version:

- I changed the GUC name pg_stash_advice.save to
pg_stash_advice.persist, since it controls both whether advice is
saved automatically and also whether it's loaded automatically at
startup time.

- I added a GUC pg_stash_advice.persist_interval, so that the interval
between writes can be configured.

- Instead of a dump_requested flag, I added a pg_atomic_uint64
change_count. This avoids needing to take &pgsa_state->lock in
LW_EXCLUSIVE mode. Even leaving that aside, I don't think a Boolean is
adequate here. Your patch cleared the flag before dumping, but that
means if the act of dumping fails, you forget that it needed to be
done. If you instead clear the flag after dumping, then you don't
realize you need to do it again if any concurrent changes happen.

- I set the restart time to BGW_DEFAULT_RESTART_INTERVAL rather than
0. Restarting the worker in a tight loop is a bad plan.

- I added a function pg_start_stash_advice_worker(). You could do
something like add pg_stash_advice to session_preload_libraries, start
using it, and use this to kick off the worker. Then eventually you can
restart the server with pg_stash_advice moved to
shared_preload_libraries.

- As you had coded it, any interrupt that jostled the worker would
trigger an immediate write-out if one was pending. That behavior seems
hard to explain and document, so I made it work more like autoprewarm,
which always respects the configured interval even in case of
interrupts.

- I added a mechanism to prevent the user from manually manipulating
stashes or stash entries when persistence is enabled but before the
dump file has been reloaded. Without this, reloading the dump file
could error out if, for example, the user already managed to recreate
a stash with the same name as one that exists in the dump file.

- As you had coded it, data is inserted into the dynamic shared memory
structures as it's read from the disk file. I felt that could produce
rather odd behavior, especially in view of the lack of the lockout
mechanism mentioned in the previous point. We might partially process
the dump file and then die with some data loaded and other data not
loaded. Other backends could see the partial results. While the
lockout mechanism by itself is sufficient to prevent that, I felt
uncomfortable about relying on that completely. It means we start
consuming shared memory even before we know whether there's an error,
and continue to consume it after we've died from an error, and it also
means we have a very hard dependency on the lockout mechanism, which
really only works if there's only ever one load and save file and
loading only happens at startup time. I felt it was better to slurp
all the data into memory first, parse it completely, and then start
making changes to shared memory only if we don't find any problems, so
I made it work that way. We replace the tabs and newlines that end
fields and lines with \0 on the fly so that we can just point into
that buffer, instead of having to pstrdup() anything. (Note that, even
if we stuck with your approach of something based on pg_get_line(), it
would probably be better to use one of the other variants, e.g.
pg_get_line_buf(), to avoid allocating new buffers constantly.)

- I completely reworked the string escaping. Your pgsa_escape_string()
had a bug where the strpbrk call didn't check for \r. Also, I didn't
like the behavior of just ignoring a backslash when it was followed by
end of string or something unexpected; I felt those should be errors.
Given the decision to slurp the whole file, as mentioned in the
previous point, it also made sense to do the unescaping in place, so
that we didn't need to allocate additional memory. I particularly
didn't like the decision to sometimes allocate memory and sometimes
not. While it was economical in a sense, it meant that the memory
consumption could be very different depending on how many entries
needed escaping.

- I completely reworked the error reporting. Now, if we hit an error
parsing the file (or doing anything else), we just signal an ERROR,
and we rely on the fact that the postmaster will restart us. It's an
explicit goal not to apply incremental changes when the file overall
is not valid, which also means that we are only concerned about
reporting the first error that we detect, which also seems good for
avoiding log spam. On the other hand, the error reports are more
specific and detailed, and now all follow the same general pattern:
"syntax error in file \"%s\" line %u: problem description here". (I
was also not entirely happy with the fact that you could potentially
call fprintf() lots of times before finally reporting an error. While
you did save and restore errno around the calls to FreeFile() and
unlink(), I think it makes the code hard to reason about; e.g. what if
a later fprintf call hit a different error than an earlier one?)

- I felt it was a bit odd to install a zero length file, so I made the
persistence mechanism remove the existing file if there's nothing to
persist when it goes to write out. I am not totally sure this was the
right call.

- I added a message when saving entries symmetrical to the one you had
for loading entries, and also some DEBUG1/2 messages in case someone
needs more details.

- I added a TAP test. This isn't as comprehensive as it could be -- in
particular, it doesn't cover all the possible error cases that occur
when trying to reload data from disk. I could add that, but it would
mean stopping and restarting the server a bunch of times, and I wasn't
sure that it was a good idea to add that much overhead for a few lines
of code coverage.

- Your documentation changes still said that the stash data would be
"restored when the first session attaches after a server restart" but
that doesn't sound like something that a user will understand, and
also wasn't what actually happened since you added the background
worker. I rewrote this.

There's almost none of your code remaining at this point, but I listed
you as a co-author for 0002. I think a case could be made for Author
or for no listing. Let me know if you have an opinion. And, please
review!

---

From: Robert Haas <[email-masked]>
Date: Thu, 2 Apr 2026 12:44:32 -0400

> other allocations? (which we have as of fe07100e82b09)

I'm under the impression that GetNamedDSA and GetNamedDSHash exist for
the purposes of making it easy for extensions to coordinate with each
other across backends, rather than being something you're supposed to
use to improve observability. I think it's potentially good for there
to be a way to see the size of every DSA that exists in the system,
but this clearly isn't that, because none of the code in src/backend
uses it when creating DSAs. You might argue that DSAs for short-lived
things like parallel query or parallel VACUUM don't need to be tracked
like this (which seems arguable), but they are also used for
long-lived contexts in the logical replication launcher, by
LISTEN/NOTIFY, by the shared-memory statistics collector, and in
GetSessionDsmHandle(), and those places don't use GetNamedDSA()
either.

Architecturally, I don't like the idea of replacing "having a pointer
to an object" with "being able to look up that object by name". I
think it's good design that pg_stash_advice creates one structure that
serves as a sort of root and then hangs everything else off of that. I
admit that leaves me not knowing quite what to do about the problem of
knowing how much memory it's using, though. Adding a function just to
return size information seems a little clunky, but it might be the
right idea: it could for example return a count of stashes, a count of
entries, the total length of the entries, and the allocated size of
the DSA.

---

From: Tom Lane <[email-masked]>
Date: Thu, 02 Apr 2026 19:43:33 -0400

My animal sifaka just showed an all-new type of test_plan_advice
failure [1]:

diff -U3 /Users/buildfarm/bf-data/HEAD/pgsql.build/src/test/regress/expected/limit.out /Users/buildfarm/bf-data/HEAD/pgsql.build/src/test/modules/test_plan_advice/tmp_check/results/limit.out
--- /Users/buildfarm/bf-data/HEAD/pgsql.build/src/test/regress/expected/limit.out	2026-04-02 12:35:13
+++ /Users/buildfarm/bf-data/HEAD/pgsql.build/src/test/modules/test_plan_advice/tmp_check/results/limit.out	2026-04-02 12:49:59
@@ -5,6 +5,8 @@
 SELECT ''::text AS two, unique1, unique2, stringu1
 		FROM onek WHERE unique1 > 50
 		ORDER BY unique1 LIMIT 2;
+WARNING:  supplied plan advice was not enforced
+DETAIL:  advice INDEX_SCAN(onek public.onek_unique1) feedback is "matched, inapplicable, failed"
  two | unique1 | unique2 | stringu1 
 -----+---------+---------+----------
      |      51 |      76 | ZBAAAA
=== EOF ===
[12:50:02.062](11.620s) not ok 1 - regression tests pass

This is unlike the other cases we've been looking at: no sub-selects,
no GEQO, not even any joins.  There is pretty much only one plausible
plan for that query, so how did it fail?

After looking around, I think the likely explanation is that the
concurrently-run alter_table.sql test feels free to mess with the set
of indexes on onek.  It doesn't drop onek_unique1, but it does
momentarily rename it:

ALTER INDEX onek_unique1 RENAME TO attmp_onek_unique1;
ALTER INDEX attmp_onek_unique1 RENAME TO onek_unique1;

I've not looked closely at pg_plan_advice, but if it matches indexes
by name then it seems there's a window here where the advice would
fail to apply.  Also, further down we find

ALTER TABLE onek ADD CONSTRAINT onek_unique1_constraint UNIQUE (unique1);
ALTER INDEX onek_unique1_constraint RENAME TO onek_unique1_constraint_foo;
ALTER TABLE onek DROP CONSTRAINT onek_unique1_constraint_foo;

which means there's a window there where there are two plausible
indexes to choose.  Will test_plan_advice cope if the transient one
is chosen?

We could imagine dodging this problem either by having alter_table.sql
test some purpose-built table instead of a shared one, or by having it
do these hacks inside transactions so that other sessions can't see
the intermediate states.  But I'm quite resistant to that answer,
because I think part of the point here is to ensure that concurrent
DDL doesn't misbehave.  (Which it doesn't: these test fragments have
been there since 2018 and 2012 respectively, and not caused issues
AFAIK.)  Preventing our tests from exercising concurrent DDL in order
to satisfy test_plan_advice is not a good plan IMO.  There's also the
prospect of a long tail of whack-a-mole as we locate other places in
the tests where this sort of thing happens occasionally.

So I'm not sure what to do here, but we have a problem.

			regards, tom lane

[1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=sifaka&dt=2026-04-02%2016%3A35%3A13

---

From: Robert Haas <[email-masked]>
Date: Thu, 2 Apr 2026 22:08:51 -0400

Thanks for the report.

> ALTER INDEX attmp_onek_unique1 RENAME TO onek_unique1;

Yeah, the fact that it said /* inapplicable */ strongly supports this
theory. There's only two ways that can happen, and an index not
existing with the expected name is one of them (and the only one
that's possible in a query involving only a single table).

> indexes to choose.  Will test_plan_advice cope if the transient one

It's going to be unhappy if the second planning cycle is incapable of
choosing the same index that the first planning cycle did. I have to
admit that it didn't occur to me that our regression tests would do
something like this. I figured they had to be operating on mostly
independent objects or things would already be broken, but I failed to
consider the possibility that there could be concurrent DDL of a sort
that wouldn't affect the normal running of the regression tests but
would affect pg_plan_advice. Or at least, if I did ever consider it, I
stopped considering it when test_plan_advice appeared to be passing.

> So I'm not sure what to do here, but we have a problem.

I'm not sure, either, and I agree that we have a problem. I'll give it
some more thought tomorrow.

---

From: Robert Haas <[email-masked]>
Date: Thu, 2 Apr 2026 22:15:36 -0400

On Thu, Apr 2, 2026 at 12:15 PM Robert Haas <[email-masked]> wrote:
> So here's v24, also dropping pg_collect_advice.

That version didn't actually pass CI. Here's v25.

---

From: Robert Haas <[email-masked]>
Date: Fri, 3 Apr 2026 13:13:47 -0400

> some more thought tomorrow.

OK, here are my thoughts.

I don't believe it's viable to change pg_plan_advice in such a way
that it won't run into trouble in cases like this.  Somebody could
argue that the choice of INDEX_SCAN(table_name index_name) was bad
design, and that I should have done something like
INDEX_SCAN(table_name indexed_columns) instead, and that might be
true. There might also be an argument that we should have both things
with different spellings, and that might also very well be true. But
we don't really know that changing that design decision would fully
stabilize test_plan_advice. The regression tests can do anything they
like, as long as they reliably pass. It now seems optimistic to me to
suppose that an index with a different name is the only current or
future issue we'll ever have. I mean, if the table were small enough
not to care about whether an index scan or a sequential scan is used,
you could concurrently drop the one and only index altogether, and
what's test_plan_advice supposed to do about that?

So, I argue that there are three possible categories of solutions
here: (1) don't let the problem cases happen in the first place, (2)
detect that a problem has happened, or (3) give up on
test_plan_advice.

In category (1), the simplest idea would be (1a) to run the tests
serially. That would probably involve running them much less often,
like in one of the CI builds but not all of them or something like
that. Another idea that I had is to (1b) try to take stronger locks on
the relations involved to prevent concurrent DDL on them, like a
ShareUpdateExclusiveLock, or (1c) some kind of bespoke interlock
specific to test_plan_advice. I think that might cause random breakage
of other types, though. Another idea in this category is to try to
make the main regression tests "pg_plan_advice clean". I know Tom
already expressed opposition to that idea, but here me out: we could
(1d) have a separate test suite that still does stuff like this, so we
don't lose test coverage, and move some stuff there. Or, instead of
completely separating it, we could (1e) have two schedule files, one
of which includes all the tests and a second of which includes only
the tests that are test_plan_advice-clean. Although my theory that the
main regression tests couldn't have multiple different sessions
simultaneously doing DDL on the same objects has been proven wrong,
I'd still be willing to bet that it's a minority position. Of course,
as Tom pointed out, there could be a "long tail" of failures here, but
maybe we could create some throwaway infrastructure to help figure
that out. For example if we're mostly worried about tables, we could
have each backend accumulate a list of table OIDs that it touched and
spit that out into the log file when it exits. That wouldn't be
committable code, but it would be enough to let us run the regression
tests with that once and see what overlaps exist. I bet there's very
low risk of newly-added tests adding more such cases: the ones that we
have are probably ancient. Of course, maybe I'm wrong about that, too,
but it's a theory that we can discuss.

In category (2), what if, (2a) whenever we see advice feedback that
we'd otherwise print, we try replanning the query a THIRD time without
any supplied advice? If we generate different advice than we did the
first time we planned it, then we know for sure that something is
unstable, and we can decide not to complain about whatever went wrong.
This isn't completely guaranteed to work, though: what if concurrent
DDL changes something between planning cycle 1 and planning cycle 2
and then changes it back before planning cycle 3? But maybe it would
be acceptable to make a rule that the main regression test case
shouldn't do that, and adjust cases that currently do to work
otherwise. If we're not willing to make any rules at all to prevent
the main regression test suite from sabotaging test_plan_advice, then
it's probably doomed. And, I think there's a reasonable argument that
insisting that the main regression test suite absolutely has to change
the definition of an object in a way that test_plan_advice will care
about and then change it back to exactly the initial state while in a
concurrent session some other backend is running queries against that
object is tantamount to legislating deliberate sabotage. But that
said, this proposal has some other imperfections as well. In
particular, a bug that caused the third planning cycle to always
produce different results than the first would hide all future
problems that test_plan_advice might have caught, which is pretty sad.
Another variant of the same basic idea is to (2b) just detect when
we've seen any shared invalidations between the start of the first
planning cycle and the end of the second, and go "never mind, don't
complain even if we saw a problem". The problem with this idea is
that, as in the previous proposal, it might make the tests too
insensitive to real issues. But I wonder if this might be fixable.
Maybe we could (2c) make test_plan_advice take planner_hook and wrap a
loop around the problem: it just keeps replanning the query via
standard_planner (which would eventually reach
test_plan_advice_advisor) until no sinval messages are absorbed
between the start and end of planner, which I think we could detect
using SharedInvalidMessageCounter, or until some retry limit is
exhausted and we error out. I'd need to try this and see how well it
works out in practice, and how often the retry is actually hit, but it
seems like it might be somewhat viable.

In category (3), the most blunt option is obviously just (3a) throw
test_plan_advice away, which I think is probably dooming
pg_plan_advice to getting silently broken in the future. I don't
really have any other ideas in this category except for (1a) already
mentioned, which is sort of a hybrid solution.

My current thought is to do some research into (1e) and (2c).
Specifically, for (1e), I want to try to figure out if this is the
only case of this type or if there are lots of others, since that
seems likely to have a pretty large bearing on what is realistic here.
And for (2c), I think I just want to try it out and see if it seems at
all feasible. Probably obviously, this is not going to happen before
next week, but I hope that the frequency of buildfarm failures is now
low enough that this isn't a critical issue. If that's wrong, let me
know, but from my point of view, even if we eventually chose (3a),
having a good a sense as possible of what the potential failure modes
are here would help to design the next solution, and AFAIK this is the
first failure we've seen since the DO_NOT_SCAN stuff went in.

(In fact, I had a little bit of trouble finding this in the BF results
even knowing it was there: filtering by test_plan_advice failures
doesn't find anything recent. sifaka's failure shows up as
TestModulesCheck-en_US.UTF-8, but frustratingly, the names for the
stage logs don't seem to quite match the name of what failed. There is
testmodules-install-check-C and
testmodules-install-check-en_US.UTF-8, but those have "install" in the
name and are punctuated differently, so it's not instantly clear that
it's the same thing. Anyway, I do see it in there now, but what I'm
saying is that if there have been other failures that are related to
this, it's possible I have missed them due to stuff like this, so it's
helpful that you (Tom) pointed this one out.)

Tom, would welcome your thoughts, if you have any, and anyone else's
thoughts as well. If none, I'll proceed as described above and update
when I know more.

Thanks,

---

From: Tom Lane <[email-masked]>
Date: Fri, 03 Apr 2026 14:20:32 -0400

> helpful that you (Tom) pointed this one out.)

I grepped the buildfarm database for 'supplied plan advice' and got
no other hits since 6455e55b0 went in.  That's not a huge sample
size of course, but probably several hundred runs so far.  If there's
another message wording I should check for, let me know.

> thoughts as well. If none, I'll proceed as described above and update

I don't like anything in category 1 except (1a) run the test scripts
serially for test_plan_advice.  As I said before, I am strongly
against allowing test_plan_advice to constrain what our tests do.

Another idea in category 2, which I think is a bit different from
any option you listed, is to repeat the "plan without advice, then
again with advice, see if it matches" process up to maybe 5-ish times
before declaring failure.  If it works any one time, then write off
the previous failures as being induced by concurrent activity.
Unlike what you mentioned, this isn't dependent on sinval checks,
which I think are next door to useless in the context of the
regression tests: there's a constant storm of sinval activity going
on, to the point where you might as well figure "check for sinval
arrival" is constant "true".

However, eyeing the calendar, I think the only options that are likely
to be stabilizable before feature freeze are (1a) run the test scripts
serially for test_plan_advice or (3a) throw test_plan_advice away.
I know you don't want to do (3a) and I understand why not.  How much
will (1a) slow things down?

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Fri, 3 Apr 2026 20:14:49 -0400

> will (1a) slow things down?

I don't know. For me, the speed of the regression tests is rarely a
bottleneck, and they run on my machine in about 12 seconds. But on
slow buildfarm machines, I'm guessing it's going to extend the runtime
significantly. But I also feel like if we've only seen one buildfarm
failure since the last round of stabilization, it might not be a
catastrophe if nothing further is done before feature freeze. In fact,
I think it might be *good*. Given the apparently-low failure rate that
we now have, it feels to me like we might want to run like this for a
month or even or two or three to get a clearer feeling for whether the
failure you saw is the only one or whether, perhaps, there are others.
Or even just how often this one happens. I mean, I'm also not that
opposed to having it made serial now if you really think that's
better. But what concerns me is I feel like we might inconvenience a
lot of people who really care about the tests running fast while at
the same time eliminating our ability to gather any more information
about the problem.

I mean, there is possibly an argument that we don't really need to
gather any more information about the problem; it does seem like we
understand what is going on here, and if we had a great, simple fix I
would probably just apply it and be done with it. But I also don't
quite understand why you're in such a rush. If we still feel like
running the tests serially is the best solution in a month, can't we
just do it then?

---

From: Tom Lane <[email-masked]>
Date: Fri, 03 Apr 2026 23:14:46 -0400

Robert Haas <[email-masked]> writes:
> ... But I also feel like if we've only seen one buildfarm
> Or even just how often this one happens.

Reasonable point.

> running the tests serially is the best solution in a month, can't we

The terms that I'm thinking in are "how much redesign will we accept
post-feature-freeze, in either pg_plan_advice or test_plan_advice,
before choosing to revert those modules entirely for v19?".  I think
that running those tests serially is a sufficiently low-risk option
that it'd be okay to put it in post-freeze, even very long after.
I'm not sure that any of the other group-1 or group-2 options you
suggested would be okay post-freeze.  (Of course, ultimately that'd
be the RMT's decision not mine.)

I believe that we probably will need to do something in this
area before v19 release.  If we're willing to commit to it being
"run the tests serially", then sure we can wait awhile before
actually doing that.  Maybe we'll even think of a better idea
... but what we can do about this post-freeze seems pretty
constrained to me.

			regards, tom lane

---

From: Lukas Fittl <[email-masked]>
Date: Sat, 4 Apr 2026 01:11:25 -0700

> That version didn't actually pass CI. Here's v25.

I've reviewed the pg_stash_advice code and documentation, and I think
this is overall sound.  As I mentioned previously, I think its a very
important addition to make pg_plan_advice work for practical problems
end users encounter.

To me this looks good to go, with three minor notes below. For context
I've spent a few hours today going through the code manually, and
doing testing.

And thank you for the detailed notes in your earlier email, and
reworking this. Regarding authorship, I'm happy to be listed as
co-author on the persistence part if you want to keep that in the
commit message. Overall I'm also willing to put in work during the
remaining cycle to test/review/address issues in pg_plan_advice or
pg_stash_advice, so we can hopefully sort out the other items being
discussed.

For 0001:

> new file mode 100644
> index 00000000000..ec60552a447
> --- /dev/null
> @@ -0,0 +1,216 @@
> +   <varlistentry>
> +    <term>
> +     <function>pg_create_advice_stash(stash_name text) returns void</function>
> +     <indexterm>
> +      <primary>pg_create_advice_stash</primary>
> +     </indexterm>
> +    </term>
> +
> +    <listitem>
> +     <para>
> +      Creates a new, empty advice stash with the given name.
> +     </para>
> +    </listitem>
> +   </varlistentry>
> +

I think we should document the restrictions on advice names here (i.e.
they must be alphanumeric or contain an underscore, not start with a
digit, and maximum NAMEDATLEN).

For 0002:

> diff --git a/contrib/pg_stash_advice/pg_stash_advice.c b/contrib/pg_stash_advice/pg_stash_advice.c
> index 15e7adf849b..1858c6a135a 100644
> --- a/contrib/pg_stash_advice/pg_stash_advice.c
> +++ b/contrib/pg_stash_advice/pg_stash_advice.c
> ...
> @@ -464,6 +522,43 @@ pgsa_drop_stash(char *stash_name)
>         }
>     }
>     dshash_seq_term(&iterator);
> +
> +    /* Bump change count. */
> +    pg_atomic_add_fetch_u64(&pgsa_state->change_count, 1);
> +}
> +
> +/*
> + * Remove all stashes and entries from shared memory.
> + *
> + * This is intended to be called before reloading from a dump file, so that
> + * a failed previous attempt doesn't leave stale data behind.
> + */
> +void
> +pgsa_reset_all_stashes(void)
> +{

I think this might be good to expose on the SQL level as well - in
case someone accidentally created a lot of stashes it could be tedious
to remove them all, e.g. if they wanted to clear all the memory after
an experiment.

> diff --git a/contrib/pg_stash_advice/stashpersist.c b/contrib/pg_stash_advice/stashpersist.c
> new file mode 100644
> index 00000000000..da96ee0d803
> --- /dev/null
> +++ b/contrib/pg_stash_advice/stashpersist.c
>...
> +            /* Parse the query ID. */
> +            errno = 0;
> +            queryId = strtoll(queryid_str, &endptr, 10);
> +            if (*endptr != '\0' || errno != 0 || queryid_str == endptr)
> +                ereport(ERROR,
> +                        (errcode(ERRCODE_DATA_CORRUPTED),
> +                         errmsg("syntax error in file \"%s\" line %u: invalid query ID \"%s\"",
> +                                PGSA_DUMP_FILE, lineno, queryid_str)));
> +

It might be worth adding a queryId == 0 check here, since we won't
check it later, and its helpful to avoid unpredictable behavior just
in case someone decided to mess with the file manually.

Thanks,
Lukas

-- 
Lukas Fittl

---

From: Andrei Lepikhov <[email-masked]>
Date: Sat, 4 Apr 2026 11:34:35 +0200

> ... but what we can do about this post-freeze seems pretty

As you work on the code, please keep the pg_plan_advice issue [1] in 
mind. I came across it while designing the optimisation in [2]. Even if 
[2] is not added to the Postgres core, this still looks like a valid 
query plan and may be proposed by an extension. So, the hinting module 
should avoid conflicts with other extensions, just as pg_hint_plan does.

[1] pg_plan_advice fails when NestLoop outer side is Sort over FunctionScan
https://www.postgresql.org/message-id/[email-masked]
[2] Try a presorted outer path when referenced by an ORDER BY prefix
https://www.postgresql.org/message-id/19a9265c-c441-4a43-bc0d-dac533438da0%40gmail.com

-- 
regards, Andrei Lepikhov,
pgEdge

---

From: Robert Haas <[email-masked]>
Date: Sat, 4 Apr 2026 14:42:19 -0400

> https://www.postgresql.org/message-id/19a9265c-c441-4a43-bc0d-dac533438da0%40gmail.com

I'll take a look at that issue when I have a free moment. We certainly
cannot promise in general that pg_plan_advice will be able to make
sense of plans that PostgreSQL's own planner does not produce; that
would require magical code. But there might be something that can be
done to ameliorate this particular instance.

By the way, I'm really glad you hit that error. That particular error
check is there precisely to find plans that pg_plan_advice isn't able
to understand, and it sounds like it is doing its job as intended.
Having problems isn't great, but knowing that you have problems is a
lot better than still having them but not knowing about it.

---

From: Andrei Lepikhov <[email-masked]>
Date: Sat, 4 Apr 2026 23:02:37 +0200

> lot better than still having them but not knowing about it.
That’s exactly what concerns me. I see it as a potential design flaw if 
the extension has to make assumptions about possible plan configurations.
I’m not sure how it works in detail, of course. However, when I designed 
Postgres replanning in the past, and made similar core changes to what 
you’ve done for pg_plan_advice, this kind of problem couldn’t have 
happened. So, I think it’s worth questioning the current approach and 
looking for other options.

---

From: Robert Haas <[email-masked]>
Date: Sat, 4 Apr 2026 18:52:21 -0400

> looking for other options.

I mean, any plan stability feature is intrinsically tied to a
particular planner. Nobody thinks you can use Aurora Postgres's Query
Plan Management feature with MySQL or DB2 or Oracle. Those products
obviously have to have their own features for plan stability. The same
is true here. There's more overlap because you're creating the plan
out of the same basic building blocks rather than an entirely
different set of things, but if you assemble them in a way that
PostgreSQL doesn't, then some things may not work. pg_plan_advice is
one of those things; the executor is another. Of course, I don't think
anybody here is keen to break stuff for no good reason, which is why I
will take a look at the report you posted. But fundamentally, it's the
same issue. If somebody uses a plugin that replaces large parts of the
plan with a CustomScan, pg_plan_advice isn't going to work with that,
either: how could it possibly? Maybe there could be some way to make
pg_plan_advice pluggable so that if extensions fiddle with the planner
they can also do matching fiddling with pg_plan_advice if they're so
inclined, but having it "just work" would require magic.

---

From: Andrei Lepikhov <[email-masked]>
Date: Sun, 5 Apr 2026 09:57:13 +0200

> Plan Management feature with MySQL or DB2 or Oracle. Those products

I don’t expect any Postgres extension to work in DB2.

These optimisations are simple. Here, I provided the optimiser with one 
extra path that it skipped itself just to reduce computational overhead 
- nice in the general case, but not ok in analytics. This extension of 
planning scope allowed the optimiser to build JOIN over the Sort 
operator, which didn’t change the main logic at all. I followed the 
usual cost-based model and used add_path.

Another optimisation improves Memoize so it can run on top of SubPlan 
when the cost model predicts many repeated parameter values. One more 
extension uses MergeJoin estimation on the required values of its inputs 
to determine how many tuples are needed from each input, which adds 
kinda 'soft' LIMIT emerged from the plan structure ... The Append node 
serves as the backbone of any partitioning or sharding setup, but 
contributors often overlook it, and we use multiple extra optimisations 
here too.

There’s a lot to say about branched out-of-core optimisations 
infrastructure, but it’s clear that supporting analytical workloads 
means adding extra features. Developers usually stick to standard 
Postgres practices, cost model and routines providing the planner with 
alternatives without forcing any 'magical' paths. So, they expect 
built-in extensions not to interfere with their code by design.

Looking back at the pg_plan_advice development cycle, I don’t see many 
discussions about the design. It seems unusual given how complex the 
planner's structure is. It makes sense to follow the typical way and let 
it serve out of the contrib for some time and see if it works well.

Introducing such a module into the core would effectively cancel 
alternative solutions, as seen with PGSS. Therefore, it is important to 
ensure the code is well-designed before proceeding. Do you agree?

---

From: Alexander Lakhin <[email-masked]>
Date: Sun, 5 Apr 2026 11:00:00 +0300

Hello Robert,

I and SQLsmith have discovered one more anomaly (reproduced starting from
e0e4c132e):
load 'test_plan_advice';
select object_type from
  (select object_type from information_schema.element_types limit 1),
  lateral
  (select sum(1) over (partition by a) from generate_series(1, 2) g(a) where false);

triggers an internal error:
ERROR:  XX000: no rtoffset for plan unnamed_subquery
LOCATION:  pgpa_plan_walker, pgpa_walker.c:110

Could you please have a look?

Best regards,
Alexander

---

From: Alexander Lakhin <[email-masked]>
Date: Sun, 5 Apr 2026 15:00:00 +0300

> LOCATION:  pgpa_plan_walker, pgpa_walker.c:110

And another error, which might be interesting to you:
CREATE EXTENSION tsm_system_time;
CREATE TABLE t(i int);
SELECT 1 FROM (SELECT i FROM t TABLESAMPLE system_time (1000)), LATERAL (SELECT i LIMIT 1);

ERROR:  XX000: plan node has no RTIs: 378
LOCATION:  pgpa_build_scan, pgpa_scan.c:200

Best regards,
Alexander

---

From: Robert Haas <[email-masked]>
Date: Mon, 6 Apr 2026 08:47:30 -0400

> ensure the code is well-designed before proceeding. Do you agree?

I don't know how anyone could disagree with the idea that PostgreSQL
code should be well-designed, but that doesn't mean that I agree that
your particular design criticism is fair, and I definitely don't.

As for the amount of design discussion on the mailing list, I was
disappointed in that, too. In addition to posting to the list, I
privately asked numerous people to help review and test. Some did, but
on the whole, I was expecting a more vigorous debate and a lot of
people telling me what an idiot I am. Instead, the most common
feedback I got was some form of "can you ship it right now, please?".
That probably has less to do with the design being good (although I
believe that it is) or my code being good (although I hope that it is)
than with people just really wanting PostgreSQL to have something of
this sort. So I am somewhat afraid that this will turn out to have
more problems than anyone has noticed so far, and maybe for reasons
that will feel dumb in hindsight. But on March 12th, I asked myself
whether more people were going to be unhappy if I committed
pg_plan_advice this release cycle or if I didn't, and my educated
guess was the latter, so I committed it. If that turns out to have
been the wrong call, then I apologize to the whole community in
advance.

But I do not apologize for the fact that pg_plan_advice tries to
interpret plan trees -- which I personally think is one of the best
design decisions I have ever made while hacking on PostgreSQL -- or
that it can't interpret the variant ones that your extension produces.

---

From: Andrei Lepikhov <[email-masked]>
Date: Mon, 6 Apr 2026 15:22:04 +0200

> that it can't interpret the variant ones that your extension produces.

I challenge solely the design of the extension, not interested in holy 
wars on the hinting approach.
Postgres modules that use hooks are second-class citizens because the 
core hooks were never designed to let an extension module be as 
effective as the core code. It's probably OK, considering safety and 
maintainability concerns.
But this extension effectively makes alternative modules third-class 
citizens (not sure such a term exists in English) - people prioritise 
contrib modules over any others. And they definitely will use this one. 
So, I envision complaints about conflicting extensions in the near 
future - think about Citus or TimescaleDB optimisations, for example.
It would be better to introduce such a code at the beginning of the 
development cycle, not right before the code freeze. At least we would 
discuss its design without rushing.

---

From: Andres Freund <[email-masked]>
Date: Mon, 6 Apr 2026 09:56:23 -0400

Hi,

On 2026-04-04 23:02:37 +0200, Andrei Lepikhov wrote:
> That’s exactly what concerns me. I see it as a potential design flaw if the
> extension has to make assumptions about possible plan configurations.
> you’ve done for pg_plan_advice, this kind of problem couldn’t have happened.
> So, I think it’s worth questioning the current approach and looking for
> other options.

You're making sweeping high-level demands, implying they're easy ("when I
designed ... this kind of problem couldn’t have happened"), without any
concrete technical suggestions for how to actually achieve that.  In very
strong language.  Your high level demand, that somehow plan shape influencing
code should just work regardless of what crazy thing extensions have done
seems ... not entirely realistic, to put it very kindly.

I suggest you rethink your approach of engaging with others.

Greetings,

Andres Freund

---

From: Robert Haas <[email-masked]>
Date: Mon, 6 Apr 2026 10:01:52 -0400

> future - think about Citus or TimescaleDB optimisations, for example.

Definitely possible.

> discuss its design without rushing.

Yes, the timing is not ideal. However, I posted the patch on October
30th and committed the main patch on March 12th. I think that's a
reasonable length of time to wait for people to provide feedback.
During that time, the only person who provided information on how this
will interact with out-of-core extensions was Lukas Fittl, who came to
the conclusion that the pgs_mask infrastructure will be reusable by
pg_hint_plan and will result in that module being simpler and
involving less code duplication. Other extension authors could have
provided feedback during that time as well, but none did, even after I
posted to my blog to try to raise the visibility of this project. As
far as I can tell, most extension developers don't pay much attention
to core development until after we ship a beta. Had I waited until
July to commit, I think there's a chance that it would have simply
resulted in me getting whatever feedback I'm going to get next summer
rather than this summer. At least this way, the issues will hopefully
be fresh in my mind when the feedback arrives.

Of course, you also seem to be assuming that whatever feedback I get
will be negative, and it may well be. But, there is also some tiny
possibility that I have done a good job and that people will like it.

---

From: Peter Geoghegan <[email-masked]>
Date: Mon, 6 Apr 2026 10:14:33 -0400

> I suggest you rethink your approach of engaging with others.

+1

-- 
Peter Geoghegan

---

From: Andrei Lepikhov <[email-masked]>
Date: Mon, 6 Apr 2026 17:11:15 +0200

> seems ... not entirely realistic, to put it very kindly.
Sorry about that.

I haven't had much practice with English. Sometimes, things I wouldn't 
normally say in technical discussions in my native language come out 
here. As well as part of the meaning definitely lost in translation.
The actual reason was to highlight that quite closely related features 
exist in the Postgres world (not only pg_hint_plan). Even if we can’t 
expose the code of enterprise forks, it worth to discuss alternative 
design ideas.

---

From: Robert Haas <[email-masked]>
Date: Mon, 6 Apr 2026 15:52:51 -0400

> Could you please have a look?

Thanks for the report. What seems to be happening here is that the
whole query is replaced by a single Result node, since the join must
be empty. But that means that unnamed_subquery doesn't make it into
the final plan tree, and then pgpa_plan_walker() is sad about not
finding it. Normally it wouldn't care, but apparently this query
involves at least one semijoin someplace that the planner considered
converting into a regular join with one side made unique, so
pgpa_plan_walker() has an entry in sj_unique_rels and then wants to
adjust that entry for the final, flattened range table, and it can't.
I'm inclined to think that the fix is just:

-            elog(ERROR, "no rtoffset for plan %s", proot->plan_name);
+            continue;

...plus a comment update, but I want to spend some time mulling over
whether that might break anything else before I go do it.

---

From: Robert Haas <[email-masked]>
Date: Mon, 6 Apr 2026 16:15:21 -0400

> LOCATION:  pgpa_build_scan, pgpa_scan.c:200

Thanks also for this report. The plan looks like this:

 Nested Loop  (cost=0.00..154.75 rows=2550 width=4)
   ->  Materialize  (cost=0.00..78.25 rows=2550 width=4)
         ->  Sample Scan on t  (cost=0.00..65.50 rows=2550 width=4)
               Sampling: system_time ('1000'::double precision)
   ->  Limit  (cost=0.00..0.01 rows=1 width=4)
         ->  Result  (cost=0.00..0.01 rows=1 width=4)

And it's unhappy because it's expecting the Materialize node to be the
RTI-bearing node. In a turn of events that will probably shock nobody
here, I also didn't quite realize that a Materialize node could get
inserted here. It's kind of a problem, too, because what if the sides
of the join were switched? Then we'd have a Nested Loop with an inner
Materialize node and would conclude that the strategy was
PGS_NESTLOOP_MATERIALIZE, when in reality it would be
PGS_NESTLOOP_PLAIN plus a Materialize node inserted at the scan level,
so the generated advice would be incorrect. I guess the fix is
probably to view a Materialize node on top of a Sample Scan for a
!repeatable_across_scans tsmhandler as part of the scan, which is kind
of annoying but probably doable. Not for the first time, I really wish
we stored an RTI set in every plan node, or (maybe more economically)
had some kind of enum in key plan nodes indicating why the node was
inserted. Right now, pg_plan_advice does a lot of reading the tea
leaves, which is great in that it avoids bloating Plan trees with
additional metadata, but a little scary in terms of being able to be
certain that one will get the right answer reliably.

I'll work on a fix.

---

From: Robert Haas <[email-masked]>
Date: Tue, 7 Apr 2026 10:17:12 -0400

> digit, and maximum NAMEDATLEN).

I committed 0001 without this change. Please feel free to propose a
clean-up patch that adds this. I wasn't certain where the best place
to add it was, or what the wording ought to be exactly.

---

From: Robert Haas <[email-masked]>
Date: Tue, 7 Apr 2026 17:21:19 -0400

> ... but what we can do about this post-freeze seems pretty

Here's a new patch set. All of these patches are new, but I'm
continuing to increment the same version number sequence.

0001 and 0002 implement the "retry a few times" idea for avoiding
test_plan_advice failures. I argue that (a) these are reasonable
post-commit stabilization that should not be blocked by feature freeze
and (b) most people here will be happier with a solution like this
that will normally cost very little than they will be with switching
test_plan_advice to executing serially. The RMT can decide whether it
agrees. The other question here is whether it's really a good idea to
apply this now considering that we've seen only one failure so far. I
think it's probably a good idea to do something like this before
release, so that we hopefully reduce the false positive rate from the
test to something much closer to zero, but I think we've still had
only the one failure, and I'm really interested in knowing how close
the failure rate is to zero already. The RMT may have an opinion on
how long to wait before doing something like this, too.

0003 fixes the problem with tablesample scans that Alexander Lakhin
reported. The bug occurs when a tablesample handler does not set
repeatable_across_scans and the resulting Sample Scan appears below a
join. The test case provided by Alexander shows the Sample Scan on the
inner side of the join, but it's also possible to construct a case
where it occurs on the outer side of the join. This commit adds tests
for both cases.

0004 fixes an oversight in commit
6455e55b0da47255f332a96f005ba0dd1c7176c2, which failed to add a new
pg_regress test to the pg_plan_advice Makefile.

0005 fixes the other issue that Alexander Lakhin recently reported,
which manifested as ERROR:  no rtoffset for plan unnamed_subquery when
trying to generate advice. That turns out to occur when a subquery is
proven empty and that subquery contains a semijoin that could have
been implemented by making one side unique. I chose a different fix
than what I mentioned in my response to Alexander's email. There was
already code that handles the case where a SubPlanRTInfo exists and is
marked dummy, and this fix extends that handling to the case where no
SubPlanRTInfo exists at all, which seems better than treating those
two cases in separate parts of the code.

I don't think that anyone will argue that 0003-0005 are things we
can't or shouldn't fix after feature freeze, and I plan to apply those
fixes shortly after feature freeze, unless there are objections or
better ideas. I could rush them in before that, too, but I don't think
what the tree needs are more people trying to commit all at once right
now.

Thanks,

---

From: Tom Lane <[email-masked]>
Date: Tue, 07 Apr 2026 18:05:47 -0400

Robert Haas <[email-masked]> writes:
> agrees.

I'm not on the RMT, but I agree this is a nicer solution.
(I didn't read these patches in detail, but in a quick once-over
they seemed plausible.)

> The other question here is whether it's really a good idea to

No strong opinion about that.  Certainly waiting a couple of weeks
to gather more data seems reasonable.

			regards, tom lane

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 8 Apr 2026 09:49:01 -0500

> to gather more data seems reasonable.

I am only 1/3 of the RMT, but I am fine with the plan as stated.

-- 
nathan

---

From: Melanie Plageman <[email-masked]>
Date: Wed, 8 Apr 2026 12:18:11 -0400

> I am only 1/3 of the RMT, but I am fine with the plan as stated.

I agree with waiting a few weeks to continue catching bugs.

As for 0001/0002 and the retry approach: if that's the best way to
avoid spurious test failures, I'm fine with it. I haven't reviewed the
code in detail and don't have an alternative to suggest. I'm
definitely against running anything serially.

As for the other ideas and suggestions so far:

I don't see a way to split up the regression test suite that wouldn't
make it harder to figure out where to add tests in the future. The
whole point is to avoid regressing pg_plan_advice when new things are
added to the planner, and that works because people don't have to
think about a pg_plan_advice -- their new test queries automatically
get coverage.

I do think there needs to be a way to run this in CI, but it doesn't
have to be on by default.

For the buildfarm, I don't have a strong opinion about whether to
limit it to some animals or some runs. Running on only some animals is
easier to reason about when you see a failure (i.e. that animal runs
with test_plan_advice, so it might be that), but running it once a day
or once a week on all animals gives broader coverage. That said, the
kind of coverage you gain from timing differences across animals --
catching races and transient issues -- may be less relevant for
test_plan_advice than for other tests.

- Melanie

---

From: Robert Haas <[email-masked]>
Date: Mon, 13 Apr 2026 12:01:25 -0400

> I agree with waiting a few weeks to continue catching bugs.

Sounds like we have a consensus. I have committed the three bug-fix
patches (unrelated to the retry-loop stuff) plus the preparatory
refactoring patch for the retry-loop patch. That renames a few
identifiers, so it seemed best to get it out of the way sooner rather
than later. I'll hold off on the main retry-loop patch for now. So far
I haven't seen any other buildfarm failures that look related to this
issue, so either I've missed some (which is certainly possible) or the
chances of failure are very low.

Thanks,

---

From: Alexander Lakhin <[email-masked]>
Date: Tue, 14 Apr 2026 21:00:01 +0300

13.04.2026 19:01, Robert Haas wrote:
> patches (unrelated to the retry-loop stuff) ...

Thanks again for committing these fixes, Robert! With all the fixes in
place, I and SQLsmith have reached another error:
CREATE TABLE t1(a int);
CREATE TABLE t2(b int);

SELECT 1 FROM t1 WHERE EXISTS
   (SELECT 1 FROM
     (SELECT 1 FROM
       (SELECT 1) LEFT JOIN t2 ON true),
     t2 WHERE a = b);

ERROR:  XX000: unique semijoin found for relids (b 3 5 7) but not observed during planning
LOCATION:  pgpa_plan_walker, pgpa_walker.c:153

Best regards,
Alexander

---

From: Tender Wang <[email-masked]>
Date: Wed, 15 Apr 2026 18:30:01 +0800

> Could you please have a look?
I did some research, and  the sj_unique_rtis contains {3,5,6,7}.
You can see that 6 is in the set. How 6 is added into the uniquerel->relids.
After deconstruct_jointree(), the joinlist is as follow:
(gdb) call nodeToString(joinlist)
$1 = 0x1ef1238 "({RANGETBLREF :rtindex 1} {RANGETBLREF :rtindex 7}
{RANGETBLREF :rtindex 5} {RANGETBLREF :rtindex 3})"
You can see that no 6 in the list.
The 6 is added when processing (7, 5), in make_join_rel(), we have below logic:

   /*
    * Add outer join relid(s) to form the canonical relids.  Any added outer
    * joins besides sjinfo itself are appended to pushed_down_joins.
    */
   joinrelids = add_outer_joins_to_relids(root, joinrelids, sjinfo,
                                 &pushed_down_joins);

In this case, 6 was added to the joinrelids.
When processing {1}, {3,5,6,7}, the {3,5,6,7} is the uniquerel, so in
the pgpa_join_path_setup(),
the {3,5,6,7} was appended to proot->sj_unique_rels.

In the plan_showdown phase, in pgpa_qf_add_plan_rtis(), we can add 7,
5, and 3 to qf->relids.
It seems difficult to add "6" to qf->relids when walking through the
plan tree.(Maybe have an easy way, I don't know too much
pg_plan_advice related code).

-- 
Thanks,
Tender Wang

---

From: Robert Haas <[email-masked]>
Date: Wed, 15 Apr 2026 15:46:58 -0400

> pg_plan_advice related code).

Thanks for looking through this.  sj_unique_rtis is actually not set
from the plan tree walk, but based on the calls to
pgpa_join_path_setup that occur during planning, so it makes sense
that the join RTI crept in there. I'm guessing that this is another
place that needs a call to pgpa_filter_out_join_relids -- I've had a
few of those bugs already.

---

From: Tender Wang <[email-masked]>
Date: Thu, 16 Apr 2026 09:45:05 +0800

> few of those bugs already.
I try a quick fix as follow:
diff --git a/contrib/pg_plan_advice/pgpa_planner.c
b/contrib/pg_plan_advice/pgpa_planner.c
index 72ef3230abc..971f301e950 100644
--- a/contrib/pg_plan_advice/pgpa_planner.c
+++ b/contrib/pg_plan_advice/pgpa_planner.c
@@ -541,6 +541,7 @@ pgpa_join_path_setup(PlannerInfo *root, RelOptInfo *joinrel,
        {
                pgpa_planner_state *pps;
                RelOptInfo *uniquerel;
+               Bitmapset *relids;

                uniquerel = jointype == JOIN_UNIQUE_OUTER ? outerrel : innerrel;
                pps = GetPlannerGlobalExtensionState(root->glob,
planner_extension_id);
@@ -562,8 +563,11 @@ pgpa_join_path_setup(PlannerInfo *root,
RelOptInfo *joinrel,
                        oldcontext = MemoryContextSwitchTo(pps->mcxt);
                        proot = pgpa_planner_get_proot(pps, root);
                        if (!list_member(proot->sj_unique_rels,
uniquerel->relids))
+                       {
+                               relids =
pgpa_filter_out_join_relids(uniquerel->relids, root->parse->rtable);
                                proot->sj_unique_rels =
lappend(proot->sj_unique_rels,
-
                         bms_copy(uniquerel->relids));
+
                         bms_copy(relids));
+                       }
                        MemoryContextSwitchTo(oldcontext);
                }
        }

postgres=# LOAD 'pg_plan_advice';
LOAD
postgres=# EXPLAIN (COSTS OFF, PLAN_ADVICE)SELECT 1 FROM t1 WHERE EXISTS
   (SELECT 1 FROM
     (SELECT 1 FROM
       (SELECT 1) LEFT JOIN t2 ON true),
     t2 WHERE a = b);
                    QUERY PLAN
---------------------------------------------------
 Hash Join
   Hash Cond: (t1.a = t2.b)
   ->  Seq Scan on t1
   ->  Hash
         ->  HashAggregate
               Group Key: t2.b
               ->  Nested Loop
                     ->  Nested Loop Left Join
                           ->  Result
                           ->  Seq Scan on t2 t2_1
                     ->  Materialize
                           ->  Seq Scan on t2
 Generated Plan Advice:
   JOIN_ORDER(t1 ("*RESULT*" t2#2 t2))
   NESTED_LOOP_PLAIN(t2#2)
   NESTED_LOOP_MATERIALIZE(t2)
   HASH_JOIN((t2 t2#2 "*RESULT*"))
   SEQ_SCAN(t1 t2#2 t2)
   SEMIJOIN_UNIQUE((t2 t2#2 "*RESULT*"))
   NO_GATHER(t1 t2 t2#2 "*RESULT*")
(20 rows)

-- 
Thanks,
Tender Wang

---

From: Robert Haas <[email-masked]>
Date: Fri, 17 Apr 2026 15:00:05 -0400

> I try a quick fix as follow:

Thanks, but that's not quite correct: it filters out the unique relids
only after testing the list, and also copies the list an extra time
unnecessarily. I've pushed a fix that I believe to be correct, with a
test case.

---

From: Michael Paquier <[email-masked]>
Date: Wed, 10 Jun 2026 09:21:25 +0900

> reasonable length of time to wait for people to provide feedback.

(Speaking with the pg_hint_plan kind-of-maintainer hat on.)
The timing is fine IMO.  In terms of integration with new APIs of
upstream, there is really nothing one can do until we are at least in
feature freeze.  Trying to work around APIs that have been committed
in the tree, which may be tuned after the initial commit, is just a
loss of time.  Things may get adjusted during beta, but the waves are
much weaker to deal with.

> be fresh in my mind when the feedback arrives.

I have an answer to this one, in the shape of the following commits in
pg_hint_plan:
https://github.com/ossc-db/pg_hint_plan/commit/e42246a82589001de2f08255d3b4d984fb134d38
https://github.com/ossc-db/pg_hint_plan/commit/75b3d0142d2a8ea0e3d656e1c95ea3fdd6e8f082
https://github.com/ossc-db/pg_hint_plan/commit/5d386d3ecb832d3ea205d1e42e305cafefbefc76

The first commit is the most relevant one, and on a number basis I
finish with that, where I have been able to basically remove *all* the
historical hacks of the model in terms of plugs it added in the
planner:
 17 files changed, 1486 insertions(+), 3820 deletions(-)

At the end, I am particularly happy with the way things are regarding
the new join_path_setup and joinrel_setup hooks, that have removed
most of the bloat.

One thing that has caused me quite a bit of headache was parallel
hints.  At the end, I have followed Lukas suggestion to remove the old
path regeneration logic that was based on an enforcement of the GUCs
and switched to the PGS logic.  This is coming with some breakages in
the module, but these are actually super minor compared to the
accumulation of weird historical behavior that we had in it:
- When specifying only a JOIN hint (without leading), we now let the
planner decide the inner/outer order depending on the cost it sees,
not the order of the clauses.  That can always be enforced with a
Leading hint, which is the same thing as the JOIN_ORDER hint in
pg_plan_advice.
- Some slight changes in the way parallel hints are propagated to
child relations, due to build_simple_rel_hook().  We cannot really
avoid that, both behaviors are debatable, edge enough that I don't
worry much in terms of plan instabilities after a major release.  We
have some degree of that for each major release, users care *a lot*
about plan stability across minor releases, work around these after
major upgrades.

I strongly suspect that all these things are just going to be noise.
The regression test suite has basically no changes.

There are still gaps between pg_plan_advice and pg_hint_plan, and the
maintenance of the latter is now muuuuuch easier (still need to
maintain some versions for the stable branches).  The end game for me
would be to close the gap and merge both things together, then drop
pg_hint_plan.  I'll try to find some victi^D^D^D^D^D resources to do
some of the leg work to do the gap here (not planning to do that
myself), for some patches to-be-proposed in v20.  We have row hints,
parallel worker hints (aka RelOptInfo.rel_parallel_workers), memoize
hints, SET hints that could be added to contrib/pg_plan_advice.  There
are also hints that negate scan behaviors.  The negation hints are
not that popular, I think, but that may worth considering.

As of today on HEAD, 60%-ish of the remaining code relates to the
custom hint string parsing and feeding into the various hint
structures.  40%-ish of the code comes from the hooks and the internal
routines used by the hooks, for something like 5k lines of code.  This
is a difference between night and day.  (There may be more
simplifications doable in the code, planning an extra round of checks
during beta.)

In short, thanks for the work you have done in the v19 release cycle
in this area, Robert and others.
--
Michael