Re: Add enable_groupagg GUC parameter to control GroupAggregate usage

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