Thread: Fix up grouping sets reorder
Hi all,
During the reorder of grouping sets into correct prefix order, if only
one aggregation pass is needed, we follow the order of the ORDER BY
clause to the extent possible, to minimize the chance that we add
unnecessary sorts. This is implemented in preprocess_grouping_sets -->
reorder_grouping_sets.
However, current codes fail to do that. For instance:
# set enable_hashagg to off;
SET
# explain verbose select * from t group by grouping sets((a,b,c),(c)) order by c,b,a;
QUERY PLAN
-------------------------------------------------------------------------------
Sort (cost=184.47..185.48 rows=404 width=12)
Output: a, b, c
Sort Key: t.c, t.b, t.a
-> GroupAggregate (cost=142.54..166.98 rows=404 width=12)
Output: a, b, c
Group Key: t.c, t.a, t.b
Group Key: t.c
-> Sort (cost=142.54..147.64 rows=2040 width=12)
Output: a, b, c
Sort Key: t.c, t.a, t.b
-> Seq Scan on public.t (cost=0.00..30.40 rows=2040 width=12)
Output: a, b, c
(12 rows)
This sort node in the above plan can be avoided if we reorder the
grouping sets more properly.
Attached is a patch for the fixup. With the patch, the above plan would
become:
# explain verbose select * from t group by grouping sets((a,b,c),(c)) order by c,b,a;
QUERY PLAN
-------------------------------------------------------------------------
GroupAggregate (cost=142.54..166.98 rows=404 width=12)
Output: a, b, c
Group Key: t.c, t.b, t.a
Group Key: t.c
-> Sort (cost=142.54..147.64 rows=2040 width=12)
Output: a, b, c
Sort Key: t.c, t.b, t.a
-> Seq Scan on public.t (cost=0.00..30.40 rows=2040 width=12)
Output: a, b, c
(9 rows)
The fix happens in reorder_grouping_sets and is very simple. In each
iteration to reorder one grouping set, if the next item in sortclause
matches one element in new_elems, we add that item to the grouing set
list and meanwhile remove it from the new_elems list. When all the
elements in new_elems have been removed, we can know we are done with
current grouping set. We should break out to continue with next grouping
set.
Any thoughts?
Thanks
Richard
During the reorder of grouping sets into correct prefix order, if only
one aggregation pass is needed, we follow the order of the ORDER BY
clause to the extent possible, to minimize the chance that we add
unnecessary sorts. This is implemented in preprocess_grouping_sets -->
reorder_grouping_sets.
However, current codes fail to do that. For instance:
# set enable_hashagg to off;
SET
# explain verbose select * from t group by grouping sets((a,b,c),(c)) order by c,b,a;
QUERY PLAN
-------------------------------------------------------------------------------
Sort (cost=184.47..185.48 rows=404 width=12)
Output: a, b, c
Sort Key: t.c, t.b, t.a
-> GroupAggregate (cost=142.54..166.98 rows=404 width=12)
Output: a, b, c
Group Key: t.c, t.a, t.b
Group Key: t.c
-> Sort (cost=142.54..147.64 rows=2040 width=12)
Output: a, b, c
Sort Key: t.c, t.a, t.b
-> Seq Scan on public.t (cost=0.00..30.40 rows=2040 width=12)
Output: a, b, c
(12 rows)
This sort node in the above plan can be avoided if we reorder the
grouping sets more properly.
Attached is a patch for the fixup. With the patch, the above plan would
become:
# explain verbose select * from t group by grouping sets((a,b,c),(c)) order by c,b,a;
QUERY PLAN
-------------------------------------------------------------------------
GroupAggregate (cost=142.54..166.98 rows=404 width=12)
Output: a, b, c
Group Key: t.c, t.b, t.a
Group Key: t.c
-> Sort (cost=142.54..147.64 rows=2040 width=12)
Output: a, b, c
Sort Key: t.c, t.b, t.a
-> Seq Scan on public.t (cost=0.00..30.40 rows=2040 width=12)
Output: a, b, c
(9 rows)
The fix happens in reorder_grouping_sets and is very simple. In each
iteration to reorder one grouping set, if the next item in sortclause
matches one element in new_elems, we add that item to the grouing set
list and meanwhile remove it from the new_elems list. When all the
elements in new_elems have been removed, we can know we are done with
current grouping set. We should break out to continue with next grouping
set.
Any thoughts?
Thanks
Richard
Attachment
Hi, On 2019-06-17 17:23:11 +0800, Richard Guo wrote: > During the reorder of grouping sets into correct prefix order, if only > one aggregation pass is needed, we follow the order of the ORDER BY > clause to the extent possible, to minimize the chance that we add > unnecessary sorts. This is implemented in preprocess_grouping_sets --> > reorder_grouping_sets. Thanks for finding! Andrew, could you take a look at that? - Andres
>>>>> "Andres" == Andres Freund <andres@anarazel.de> writes: >> During the reorder of grouping sets into correct prefix order, if >> only one aggregation pass is needed, we follow the order of the >> ORDER BY clause to the extent possible, to minimize the chance that >> we add unnecessary sorts. This is implemented in >> preprocess_grouping_sets --> reorder_grouping_sets. Andres> Thanks for finding! Andres> Andrew, could you take a look at that? Yes. -- Andrew (irc:RhodiumToad)
>>>>> "Richard" == Richard Guo <riguo@pivotal.io> writes: Richard> Hi all, Richard> During the reorder of grouping sets into correct prefix order, Richard> if only one aggregation pass is needed, we follow the order of Richard> the ORDER BY clause to the extent possible, to minimize the Richard> chance that we add unnecessary sorts. This is implemented in Richard> preprocess_grouping_sets --> reorder_grouping_sets. Richard> However, current codes fail to do that. You're correct, thanks for the report. Your fix works, but I prefer to refactor the conditional logic slightly instead, removing the outer if{}. So I didn't use your exact patch in the fix I just committed. -- Andrew (irc:RhodiumToad)