Re: Allowing join removals for more join types - Mailing list pgsql-hackers

David Rowley <dgrowleyml@gmail.com> writes:
> On Tue, Jul 8, 2014 at 4:28 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'm a bit skeptical as to whether testing for that case is actually worth
>> any extra complexity.  Do you have a compelling use-case?  But anyway,
>> if we do want to allow it, why does it take any more than adding a check
>> for Consts to the loops in query_is_distinct_for?  It's the targetlist
>> entries where we'd want to allow Consts, not the join conditions.

> I don't really have a compelling use-case, but you're right, it's just a
> Const check in query_is_distinct_for(), it seems simple enough so I've
> included that in my refactor of the patch to use query_is_distinct_for().
> This allows the regression tests all to pass again.

Meh.  "I wrote a regression test that expects it" is a pretty poor
rationale for adding logic.  If you can't point to a real-world case
where this is important, I'm inclined to take it out.

If we were actually serious about exploiting such cases, looking for
bare Consts would be a poor implementation anyhow, not least because
const-folding has not yet been applied to the sub-select.  I think we'd
want to do it for any pseudoconstant expression (no Vars, no volatile
functions); which is a substantially more expensive test.

> 1. The fast path code that exited in join_is_removable() for subquery's
> when the subquery had no group or distinct clause is now gone. I wasn't too
> sure that I wanted to assume too much about what query_is_distinct_for may
> do in the future and I thought if I included some logic in
> join_is_removable() to fast path, that one day it may fast path wrongly.

Or put a quick-check subroutine next to query_is_distinct_for().  The code
we're skipping here is not so cheap that I want to blow off skipping it.
On review it looks like analyzejoins.c would possibly benefit from an
earlier fast-path check as well.

> 2. The patch I submitted here
> http://www.postgresql.org/message-id/CAApHDvrfVkH0P3FAooGcckBy7feCJ9QFanKLkX7MWsBcxY2Vcg@mail.gmail.com
> if that gets accepted then it makes the check for set returning functions
> in join_is_removable void.

Right (and done, if you didn't notice already).

TBH I find the checks for FOR UPDATE and volatile functions to be
questionable as well.  We have never considered those things to prevent
pushdown of quals into a subquery (cf subquery_is_pushdown_safe).  I think
what we're talking about here is pretty much equivalent to pushing an
always-false qual into the subquery; if people haven't complained about
that, why should they complain about this?  Or to put it in slightly more
principled terms, we've attempted to prevent subquery optimization from
causing volatile expressions to be evaluated *more* times than the naive
reading of the query would suggest, but we have generally not felt that
we needed to prevent them from happening *fewer* times.
        regards, tom lane