public inbox for [email protected]  

help / color / mirror / Atom feed

From: David Rowley <[email protected]>
To: Tatsuro Yamada <[email protected]>
Cc: Ashutosh Bapat <[email protected]>
Cc: [email protected]
Subject: Re: Add enable_groupagg GUC parameter to control GroupAggregate usage
Date: Mon, 16 Jun 2025 14:49:57 +1200
Message-ID: <CAApHDvqq21KL9-eCfUyAUqxEyz40dut8+ehQHJD3Rh75iEMycw@mail.gmail.com> (raw)
In-Reply-To: <CAOKkKFvgjAwtUFKh3baZ7BcQ5u+wS_DO+2n7dh0un+19v_VOzQ@mail.gmail.com>
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>

On Wed, 11 Jun 2025 at 20:37, Tatsuro Yamada <[email protected]> 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

view thread (18+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Add enable_groupagg GUC parameter to control GroupAggregate usage
  In-Reply-To: <CAApHDvqq21KL9-eCfUyAUqxEyz40dut8+ehQHJD3Rh75iEMycw@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox