pg_*_advice: tsv load failure, etc.

On Thu, Aug 27, 2026 at 1:18 PM Noah Misch <noah@leadboat.com> wrote:

An Opus 4.8 review of pg_*_advice commits wrote test cases, attached, for
several malfunctions. The test patch also contains its markdown report. A
lot of its findings are more like long-term bugfix opportunities than open
items, but I recommend treating at least these as an open item batch:

### 1. An empty (non-NULL) stashed advice string persists a TSV line the loader rejects

That's a bug.

### 3. pg_start_stash_advice_worker() destroys an existing pg_stash_advice.tsv when persistence was off at boot, silently losing all persisted advice

I wasn't 100% convinced this is unintentional, but Opus is more convinced.
Worth a look from you.

This seems like complaining that when you turn the lights off the room
gets really dark. You should only start the worker if you're hoping
for what is currently in memory to get written out to disk. In this
example, what's in memory is nothing, so the file is removed instead.

### 14. Stash-supplied advice silently overrides an explicitly-set pg_plan_advice.advice for the same query; the precedence is documented nowhere

I do feel the GUC should preempt the stash. Best to solidify this behavior
before release.

IMHO, it's impossible to look at
pg_plan_advice_get_supplied_query_advice() and think that the current
behavior was anything but intentional. Of course, that doesn't mean
it's optimal, but if you'd like to propose changing it, I suggest
starting a new thread on that specific topic and making some kind of
argument for your preferred behavior. I suspect a lot of people are
going to think that this thread is just about bug-fixing, rather than
proposed behavior changes. I'm fine with changing this if others
agree.

Here's the second tier, as I see it:

### 5. pg_plan_advice.advice / stash advice changes are silently ignored by an already-cached generic plan

This is a planner control feature; it does not affect behavior other
than at plan time. I don't see that as a bug. That said, I think it
would be perfectly valid for someone to try to figure a way for advice
stash changes to invalidate plans, but I suspect that will require
significant new infrastructure. Plan invalidation is generally tied to
catalog modifications, and here we would instead want to tie it to a
plan ID. We could do that by adding a custom invalidation type to
sinval.h just for the use of pg_stash_advice, but would be a pretty
serious piece of core infrastructure for an as-yet-unproven contrib
module to use to solve a problem which (for all we know now) may have
little practical impact.

### 8. pg_set_stashed_advice stores advice with no syntax validation, so malformed advice is accepted then emits a plan-time WARNING on every execution and applies nothing

I'm not saying we couldn't change this, but I think changing it right
before release would be a questionable decision. The revised behavior
would probably be nicer for users, but I think it would only be
slightly nicer. Also, we should weigh any potential benefit against
the risk of messing something up under time pressure. Overall, I feel
like this is a feature request that is arriving about six months too
late. If someone feels motivated to work on it, they can do it for
v20. It would be a backward-incompatibility, but I think the number of
people intentionally storing invalid strings in their advice stashes
will be very small, so I don't think it would be enough of a backward
incompatibility to worry about.

Looking through the other findings in this report:

#2 suggests that we should add a CHECK_FOR_INTERRUPTS() call in some
code path. That seems like a bug fix to me, so I'll work on that.

#4 says GEQO doesn't know about plan advice. That's true. The observed
behavior is implied by this statement in the documentation: "... the
use of plan advice can only affect which plan the planner chooses from
among those it believes to be viable. It can never force the choice of
a plan which the planner refused to consider in the first place."
Maybe we should add a specific mention of GEQO. I see that an XXX
about GEQO has crept into the committed version of
contrib/pg_plan_advice/README, and that should be replaced with
something more appropriate.

#6 purports to be a bug, but seems like pedantry.

#7 looks like it might be a real bug. I will investigate.

#9 is wrong about what the behavior should be. There could be an
argument for tidying something up here, but I don't think it really
matters.

#10 is similar to #7 and might also be a real bug. I will investigate.

#11 doesn't seem valid. I had no intent of making that case work as
described, and I don't think it would be useful.

#12 is already fixed.

#13 seems like there might be some behavior in there that somebody
might like to change for some reason at some point, but there's no
clear defect.

#15 and #16 are design behavior.

#17 seems like another place where maybe there's some room for
improvement and maybe there isn't, but I don't think we need to worry
about it right now.

#18 is bogus.

#19 might be a real bug, but needs validation, and can't be easily
validated with in-core code.

#20 seems like it might be suboptimal behavior, but it's not
immediately clear to me what would be better, and it seems unlikely to
matter very much in practice. I'm open to proposed changes if someone
wants to spend time thinking about this.

Overall, I think this report does more to show that this feature is
good shape than otherwise. Claude's best candidate for a major bug
only affects users who stash advice strings for queries that don't
need a stashed advice string. Certainly, that and a few of the other
things shown here are worth correcting, but if somebody showed me the
subset of this list that actually consists of valid issues and told me
that was what we'd had to fix in 19.1 or 19.2 or whatever, I would be
genuinely delighted that we had somehow gotten enough people using
pg_plan_advice and pg_stash_advice to find this kind of thing.

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

