Re: pg_plan_advice

On Mon, Jan 19, 2026 at 5:53 AM Jakub Wartak
<jakub(dot)wartak(at)enterprisedb(dot)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 = true, it's an unplanned or dummy subquery. */
rtinfo->dummy = 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 == 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