Re: [HACKERS] GSets: Fix bug involving GROUPING and HAVING together
On 2015-07-24 11:34:22 +0100, Andrew Gierth wrote: Andrew == Andrew Gierth and...@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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSets: Fix bug involving GROUPING and HAVING together
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! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSets: Fix bug involving GROUPING and HAVING together
Andrew == Andrew Gierth and...@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)
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index a6ce96e..2866176 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -574,13 +574,12 @@ subquery_planner(PlannerGlobal *glob, Query *parse,
if (contain_agg_clause(havingclause) ||
contain_volatile_functions(havingclause) ||
- contain_subplans(havingclause) ||
- parse-groupingSets)
+ contain_subplans(havingclause))
{
/* keep it in HAVING */
newHaving = lappend(newHaving, havingclause);
}
- else if (parse-groupClause)
+ else if (parse-groupClause !parse-groupingSets)
{
/* move it to WHERE */
parse-jointree-quals = (Node *)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSets: Fix bug involving GROUPING and HAVING together
Hi, This will fail too. Note that, when we have only one element in GROUPING SETS, we add that in group by list and set parse-groupingSets to NULL. And hence it will have same issue. However tests added in my patch failing too. Thanks -- Jeevan B Chalke Principal Software Engineer, Product Development EnterpriseDB Corporation The Enterprise PostgreSQL Company
Re: [HACKERS] GSets: Fix bug involving GROUPING and HAVING together
Jeevan == Jeevan Chalke jeevan.cha...@enterprisedb.com writes: Jeevan Hi, Jeevan This will fail too. This is in addition to your patch, not instead of it. -- Andrew (irc:RhodiumToad) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSets: Fix bug involving GROUPING and HAVING together
Jeevan == Jeevan Chalke jeevan.cha...@enterprisedb.com writes: Jeevan Basically, when we have only one element in GROUING SETS, we Jeevan are assuming it as a simple GROUP BY with one column. Due to Jeevan 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, then Jeevan too we can use GROUPING in HAVING clause. But I guess it is Jeevan 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) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] GSets: Fix bug involving GROUPING and HAVING together
On Tue, Jul 14, 2015 at 4:23 PM, Andrew Gierth and...@tao11.riddles.org.uk wrote: Jeevan == Jeevan Chalke jeevan.cha...@enterprisedb.com writes: Jeevan Basically, when we have only one element in GROUING SETS, we Jeevan are assuming it as a simple GROUP BY with one column. Due to Jeevan 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 have observed that. But I thought that it is indeed intentional. As we don't want to choose code path for GSets when we have only one column which is converted to plain group by and thus setting parse-groupingSets to FALSE. I'll look into this. OK. Thanks In the meantime your patch looks OK (and necessary) to me. Jeevan The side effect is that, if we have plain group by clause, then Jeevan too we can use GROUPING in HAVING clause. But I guess it is Jeevan 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) -- Jeevan B Chalke Principal Software Engineer, Product Development EnterpriseDB Corporation The Enterprise PostgreSQL Company
