On Thu, Nov 30, 2017 at 12:20 PM, David Rowley
<david.rowley@2ndquadrant.com> wrote:
> Thanks for looking over this and my apologies for the delay in my
> response. I've been on leave and out of range of the internet for most
> of that time.
>
> On 23 November 2017 at 02:30, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>>
>> @@ -597,15 +615,25 @@ rel_supports_distinctness(PlannerInfo *root,
>> RelOptInfo *rel)
>> + if (root->parse->distinctClause != NIL)
>> + return true;
>> +
>> + if (root->parse->groupClause != NIL && !root->parse->hasAggs)
>> + return true;
>> +
>>
>> The other callers of rel_supports_distinctness() are looking for distinctness
>> of the given relation, whereas the code change here are applicable to any
>> relation, not just the given relation. I find that confusing. Looking at the
>> way we are calling rel_supports_distinctness() in join_is_removable() this
>> change looks inevitable, but still if we could reduce the confusion, that will
>> be good. Also if we could avoid duplication of comment about unique index, that
>> will be good.
>
> When I put this together I'd considered leaving
> rel_supports_distinctness() alone since the other uses of the function
> can't make use of the new checks. Since you mention this, then I think
> it's likely better just to go with that and just special case the code
> in join_is_removable() to check for rel_supports_distinctness() and
> the DISTINCT/GROUP BY clauses too. This will mean less false positives
> for usages such as in innerrel_is_unique() which would end up causing
> a bit of extra work before possibly failing a bit later on. I've made
> changes in the attached to do things this way.
>
>> May be you want to add a testcase with DISTINCT ON.
>
> Good idea. I've also added a test case for this, and also one to
> ensure non-RTE_RELATION relations can be removed too.
>
> Updated patch attached.
>
> Thanks again for the review.
Thanks for considering those suggestions. The patch looks good to me.
It applies cleanly, compiles on my laptop without warnings or errors.
make check does not show any failures. Marking it as ready for
committer.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
