Re: remove_useless_joins vs. bug #19560

On Wed, Sep 9, 2026 at 11:54 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> The hole in this logic is the assumption that eval_const_expressions
> will act the same with Vars of upper query levels as with Vars of the
> current query level. That's visibly not so at present --- see for
> example const-simplification of NullTest nodes on known-not-null Vars.
> var_is_nonnullable just throws up its hands if var->varlevelsup != 0.
> (Arguably that's a bug, or at least an oversight: couldn't it crawl
> up the PlannerInfo tree to the appropriate level? But my point is
> that we shouldn't assume that this will always work perfectly.)

Ah, I see your point. Right, that makes eval_const_expressions not
that level-independent. That means my argument for letting the
subquery fold the pushed-down copies of PHVs doesn't hold.

Now I think your suggestion is the right way to go. We should
preprocess each of the upper-level PHVs once at the level it belongs
to, and then the subquery can leave them alone.

I think a straightforward way to achieve that is that in
subquery_planner, we walk the whole query tree and preprocess all of
the current level's PHVs that got pushed down into subqueries, up
front before anything consumes them. Then the subquery's own
processing can just ignore upper-level PHVs. eval_const_expressions
returns them as-is, and flatten_join_alias_vars stops descending into
them.

This still keeps the copies around rather than relying solely on the
PlaceHolderInfo, but since they'd be folded the same way as the
owning-level version, maybe it still gets the consistency we want to
have.

I tried coding with this idea and ended up with the attached v3.

One problem with this is that we walk the whole query tree to find
those copies, which costs some planning cycles. We could gate it on
root->glob->lastPHId != 0 to skip queries with no PHVs at all, but
beyond that I don't have a good way to narrow it down.

Another thing to note. The copies' SubLinks would turn into SubPlans
in this early pass, so a few SubPlan/InitPlan numbers shift in the
regression output. Just renumbering.

- Richard