Horton

Thread

Commits

  1. Repair logic for reordering grouping sets optimization.

  1. Fix up grouping sets reorder

    Richard Guo <riguo@pivotal.io> — 2019-06-17T09:23:11Z

    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

  2. Re: Fix up grouping sets reorder

    Andres Freund <andres@anarazel.de> — 2019-06-17T17:33:24Z

    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

  3. Re: Fix up grouping sets reorder

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2019-06-17T18:47:38Z

    >>>>> "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)

  4. Re: Fix up grouping sets reorder

    Andrew Gierth <andrew@tao11.riddles.org.uk> — 2019-06-30T23:00:33Z

    >>>>> "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)