Re: pg_plan_advice (now with transparent SQL plan performance overrides - pg_stash_advice)

On Tue, Mar 3, 2026 at 7:56 PM Robert Haas <robertmhaas@gmail.com> wrote:

Hi Robert,

> On Tue, Mar 3, 2026 at 6:42 AM Jakub Wartak
> <jakub.wartak@enterprisedb.com> wrote:
> > 1. First thought is that I found it quite surprising, we now have 3 modules
> > and it's might cause confusion and lack of consistency:
> > - 3 can to be in shared_preload_libraries (pg_stash_advice,
> > pg_plan_advice, pg_collecti_advice)
> > - 2 others can use CREATE EXTENSION (pg_stash_advice, pg_collect_advice), but
> >   "create extension pg_plan_advice;" fails
> > so maybe they all should behave the same as people (including me) won't read
> > the docs and just blindly add it here and there and issue CREATE EXTENSION,
> > but it's going to be hard to remember for which ones (?) So we need more
> > consistency?
>
> That's a possible point of view, but the flip side is that then it's
> all-or-nothing. I think pg_plan_advice is a toolkit that bridge as a
> rather large gulf between the theoretical power to walk plan trees and
> use pgs_mask to modify behavior and the practicalities of actually
> doing so. I think we would be well-advised to think of pg_plan_advice
> as a core of functionality that, some day, we might even think of
> moving into core, and pg_collect_advice and pg_stash_advice as things
> that can be built around that core. I think it's important to have
> demonstrations available that show that pg_plan_advice is not a
> "sealed hood" that you must accept exactly as it is, but rather a
> toolkit that you can do a variety of things with. pg_collect_advice
> and pg_stash_advice are examples of what you can do, but many
> variations on those same themes are possible.

This is micro-thing, feel free to ignore, but well I was after something much
more easy: `CREATE EXTENSION pg_plan_advice`to be no-op without any failure
even if it doesnt provide any views or fucntions right now (so empty
share/extension/pg_plan_advice.control and -1.0.sql) or at least some dummy
function, just so that CREATE EXTENSION pg_plan_advice wouldn't fail.
This is nothing technical, it's just sharp rough edge for user (but
technically sound), that 2 are deployed with CREATE EXTENSION but 3rd one is
not. I just think that if we put 3 into shared_preload_libraries then I won't
have to think for which ones to exec CREATE EXTENSION (I would just blindly do
it for all and the error wouldn't make somebody unhappy that something is
potentially not working because CREATE EXTENSION is not for it) - it's pure
user-focused usability feedback.

> > 2. Should pgca always duplicate entries like that? (each call gets new entry?)
> > Shouldn't it just update collection_time for identical calls of
> > userid/dbid/queryid?
>
> To make it do that, we would need a completely different way of
> storing entries, in order to avoid a linear scan of all collected data
> for each new query. It would be a completely different module. I think
> we probably want to have something like that, but it isn't this. See
> previous point.
>
[..]
> The functionality has not changed since I first posted this. It's only
> had bug fixes and now some refactoring.

OK, I misremembered it then.

> > 3c. However for pgbench -M prepared, such online plan alterations are
> > strangley not effective.
> [....]
> > 3d. ... however restarting restarting pgbench helps and the session
> > changes plan (and thus
> > performance characteristics).
> > 3e. As this was pretty concerning, I've repeated the pgbench -M prepared
> > excercise and I've figured out that I could achieve effect that I wanted (
> > boucing between fast <-> slow immediatley) by injecting call via gdb on that
> > backend to InvalidateSystemCaches(). Kind of brute-force, but only
> > then it worked
>
> One thing to keep in mind is that the whole point of -M prepared is to
> avoid replanning. My guess is that what you're seeing here is that
> only replan when something invalidates the plan, which seems more like
> a feature than a bug, although possibly somebody is going to want a
> way to force an invalidation when the stashed advice changes. That's a
> pretty big hammer, though. Does this behavior go away if you use -M
> extended?

Yes with -M extended it is instant. I have found also that with
-M prepared I can do simple one-time `analyze pgbench_accounts` (when changing
SEQ_SCAN <-> INDEX_SCAN for that table) and that is also enough for
the backend to immediatley see (and react to) to what's in the active
configured stash even for future changes without further ANALYZEs.
Not sure if pg_stash_advice needs a function to flush-force all backends,
so the plans are 100% effecitve, as apparently we seem to have ANALYZE
already, but it is not that obvious that one might want to use it.

