Re: remove_useless_joins vs. bug #19560

On Thu, Sep 17, 2026 at 12:15 AM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
>
> On Thu, Sep 17, 2026 at 11:52 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > The main consumer of varnosyn is ruleutils.c, so I don't like
> > the fact that this test case isn't showing what ruleutils will
> > do with the Var. If we make it be "explain verbose", do we
> > get a display of the problematic Var?
>
> The Var is displayed, but not via varnosyn. ruleutils.c uses varnosyn
> only when dpns->plan is NULL, and EXPLAIN always sets the plan before
> deparsing an expression, so it prints the semantic referent (a.b_id).
>
> But I think it'd still be better to use "explain verbose", so at least
> what ruleutils prints is recorded. I'll make that change before push.
>
> - Richard
>
>
Hi Richard,

I found what looks like a regression from e28cf96e935
("Fix stale copies of PHVs in subqueries"). In particular, I think there is
still a way for an upper-level PHV containing an unprocessed SubLink to get
into a subquery after preprocess_subquery_phvs() has already run.

Here is a reproducer on current master:
create table t (a int, b int);
create table u (c int, d int);

select 1
from (t left join (select (select 1) as x) s on true) j
where (select 1 where j is null) is null;

On current master I get:
ERROR: unrecognized node type: 22

There are similar failures with RIGHT/FULL JOIN. For example, a nested join
shape can instead reach:
ERROR: cannot handle unplanned sub-select

I bisected this to e28cf96e935. The cases I tested work on e28cf96e935^ and
fail on e28cf96e935/current master. I also reproduced the regression on
REL_16_STABLE and REL_18_STABLE; source inspection shows the same relevant
code in the other branches to which this was back-patched.

The problem seems to be an ordering hole in the new PHV preprocessing scheme.

preprocess_subquery_phvs() runs early in subquery_planner(), on the assumption
that copies of upper-level PHVs in lower-level queries will be preprocessed at
their owning level before the lower query sees them. Later, however,
flatten_join_alias_vars() can expand a whole-row join alias inside a lower
Query and introduce a fresh upper-level PHV there.

One way this happens is when a joinaliasvars entry is already a PHV because a
pulled-up subquery was on the nullable side of an outer join. Another is when
add_nullingrels_if_needed() has to wrap the RowExpr produced by whole-row
join-alias expansion in a new PHV.

At that point preprocess_subquery_phvs() has already run, so the newly
introduced PHV can still contain a raw SubLink.

Later process_sublinks_mutator() deliberately leaves an upper-level PHV alone,
consistent with the new assumption that its contents were already processed at
its owning level. build_subplan() makes the same assumption after
e28cf96e935.
The result is that the raw SubLink survives inside the PHV.

With some instrumentation, for a failing case I see the SubPlan argument reach
build_subplan() as a PHV whose phexpr is still T_SubLink, whereas in a
corresponding working case the expression has already become a Param.

I also tried the obvious experiment of restoring the old
IsA(arg, PlaceHolderVar)

case in build_subplan() so that SS_process_sublinks() is run there. That fixes
these reproducers, but it is not a valid fix: it trips
Assert(!IsA(node, SubLink))

in process_sublinks_mutator() on the join.sql tests added by e28cf96e935,
resulting in many regression failures. So I don't think simply
restoring the old
build_subplan behavior is the answer.

This seems to violate the invariant established by e28cf96e935: an
upper-level PHV
encountered by a lower query may be left untouched only if all such copies have
already been preprocessed at their owning level. flatten_join_alias_vars() can
create or push down another such copy after that preprocessing pass.

I think the fix therefore needs to be on the producing/preprocessing side rather
than in build_subplan() -- either ensuring these alias-generated PHVs
are processed
at their owning level before they can be copied down, or arranging another
appropriately scoped PHV preprocessing step after join-alias flattening.

I have not tried to turn either of those into a patch yet, since I
wanted to check
whether you agree with this diagnosis first.

Regards,
Haibo