From a1637caee9c77e30aaf4afb5c51e15cb67c4f3e3 Mon Sep 17 00:00:00 2001 From: Andrew Gierth Date: Sun, 30 Jun 2019 23:49:25 +0100 Subject: Repair logic for reordering grouping sets optimization. The logic in reorder_grouping_sets to order grouping set elements to match a pre-specified sort ordering was defective, resulting in unnecessary sort nodes (though the query output would still be correct). Repair, simplifying the code a little, and add a test. Per report from Richard Guo, though I didn't use their patch. Original bug seems to have been my fault. Backpatch back to 9.5 where grouping sets were introduced. Discussion: https://postgr.es/m/CAN_9JTzyjGcUjiBHxLsgqfk7PkdLGXiM=pwM+=ph2LsWw0WO1A@mail.gmail.com --- src/backend/optimizer/plan/planner.c | 39 +++++++++++++++++------------------- 1 file changed, 18 insertions(+), 21 deletions(-) (limited to 'src/backend/optimizer/plan/planner.c') diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 2eeaf8539df..f6047804904 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -3255,7 +3255,6 @@ static List * reorder_grouping_sets(List *groupingsets, List *sortclause) { ListCell *lc; - ListCell *lc2; List *previous = NIL; List *result = NIL; @@ -3265,35 +3264,33 @@ reorder_grouping_sets(List *groupingsets, List *sortclause) List *new_elems = list_difference_int(candidate, previous); GroupingSetData *gs = makeNode(GroupingSetData); - if (list_length(new_elems) > 0) + while (list_length(sortclause) > list_length(previous) && + list_length(new_elems) > 0) { - while (list_length(sortclause) > list_length(previous)) - { - SortGroupClause *sc = list_nth(sortclause, list_length(previous)); - int ref = sc->tleSortGroupRef; + SortGroupClause *sc = list_nth(sortclause, list_length(previous)); + int ref = sc->tleSortGroupRef; - if (list_member_int(new_elems, ref)) - { - previous = lappend_int(previous, ref); - new_elems = list_delete_int(new_elems, ref); - } - else - { - /* diverged from the sortclause; give up on it */ - sortclause = NIL; - break; - } + if (list_member_int(new_elems, ref)) + { + previous = lappend_int(previous, ref); + new_elems = list_delete_int(new_elems, ref); } - - foreach(lc2, new_elems) + else { - previous = lappend_int(previous, lfirst_int(lc2)); + /* diverged from the sortclause; give up on it */ + sortclause = NIL; + break; } } + /* + * Safe to use list_concat (which shares cells of the second arg) + * because we know that new_elems does not share cells with anything. + */ + previous = list_concat(previous, new_elems); + gs->set = list_copy(previous); result = lcons(gs, result); - list_free(new_elems); } list_free(previous); -- cgit v1.2.3