Re: pg_plan_advice

On Tue, Jan 6, 2026 at 11:36 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Mon, Dec 29, 2025 at 8:15 PM Lukas Fittl <lukas(at)fittl(dot)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