Re: remove_useless_joins vs. bug #19560

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.

For example:

create table t (id int primary key);

select (select ss1.c1 from int4_tbl i43 offset 0)
from (select case when false then remov.id end as c1
from int4_tbl i41 left join t remov on i41.f1 = remov.id) ss1
right join int4_tbl i42 on true;

The subselect references ss1.c1, whose value is a PHV wrapping "case
when false then remov.id end". At the outer level that CaseExpr lives
only inside the subselect, and eval_const_expressions does not descend
into a sub-Query, so the outer level never folds it. It gets folded
only when the subselect is planned and its own eval_const_expressions
recurses into the upper-level PHV.

If eval_const_expressions skips outer-level PHVs, that fold no longer
happens, and the raw CaseExpr ends up in the SubPlan's args. For this
query the consequence is that remov.id stays referenced, and the
i41/remov join fails to be removed.

> If we do that over again, the best-case result is that we waste
> cycles. The worst-case result is that we end up with an expression
> that looks different from what it looks like in the outer level
> and then that causes problems. (I don't have any specific ideas
> in mind about how the doubly-processed expression could come to be
> different. But if we accept your point that "it's unsafe to do
> preprocessing twice", it seems like this is fertile ground for
> trouble.)

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

I'm not too worried about the wasted cycles. ISTM a walk over an
already-simplified expression is cheap. Your worst-case concern is
fair, but I don't think a second eval_const_expressions pass can
produce a different-looking expression in a way that would cause
problems, otherwise we should already see related bug reports from the
places that already run it twice.

> 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 already explained why
> I think eval_const_expressions should too, and I wonder if there
> are not other places that should as well. Fundamentally it seems
> like the policy across the board ought to be that subqueries keep
> their hands off upper-level PHVs. Now, once we've applied
> SS_replace_correlation_vars, the question is moot because all such
> PHVs will have been replaced by Params, but anything that runs
> before that needs a close look.

I went through the steps that run before SS_replace_correlation_vars.
They are the earlier steps of preprocess_expression:

1. flatten_join_alias_vars: descends and level-dependent, so it must
skip outer-level PHVs.

2. eval_const_expressions: level-independent, so it's safe on an
outer-level PHV, and as shown above it needs to run on them.

3. canonicalize_qual: only restructures AND/OR/NOT; a PHV is an
opaque leaf to it, so it never descends into the phexpr.

4. convert_saop_to_hashed_saop: descends, but only sets hashfuncid on
ScalarArrayOpExprs. That's level-independent and idempotent.

5. SS_process_sublinks: already skips outer-level PHVs.

So flatten_join_alias_vars isn't alone. SS_process_sublinks already
skips.

- Richard