Horton

Thread

  1. Add enable_groupagg GUC parameter to control GroupAggregate usage

    Tatsuro Yamada <yamatattsu@gmail.com> — 2025-06-06T07:59:09Z

    Hi hackers,

When I measured the execution time of a certain query with parallel query
enabled and disabled, I found that the execution time was slower when
parallel query was enabled.

To improve the performance of the parallel query, I considered adjusting
the execution plan and attempted to switch from GroupAggregate to
HashAggregate. However, I noticed that there was no GUC parameter to
disable GroupAggregate.

Therefore, I propose adding a new GUC parameter: enable_groupagg.

Below are the results of a performance test where I disabled
GroupAggregate using enable_groupagg. In this case, the planner chose
HashAggregate instead, which improved performance by about 35 times.

# Query Execution Results (Average of 3 measurements)
- With parallel query:                                 39546 seconds
- With parallel query and enable_groupagg turned off:   1115 seconds

# Query and Data Used (attached to this email)
- Query: test_query.sql
- Data:  create_table.sql

# The steps to run the test are as follows.
For example, on psql:

1. Create tables:
    \i create_table.sql

2. Execute a query:
    \i test_query.sql

3. Execute a query using the new GUC parameter:
    set enable_groupagg to off;
    \i test_query.sql

As a benefit to users, while there has previously been a GUC parameter
to control HashAggregate, there was no corresponding way to control
GroupAggregate. This patch addresses that, giving users more flexibility
in tuning execution plans.

I've attached a WIP patch that adds this GUC parameter. I would
appreciate any feedback, especially regarding how many test cases I
should create.

To create new test cases for enable_groupagg, I looked into existing
test cases that use enable_hashagg and found that it is used in many
places (62 places). Should I add a test case for enable_groupagg in
the same place as enable_hashagg? I think that adding a new feature
requires a minimum number of test cases, so I would appreciate your
advice.


Additionally, based on the execution plan, I suspect the slowdown in the
parallel query might be caused by misestimates related to Sort or
Gather Merge.
While resolving those misestimates would ideally improve the root issue,
I'd like to keep the focus of this thread on adding the GUC parameter.
Then, I plan to report or address the estimation problem in a separate
thread.

Thanks,
Tatsuro Yamada

  2. Re: Add enable_groupagg GUC parameter to control GroupAggregate usage

    Tatsuro Yamada <yamatattsu@gmail.com> — 2025-06-06T10:03:27Z

    Hi,

# Query Execution Results (Average of 3 measurements)
> - With parallel query:                                 39546 seconds
> - With parallel query and enable_groupagg turned off:   1115 seconds
>

Oops, I made a mistake. The correct execution time is:
- With parallel query:                                 39546 ms
- With parallel query and enable_groupagg turned off:   1115 ms

Regards,
Tatsuro Yamada

  3. Re: Add enable_groupagg GUC parameter to control GroupAggregate usage

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2025-06-09T10:50:39Z

    On Fri, Jun 6, 2025 at 1:29 PM Tatsuro Yamada <yamatattsu@gmail.com> wrote:

> Hi hackers,
>
> When I measured the execution time of a certain query with parallel query
> enabled and disabled, I found that the execution time was slower when
> parallel query was enabled.
>
> To improve the performance of the parallel query, I considered adjusting
> the execution plan and attempted to switch from GroupAggregate to
> HashAggregate. However, I noticed that there was no GUC parameter to
> disable GroupAggregate.
>
> Therefore, I propose adding a new GUC parameter: enable_groupagg.
>
> Below are the results of a performance test where I disabled
> GroupAggregate using enable_groupagg. In this case, the planner chose
> HashAggregate instead, which improved performance by about 35 times.
>
> # Query Execution Results (Average of 3 measurements)
> - With parallel query:                                 39546 seconds
> - With parallel query and enable_groupagg turned off:   1115 seconds
>
> # Query and Data Used (attached to this email)
> - Query: test_query.sql
> - Data:  create_table.sql
>
> # The steps to run the test are as follows.
> For example, on psql:
>
> 1. Create tables:
>     \i create_table.sql
>
> 2. Execute a query:
>     \i test_query.sql
>
> 3. Execute a query using the new GUC parameter:
>     set enable_groupagg to off;
>     \i test_query.sql
>
> As a benefit to users, while there has previously been a GUC parameter
> to control HashAggregate, there was no corresponding way to control
> GroupAggregate. This patch addresses that, giving users more flexibility
> in tuning execution plans.
>
> I've attached a WIP patch that adds this GUC parameter. I would
> appreciate any feedback, especially regarding how many test cases I
> should create.
>

