On Wed, Dec 10, 2025 at 6:43 AM Jakub Wartak <[email protected]> wrote: > 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
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. > 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). 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"). > 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. 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. > 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 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. -- Robert Haas EDB: http://www.enterprisedb.com
