Re: Allowing join removals for more join types

On Wed, Jun 25, 2014 at 10:03 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:

> On 23 June 2014 12:06, David Rowley <dgrowley(at)gmail(dot)com> wrote:
>
> >> It's not clear to me where you get the term "sortclause" from. This is
> >> either the groupclause or distinctclause, but in the test cases you
> >> provide this shows this has nothing at all to do with sorting since
> >> there is neither an order by or a sorted aggregate anywhere near those
> >> queries. Can we think of a better name that won't confuse us in the
> >> future?
> >>
> >
> > I probably got the word "sort" from the function targetIsInSortList,
> which
> > expects a list of SortGroupClause. I've renamed the function to
> > sortlist_is_unique_on_restrictinfo() and renamed the sortclause
> parameter to
> > sortlist. Hopefully will reduce confusion about it being an ORDER BY
> clause
> > a bit more. I think sortgroupclauselist might be just a bit too long.
> What
> > do you think?
>
> OK, perhaps I should be clearer. The word "sort" here seems completely
> misplaced and we should be using a more accurately descriptive term.
> It's slightly more than editing to rename things like that, so I'd
> prefer you cam up with a better name.
>
>
hmm, I do see what you mean and understand the concern, but I was a bit
stuck on the fact it is a list of SortGroupClause after all. After a bit of
looking around the source I found a function called grouping_is_sortable
which seems to be getting given ->groupClause and ->distinctClause in a few
places around the source. I've ended up naming the
function groupinglist_is_unique_on_restrictinfo, but I can drop the word
"list" off of that if that's any better? I did have it named
clauselist_is_unique_on_restictinfo for a few minutes, but then I noticed
that this was not very suitable since the calling function uses the
variable name clause_list for the restrictinfo list, which made it even
more confusing.

Attached is a delta patch between version 1.2 and 1.3, and also a
completely updated patch.

> Did you comment on the transitive closure question? Should we add a
> test for that, whether or not it works yet?
>
>
In my previous email.

I could change the the following to use c.id in the targetlist and group by
clause, but I'm not really sure it's testing anything new or different.

EXPLAIN (COSTS OFF)
SELECT a.id FROM a
LEFT JOIN (SELECT b.id,1 as dummy FROM b INNER JOIN c ON b.id = c.id GROUP
BY b.id) b ON a.id = b.id AND b.dummy = 1;

Regards

David Rowley

> Other than that it looks pretty good to commit, so I'll wait a week
> for other objections then commit.
>
> --
> Simon Riggs http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>