Re: pg_plan_advice

On Mon, Dec 15, 2025 at 12:06 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Here's v7.

I'm excited about this patch series, and in an effort to help land the
infrastructure, here is a review of 0001 - 0003 to start:

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.".

From my medium-level understanding of the planner, I don't think the lack
of tracking unplanned subqueries
in subrtinfos is a problem, at least for the pg_plan_advice type use cases.

---

For 0002:

It might be helpful to clarify in a comment that ElidedNode's plan_node_id
represents the surviving node, not that of the elided node.

I also noticed that this currently doesn't support cases where multiple
nodes are elided, e.g. with multi-level table partitioning:

CREATE TABLE pt (l1 date, l2 text) PARTITION BY RANGE (l1);
CREATE TABLE pt_202512 PARTITION OF pt FOR VALUES FROM ('2025-12-01') TO
('2026-01-01') PARTITION BY LIST (l2);
CREATE TABLE pt_202512_TEST PARTITION OF pt_202512 FOR VALUES IN ('TEST');

EXPLAIN (RANGE_TABLE) SELECT * FROM pt WHERE l1 = '2025-12-15' AND l2 =
'TEST';

QUERY PLAN
-------------------------------------------------------------------
Seq Scan on pt_202512_test pt (cost=0.00..29.05 rows=1 width=36)
Filter: ((l1 = '2025-12-15'::date) AND (l2 = 'TEST'::text))
Scan RTI: 3
Elided Node Type: Append
Elided Node RTIs: 1 <=== This is missing RTI 2
RTI 1 (relation, inherited, in-from-clause):
Relation: pt
RTI 2 (relation, inherited, in-from-clause):
Relation: pt_202512
RTI 3 (relation, in-from-clause):
Relation: pt_202512_test
Unprunable RTIs: 1 2 3

In a quick test, adding child_append_relid_sets (from 0003) to the relids
being passed to record_elided_node fixes
that. Presumably the case of partitionwise join relids doesn't matter,
because that would prevent it being elided.

---

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.

---

Note that 0005 needs a rebase,
since 48d4a1423d2e92d10077365532d92e059ba2eb2e changed the
GetNamedDSMSegment API.

You may also want to move the CF entry to the PG19-4 commitfest so CFbot
runs again.

Thanks,
Lukas

--
Lukas Fittl