Re: remove_useless_joins vs. bug #19560

Richard Guo <guofenglinux(at)gmail(dot)com> writes:
> On Thu, Sep 3, 2026 at 11:55 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> It still feels to me that that's the right thing to do. With your
>> v2 patch, subquery_planner will have taken care of preprocessing
>> any outer-level PHVs before the subquery's own preprocessing starts.

> Hmm, not for all of them. My v2 patch only preprocesses the
> outer-level PHVs that were pushed into LATERAL subquery RTEs, since
> that is where find_lateral_references pulls them from. It does not
> touch PHVs that were pushed into a SubLink's subselect, and for those
> the subquery's own preprocessing is still the only thing that folds
> them.

Ugh. That sounds like a recipe for bugs; I don't think we want
different PHVs in the same query level to be folded at different
times if we can help it.

> I think I didn't state my earlier point clearly, sorry about that.
> What I meant is that preprocess_expression as a whole is not safe to
> run twice, because its SS_process_sublinks step would hit
> Assert(!IsA(node, SubPlan)) on the SubPlans that the first run
> created.

Right. We could of course drop that Assert in favor of ignoring
SubPlans there. But the interaction between SS_process_sublinks and
SS_replace_correlation_vars is delicate enough that I doubt we want to
risk running that sequence twice.

> eval_const_expressions by itself does seem safe to run
> twice, and I'm pretty certain that there are existing code paths where
> that can happen, for example preprocess_expression deliberately
> re-runs it for EXPRKIND_RTFUNC_LATERAL.

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

> I'm not too worried about the wasted cycles. ISTM a walk over an
> already-simplified expression is cheap.

It's not *that* cheap; we will for example be repeating operator
volatility lookups, not to mention copying the tree. But I agree
that this concern is much weaker than correctness.

>> I think that your v2 patch is fundamentally the right way forward,
>> but I'm worried that it's not complete yet. In particular it
>> seems very strange that only flatten_join_alias_vars_mutator needs
>> to worry about skipping outer-level PHVs.

> I went through the steps that run before SS_replace_correlation_vars.

Thanks for doing that analysis. I remain concerned though about
inconsistent processing of PHV contents. I think the sketch I'd
rather see is that we simply ignore the contents of any uplevel
PHV during subquery processing, relying on the singly-preprocessed
version sitting in the owning query level's PlaceHolderInfo when
it comes time to do something interesting with it.

regards, tom lane