I first thought enable_hashagg should be sufficient to choose one strategy
over the other. But that is not true, enable_hashagg = true allows both the
strategies, enable_hashagg = false disables just hash strategy. There's no
way to disable group agg alone. So I think it makes sense to have this GUC.

I am surprised that we didn't see this being a problem for so long.

We seem to disable mixed strategy when enable_hashagg is false. Do we want
to do the same when enable_groupagg = false as well?


>
> To create new test cases for enable_groupagg, I looked into existing
> test cases that use enable_hashagg and found that it is used in many
> places (62 places). Should I add a test case for enable_groupagg in
> the same place as enable_hashagg? I think that adding a new feature
> requires a minimum number of test cases, so I would appreciate your
> advice.
>

Some of those instances are for plan stability, all of which need not be
replicated. But some of them explicitly test sort based grouping. For rest
of them hash based plan seems to be the best one, so explicit
enable_groupagg = false is not needed. We will need some test to test the
switch though.


>
>
> Additionally, based on the execution plan, I suspect the slowdown in the
> parallel query might be caused by misestimates related to Sort or
> Gather Merge.
> While resolving those misestimates would ideally improve the root issue,
> I'd like to keep the focus of this thread on adding the GUC parameter.
> Then, I plan to report or address the estimation problem in a separate
> thread.
>
>
+1.

-- 
Best Wishes,
Ashutosh Bapat

  4. Re: Add enable_groupagg GUC parameter to control GroupAggregate usage

    Tatsuro Yamada <yamatattsu@gmail.com> — 2025-06-11T03:07:44Z

    Hi Ashutosh,

Thanks for your reply!

>I first thought enable_hashagg should be sufficient to choose one strategy
over the other. But that is not true, enable_hashagg = true allows both the
strategies, enable_hashagg = false disables just hash strategy. There's no
way to disable group agg alone. So I think it makes sense to have this GUC.

Yes, exactly! Thank you for the clarification.


>I am surprised that we didn't see this being a problem for so long.

I was also wondering why this GUC parameter hadn't been introduced
earlier.


>We seem to disable mixed strategy when enable_hashagg is false. Do we want
to do the same when enable_groupagg = false as well?

As I mentioned in my previous email, setting enable_groupagg = false
resulted in better execution time compared to the mixed strategy (which,
as I understand, includes both HashAgg and GroupAgg). So, I believe
this new GUC parameter would be helpful for users in certain situations.

Here’s how the current and proposed behaviors compare:

Current behavior:
    enable_hashagg = ON  → Mixed
    enable_hashagg = OFF → GroupAgg

After applying the patch:
    enable_hashagg = ON,  enable_groupagg = ON  → Mixed
    enable_hashagg = OFF, enable_groupagg = ON  → GroupAgg
    enable_hashagg = ON,  enable_groupagg = OFF → HashAgg (new)
    enable_hashagg = OFF, enable_groupagg = OFF → GroupAgg

In addition, if the both parameters = OFF, GroupAgg will be selected in
the patch. The reason is that I found a comment that HashAgg might use
too much memory, so I decided to prioritize using GroupAgg, which is
more secure.

In the last case, I chose to default to GroupAgg since I found a
comment suggesting HashAgg might consume excessive memory, so GroupAgg
seemed the safer fallback.

Some hackers may want to compare the actual execution plans and times
under different GUC settings, so I've attached my test results in
"test_result.txt".