On Fri, Aug 28, 2026 at 10:21 AM Robert Haas <robertmhaas@gmail.com> wrote:

That's a bug.

Here's a patch. Still working on the rest.

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

On Fri, Aug 28, 2026 at 4:21 PM Robert Haas <robertmhaas@gmail.com> wrote:

### 5. pg_plan_advice.advice / stash advice changes are silently ignored by an already-cached generic plan

This is a planner control feature; it does not affect behavior other
than at plan time. I don't see that as a bug. That said, I think it
would be perfectly valid for someone to try to figure a way for advice
stash changes to invalidate plans, but I suspect that will require
significant new infrastructure. Plan invalidation is generally tied to
catalog modifications, and here we would instead want to tie it to a
plan ID. We could do that by adding a custom invalidation type to
sinval.h just for the use of pg_stash_advice, but would be a pretty
serious piece of core infrastructure for an as-yet-unproven contrib
module to use to solve a problem which (for all we know now) may have
little practical impact.

FWIW, this was already discussed back in the day I think in [1]/messages/by-id/CA+TgmoYO0qtqz+V7S4q0e_dLhLrrsMxA51t5wks_y8Skv6cdRQ@mail.gmail.com. I wouldn't
remeber it otherwise if not the mailing list, but apparently ANALYZE on the
table affected was enough to trigger the change [2]/messages/by-id/CA+Tgmob8O4TbZVr2zoqm5m-Zp6fj-8iBh=0u-xfiy5Xr5MNFCQ@mail.gmail.com. Perhaps adding one
statement to the docs would be good enough to cover it for now, like: "In
order to change the already cached plans, issue ANALYZE on relevant tables."
(because apparently without it it now confused 2 people and 1 AI ;))

-J.

[1]: /messages/by-id/CA+TgmoYO0qtqz+V7S4q0e_dLhLrrsMxA51t5wks_y8Skv6cdRQ@mail.gmail.com
[2]: /messages/by-id/CA+Tgmob8O4TbZVr2zoqm5m-Zp6fj-8iBh=0u-xfiy5Xr5MNFCQ@mail.gmail.com

On Fri, Aug 28, 2026 at 10:21:29AM -0400, Robert Haas wrote:

On Thu, Aug 27, 2026 at 1:18 PM Noah Misch <noah@leadboat.com> wrote:

### 14. Stash-supplied advice silently overrides an explicitly-set pg_plan_advice.advice for the same query; the precedence is documented nowhere

I do feel the GUC should preempt the stash. Best to solidify this behavior
before release.

IMHO, it's impossible to look at
pg_plan_advice_get_supplied_query_advice() and think that the current
behavior was anything but intentional. Of course, that doesn't mean
it's optimal, but if you'd like to propose changing it, I suggest
starting a new thread on that specific topic and making some kind of
argument for your preferred behavior. I suspect a lot of people are
going to think that this thread is just about bug-fixing, rather than
proposed behavior changes. I'm fine with changing this if others
agree.

I agree the pg_plan_advice implementation gives what the code calls "advisors"
precedence over this GUC. I opined this way because, as a user, I expect
more-specific knobs to override less-specific knobs. The stash is the less
specific of the two. That said, I don't plan to pursue this precedence
question further.

Here's the second tier, as I see it:

### 5. pg_plan_advice.advice / stash advice changes are silently ignored by an already-cached generic plan

This is a planner control feature; it does not affect behavior other
than at plan time. I don't see that as a bug. That said, I think it
would be perfectly valid for someone to try to figure a way for advice
stash changes to invalidate plans, but I suspect that will require
significant new infrastructure. Plan invalidation is generally tied to
catalog modifications, and here we would instead want to tie it to a
plan ID. We could do that by adding a custom invalidation type to
sinval.h just for the use of pg_stash_advice, but would be a pretty
serious piece of core infrastructure for an as-yet-unproven contrib
module to use to solve a problem which (for all we know now) may have
little practical impact.

True, best to leave that as-is.

On Mon, Aug 31, 2026 at 12:55 AM Robert Haas <robertmhaas@gmail.com> wrote:

Here's a patch. Still working on the rest.

Here it is again, with a second patch to add CHECK_FOR_INTERRUPTS()
and check_stack_depth() calls.

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

On Thu, Sep 3, 2026 at 4:11 PM Robert Haas <robertmhaas@gmail.com> wrote:

Here it is again, with a second patch to add CHECK_FOR_INTERRUPTS()
and check_stack_depth() calls.

v3, with more patches.

0001 fixes finding #1 from the original report, namely, that empty
advice string causes a pg_stash_advice.tsv reload failure.

0002 fixes finding #2 from the original report, the lack of
CHECK_FOR_INTERRUPTS() calls inside loops, especially the JOIN_ORDER()
loop. It also adds check_stack_depth() calls in to various recursive
functions.

0003 fixes finding #4 from the original report by documenting the
intended interaction with GEQO and removing the XXX comment from the
README.

