Re: Removing unneeded self joins

On 31/10/2020 11:26, Andrey V. Lepikhov wrote:
> + /*
> + * Process restrictlist to seperate out the self join quals from
> + * the other quals. e.g x = x goes to selfjoinquals and a = b to
> + * otherjoinquals.
> + */
> + split_selfjoin_quals(root, restrictlist, &selfjoinquals,
> + &otherjoinquals);
> +
> + if (list_length(selfjoinquals) == 0)
> + {
> + /*
> + * Have a chance to remove join if target list contains vars from
> + * the only one relation.
> + */
> + if (list_length(otherjoinquals) == 0)
> + {
> + /* Can't determine uniqueness without any quals. */
> + continue;
> +
> + }
> + else if (!tlist_contains_rel_exprs(root, joinrelids, inner))
> + {
> + if (!innerrel_is_unique(root, joinrelids, outer->relids,
> + inner, JOIN_INNER, otherjoinquals,
> + false))
> + continue;
> + }
> + else
> + /*
> + * The target list contains vars from both inner and outer
> + * relations.
> + */
> + continue;
> + }
> +
> + /*
> + * Determine if the inner table can duplicate outer rows. We must
> + * bypass the unique rel cache here since we're possibly using a
> + * subset of join quals. We can use 'force_cache' = true when all
> + * join quals are selfjoin quals. Otherwise we could end up
> + * putting false negatives in the cache.
> + */
> + else if (!innerrel_is_unique(root, joinrelids, outer->relids,
> + inner, JOIN_INNER, selfjoinquals,
> + list_length(otherjoinquals) == 0))
> + continue;

I don't understand the logic here. If 'selfjoinquals' is empty, it means
that there is no join qual between the two relations, right? How can we
ever remove the join in that case? And how does the target list affect
that? Can you give an example query of that?

> --- a/src/test/regress/expected/join.out
> +++ b/src/test/regress/expected/join.out
> @@ -4553,11 +4553,13 @@ explain (costs off)
> select p.* from
> (parent p left join child c on (p.k = c.k)) join parent x on p.k = x.k
> where p.k = 1 and p.k = 2;
> - QUERY PLAN
> ---------------------------
> + QUERY PLAN
> +------------------------------------------------
> Result
> One-Time Filter: false
> -(2 rows)
> + -> Index Scan using parent_pkey on parent x
> + Index Cond: (k = 1)
> +(4 rows)
>
> -- bug 5255: this is not optimizable by join removal
> begin;

That doesn't seem like an improvement...

My general impression of this patch is that it's a lot of code to deal
with a rare special case. At the beginning of this thread there was
discussion on the overhead that this might add to planning queries that
don't benefit, but adding a lot of code isn't nice either, even if the
performance is acceptable. That's not necessarily a show-stopper, but it
does make me much less excited about this. I'm not sure what to suggest
to do about that, except a really vague "Can you make is simpler?"

- Heikki