Re: remove_useless_joins vs. bug #19560

Richard Guo <guofenglinux(at)gmail(dot)com> writes:
> Thanks for the patchset. Nice cleanup:

Thanks for looking at it!

> I noticed that the call to remove_rels_from_query_tree() at the end of
> remove_useless_joins() performs a full traversal over the query tree
> to delete the removed relids. I wonder if this is a bit too
> brute-force, as AFAICS only PHVs can still contain references to the
> removed relids. If there were any other remaining references,
> join_is_removable() wouldn't have considered the join removable in the
> first place.

Yeah, I think this is probably true. The Claude-written comment
claims that the removed OJ's relid could appear in surviving Vars'
nullingrels, but I don't see how that could be: such Vars would
have to refer to the removed baserel, and then they should have
prevented the join removal.

> If that's correct, perhaps we can simplify
> remove_rels_from_query_tree() to something lighter, similar to
> remove_result_refs().

Meh. I don't think there's a lot to be gained there: we still
have to walk the whole tree, and I'm not excited about duplicating
tree-walking code. We could maybe use substitute_phv_relids from
prepjointree.c, but I see that has Assert(!IsA(node, AppendRelInfo));
which we'd have to drop. I doubt there'd be any measurable
improvement over the ChangeVarNodes coding, anyway.

> Also, the issue with duplicate clauses after self-join elimination
> reported in bug #19435 is still present in v4. It would be great if
> we could address that in passing as well.

I'm not excited about that either. AFAICS, with the #19435 query:

SELECT 1 AS c1
FROM (
pg_table_a AS tom0
RIGHT JOIN (
(pg_table_a AS tom1 NATURAL JOIN pg_table_a AS tom2)
RIGHT JOIN pg_table_a AS tom3
ON tom1.col_bool IS NOT NULL
)
ON tom1.col_bool
);

The NATURAL JOIN produces quals

tom1.id = tom2.id AND tom1.col_bool = tom2.col_bool

SJE correctly replaces those with

tom2.id IS NOT NULL AND tom2.col_bool IS NOT NULL

We do, later on, detect that the id clause is unnecessary because of
the id column's NOT NULL constraint. But we don't notice that the
generated col_bool clause is redundant with the user-written
"tom1.col_bool IS NOT NULL" clause appearing in a higher join level.
I don't think it's incumbent on us to fix redundantly-written
user queries, and even if we took that on, the SJE code would not
be the place for it. You can get similar redundancies without
any SJE happening, eg

regression=# explain (costs off)
select * from pg_table_a as tom1 left join
(select * from pg_table_a as tom2 where tom2.col_bool is not null) tom3
on tom3.col_bool is not null;
QUERY PLAN
---------------------------------------------------------------------------
Nested Loop Left Join
-> Seq Scan on pg_table_a tom1
-> Materialize
-> Seq Scan on pg_table_a tom2
Filter: ((col_bool IS NOT NULL) AND (col_bool IS NOT NULL))
(5 rows)

> Here is a simplified repro:
> create table t (a int unique, b int);
> explain (costs off)
> select * from t t1 left join
> (t t2 join t t3 on t2.a = t3.a) on t3.a is not null;

On what grounds are these not just stupidly-written queries?
We shouldn't fail of course (which was the actual complaint in
#19435), but I'm not excited about removing the user's redundancy.

If somebody held a gun to my head and said "fix that", I'd probably
teach distribute_quals_to_rels to de-duplicate quals as they go into
baserestrictinfo and joinrestrictinfo lists. But I'm quite certain
that the planner cycles spent on that would be a net loss for most
people.

regards, tom lane