Re: remove_useless_joins vs. bug #19560

[ after a good deal of navel-gazing... ]

Richard Guo <guofenglinux(at)gmail(dot)com> writes:
> I'm also not sure about skipping outer-level PHVs in
> eval_const_expressions. I'm worried that that can cause us to carry
> the raw expressions in the outer query somewhere.

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

regards, tom lane