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>
 <CA+TgmobwaT=PXPDDrgDup+jA8KHBbkxigtziD-zNzAKKkQYVgQ@mail.gmail.com>
 <CAOYmi+mOmEW=amDRQMfw6-Fb3ZmDEQFaJzwk8Bc8W8DzaP85XQ@mail.gmail.com>
 <CA+TgmoaX2AMW4cdFM3OngBJxmxpkdmzF33R7-CWhvRLfucbFMg@mail.gmail.com>
 <CAOYmi+njnRGcomnxTY6vsEW3wWigc0ruB0EyWFpb+PVVE8sWpw@mail.gmail.com>
 <CAOYmi+k4AyWCQHK=XVF99KVDuFkqxcADao61OWGLxu0nRYMONQ@mail.gmail.com>
 <CA+TgmoZ0x3ym_oueXRWzbM_=6ucKoPZVGj3rRMLBDC_FnetXDw@mail.gmail.com>
 <CAP53Pkycc=7N2bLzVT3x+qE1JamvRZWev5tFjdLJ1+-AV3Di+Q@mail.gmail.com>
 <CA+TgmoaKhuD91RnazbRyGkmP7--JdNq8oNDC3UcgTZSWbMxC7w@mail.gmail.com>
 <CAP53Pkw5-wMEeDJXFmqo_RTyL_spzCXb7HHKrbSnQqokVoZcNQ@mail.gmail.com>
 <CA+Tgmob-69bzbdi3U_QtebqAf6u1y8js=5=oNK639csVe1VbhA@mail.gmail.com>
 <CAKZiRmz4zrZfx_wgzkTD88HzKiA22HKPUnubR3is0bjKra6Utg@mail.gmail.com>
 <CA+Tgmobp8gcB9DOxNUL10xAXXq=ya6oHDUJqtNLf_3ojxm5-fQ@mail.gmail.com>
 <CAKZiRmznM4cc_N5qQBgEdDj9F5J8=zTJbWm9erZzhp9jP7LMbg@mail.gmail.com>
 <CA+Tgmoa5HjTnBMNQuW5QrCUacC9gdok+02a2Y9TTGWtTQ6OBvQ@mail.gmail.com> <CAKZiRmzpO=PpLmXEQUO5CETzT0mmLhT5P656hetseKQUgZ6BQg@mail.gmail.com>
In-Reply-To: <CAKZiRmzpO=PpLmXEQUO5CETzT0mmLhT5P656hetseKQUgZ6BQg@mail.gmail.com>
From: Robert Haas <robertmhaas@gmail.com>
Date: Mon, 19 Jan 2026 14:59:57 -0500
Message-ID: <CA+TgmobRAjY+xs_=fKMJ4NgW_i4bMoD5kW7oKrGACLLcY04ysA@mail.gmail.com>
Subject: Re: pg_plan_advice
To: Jakub Wartak <jakub.wartak@enterprisedb.com>
Cc: Lukas Fittl <lukas@fittl.com>, Jacob Champion <jacob.champion@enterprisedb.com>, 
	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%2BTgmobRAjY%2Bxs_%3DfKMJ4NgW_i4bMoD5kW7oKrGACLLcY04ysA%40mail.gmail.com>
Precedence: bulk

On Mon, Jan 19, 2026 at 5:53=E2=80=AFAM Jakub Wartak
<jakub.wartak@enterprisedb.com> wrote:
> 2. I couldn't find any glaring issue during code review of v10-000[124]. =
But I
>    have some questions:
>    a) v10-0001 - any example producing such a dummy subplan? (whatever
> I've tried I
>       cannot come up with one)

If you just add something like this, you can see lots of examples from
the regression tests:

         /* When recursing =3D true, it's an unplanned or dummy subquery. *=
/
         rtinfo->dummy =3D recursing;
+        if (rtinfo->dummy)
+            elog(WARNING, "hey look i'm a dummy");

Here's a stripped down example that doesn't require the regression database=
:

EXPLAIN SELECT * FROM random() UNION SELECT * FROM random() WHERE false;

>    b) v10-0001 - maybe we could add a comment nearby "dummy" struct
> member to look
>       on pgpa_plan_walker() on example how to use it, but that's part of =
v5 and
>       contrib...

Maybe. I think people should know how to grep for stuff like that, and
I don't want to introduce forward dependencies.

>    c) In v10-0004, maybe in pathnodes.h we could use typedef enum rather =
than
>       list of #defines? (see attached)

I personally hate that style and I think Andres loves it. Whee!

> 3. Yes, I could too also repro Jacob's and get the same failure, so it's =
real:
>    TRAP: failed Assert("child_target->ttype =3D=3D PGPA_TARGET_IDENTIFIER=
"),
>    File: "../contrib/pg_plan_advice/pgpa_walker.c", Line: 679, PID: 32344

I have responded to that separately.

> 4. Some raw perf numbers on non-assert builds (please ignore +/- 3%
> jumps), it just hurts
>    in one scenario where oq2 drops like 9% of juice (quite expected, it's=
 not
>    an issue to be, just posting full results)
>
> tps                           oq1  oq2    oq3  oq4
> master                        41   14745  439  435
> master+v10-000[1-4]           42   15055  439  432
> master+v10full                41   14734  429  437
> master+v10full+loaded         42   15014  442  438
> master+v10full+loaded+advice  41   13481  424  439
>
> (same but in percentages)
> %tps_to_master                oq1  oq2    oq3  oq4
> master                        100  100    100  100
> master+v10-000[1-4]           102  102    100  99
> master+v10full                100  100    98   100
> master+v10full+loaded         102  102    101  101
> master+v10full+loaded+advice  100  91     97   101

I think these numbers look pretty good. I mean, there is obviously
room for improvement. We should look at where the CPU cycles are going
in the oq2 case and try to optimize. But even without that, it's not
terrible, IMHO.

> So out of curiosity the oq2 on 1 CPU core behavior looks like below:
> - no advices --> ~1000 TPS
> - enabled pg_plan_advice.advice to lengthy, but unrelated thing and it
> gets ~890TPS

I'm not sure exactly where the CPU cycles are going here, but one
known problem is that we have to re-parse the advice string for every
query. This thread discusses the challenges of creating some
infrastructure that would allow us to avoid that:

http://postgr.es/m/f87504a6-9dfd-4467-89de-84232cb54f72@gmail.com

Maybe I should start thinking about other ways to avoid that overhead,
because that thread doesn't seem to be progressing much, but maybe the
reparsing isn't even the main problem.

> - in both cases (empty and set) the bottleneck seems to in palloc0, but
>     empty plan_advice: it's more like palloc0() <- newNode() <-
> create_index_path()
>     <- build_index_paths()
>     with plan_advice set: palloc0() <- newNode() <- create_nestloop_path(=
) ..

I've also seen some palloc-related issues with the patch -- it has to
build some data structures and that does palloc stuff -- but there
shouldn't really be any difference in the code paths you show here.
That's just core code, which should be doing the same thing either way
if the advice is not relevant.

> - so if anything people should not put something there blindly, but just =
SET
>   and RESET afterwards (unless we get pinning of SQL plan id to advices) =
as
>   this might have cost in high-TPS scenarios.

Yes, I think that's definitely a potential issue. I'd like the
overhead of this module to be as low as possible, but it's bound to
have at least some overhead, and people will have to decide whether
it's worth it.

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