>Some of those instances are for plan stability, all of which need not be
replicated. But some of them explicitly test sort based grouping. For rest
of them hash based plan seems to be the best one, so explicit
enable_groupagg = false is not needed. We will need some test to test the
switch though.

Thanks for your advice. I'll create a regression test and send a new patch
to -hackers in my next email.

Regards,
Tatsuro Yamada

  5. Re: Add enable_groupagg GUC parameter to control GroupAggregate usage

    Tatsuro Yamada <yamatattsu@gmail.com> — 2025-06-11T08:36:58Z

    Hi Ashtosh and hackers,


> >Some of those instances are for plan stability, all of which need not be
> replicated. But some of them explicitly test sort based grouping. For rest
> of them hash based plan seems to be the best one, so explicit
> enable_groupagg = false is not needed. We will need some test to test the
> switch though.
>
> Thanks for your advice. I'll create a regression test and send a new patch
> to -hackers in my next email.
>

I created a regression test to check the enable_groupagg parameter in
the new patch.
To ensure plan stability, I disabled parallel query by setting the
max_parallel_*
parameters to 0.

Any feedback is welcome.
Please see the attached file.

Thanks,
Tatsuro Yamada

  6. Re: Add enable_groupagg GUC parameter to control GroupAggregate usage

    David Rowley <dgrowleyml@gmail.com> — 2025-06-16T02:49:57Z

    On Wed, 11 Jun 2025 at 20:37, Tatsuro Yamada <yamatattsu@gmail.com> wrote:
> I created a regression test to check the enable_groupagg parameter in
> the new patch.
> To ensure plan stability, I disabled parallel query by setting the max_parallel_*
> parameters to 0.
>
> Any feedback is welcome.

Typically, in the regression tests we've used enable_sort to force a
HashAgg. There are certainly times when that's not good enough and you
might also need to disabe enable_indexscan too, so I understand the
desire to add this GUC.

It's probably going to be worth going over the regression tests to
find where we use enable_sort to disable GroupAgg and replace those
with your new GUC. Otherwise, people looking at those tests in the
future will be a bit confused as to why the test didn't just SET
enable_groupagg TO false;  These will likely be good enough to serve
as your test, rather than creating a new table to test this feature.

I think you should also look at create_setop_path(), as I imagine that
the same arguments for using enable_hashagg in that function apply
equally to enable_groupagg.

+ if (aggstrategy == AGG_SORTED && !enable_groupagg && enable_hashagg)
+ ++disabled_nodes;

This code looks a bit strange. You're only going to disable it if hash
agg is enabled? If they're both disabled, let add_path() decide.
Anyone who complains that they didn't get the aggregate type they
wanted with both enable_hashagg and enable_groupagg set to off hasn't
got a leg to stand on.

David

  7. Re: Add enable_groupagg GUC parameter to control GroupAggregate usage

    Tatsuro Yamada <yamatattsu@gmail.com> — 2025-06-20T10:34:40Z

    Hi David,

>Typically, in the regression tests we've used enable_sort to force a
>HashAgg. There are certainly times when that's not good enough and you
>might also need to disabe enable_indexscan too, so I understand the
desire to add this GUC.

Thank you for the explanation.
I wasn't aware that enable_sort could be used to switch from GroupAgg
to HashAgg.
>From a user's perspective, I think many would expect that if
enable_hashagg exists, then enable_groupagg would as well. Adding
such a GUC parameter seems intuitive and reasonable.So, I believe
there's value in introducing the parameter I proposed.

On the other hand, if this technique (using enable_sort) is already
widely known and commonly used, and I'm simply unfamiliar with it,
then perhaps adding this GUC might not be worth the effort.

If several people support the idea of adding this GUC parameter,
I'm thinking of creating and submitting a patch that incorporates your
comment below. I believe both David and Ashutosh support the proposal.
Can I go ahead as planned?

What do others involved in planner and plan-tuning think?


