From 0e33f7c66682401180e037ec0fd8564401241c0a Mon Sep 17 00:00:00 2001 From: Alexander Korotkov Date: Tue, 21 May 2024 03:13:11 +0300 Subject: [PATCH v4 5/5] Teach group_keys_reorder_by_pathkeys() about redundant SortGroupClause's We don't have strict guarantees that there shouldn't be redundant SortGroupClause's. Although there is no confirmed way to generate duplicate SortGroupClause's, this commit teaches group_keys_reorder_by_pathkeys() to pull all the SortGroupClause matching to pathkey at once. Discussion: https://postgr.es/m/a663f0f6-cbf6-49aa-af2e-234dc6768a07%40postgrespro.ru Author: Andrei Lepikhov --- src/backend/optimizer/path/pathkeys.c | 41 ++++++++++++++++++--------- 1 file changed, 27 insertions(+), 14 deletions(-) diff --git a/src/backend/optimizer/path/pathkeys.c b/src/backend/optimizer/path/pathkeys.c index 793c8ad2930..0b7e9cd48db 100644 --- a/src/backend/optimizer/path/pathkeys.c +++ b/src/backend/optimizer/path/pathkeys.c @@ -401,7 +401,7 @@ group_keys_reorder_by_pathkeys(List *pathkeys, List **group_pathkeys, foreach(lc, pathkeys) { PathKey *pathkey = (PathKey *) lfirst(lc); - SortGroupClause *sgc; + bool found = false; /* * Pathkeys are built in a way that allows simply comparing pointers. @@ -416,28 +416,40 @@ group_keys_reorder_by_pathkeys(List *pathkeys, List **group_pathkeys, /* * Since 1349d27 pathkey coming from underlying node can be in the * root->group_pathkeys but not in the processed_groupClause. So, we - * should be careful here. + * should be careful here. Also, there could be redundant + * SortGroupClause's. So, we need to pull of all the matching + * SortGroupClause's, but ensure there is at least one. */ - sgc = get_sortgroupref_clause_noerr(pathkey->pk_eclass->ec_sortref, - *group_clauses); - if (!sgc) - /* The grouping clause does not cover this pathkey */ - break; + foreach_ptr(SortGroupClause, sgc, *group_clauses) + { + if (sgc->tleSortGroupRef == pathkey->pk_eclass->ec_sortref) + { + /* + * Sort group clause should have an ordering operator as long + * as there is an associated pathkey. + */ + Assert(OidIsValid(sgc->sortop)); - /* - * Sort group clause should have an ordering operator as long as there - * is an associated pathkey. - */ - Assert(OidIsValid(sgc->sortop)); + new_group_clauses = lappend(new_group_clauses, sgc); + found = true; + } + } + + /* Check if the grouping clause does not cover this pathkey */ + if (!found) + break; new_group_pathkeys = lappend(new_group_pathkeys, pathkey); - new_group_clauses = lappend(new_group_clauses, sgc); + } /* remember the number of pathkeys with a matching GROUP BY key */ n = list_length(new_group_pathkeys); - /* append the remaining group pathkeys (will be treated as not sorted) */ + /* + * Append the remaining group pathkeys (will be treated as not sorted) and + * grouping clauses. + */ *group_pathkeys = list_concat_unique_ptr(new_group_pathkeys, *group_pathkeys); *group_clauses = list_concat_unique_ptr(new_group_clauses, @@ -544,6 +556,7 @@ get_useful_group_keys_orderings(PlannerInfo *root, Path *path) } } #endif + return infos; } -- 2.39.3 (Apple Git-145)