On Tue, Mar 3, 2026 at 6:42 AM Jakub Wartak <[email protected]> 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. > 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. > It floods like that for everything, most visible with couple of independent > starts of pgbench -M prepared. Maybe I'm wrong , but I don't think it worked > like that earlier before refactor? The functionality has not changed since I first posted this. It's only had bug fixes and now some refactoring. > 3a. Because query_id each time will be different for every query even > in standard > pgbench mode, I've managed to achieve it only using prepared statements. > Realistically we'll need some way of modular matching not just on query_id OR > query_id should be changed how it being calculcated or maybe by some other > means > (argument is: not all users/apps use prepared statements): Sure. See once again the first point in this email, about how this is a toolkit. You can write your own module and use pg_plan_advice_add_advisor to add your own hook that does the matching any way it likes and returns any string it wants. Maybe someone will add that functionality to pg_stash_advice at some point in the future, or maybe somebody will add a separate module that does it, either in contrib or on PGXN or wherever they want to publish it. There are lots of nice things here that people could want to have, but none of that is going to happen in time for PostgreSQL 19. The very most we're going to get is what I've already posted. It's way too late to think about reinventing query jumbling or even designing a new way to do query matching. Feature freeze is in just over a month. The question is whether it's reasonable to commit what I've already got, not whether it's reasonable to start a whole new subproject. > 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? > 4. The familiy of pg_*stash*() functions could return some ::bool result > instead > of void. Like true? (it's usage "feeling" that leaves one wondering if > the command > was effective or not, e.g. to get consistency with let's say pg_reload_conf()) I'm up for changing the return value to something more informative if there's an informative thing to return, but if there is no information being returned, I stand by the choice of void as most appropriate. > 5. QQ: will pg_stash_advice persist the stashes one day? I have no current plan to implement that, but who knows? > 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. > 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. > 8. I'm wondering if we should apply standard PostgreSQL case insensitivity > rule (e.g. like for relations) for those stashes? On one front we ignore case > sensitivty for objects, one another this is GUC so perhaps it is OK (I > feel it is > ok, but I wanted to ask). Yeah, I'm not sure. I wondered about this, too. > 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. > 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): > > create table t1 as select * from generate_series(1, 100000) as id; > create unique index t1_pk on t1 (id); > analyze t1; > > -- OK "matched" > postgres=# set pg_plan_advice.advice to 'INDEX_ONLY_SCAN(t1@minmax_1 > public.t1_pk)'; > SET > postgres=# explain (plan_advice, costs off) select max(id) from t1; > QUERY PLAN > ----------------------------------------------------------- > Result > Replaces: MinMaxAggregate > InitPlan minmax_1 > -> Limit > -> Index Only Scan Backward using t1_pk on t1 > Index Cond: (id IS NOT NULL) > Supplied Plan Advice: > INDEX_ONLY_SCAN(t1@minmax_1 public.t1_pk) /* matched */ > Generated Plan Advice: > INDEX_ONLY_SCAN(t1@minmax_1 public.t1_pk) > NO_GATHER(t1@minmax_1) > (11 rows) > > Manual SET, wont work and it is failed (which is OK) > > postgres=# set pg_plan_advice.advice to 'SEQ_SCAN(t1)'; > SET > postgres=# explain (plan_advice, costs off) select max(id) from t1; > QUERY PLAN > ---------------------------------------------------------- > Result > Replaces: MinMaxAggregate > InitPlan minmax_1 > -> Limit > -> Index Only Scan Backward using t1_pk on t1 > Index Cond: (id IS NOT NULL) > Supplied Plan Advice: > SEQ_SCAN(t1) /* matched, failed */ > Generated Plan Advice: > INDEX_ONLY_SCAN(t1@minmax_1 public.t1_pk) > NO_GATHER(t1@minmax_1) > > However with below SEQ_SCAN is applied/matched, but marked as failed > (so bug?): > > postgres=# set pg_plan_advice.advice to 'SEQ_SCAN(t1@minmax_1)'; > SET > postgres=# explain (plan_advice, costs off) select max(id) from t1; > QUERY PLAN > ----------------------------------------------- > Aggregate > -> Seq Scan on t1 > Supplied Plan Advice: > SEQ_SCAN(t1@minmax_1) /* matched, failed */ > Generated Plan Advice: > SEQ_SCAN(t1) > NO_GATHER(t1) 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. Here again, we can do more things in future releases, but it's too late to add more to the scope for this release. There's still plenty of time to fix bugs, but this isn't a bug. We'd need a whole lot of new machinery to prevent this sort of thing, including new core hooks and new syntax. Here again, we have the freedom to decide that I chose the scope wrongly and that this is therefore not shippable as is, but I do not think there is time to significantly expand the scope at this point. -- Robert Haas EDB: http://www.enterprisedb.com