If there would be such function to gurantee, we probably wouldn't see
complaints like 'I have done this and session still is using old plan'.

>
> > 5. QQ: will pg_stash_advice persist the stashes one day?
>
> I have no current plan to implement that, but who knows?

OK, so perhaps docs for pg_create_advice_stash() and pg_set_stashed_advice()
should mention those 'stashes' are not persistent across restarts. Without
this I can already hear scream of some users from the future that they
applied advice, it fixed problem and after some time it disappeared (In
other RDBMS it is persistent, so users coming from there might have such
expectations).

Also related, we could also mention that this information is not replicated to
standbys as this is in-memory only.

> > 6. Any idea for better name than 'stash' ? :) It's some new term that is for
> > sure not wildly recognized. Some other used name across industry: plan
> > stability,
> > advice freeze, plan freeze, plan force, baselines, plan management, force plan,
> > force paths, SQL plan optimization profiles, query store (MSSQL).
>
> I'm up to change this if there's a consensus on what it should be
> called, but that will require more than 1 vote. I picked stash because
> I wanted to capture the idea of sticking something in a place where
> you could grab it easily. Personally, my main critique of that name is
> that this particular module matches by query ID and you could instead
> match by $WHATEVER, so what would you call the next module that also
> stashes advice strings but keyed in some slightly different way?
> (Likewise, what do you call the next alternative to pg_collect_advice
> that uses different collection criteria or storage?)
>
> Honestly, I was hoping and sort of expecting to get a lot more naming
> feedback back when I first posted this. Don't call it advice, call it
> hints or guidance or tweaks! Don't write HASH_JOIN(foo), write
> JOIN_METHOD(foo, HASH_JOIN)! And so forth. On the one hand, the fact
> that I didn't get that back then has made this less work, and I'm
> still pretty happy with my choices. Furthermore, it's still not too
> late to do some renaming if we agree on what that should look like. At
> the same time, I don't particularly want to get into a fun and
> exciting game of design-by-committee-under-time-pressure. As a
> Demotivators poster said many years ago, "none of us is as dumb as all
> of us." I freely admit that some of my naming and some of my design
> decisions are almost certainly suboptimal, but at the same time, it
> would be easy to underestimate the amount of thought that I've put
> into some of the naming, so I'd only like to change it if we have a
> pretty clear consensus that any given counter-proposal is better than
> what I did. I especially do not want to go change it and then have to
> change it all again because somebody else shows up with a new opinion.

Well IMHO all the rest naming in pretty great shape and I think that
pg_[collect|plan]_advice are great names too. It's just that `stash`
keyword doesn't ring a bell to me at all that `pg_stash_advice` is
related in any way to online/transparent/runtime plan modification and
can be used to alter plans for other backends. Something like
`pg_deploy_advice` or `pg_apply_advice` would be more in line with the
other two, but perhaps it's just me..

> > 7. I saw you have written that in docs to be careful about memory use, but
> > wouldn't be it safer if that maximum memory for pgca (when collecting in shared
> > with almost infinite limite) would be still subject to like let's say 5% of s_b
> > (or any other number here)?
>
> It depends on what you mean by safer. One thing that is unsafe is
> running the computer out of memory. However, another thing that is
> unsafe is getting the wrong answer. I felt that limiting the number of
> queries was a good compromise. If you limit it to a certain amount of
> memory usage, then (1) it's much harder to actually figure out whether
> we're over budget and how much we need to free to be under budget and
> (2) I suspect it's also harder to be sure whether you've set the limit
> high enough to not lose any of the data you intended to retain. I
> think that a fixed limit like 5% of shared_buffers would be, bluntly,
> completely nuts. There is no reason to suppose that the amount of
> memory someone is willing to use to store advice has anything to do
> with the size of shared_buffers. If you have 100 sessions running
> pgbench and you enabled the local collector with that limit in all of
> them, you would be using 500% of shared_buffers in advice-storage
> memory and that would probably take down the server. On the other
> hand, if you're using the shared collector with shared_buffers=2GB,
> that is only 10MB, which might easily be too small to store all the
> data you care about even on a test system.
>
> The reason I set the limits in terms of number of queries was that (1)
> it's easy for the code to figure out whether it's over the limit and
> easy for it to reduce utilization to the limit and (2) I thought that
> people using this were likely to have a better idea how many queries
> they wanted to collect than how many MB/GB they wanted to collect. Of
> course, (1) could be solved with enough effort and (2) could be
> wrong-headed on my part, but if somebody wanted this changed, it would
> have been nice to know that a few months ago when I first posted this
> rather than now.