>It's probably going to be worth going over the regression tests to
>find where we use enable_sort to disable GroupAgg and replace those
>with your new GUC. Otherwise, people looking at those tests in the
>future will be a bit confused as to why the test didn't just SET
>enable_groupagg TO false;  These will likely be good enough to serve
>as your test, rather than creating a new table to test this feature.

I agree.
Replacing existing enable_sort usages in regression tests with the new
GUC parameter would help future developers understand the intent more
clearly.
Also, I realized we can sufficiently test the feature without creating
a new table.


>I think you should also look at create_setop_path(), as I imagine that
>the same arguments for using enable_hashagg in that function apply
>equally to enable_groupagg.

I looked into create_setop_path(), and I see that the aggregation
method is currently controlled as follows:
=====
        /*
         * Mark the path as disabled if enable_hashagg is off.  While this
         * isn't exactly a HashAgg node, it seems close enough to justify
         * letting that switch control it.
         */
        if (!enable_hashagg)
            pathnode->path.disabled_nodes++;
=====
I plan to add enable_groupagg handling in a similar way.


>+ if (aggstrategy == AGG_SORTED && !enable_groupagg && enable_hashagg)
>+ ++disabled_nodes;
>
>This code looks a bit strange. You're only going to disable it if hash
>agg is enabled? If they're both disabled, let add_path() decide.
>Anyone who complains that they didn't get the aggregate type they
>wanted with both enable_hashagg and enable_groupagg set to off hasn't
>got a leg to stand on.

My intention was to make groupagg the fallback when both are disabled.
However, I understand that if the both are disabled, it's acceptable to
let the planner decide which strategy to use. I'll revise that part
accordingly.


Thanks,
Tatsuro Yamada

  8. Re: Add enable_groupagg GUC parameter to control GroupAggregate usage

    Richard Guo <guofenglinux@gmail.com> — 2026-06-24T23:05:28Z

    On Fri, Jun 20, 2025 at 7:35 PM Tatsuro Yamada <yamatattsu@gmail.com> wrote:
> Thank you for the explanation.
> I wasn't aware that enable_sort could be used to switch from GroupAgg
> to HashAgg.
> From a user's perspective, I think many would expect that if
> enable_hashagg exists, then enable_groupagg would as well. Adding
> such a GUC parameter seems intuitive and reasonable.So, I believe
> there's value in introducing the parameter I proposed.
>
> On the other hand, if this technique (using enable_sort) is already
> widely known and commonly used, and I'm simply unfamiliar with it,
> then perhaps adding this GUC might not be worth the effort.
>
> If several people support the idea of adding this GUC parameter,
> I'm thinking of creating and submitting a patch that incorporates your
> comment below. I believe both David and Ashutosh support the proposal.
> Can I go ahead as planned?
>
> What do others involved in planner and plan-tuning think?

I think this is a nice idea.  It is true that we can sometimes use
enable_sort to push the planner off a sorted grouping plan, but it is
a much bigger hammer, as it discourages every Sort in the query, not
just the one under the grouping, so it can move other parts of the
plan around and doesn't really isolate the grouping choice.  And it
doesn't always work anyway.  If the grouping input happens to be
cheaply sorted there's no Sort to penalize, so the sorted plan
survives.  I ran into exactly that in union.sql, where enable_sort=off
isn't really producing the hashed INTERSECT it looks like it is
testing.  Look at this plan from union.out:

-- check hashed implementation
set enable_hashagg = true;
set enable_sort = false;

explain (costs off)
select from generate_series(1,5) intersect select from generate_series(1,3);
                        QUERY PLAN
----------------------------------------------------------
 SetOp Intersect
   ->  Function Scan on generate_series
   ->  Function Scan on generate_series generate_series_1
(3 rows)

Also, without such a GUC, there are some paths that we have no way to
generate.  Look at this comment in union.sql:

-- We've no way to check hashed UNION as the empty pathkeys in the Append are
-- fine to make use of Unique, which is cheaper than HashAggregate and we've
-- no means to disable Unique.


Regarding the patch, currently the new GUC only covers GroupAggregate.
I think we should make it to cover all sort-based equivalents of
enable_hashagg, such as the sorted mode of SetOp (as David suggested),
sort-based Unique step used for DISTINCT and semijoin
unique-ification, and maybe also Group nodes.


