Horton
Menu

Re: pg_plan_advice

Lukas Fittl <lukas@fittl.com>

View thread
From: Lukas Fittl <lukas@fittl.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Jacob Champion <jacob.champion@enterprisedb.com>, Dian Fay <di@nmfay.com>, Matheus Alcantara <matheusssilv97@gmail.com>, Jakub Wartak <jakub.wartak@enterprisedb.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2026-01-11T20:20:32Z
Lists: pgsql-hackers

Attachments

On Tue, Jan 6, 2026 at 11:36 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Mon, Dec 29, 2025 at 8:15 PM Lukas Fittl <lukas@fittl.com> wrote:
> > For 0001, I'm not sure the following comment is correct:
> >
> > > /* When recursing = true, it's an unplanned or dummy subquery. */
> > > rtinfo->dummy = recursing;
> >
> > Later in that function we only recurse if its a dummy subquery - in the case of an unplanned subquery (rel->subroot == NULL)
> > add_rtes_to_flat_rtable won't be called again (instead the relation RTEs are directly added to the finalrtable). Maybe we can
> > clarify that comment as "When recursing = true, it's a dummy subquery or its children.".
>
> Presumably, a child of an unplanned or dummy subquery will also be
> unplanned or dummy, so I'm not sure I understand the need to clarify
> here.

I think I was more trying to argue that unplanned subqueries are not
actually being considered here, since the recursing flag will never be
true for an unplanned subquery. The "or its children" part was more to
capture my understanding, and seems fine to omit too.

> > I also noticed that this currently doesn't support cases where multiple nodes are elided, e.g. with multi-level table partitioning:
>
> ...
> I'm not really sure there's a problem here. We definitely do not want
> to end up with something like "Elided Node RTIs: 1 2". What I've found
> experimentally is that it's often important to preserve relid sets,
> but you need to preserve them as sets, not individually. I hesitate a little bit
> to design something without a use case in mind, but maybe you have
> one?

It just seemed inconsistent to me, but I think I follow your argument
as to why just adding it to the set isn't correct. I don't have a
particular use case beyond advice/hint application in mind, so if this
works in your assessment and is not an oversight, that sounds good to
me.

>
> > For 0003:
> >
> > I also find the "cars" variable suffix a bit hard to understand, but not sure a comment next to the variables is that useful.
> > Separately, the noise generated by all the additional "_cars" variables isn't great.
> >
> > I wonder a little bit if we couldn't introduce a better abstraction here, e.g. a struct "AppendPathInput" that contains the
> > two related lists, and gets populated by accumulate_append_subpath/get_singleton_append_subpath and then
> > passed to create_append_path as a single argument.
>
> I spent some time thinking about this day and haven't been quite able
> to come up with something that I like. The problem is that
> pa_partial_subpaths and pa_nonpartial_subpaths share a single
> child_append_relid_sets variable, namely pa_subpath_cars, and
> accumulate_append_subpaths gets called with that as the last argument
> and different things for the previous two. One thing I tried was
> making the AppendPathInput struct contain three lists rather than two,
> but then accumulate_append_subpath() needs an argument that makes it
> work in one of three different modes:
>
> Mode 1: normal -- add everything to the "normal" list
> Mode 2: building parallel-aware append with partial path -- add things
> to the "normal" list except for parallel-aware appends which need to
> be split between the normal and special lists
> Mode 3: building parallel-aware append with non-partial path -- add
> things to the "special" list
>

Yeah, the difference in these modes makes this a bit challenging.

I wonder a bit if we shouldn't instead focus on this being about the
inputs to create_append_path (and the 4 different variants of calling
it in add_paths_to_append_rel), and make sure we group some of them
together in a struct, but still pass the individual fields of that
struct to accumulate_append_subpaths.

I've sketched out what I mean in the attached (once as a patch on top
of v8, and then again as a separate patch that's combined with
v8/0003). That makes add_paths_to_append_rel easier to understand (to
me at least), at a slight increase in complexity in cases where we
call create_append_path without passing child_append_relid_sets or
partial subpaths.

Thanks,
Lukas

-- 
Lukas Fittl

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.