From 793eb94e31387738c72a8d7d702a6aecc9c5edee Mon Sep 17 00:00:00 2001 From: Andrew Gierth Date: Sun, 30 Jun 2019 23:49:29 +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 1bfcd5915e9..078df86b5ea 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -3049,7 +3049,6 @@ static List * reorder_grouping_sets(List *groupingsets, List *sortclause) { ListCell *lc; - ListCell *lc2; List *previous = NIL; List *result = NIL; @@ -3058,34 +3057,32 @@ reorder_grouping_sets(List *groupingsets, List *sortclause) List *candidate = lfirst(lc); List *new_elems = list_difference_int(candidate, previous); - 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); + result = lcons(list_copy(previous), result); - list_free(new_elems); } list_free(previous); -- cgit v1.2.3