On Wed, 11 Jun 2025 at 20:37, Tatsuro Yamada <yamatat...@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