0004 fixes a GEQO-related oversight in the original disabled_nodes
patch that could result in the intended behavior from 0003 not holding
in all cases. In principle, this should be back-patched to v18, but
since the non-hacky way of fixing it requires an ABI change and
nobody's complained, I am inclined, at least for now, to fix only v19
and master.

0005 fixes finding #7 form the original report, namely that something
like JOIN_ORDER(f {d1 d2}) tends to get marked /* matched, failed */
rather than /* matched */ even when everything is working. I suppose I
sort of knew about this and thought it wasn't that big of a deal, but
I now think that may have been laziness on my part rather than a
principled position.

There are still some more things to fix here, so I'll keep working on
this next week.

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

On Fri, Sep 4, 2026 at 3:40 PM Robert Haas <robertmhaas@gmail.com> wrote:

There are still some more things to fix here, so I'll keep working on
this next week.

Here is a new patch set. Since my last update, I have committed the
previous 0001 as dc33cca1859e2d2500ee8f3c0e564561428b4fcf and the
previous 0002 as 1129570f654e96d45bbc63397079171af29710b9. In this new
patch set, 0001 and 0002 correspond to the previous 0003 and 0004, and
improve the code and documentation in terms of how this feature
interacts with GEQO.

0003 from this patch set corresponds to the previous 0005, but the
scope has been expanded to fix additional problems in the same area of
the code. Generally, all of these problems stem from advice
enforcement (which tries to make the plan obey the advice) being out
of step with advice feedback (which says whether the plan actually did
obey the advice). As far as I have found so far, those things are in
lock step for all of the cases that test_plan_advice exercises, or to
say it differently, they're in lock step for all the kinds of advice
that pg_plan_advice generates itself. But if you write your own custom
advice strings, you can do things that are legal but not what the
advice generator would have done, and then problems ensue. For
example, you could write JOIN_ORDER((a b) c) instead of the more
intuitive JOIN_ORDER(a b c); the meaning is identical either way. This
patch fixes several of the worst issues; see the commit message for
details. There are also some cases that it doesn't fix. I know of two
such cases, one easy and one hard.

The easy problem is with zero-element sublists, like JOIN_ORDER(a ()).
I suspect the correct thing to do is reject this case in the advice
string parser, similar to what
8c9c2e5c09aeda713640ccb26bfca0fdccc310d5 did for FOREIGN_JOIN. I have
not deeply investigated this yet so maybe there's some wrinkle that
needs considering, but I expect fixing this to be straightforward.

The hard problem is finding #10 from Noah's original report, which is
the case where you have two partitions of the same parent table that
have the same name but are in different schemas, and you omit the
schema name from the relation identifier. That is, you have a table,
let's say foo, with partitions bar.foopart and baz.foopart. Instead of
writing something like SEQ_SCAN(foo/bar.foopart) as the advice
generator would do, you choose to write SEQ_SCAN(foo/foopart), making
it ambiguous which child relation you're talking about. What happens
right now is that advice enforcement will enforce that restriction
against both children, but advice feedback will think that doesn't
affect either one, so if you look at the advice feedback with EXPLAIN
or print it out via pg_plan_advice.feedback_warnings, you'll see
"matched, failed" instead of just "matched". Obviously, this kind of
sucks. Of course, the blast radius is limited by the fact that almost
nobody names their partitions this way, but theoretically they could
and nobody likes features that work most of the time.

Unfortunately, it is not at all straightforward to fix this. Pretty
much everything the advice feedback code starts by mapping each
relation identifier to an RTI, and giving up if the number of matches
is not exactly 1. Since the whole purpose of relation identifiers is
to map uniquely to relations appearing in the query, this is in
general fine, but the case described in the previous paragraph breaks
it. Generalizing the logic in the advice feedback code to be able to
handle a relation identifier that maps to multiple RTIs looks like a
bad idea. It would require massive adjustments to the code for a case
that almost nobody has in real life, and the resulting code would be
complicated and hard to understand and probably have corner cases
where the performance is terrible. So I think something else has to
give. A possible zero-order solution is to just document that advice
feedback isn't guaranteed to work properly if you do this, so maybe
you shouldn't. That is obviously not amazing but I think we could live
with it for v19. What I'm thinking is probably better is to impose a
restriction sufficient to keep this case from arising in the first
place, such as:

1. Just rip out all the logic that allows the partition schema to be
omitted, and require it always. This is only sad for people writing
advice strings manually, since generated advice always includes the
schema anyway.

2. Refuse to enforce advice if the partition schema is omitted and
there's more than one partition with the same partition name. This
changes the rule from "you can leave out the partition schema" to "you
can leave out the partition schema when no ambiguity is thereby
created," and it makes the existing behavior of the advice feedback
system correct in retrospect. This seems like it would make just about
nobody sad and therefore be just about perfect, except that I am not
sure there's any way of implementing it that doesn't result in causing
even bigger problems that the one it's trying to solve. More
investigation needed.

Or maybe there is another idea that is better than either of those; I
need to look into this further.

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