From: | David Rowley <dgrowleyml(at)gmail(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | David Rowley <dgrowley(at)gmail(dot)com>, Simon Riggs <simon(at)2ndquadrant(dot)com>, Noah Misch <noah(at)leadboat(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Dilip kumar <dilip(dot)kumar(at)huawei(dot)com> |
Subject: | Re: Allowing join removals for more join types |
Date: | 2014-07-08 08:45:15 |
Message-ID: | CAApHDvq+ebv=uaXZYebuPoVM+St7JxT7LKqdf86RVEkDORtJoA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Jul 8, 2014 at 4:28 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> David Rowley <dgrowleyml(at)gmail(dot)com> writes:
> > On Mon, Jul 7, 2014 at 4:15 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> I poked around to see if we didn't have some code already for that, and
> >> soon found that not only do we have such code
> (equality_ops_are_compatible)
> >> but actually almost this entire patch duplicates logic that already
> exists
> >> in optimizer/util/pathnode.c, to wit create_unique_path's subroutines
> >> query_is_distinct_for et al. So I'm thinking what this needs to turn
> into
> >> is an exercise in refactoring to allow that logic to be used for both
> >> purposes.
>
> > Well, it seems that might just reduce the patch size a little!
> > I currently have this half hacked up to use query_is_distinct_for, but I
> > see there's no code that allows Const's to exist in the join condition. I
> > had allowed for this in groupinglist_is_unique_on_restrictinfo() and I
> > tested it worked in a regression test (which now fails). I think to fix
> > this, all it would take would be to modify query_is_distinct_for to take
> a
> > list of Node's rather than a list of column numbers then just add some
> > logic that skips if it's a Const and checks it as it does now if it's a
> Var
> > Would you see a change of this kind a valid refactor for this patch?
>
> 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.
I've included an updated patch and a delta patch.
Now a couple of things to note:
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.
Perhaps we could protect against this with a small note in
query_is_distinct_for().
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.
Regards
David Rowley
Attachment | Content-Type | Size |
---|---|---|
subquery_leftjoin_removal_v1.5.delta.patch | application/octet-stream | 11.0 KB |
subquery_leftjoin_removal_v1.5.patch | application/octet-stream | 18.4 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Ali Akbar | 2014-07-08 10:03:19 | Re: [REVIEW] Re: Fix xpath() to return namespace definitions |
Previous Message | David Rowley | 2014-07-08 08:27:08 | query_is_distinct_for does not take into account set returning functions |