Re: remove_useless_groupby_columns is too enthusiastic

On Wed, 13 Jul 2022 at 05:31, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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