Re: Allowing join removals for more join types

On Mon, Jul 7, 2014 at 4:15 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> David Rowley <dgrowley(at)gmail(dot)com> writes:
> > On 6 July 2014 03:20, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> Just to note that I've started looking at this, and I've detected a
> rather
> >> significant omission: there's no check that the join operator has
> anything
> >> to do with the subquery's grouping operator.
>
> > hmm, good point. If I understand this correctly we can just ensure that
> the
> > same operator is used for both the grouping and the join condition.
>
> Well, that's sort of the zero-order solution, but it doesn't work if the
> join operators are cross-type.
>
> 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 notice that create_unique_path is not paying attention to the question

> of whether the subselect's tlist contains SRFs or volatile functions.
> It's possible that that's a pre-existing bug.
>
>
*shrug*, perhaps the logic for that is best moved into
query_is_distinct_for then? It might save a bug in the future too that way.

Regards

David Rowley