I rebased your last patch and made some changes.  Attached is what I
ended up with.

- Richard

  9. Re: Add enable_groupagg GUC parameter to control GroupAggregate usage

    Richard Guo <guofenglinux@gmail.com> — 2026-06-25T03:18:32Z

    On Thu, Jun 25, 2026 at 8:05 AM Richard Guo <guofenglinux@gmail.com> wrote:
> I rebased your last patch and made some changes.  Attached is what I
> ended up with.

I looked into the AGG_MIXED case some more, and I don't think the v3
patch handles it correctly.  I'd assumed an AGG_MIXED plan always does
some sorted grouping, but that's not true.  For the common cube/rollup
queries the grouping sets always include the empty set, which can't be
hashed, so even the maximally-hashed plan comes out as AGG_MIXED.
Disabling it under enable_groupagg therefore penalizes the very plan
we want.

To recap how grouping sets are costed: create_groupingsets_path loops
over the rollups and calls cost_agg once per rollup.  The first call
gets the path's overall strategy (AGG_MIXED for a mixed plan); the
rest come through as AGG_HASHED or AGG_SORTED, one per rollup, with
their disabled_nodes summed into the path.  So an AGG_MIXED plan's
sorted grouping shows up in the per-rollup AGG_SORTED calls, not in
the AGG_MIXED call itself.

And the empty grouping set is really computed like AGG_PLAIN, with no
hash table and no sort, so it shouldn't bump disabled_nodes at all.
It reaches cost_agg with numGroupCols == 0, which makes it easy to
tell apart.  So in the attached patch AGG_MIXED is disabled only by
enable_hashagg, and AGG_SORTED is disabled by enable_groupagg only
when numGroupCols > 0.  That also lets the tests in groupingsets.sql
use enable_groupagg, which they couldn't before.

- Richard

  10. RE: Add enable_groupagg GUC parameter to control GroupAggregate usage

    Tatsuro Yamada <tatsuro.yamada@ntt.com> — 2026-06-25T09:11:59Z

    Hi Richard!

> > What do others involved in planner and plan-tuning think?
> 
> I think this is a nice idea.  It is true that we can sometimes use
> enable_sort to push the planner off a sorted grouping plan, but it is
> a much bigger hammer, as it discourages every Sort in the query, not
> just the one under the grouping, so it can move other parts of the
> plan around and doesn't really isolate the grouping choice.  And it
> doesn't always work anyway.  If the grouping input happens to be
> cheaply sorted there's no Sort to penalize, so the sorted plan
> survives.  I ran into exactly that in union.sql, where enable_sort=off
> isn't really producing the hashed INTERSECT it looks like it is
> testing.  Look at this plan from union.out:
> 
> -- check hashed implementation
> set enable_hashagg = true;
> set enable_sort = false;
> 
> explain (costs off)
> select from generate_series(1,5) intersect select from generate_series(1,3);
>                         QUERY PLAN
> ----------------------------------------------------------
>  SetOp Intersect
>    ->  Function Scan on generate_series
>    ->  Function Scan on generate_series generate_series_1
> (3 rows)
> 
> Also, without such a GUC, there are some paths that we have no way to
> generate.  Look at this comment in union.sql:
> 
> -- We've no way to check hashed UNION as the empty pathkeys in the
> Append are
> -- fine to make use of Unique, which is cheaper than HashAggregate and
> we've
> -- no means to disable Unique.

Thank you for your comments! 
I'm glad that you appreciate the value of the patch. 

Regarding the comments,
I hadn't realized that there were other plan nodes besides GroupAgg that 
would also benefit from this GUC. I believe this feature would be useful to 
many users. I also think it could be valuable for testing and tuning tools 
such as pg_plan_advice, developed by Robert. For that reason, I'd like to 
continue improving the patch with the goal of getting it committed for PG20.

