MIME-Version: 1.0
References: 
 <CA+TgmoZ-Jh1T6QyWoCODMVQdhTUPYkaZjWztzP1En4=ZHoKPzw@mail.gmail.com>
 <CAKZiRmxtJAFG7e1+Vs9B8ngON=AOzJbuws+1ZeH4LsbJh5AzoQ@mail.gmail.com>
 <CA+TgmoY9Ne_Sh10u6LSPc3wvOQPLp3kF9nBp3nqJEG2JGF2QiA@mail.gmail.com>
 <CA+Tgmoa57S6mP=aTOXH2-gDAL4TMO1WbGgrHSg0s6J4zUH=04g@mail.gmail.com>
 <DEB1SAGVDX7S.JMV96X4R0RCS@gmail.com>
 <CA+Tgmoaf__2B0BUL+vrg28P+3buX=Ti-kybqkHiLTtFrrCfzuA@mail.gmail.com>
 <CA+TgmoYpcLNOuypOTdgCSLW7FuA=t6BtB3meTARHX2-Dj_81xQ@mail.gmail.com>
 <DEFNP3WX7BLX.1X7DS1P9X6JIF@nmfay.com>
 <CA+TgmoZjv9OyFu1Gkt78w0vWEti8S33w8trYHmErf-GMmGSi=w@mail.gmail.com>
 <DELPC4B56Y5A.3OBZ3WCH3T4VX@nmfay.com>
 <CA+TgmoaOSBQD9Ux4eG40w723ZN=c0J7p-+oX4+J8urUeyLMo5w@mail.gmail.com>
 <CAKZiRmww2jEHQxAH5gtpcNUG2n09Vx0bFqwbpmmc29s3STh_dg@mail.gmail.com>
In-Reply-To: 
 <CAKZiRmww2jEHQxAH5gtpcNUG2n09Vx0bFqwbpmmc29s3STh_dg@mail.gmail.com>
From: Robert Haas <robertmhaas@gmail.com>
Date: Wed, 10 Dec 2025 08:33:11 -0500
Message-ID: 
 <CA+TgmoZDa+ZL8a1HFGyGdrRX9+MXcYNm_2tfm=Wp=AY9FehQSw@mail.gmail.com>
Subject: Re: pg_plan_advice
To: Jakub Wartak <jakub.wartak@enterprisedb.com>
Cc: Dian Fay <di@nmfay.com>, Matheus Alcantara <matheusssilv97@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CA%2BTgmoZDa%2BZL8a1HFGyGdrRX9%2BMXcYNm_2tfm%3DWp%3DAY9FehQSw%40mail.gmail.com>
Precedence: bulk

On Wed, Dec 10, 2025 at 6:43=E2=80=AFAM Jakub Wartak
<jakub.wartak@enterprisedb.com> 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=3D'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=3D'format xml,
plan_advice' or auto_explain.log_custom_options=3D'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 =3D
> '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=3D"-c
> shared_preload_libraries=3D'pg_plan_advice'"  meson test  # It was clean
> 2. PG_TEST_INITDB_EXTRA_OPTS=3D"-c
> shared_preload_libraries=3D'pg_plan_advice'" PGOPTIONS=3D"-c
> pg_plan_advice.advice=3DNESTED_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=3D"-c
> shared_preload_libraries=3D'pg_plan_advice' -c
> pg_plan_advice.shared_collection_limit=3D42"  meson test # It was clean
> too

You can set pg_plan_advice.always_explain_supplied_advice=3Dfalse 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.

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