On Wed, 13 Jul 2022 at 05:31, Tom Lane <t...@sss.pgh.pa.us> wrote: > I tried the attached quick-hack patch that just prevents > remove_useless_groupby_columns from removing anything that > appears in ORDER BY. That successfully fixes the complained-of > case, and it doesn't change any existing regression test results. > I'm not sure whether there are any cases that it makes significantly > worse.
What I am concerned about with this patch is that for Hash Aggregate, we'll always want to remove the useless group by clauses to minimise the amount of hashing and equal comparisons that we must do. So the patch makes that case worse in favour of possibly making Group Aggregate cases better. I don't think that's going to be a great trade-off as Hash Aggregate is probably more commonly used than Group Aggregate, especially so when the number of rows in each group is large and there is no LIMIT clause to favour a cheap startup plan. Maybe the fix for this should be: 1. Add a new bool "isredundant_groupby" field to SortGroupClause, 2. Rename remove_useless_groupby_columns() to mark_redundant_groupby_columns() and have it set the isredundant_groupby instead of removing items from the list, 3. Adjust get_useful_group_keys_orderings() so that it returns additional PathKeyInfo with the isredundant_groupby items removed, 4. Adjust the code in add_paths_to_grouping_rel() so that it always uses the minimal set of SortGroupClauses for Hash Aggregate paths, Perhaps a valid concern with the above is all the additional Paths we'd consider if we did #3. But maybe that's not so bad as that's not a multiplicate problem like it would be adding additional Paths to base and joinrels. We'd probably still want to keep preprocess_groupclause() as get_useful_group_keys_orderings() is not exhaustive in its search. David