> Regarding the patch, currently the new GUC only covers GroupAggregate.
> I think we should make it to cover all sort-based equivalents of
> enable_hashagg, such as the sorted mode of SetOp (as David suggested),
> sort-based Unique step used for DISTINCT and semijoin
> unique-ification, and maybe also Group nodes.

It sounds like there are several plan nodes that could be covered by this GUC 
(such as SetOp, sort-based Unique for DISTINCT, semijoin uniqueification, 
and Group nodes). Do you think we should cover all of them before the patch 
would be considered complete enough for commit?

I plan to work through them one by one, but if others are interested in 
contributing, I would certainly welcome the collaboration. That would help 
move the patch forward more quickly and allow us to gather feedback on 
different parts of the design in parallel.

As a practical matter, only part of my time can be devoted to community 
development work. If I end up handling everything myself, progress may be 
slower, and I'd like to avoid the patch losing momentum.
A related challenge is that extending the patch to cover more plan nodes will 
likely increase the scope of the regression test changes. Perhaps it would 
make sense to split the work into smaller patches, allowing each set of 
regression test changes to be reviewed independently.

I'd be interested to hear your thoughts on how best to proceed, and whether 
dividing the work into smaller pieces would make sense.

Regards,
Tatsuro Yamada


> - Richard

  11. Re: Add enable_groupagg GUC parameter to control GroupAggregate usage

    Richard Guo <guofenglinux@gmail.com> — 2026-06-26T06:26:20Z

    On Thu, Jun 25, 2026 at 6:12 PM Tatsuro Yamada <tatsuro.yamada@ntt.com> wrote:
> It sounds like there are several plan nodes that could be covered by this GUC
> (such as SetOp, sort-based Unique for DISTINCT, semijoin uniqueification,
> and Group nodes). Do you think we should cover all of them before the patch
> would be considered complete enough for commit?

Yeah, I think so.  The v4 patch actually already did that.  You can
find these changes in create_setop_path(), create_unique_path(), and
cost_group().

> I'd be interested to hear your thoughts on how best to proceed, and whether
> dividing the work into smaller pieces would make sense.

It seems that the scope of the regression test changes isn't too
large, so I think keeping them in one patch is fine.

- Richard

  12. RE: Add enable_groupagg GUC parameter to control GroupAggregate usage

    Tatsuro Yamada <tatsuro.yamada@ntt.com> — 2026-06-29T10:02:29Z

    Hi Richard,

> Yeah, I think so.  The v4 patch actually already did that.  You can
> find these changes in create_setop_path(), create_unique_path(), and
> cost_group().

Thank you for the clarification. I've reviewed the v4 patch and confirmed 
that it already covers SetOp, Unique, Group, etc.
I ran make installcheck in my environment and all tests passed without 
errors.

Since the enable_groupagg parameter also affects SetOp, Unique, and 
other nodes, I've created documentation patches to clarify this. I'm attaching 
them to this email. They apply on top of your v4 patch.

- 0001 patch: This improves the documentation for the enable_groupagg 
    parameter so that users can more easily understand which operations it 
    affects. If you agree with the changes, I'd like to post an updated patch 
    that integrates this into v4.

- 0002 patch: This provides a similar improvement for the enable_hashagg 
    parameter documentation. Since it concerns enable_hashagg, I'm happy 
    to move this to a separate thread if you prefer.

Best Regards,
Tatsuro Yamada