I havent reported it earlier, well because there was little sense playing with
collection implementation much earlier if we didn't have pinning of the plans
itself (and the whole debate query_id also seemed pointless back then). Well
anyway, it is much like work_mem danger, but work_mem is about well specific
amount of memory (* N plan nodes) so it's just easier to put safer value there.

How about if we would just measure (estimate) with some small table number of
entries vs memory used and put that into the docs?, so users are wary that
they shouldn't just blidnly set it to high value? E.g. with
1000 local limit I get ~280kB for pg_collect_advice context and with 1000000
local limit I've got it to ~50MB and stopped looking further (it was still
growing). Itself that's not terrible but higher values with lots of backends
might cause huge memory pressure (OOMs).

Or maybe other idea: is there is possibility of making GUCs like
local_collection_limits/local_collector settable only using
SET/SET LOCAL, but not global? I mean what's the point of having being
able to collect locally system-wide when realistically I cannot
pull it back from backend-local memory? (and this removes the danger
of multple backends goind wild with memory together).

Below maybe useful for others - if they trying this, if testing performance
please ensure you do not have Casserts as with Casserts enabling higher
local_collection_limits are barley usable:

progress: 137.0 s, 1516.9 tps, lat 0.659 ms stddev 0.174, 0 failed
progress: 138.0 s, 1469.1 tps, lat 0.680 ms stddev 0.174, 0 failed
progress: 139.0 s, 1050.0 tps, lat 0.952 ms stddev 0.206, 0 failed
progress: 140.0 s, 922.0 tps, lat 1.085 ms stddev 0.194, 0 failed
[..]
progress: 143.0 s, 664.0 tps, lat 1.504 ms stddev 0.377, 0 failed
progress: 144.0 s, 595.0 tps, lat 1.681 ms stddev 0.386, 0 failed
progress: 145.0 s, 531.0 tps, lat 1.883 ms stddev 0.402, 0 failed
[..]
progress: 159.0 s, 260.0 tps, lat 3.840 ms stddev 0.607, 0 failed
progress: 160.0 s, 271.0 tps, lat 3.702 ms stddev 0.447, 0 failed

resetting it back to 0 or disabling local collector and reloading won't
fix it, backend needs to re-establish connection. Even with just 10000
local collection limit it just gets down from ~1500 tps to 900 tps.
It's seems the impact on CPU coming to be from exec_simple_query()
-> finish_xact_command() -> MemoryContextCheck() -> AllocSetCheck(),
so memory context validation that have literally nothing to do with
with this patch (other than it using a lot of memory in those scenarios)

> > 9. If IsQueryIdEnabled() is false (even after trying out to use 'auto')
> > shouldn't this module raise warning when pg_stash_advice.stash_name != NULL?
>
> I think the bar for printing a warning on every single query is
> extremely high. Doing that just because of a questionable
> configuration setting doesn't seem like a good idea.

Why on every single query? I'm thinking that this should bail out in pgca&pgsa
during initialization of those modules. What's the point of those modules
if queryid is always 0? (I'm assuming somebody has compue_query_id=off and
still loaded those modules).

> > 10. I'm was here mainly for v18-0007 (pg_stash_advice), but this still looks
> > like some small bug to me (minmax matching in v18-0003):
> >
[..]
>
> I think this is just out of scope for now. It's documented that we
> don't have a way of controlling aggregation behavior at present. [..]

Oh ok, I've completley missed "pgplanadvice-limitations" SGML section about
aggregates, so sure - fair limitation.

BTW: the good news is that I haven't seen a single crash when throwing
wild stuff on it or having strange ideas at pg_stash_advice usage.

-J.