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.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
