Обсуждение: GSets: Fix bug involving GROUPING and HAVING together

>>>>> "Jeevan" == Jeevan Chalke <jeevan.chalke@enterprisedb.com> writes:
Jeevan> Basically, when we have only one element in GROUING SETS, weJeevan> are assuming it as a simple GROUP BY with
onecolumn. Due toJeevan> which we are ending up with this error.
 
Jeevan> If we have ROLLUP/CUBE or GROUPING SETS with multiple elements,Jeevan> then we are not getting this error.

There's two issues here. One you correctly identified, which is that
contain_agg_clause() should be true for GroupingFuncs too.

The other is that in subquery_planner, the optimization of converting
HAVING clauses to WHERE clauses is suppressed if parse->groupingSets
isn't empty. (It is empty if there's either no group by clause at all,
or if there's exactly one grouping set, which must not be empty, however
derived.) This is costing us some optimizations, especially in the case
of an explicit GROUP BY () clause; I'll look into this.

In the meantime your patch looks OK (and necessary) to me.
Jeevan> The side effect is that, if we have plain group by clause, thenJeevan> too we can use GROUPING in HAVING
clause.But I guess it isJeevan> fine.
 

GROUPING is, per spec, explicitly allowed anywhere you could have an
aggregate call, whether the group by clause is plain or not.

-- 
Andrew (irc:RhodiumToad)

>>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes:

 Andrew> The other is that in subquery_planner, the optimization of
 Andrew> converting HAVING clauses to WHERE clauses is suppressed if
 Andrew> parse->groupingSets isn't empty. (It is empty if there's either
 Andrew> no group by clause at all, or if there's exactly one grouping
 Andrew> set, which must not be empty, however derived.) This is costing
 Andrew> us some optimizations, especially in the case of an explicit
 Andrew> GROUP BY () clause; I'll look into this.

I'm inclined to go with the simplest approach here, which copies the
quals if there are grouping sets. The only way we could safely move a
qual without copying is if we can show that none of the grouping sets is
empty, that is (), and at this point the grouping set list has not been
expanded so it's not trivial to determine that.

Patch attached.

--
Andrew (irc:RhodiumToad)

>>>>> "Jeevan" == Jeevan Chalke <jeevan.chalke@enterprisedb.com> writes:
Jeevan> Hi,Jeevan> This will fail too.

This is in addition to your patch, not instead of it.

-- 
Andrew (irc:RhodiumToad)

On 2015-07-14 14:51:09 +0530, Jeevan Chalke wrote:
> Fix this by adding GroupingFunc node in this walker.  We do it correctly in
> contain_aggs_of_level_walker() in which we have handling for GroupingFunc
> there.
> 
> Attached patch to fix this.

Pushed, thanks for fix!

On 2015-07-24 11:34:22 +0100, Andrew Gierth wrote:
> >>>>> "Andrew" == Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> 
>  Andrew> The other is that in subquery_planner, the optimization of
>  Andrew> converting HAVING clauses to WHERE clauses is suppressed if
>  Andrew> parse->groupingSets isn't empty. (It is empty if there's either
>  Andrew> no group by clause at all, or if there's exactly one grouping
>  Andrew> set, which must not be empty, however derived.) This is costing
>  Andrew> us some optimizations, especially in the case of an explicit
>  Andrew> GROUP BY () clause; I'll look into this.
> 
> I'm inclined to go with the simplest approach here, which copies the
> quals if there are grouping sets. The only way we could safely move a
> qual without copying is if we can show that none of the grouping sets is
> empty, that is (), and at this point the grouping set list has not been
> expanded so it's not trivial to determine that.

I pushed this to both master and 9.5. While this isn't strictly a
bugfix, it seemed better to keep the branches the same at this point.