> -----Original Message-----
> From: Richard Guo <guofenglinux@gmail.com>
> Sent: Friday, June 26, 2026 3:26 PM
> To: Tatsuro Yamada（山田達朗） <tatsuro.yamada@ntt.com>
> Cc: Tatsuro Yamada <yamatattsu@gmail.com>; David Rowley
> <dgrowleyml@gmail.com>; Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com>; pgsql-hackers@lists.postgresql.org
> Subject: Re: Add enable_groupagg GUC parameter to control
> GroupAggregate usage
> 
> On Thu, Jun 25, 2026 at 6:12 PM Tatsuro Yamada <tatsuro.yamada@ntt.com>
> wrote:
> > It sounds like there are several plan nodes that could be covered by this
> GUC
> > (such as SetOp, sort-based Unique for DISTINCT, semijoin uniqueification,
> > and Group nodes). Do you think we should cover all of them before the
> patch
> > would be considered complete enough for commit?
> 
> Yeah, I think so.  The v4 patch actually already did that.  You can
> find these changes in create_setop_path(), create_unique_path(), and
> cost_group().
> 
> > I'd be interested to hear your thoughts on how best to proceed, and
> whether
> > dividing the work into smaller pieces would make sense.
> 
> It seems that the scope of the regression test changes isn't too
> large, so I think keeping them in one patch is fine.
> 
> - Richard

  13. Re: Add enable_groupagg GUC parameter to control GroupAggregate usage

    Richard Guo <guofenglinux@gmail.com> — 2026-07-07T06:47:21Z

    On Mon, Jun 29, 2026 at 7:02 PM Tatsuro Yamada <tatsuro.yamada@ntt.com> wrote:
> Since the enable_groupagg parameter also affects SetOp, Unique, and
> other nodes, I've created documentation patches to clarify this. I'm attaching
> them to this email. They apply on top of your v4 patch.

I've thought it over, and I'm inclined to leave the docs as they are.

My main concern is consistency with the surrounding parameters.  All
of the other enable_* GUCs are documented conceptually rather than by
the specific node types they affect, and none of them enumerate the
nodes you'd see in EXPLAIN.  Unique and SetOp are really EXPLAIN
labels rather than user-facing concepts, and I'd argue the existing
wording already covers them, since sort-based Unique and sorted SetOp
are both forms of sort-based grouping.

Similarly, enable_hashagg has covered hash-based SetOp for a long time
without us documenting it, which is the same convention at work.  I'd
rather not add that now just to match the new parameter.

Attached is a rebased patch to make cfbot work again.

- Richard

  14. Re: Add enable_groupagg GUC parameter to control GroupAggregate usage

    Richard Guo <guofenglinux@gmail.com> — 2026-07-07T06:53:20Z

    On Tue, Jul 7, 2026 at 3:47 PM Richard Guo <guofenglinux@gmail.com> wrote:
> Attached is a rebased patch to make cfbot work again.

I intend to push this patch soon.  That way newly added test cases can
start using enable_groupagg directly.  Otherwise we'll have to keep
rebasing it to convert any new tests that use enable_sort to force
hashed grouping.  Any thoughts?

- Richard

  15. RE: Add enable_groupagg GUC parameter to control GroupAggregate usage

    Tatsuro Yamada <tatsuro.yamada@ntt.com> — 2026-07-08T11:09:51Z

    Hi Richard,

>I've thought it over, and I'm inclined to leave the docs as they are.
>
>My main concern is consistency with the surrounding parameters.  All
>of the other enable_* GUCs are documented conceptually rather than by
>the specific node types they affect, and none of them enumerate the
>nodes you'd see in EXPLAIN.  Unique and SetOp are really EXPLAIN
>labels rather than user-facing concepts, and I'd argue the existing
>wording already covers them, since sort-based Unique and sorted SetOp
>are both forms of sort-based grouping.
>
>Similarly, enable_hashagg has covered hash-based SetOp for a long time
>without us documenting it, which is the same convention at work.  I'd
>rather not add that now just to match the new parameter.

Thanks for your comments and the rebased patch.
I understand and agree with the decision not to modify the documentation.

My initial motivation for proposing the documentation update was to make it 
clearer which plan nodes are affected by these GUC parameters. While reviewing 
your previous patch, I realized that enable_hashagg and enable_groupagg affect 
not only HashAgg and GroupAgg nodes but also other plan nodes.

However, documenting that level of detail only for these parameters would be 
inconsistent with the descriptions of the other enable_* GUCs. So I agree that 
we should keep the current documentation as it is.


>Attached is a rebased patch to make cfbot work again.

I applied the attached v5 patch to commit 57f93af36, and confirmed that all 
regression tests completed successfully.

Regards,
Tatsuro Yamada


