On Tue, Jul 8, 2014 at 4:28 AM, Tom Lane <t...@sss.pgh.pa.us> wrote: > David Rowley <dgrowle...@gmail.com> writes: > > On Mon, Jul 7, 2014 at 4:15 AM, Tom Lane <t...@sss.pgh.pa.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/caaphdvrfvkh0p3faoogcckby7fecj9qfanklkx7mwsbcxy2...@mail.gmail.com if that gets accepted then it makes the check for set returning functions in join_is_removable void. Regards David Rowley
subquery_leftjoin_removal_v1.5.delta.patch
Description: Binary data
subquery_leftjoin_removal_v1.5.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
