Horton
Menu

Re: pg_plan_advice

Robert Haas <robertmhaas@gmail.com>

View thread
From: Robert Haas <robertmhaas@gmail.com>
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>
Date: 2026-01-19T19:59:57Z
Lists: pgsql-hackers 
On Mon, Jan 19, 2026 at 5:53 AM 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 = 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

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. pg_plan_advice: Fix another unique-semijoin bug.

  2. pg_plan_advice: Export feedback-related definitions.

  3. pg_plan_advice: Fix a bug when a subquery is pruned away entirely.

  4. pg_plan_advice: Add alternatives test to Makefile.

  5. pg_plan_advice: Handle non-repeatable TABLESAMPLE scans.

  6. pg_stash_advice: Allow stashed advice to be persisted to disk.

  7. Add pg_stash_advice contrib module.

  8. pg_plan_advice: Avoid assertion failure with partitionwise aggregate.

  9. pg_plan_advice: Invent DO_NOT_SCAN(relation_identifier).

  10. Add an alternative_plan_name field to PlannerInfo.

  11. pg_plan_advice: Refactor to invent pgpa_planner_info

  12. Respect disabled_nodes in fix_alternative_subplan.

  13. get_memoize_path: Don't exit quickly when PGS_NESTLOOP_PLAIN is unset.

  14. test_plan_advice: Set TAP test priority 50 in meson.build.

  15. pg_plan_advice: Avoid a crash under GEQO.

  16. Test pg_plan_advice using a new test_plan_advice module.

  17. pg_plan_advice: Always install pg_plan_advice.h, and in the right place

  18. pg_plan_advice: Fix failures to accept identifier keywords.

  19. Add pg_plan_advice contrib module.

  20. Allow extensions to mark an individual index as disabled.

  21. Replace get_relation_info_hook with build_simple_rel_hook.

  22. Store information about Append node consolidation in the final plan.

  23. Store information about elided nodes in the final plan.

  24. Store information about range-table flattening in the final plan.

  25. Pass cursorOptions to planner_setup_hook.

  26. Fix PGS_CONSIDER_NONPARTIAL interaction with Materialize nodes.

  27. Fix mistakes in commit 4020b370f214315b8c10430301898ac21658143f

  28. Allow for plugin control over path generation strategies.

  29. Update some comments for fasthash

  30. Allow passing a pointer to GetNamedDSMSegment()'s init callback.

  31. Don't reset the pathlist of partitioned joinrels.

  32. Treat number of disabled nodes in a path as a separate cost metric.