MIME-Version: 1.0
References: 
 <CAOKkKFvYHSEsFazkrf9bRH14p-H27XMaqbZfRYjS6EHBruvZMQ@mail.gmail.com>
 <CAExHW5tPhCw5HpHG6F7uAXx3re_gRXqm_ryJFjFQ2+WrVY+hPQ@mail.gmail.com>
 <CAOKkKFsLbbpVMCG2_bV8u7M2sXLTs0y_yfMpywKekGi+qKtWKw@mail.gmail.com>
 <CAOKkKFvgjAwtUFKh3baZ7BcQ5u+wS_DO+2n7dh0un+19v_VOzQ@mail.gmail.com>
In-Reply-To: 
 <CAOKkKFvgjAwtUFKh3baZ7BcQ5u+wS_DO+2n7dh0un+19v_VOzQ@mail.gmail.com>
From: David Rowley <dgrowleyml@gmail.com>
Date: Mon, 16 Jun 2025 14:49:57 +1200
Message-ID: 
 <CAApHDvqq21KL9-eCfUyAUqxEyz40dut8+ehQHJD3Rh75iEMycw@mail.gmail.com>
Subject: Re: Add enable_groupagg GUC parameter to control GroupAggregate usage
To: Tatsuro Yamada <yamatattsu@gmail.com>
Cc: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>,
 pgsql-hackers@lists.postgresql.org
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAApHDvqq21KL9-eCfUyAUqxEyz40dut8%2BehQHJD3Rh75iEMycw%40mail.gmail.com>
Precedence: bulk

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