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>
 <CAOYmi+=g+MMoOpWkk2weXWKJcKH0eKey8gKHHdH0dF4Tiawrhw@mail.gmail.com>
In-Reply-To: 
 <CAOYmi+=g+MMoOpWkk2weXWKJcKH0eKey8gKHHdH0dF4Tiawrhw@mail.gmail.com>
From: Robert Haas <robertmhaas@gmail.com>
Date: Tue, 9 Dec 2025 14:45:47 -0500
Message-ID: 
 <CA+TgmobwaT=PXPDDrgDup+jA8KHBbkxigtziD-zNzAKKkQYVgQ@mail.gmail.com>
Subject: Re: pg_plan_advice
To: Jacob Champion <jacob.champion@enterprisedb.com>
Cc: Dian Fay <di@nmfay.com>, Matheus Alcantara <matheusssilv97@gmail.com>,
	Jakub Wartak <jakub.wartak@enterprisedb.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%2BTgmobwaT%3DPXPDDrgDup%2BjA8KHBbkxigtziD-zNzAKKkQYVgQ%40mail.gmail.com>
Precedence: bulk

On Mon, Dec 8, 2025 at 8:19=E2=80=AFPM Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
> I really like this idea! Telling the planner, "if you need to make a
> decision for [this thing], choose [this way]," seems to be a really
> nice way of sidestepping many of the concerns with "user control".
>
> I've started an attempt to throw a fuzzer at this, because I'm pretty
> useless when it comes to planner/optimizer review. I don't really know
> what the overall fuzzing strategy is going to be, given the multiple
> complicated inputs that have to be constructed and somehow correlated
> with each other, but I'll try to start small and expand:
>
> a) fuzz the parser first, because it's easy and we can get interesting in=
puts
> b) fuzz the AST utilities, seeded with "successful" corpus members from a=
)
> c) stare really hard at the corpus of b) and figure out how to
> usefully mutate a PlannedStmt with it
> d) use c) to fuzz pgpa_plan_walker, then pgpa_output_advice, then...?

Cool. I'm bad at fuzzing, but I think fuzzing by someone who is good
at it is very promising for this kind of patch.

> I'm in the middle of an implementation of b) now, and it noticed the
> following code (which probably bodes well for the fuzzer itself!):
>
> >        if (rid->partnsp =3D=3D NULL)
> >            result =3D psprintf("%s/%s", result,
> >                              quote_identifier(rid->partnsp));
>
> I assume that should be quote_identifier(rid->partrel)?

Yes, thanks. Fixed locally. By the way, if your fuzzer can also
produces some things to add contrib/pg_plan_advice/sql for cases like
this, that would be quite helpful. Ideally I would have caught this
with a manually-written test case, but obviously that didn't happen.

> =3D Other Notes =3D
>
> GCC 11 complains about the following code in pgpa_collect_advice():
>
> >        dsa_area   *area =3D pg_plan_advice_dsa_area();
> >        dsa_pointer ca_pointer;
> >
> >        pgpa_make_collected_advice(userid, dbid, queryId, now,
> >                                   query_string, advice_string, area,
> >                                   &ca_pointer);
> >        pgpa_store_shared_advice(ca_pointer);
>
> It doesn't know that area is guaranteed to be non-NULL, so it can't
> prove that ca_pointer is initialized.

I don't know what to do about that. I can understand why it might be
unable to prove that, but I don't see an obvious way to change the
code that would make life easier. I could add Assert(area !=3D NULL)
before the call to pgpa_make_collected_advice() if that helps.

> (GCC also complains about unique_nonjoin_rtekind() not initializing
> the rtekind, but I think that's because of a bug [1].)

This one could be fixed with a dummy initialization, if needed.

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