> -----Original Message-----
> From: Richard Guo <guofenglinux@gmail.com>
> Sent: Tuesday, July 7, 2026 3:47 PM
> To: Tatsuro Yamada（山田達朗） <tatsuro.yamada@ntt.com>
> Cc: Tatsuro Yamada <yamatattsu@gmail.com>; David Rowley
> <dgrowleyml@gmail.com>; Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com>; pgsql-hackers@lists.postgresql.org
> Subject: Re: Add enable_groupagg GUC parameter to control GroupAggregate
> usage
> 
> On Mon, Jun 29, 2026 at 7:02 PM Tatsuro Yamada
> <tatsuro.yamada@ntt.com> wrote:
> > Since the enable_groupagg parameter also affects SetOp, Unique, and
> > other nodes, I've created documentation patches to clarify this. I'm
> attaching
> > them to this email. They apply on top of your v4 patch.
> 
> I've thought it over, and I'm inclined to leave the docs as they are.
> 
> My main concern is consistency with the surrounding parameters.  All
> of the other enable_* GUCs are documented conceptually rather than by
> the specific node types they affect, and none of them enumerate the
> nodes you'd see in EXPLAIN.  Unique and SetOp are really EXPLAIN
> labels rather than user-facing concepts, and I'd argue the existing
> wording already covers them, since sort-based Unique and sorted SetOp
> are both forms of sort-based grouping.
> 
> Similarly, enable_hashagg has covered hash-based SetOp for a long time
> without us documenting it, which is the same convention at work.  I'd
> rather not add that now just to match the new parameter.
> 
> Attached is a rebased patch to make cfbot work again.
> 
> - Richard

  16. RE: Add enable_groupagg GUC parameter to control GroupAggregate usage

    Tatsuro Yamada <tatsuro.yamada@ntt.com> — 2026-07-08T11:54:13Z

    Hi Richard,

>> Attached is a rebased patch to make cfbot work again.
>
>I intend to push this patch soon.  That way newly added test cases can
>start using enable_groupagg directly.  Otherwise we'll have to keep
>rebasing it to convert any new tests that use enable_sort to force
>hashed grouping.  Any thoughts?

I'm in favor of committing this.
Before you do, I'd like to clarify one point regarding the commit message. 
Would it make sense to list both of us as authors?

The reason I ask is that you made substantial changes to the patch, 
including handling additional plan nodes such as SetOp and adding regression 
tests. Given those contributions, I wondered whether it would be more 
appropriate for both of us to be listed as authors. (I've seen past commits that 
included two Author: lines.)
I'm happy to leave that decision to you.

By the way, I re-ran the test query from the initial email in this thread.
The execution times were:

  Default: 25,791.642 ms
  SET enable_groupagg TO off;: 1,006.839 ms

This again confirmed an approximately 25x speedup.

Of course, the actual benefit depends on the specific query, but I believe 
this parameter will allow users to improve query performance in more situations.

Regards,
Tatsuro Yamada

> -----Original Message-----
> From: Richard Guo <guofenglinux@gmail.com>
> Sent: Tuesday, July 7, 2026 3:53 PM
> To: Tatsuro Yamada（山田達朗） <tatsuro.yamada@ntt.com>
> Cc: Tatsuro Yamada <yamatattsu@gmail.com>; David Rowley
> <dgrowleyml@gmail.com>; Ashutosh Bapat
> <ashutosh.bapat.oss@gmail.com>; pgsql-hackers@lists.postgresql.org
> Subject: Re: Add enable_groupagg GUC parameter to control GroupAggregate
> usage
> 
> On Tue, Jul 7, 2026 at 3:47 PM Richard Guo <guofenglinux@gmail.com>
> wrote:
> > Attached is a rebased patch to make cfbot work again.
> 
> I intend to push this patch soon.  That way newly added test cases can
> start using enable_groupagg directly.  Otherwise we'll have to keep
> rebasing it to convert any new tests that use enable_sort to force
> hashed grouping.  Any thoughts?
> 
> - Richard