Re: pg_*_advice: tsv load failure, etc.

On Thu, Aug 27, 2026 at 1:18 PM Noah Misch <noah(at)leadboat(dot)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