From f962e6d110c91e1ba615059eb74c37d633ac1824 Mon Sep 17 00:00:00 2001 From: Richard Guo Date: Mon, 25 Sep 2023 09:39:18 +0800 Subject: [PATCH v1] Mark expressions nullable by grouping sets When generating window_pathkeys, distinct_pathkeys or sort_pathkeys, we failed to realize that the grouping/ordering expressions might be nullable by grouping sets. As a result, we may incorrectly deem that the PathKeys are redundant by EquivalenceClass processing and thus remove them from the pathkeys list. That would lead to wrong results in some cases. To fix it, mark expressions nullable by grouping sets with a dummy RTE. --- src/backend/optimizer/path/pathkeys.c | 11 ++ src/backend/optimizer/plan/planner.c | 156 +++++++++++++++++++- src/backend/optimizer/plan/setrefs.c | 20 ++- src/backend/optimizer/prep/prepjointree.c | 1 + src/include/nodes/parsenodes.h | 11 ++ src/include/nodes/pathnodes.h | 7 + src/include/optimizer/paths.h | 1 + src/test/regress/expected/groupingsets.out | 164 +++++++++++++++++---- src/test/regress/sql/groupingsets.sql | 38 +++++ 9 files changed, 379 insertions(+), 30 deletions(-) diff --git a/src/backend/optimizer/path/pathkeys.c b/src/backend/optimizer/path/pathkeys.c index fdb60aaa8d..1637bc64de 100644 --- a/src/backend/optimizer/path/pathkeys.c +++ b/src/backend/optimizer/path/pathkeys.c @@ -26,6 +26,7 @@ #include "optimizer/pathnode.h" #include "optimizer/paths.h" #include "partitioning/partbounds.h" +#include "rewrite/rewriteManip.h" #include "utils/lsyscache.h" @@ -1141,6 +1142,7 @@ make_pathkeys_for_sortclauses(PlannerInfo *root, &sortclauses, tlist, false, + false, &sortable); /* It's caller error if not all clauses were sortable */ Assert(sortable); @@ -1158,6 +1160,9 @@ make_pathkeys_for_sortclauses(PlannerInfo *root, * give rise to redundant pathkeys are removed from the sortclauses list * (which therefore must be pass-by-reference in this version). * + * If remove_grouping_set_rtindex is true, then we need to remove the RT index + * of grouping sets from the sort exprs before we make PathKeys for them. + * * *sortable is set to true if all the sort clauses are in fact sortable. * If any are not, they are ignored except for setting *sortable false. * (In that case, the output pathkey list isn't really useful. However, @@ -1171,6 +1176,7 @@ make_pathkeys_for_sortclauses_extended(PlannerInfo *root, List **sortclauses, List *tlist, bool remove_redundant, + bool remove_grouping_set_rtindex, bool *sortable) { List *pathkeys = NIL; @@ -1189,6 +1195,11 @@ make_pathkeys_for_sortclauses_extended(PlannerInfo *root, *sortable = false; continue; } + if (remove_grouping_set_rtindex && + bms_is_member(sortcl->tleSortGroupRef, root->nullable_sortgroup_refs)) + sortkey = (Expr *) remove_nulling_relids((Node *) sortkey, + bms_make_singleton(GROUPING_SET_RTINDEX), + NULL); pathkey = make_pathkey_from_sortop(root, sortkey, sortcl->sortop, diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 44efb1f4eb..0c87046aee 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -50,6 +50,7 @@ #include "optimizer/paramassign.h" #include "optimizer/pathnode.h" #include "optimizer/paths.h" +#include "optimizer/placeholder.h" #include "optimizer/plancat.h" #include "optimizer/planmain.h" #include "optimizer/planner.h" @@ -133,6 +134,9 @@ static Node *preprocess_expression(PlannerInfo *root, Node *expr, int kind); static void preprocess_qual_conditions(PlannerInfo *root, Node *jtnode); static void grouping_planner(PlannerInfo *root, double tuple_fraction); static grouping_sets_data *preprocess_grouping_sets(PlannerInfo *root); +static void markNullableByGroupingSets(PlannerInfo *root); +static void markTargetEntryNullable(PlannerInfo *root, TargetEntry *tle, + int rtindex); static List *remap_to_groupclause_idx(List *groupClause, List *gsets, int *tleref_to_colnum_map); static void preprocess_rowmarks(PlannerInfo *root); @@ -641,6 +645,7 @@ subquery_planner(PlannerGlobal *glob, Query *parse, root->plan_params = NIL; root->outer_params = NULL; root->planner_cxt = CurrentMemoryContext; + root->nullable_sortgroup_refs = NULL; root->init_plans = NIL; root->cte_plan_ids = NIL; root->multiexpr_params = NIL; @@ -2037,6 +2042,9 @@ preprocess_grouping_sets(PlannerInfo *root) gd->unsortable_refs = NULL; gd->unsortable_sets = NIL; + /* Mark the exprs in TargetEntrys nullable by the grouping sets */ + markNullableByGroupingSets(root); + /* * We don't currently make any attempt to optimize the groupClause when * there are grouping sets, so just duplicate it in processed_groupClause. @@ -2198,6 +2206,126 @@ preprocess_grouping_sets(PlannerInfo *root) return gd; } +/* + * If the expression of a TargetEntry is nullable by grouping sets, set its + * nullingrels to include the RT index of grouping sets. + * + * We also compute nullable_sortgroup_refs in this function. + */ +static void +markNullableByGroupingSets(PlannerInfo *root) +{ + Query *parse = root->parse; + ListCell *lc; + + /* nothing to do if there are no grouping sets */ + if (parse->groupingSets == NIL) + return; + + foreach(lc, parse->groupClause) + { + SortGroupClause *gc = lfirst_node(SortGroupClause, lc); + Index ref = gc->tleSortGroupRef; + ListCell *l; + + foreach(l, parse->groupingSets) + { + List *content = lfirst(l); + + /* + * The current TargetEntry is a group clause but not contained in + * this grouping set, so it would be nulled by the grouping sets. + * Mark it so. + */ + if (!list_member_int(content, ref)) + { + TargetEntry *tle; + + tle = get_sortgroupref_tle(ref, parse->targetList); + markTargetEntryNullable(root, tle, GROUPING_SET_RTINDEX); + + root->nullable_sortgroup_refs = + bms_add_member(root->nullable_sortgroup_refs, ref); + + break; + } + } + } +} + +/* + * Mark the expr of the given TargetEntry nullable by the 'rtindex'. + */ +static void +markTargetEntryNullable(PlannerInfo *root, TargetEntry *tle, int rtindex) +{ + Node *newnode; + + newnode = (Node *) copyObject(tle->expr); + if (IsA(newnode, Var)) + { + Var *var = (Var *) newnode; + + var->varnullingrels = bms_add_member(var->varnullingrels, rtindex); + } + else if (IsA(newnode, PlaceHolderVar)) + { + PlaceHolderVar *phv = (PlaceHolderVar *) newnode; + + phv->phnullingrels = bms_add_member(phv->phnullingrels, rtindex); + } + else + { + /* + * XXX Where should we add the nullingrels if tle->expr is neither a + * Var nor a PlaceHolderVar? Wrap it in a new PlaceHolderVar to carry + * the nullingrels? + */ + + Relids relids = pull_varnos(root, newnode); + + /* + * If tle->expr is not variable-free, we set the nullingrels of Vars or + * PHVs that are contained in the expr. This is not really 'correct' + * in theory, because it is the whole expression that can be nullable + * by grouping sets, not its individual vars. But it works in + * practice, because what we need is that the expression can be somehow + * distinguished from the same expression in ECs, and marking its vars + * is sufficient for this purpose. + */ + if (!bms_is_empty(relids)) + { + newnode = add_nulling_relids(newnode, + relids, + bms_make_singleton(rtindex)); + } + else /* variable-free? */ + { + /* + * For Const, we can wrap it in a new PlaceHolderVar to carry the + * nullingrels. + */ + if (IsA(newnode, Const)) + { + PlaceHolderVar *newphv; + Relids phrels = + get_relids_in_jointree((Node *) root->parse->jointree, + true, false); + + newphv = make_placeholder_expr(root, (Expr *) newnode, phrels); + newphv->phnullingrels = + bms_add_member(newphv->phnullingrels, rtindex); + newnode = (Node *) newphv; + } + /* + * TODO What if the variable-free expression is not Const? + */ + } + } + + tle->expr = (Expr *) newnode; +} + /* * Given a groupclause and a list of GroupingSetData, return equivalent sets * (without annotation) mapped to indexes into the given groupclause. @@ -3439,9 +3567,20 @@ standard_qp_callback(PlannerInfo *root, void *extra) if (grouping_is_sortable(groupClause)) { - root->group_pathkeys = make_pathkeys_for_sortclauses(root, - groupClause, - tlist); + bool sortable; + + /* + * Note that the groupClause is logically below the grouping sets. + * So set remove_grouping_set_rtindex to true. + */ + root->group_pathkeys = + make_pathkeys_for_sortclauses_extended(root, + &groupClause, + tlist, + false, + true, + &sortable); + Assert(sortable); root->num_groupby_pathkeys = list_length(root->group_pathkeys); } else @@ -3467,6 +3606,7 @@ standard_qp_callback(PlannerInfo *root, void *extra) &root->processed_groupClause, tlist, true, + false, &sortable); if (!sortable) { @@ -3517,6 +3657,7 @@ standard_qp_callback(PlannerInfo *root, void *extra) &root->processed_distinctClause, tlist, true, + false, &sortable); if (!sortable) root->distinct_pathkeys = NIL; @@ -5377,7 +5518,15 @@ make_group_input_target(PlannerInfo *root, PathTarget *final_target) { /* * It's a grouping column, so add it to the input target as-is. + * + * Note that the target is logically below the grouping sets. So + * if the grouping expr is nullable by grouping sets, we need to + * remove the RT index of grouping sets from its nullingrels. */ + if (bms_is_member(sgref, root->nullable_sortgroup_refs)) + expr = (Expr *) remove_nulling_relids((Node *) expr, + bms_make_singleton(GROUPING_SET_RTINDEX), + NULL); add_column_to_pathtarget(input_target, expr, sgref); } else @@ -6058,6 +6207,7 @@ make_pathkeys_for_window(PlannerInfo *root, WindowClause *wc, &wc->partitionClause, tlist, true, + false, &sortable); Assert(sortable); diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c index 5700bfb5cd..0213cfcc42 100644 --- a/src/backend/optimizer/plan/setrefs.c +++ b/src/backend/optimizer/plan/setrefs.c @@ -26,6 +26,7 @@ #include "optimizer/subselect.h" #include "optimizer/tlist.h" #include "parser/parse_relation.h" +#include "rewrite/rewriteManip.h" #include "tcop/utility.h" #include "utils/lsyscache.h" #include "utils/syscache.h" @@ -2419,14 +2420,29 @@ set_upper_references(PlannerInfo *root, Plan *plan, int rtoffset) /* If it's a sort/group item, first try to match by sortref */ if (tle->ressortgroupref != 0) { + Expr *expr = tle->expr; + + /* + * If it's an Agg node for grouping sets, any Vars and PHVs + * appearing here should have nullingrels that include the effects + * of the grouping sets, ie they will have nullingrels equal to the + * input Vars/PHVs' nullingrels plus the RT index of grouping sets. + * In order to perform exact match, we can remove the RT index of + * grouping sets first. + */ + if (IsA(plan, Agg) && + bms_is_member(tle->ressortgroupref, root->nullable_sortgroup_refs)) + expr = (Expr *) remove_nulling_relids((Node *) expr, + bms_make_singleton(GROUPING_SET_RTINDEX), + NULL); newexpr = (Node *) - search_indexed_tlist_for_sortgroupref(tle->expr, + search_indexed_tlist_for_sortgroupref(expr, tle->ressortgroupref, subplan_itlist, OUTER_VAR); if (!newexpr) newexpr = fix_upper_expr(root, - (Node *) tle->expr, + (Node *) expr, subplan_itlist, OUTER_VAR, rtoffset, diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c index 73ff40721c..75c3d7230c 100644 --- a/src/backend/optimizer/prep/prepjointree.c +++ b/src/backend/optimizer/prep/prepjointree.c @@ -988,6 +988,7 @@ pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte, subroot->plan_params = NIL; subroot->outer_params = NULL; subroot->planner_cxt = CurrentMemoryContext; + subroot->nullable_sortgroup_refs = NULL; subroot->init_plans = NIL; subroot->cte_plan_ids = NIL; subroot->multiexpr_params = NIL; diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index fef4c714b8..af066c3a67 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -1023,6 +1023,17 @@ typedef enum RTEKind * present during parsing or rewriting */ } RTEKind; +/* + * RT index for grouping sets. + * + * There might be vars in the target list that are nulled by grouping sets, so + * we need a RT index for grouping sets to mark these vars in nullingrels. In + * practice we do not need to create a real RangeTblEntry for grouping sets. + * + * Here we use 0 because it's not a valid outer join relid. + */ +#define GROUPING_SET_RTINDEX 0 + typedef struct RangeTblEntry { pg_node_attr(custom_read_write, custom_query_jumble) diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h index 5702fbba60..e9498f7d96 100644 --- a/src/include/nodes/pathnodes.h +++ b/src/include/nodes/pathnodes.h @@ -265,6 +265,13 @@ struct PlannerInfo */ Relids all_query_rels; + /* + * nullable_sortgroup_refs is a Relids set of all the ressortgrouprefs of + * TargetEntrys that are nullable by grouping sets. This is computed in + * preprocess_grouping_sets. + */ + Relids nullable_sortgroup_refs; + /* * join_rel_list is a list of all join-relation RelOptInfos we have * considered in this planning run. For small problems we just scan the diff --git a/src/include/optimizer/paths.h b/src/include/optimizer/paths.h index 50bc3b503a..7f260fc3f0 100644 --- a/src/include/optimizer/paths.h +++ b/src/include/optimizer/paths.h @@ -235,6 +235,7 @@ extern List *make_pathkeys_for_sortclauses_extended(PlannerInfo *root, List **sortclauses, List *tlist, bool remove_redundant, + bool remove_grouping_set_rtindex, bool *sortable); extern void initialize_mergeclause_eclasses(PlannerInfo *root, RestrictInfo *restrictinfo); diff --git a/src/test/regress/expected/groupingsets.out b/src/test/regress/expected/groupingsets.out index a3b9aaca84..9cb3971511 100644 --- a/src/test/regress/expected/groupingsets.out +++ b/src/test/regress/expected/groupingsets.out @@ -442,19 +442,22 @@ select * from ( group by grouping sets(1, 2) ) ss where x = 1 and q1 = 123; - QUERY PLAN --------------------------------------------- + QUERY PLAN +-------------------------------------------------- Subquery Scan on ss Output: ss.x, ss.q1, ss.sum Filter: ((ss.x = 1) AND (ss.q1 = 123)) -> GroupAggregate Output: (1), i1.q1, sum(i1.q2) - Group Key: 1 + Group Key: (1) Sort Key: i1.q1 Group Key: i1.q1 - -> Seq Scan on public.int8_tbl i1 - Output: 1, i1.q1, i1.q2 -(10 rows) + -> Sort + Output: (1), i1.q1, i1.q2 + Sort Key: (1) + -> Seq Scan on public.int8_tbl i1 + Output: 1, i1.q1, i1.q2 +(13 rows) select * from ( select 1 as x, q1, sum(q2) @@ -736,15 +739,18 @@ select a, b, sum(v.x) -- Test reordering of grouping sets explain (costs off) select * from gstest1 group by grouping sets((a,b,v),(v)) order by v,b,a; - QUERY PLAN ------------------------------------------------------------------------------- - GroupAggregate - Group Key: "*VALUES*".column3, "*VALUES*".column2, "*VALUES*".column1 - Group Key: "*VALUES*".column3 - -> Sort - Sort Key: "*VALUES*".column3, "*VALUES*".column2, "*VALUES*".column1 - -> Values Scan on "*VALUES*" -(6 rows) + QUERY PLAN +------------------------------------------------------------------------------------ + Incremental Sort + Sort Key: "*VALUES*".column3, "*VALUES*".column2, "*VALUES*".column1 + Presorted Key: "*VALUES*".column3 + -> GroupAggregate + Group Key: "*VALUES*".column3, "*VALUES*".column2, "*VALUES*".column1 + Group Key: "*VALUES*".column3 + -> Sort + Sort Key: "*VALUES*".column3, "*VALUES*".column2, "*VALUES*".column1 + -> Values Scan on "*VALUES*" +(9 rows) -- Agg level check. This query should error out. select (select grouping(a,b) from gstest2) from gstest2 group by a,b; @@ -816,16 +822,18 @@ select a,count(*) from gstest2 group by rollup(a) having a is distinct from 1 or explain (costs off) select a,count(*) from gstest2 group by rollup(a) having a is distinct from 1 order by a; - QUERY PLAN ----------------------------------- - GroupAggregate - Group Key: a - Group Key: () - Filter: (a IS DISTINCT FROM 1) - -> Sort - Sort Key: a - -> Seq Scan on gstest2 -(7 rows) + QUERY PLAN +---------------------------------------- + Sort + Sort Key: a + -> GroupAggregate + Group Key: a + Group Key: () + Filter: (a IS DISTINCT FROM 1) + -> Sort + Sort Key: a + -> Seq Scan on gstest2 +(9 rows) select v.c, (select count(*) from gstest2 group by () having v.c) from (values (false),(true)) v(c) order by v.c; @@ -2150,4 +2158,110 @@ select (select grouping(v1)) from (values ((select 1))) v(v1) group by v1; 0 (1 row) +-- expressions nullable by grouping sets +explain (costs off) +select distinct on (a, b) a, b +from (values (1, 1), (2, 2)) as t (a, b) where a = b +group by grouping sets((a, b), (a)) +order by a, b; + QUERY PLAN +---------------------------------------------------------------- + Unique + -> Sort + Sort Key: "*VALUES*".column1, "*VALUES*".column2 + -> HashAggregate + Hash Key: "*VALUES*".column1, "*VALUES*".column2 + Hash Key: "*VALUES*".column1 + -> Values Scan on "*VALUES*" + Filter: (column1 = column2) +(8 rows) + +select distinct on (a, b) a, b +from (values (1, 1), (2, 2)) as t (a, b) where a = b +group by grouping sets((a, b), (a)) +order by a, b; + a | b +---+--- + 1 | 1 + 1 | + 2 | 2 + 2 | +(4 rows) + +explain (costs off) +select distinct on (a, b+1) a, b+1 +from (values (1, 0), (2, 1)) as t (a, b) where a = b+1 +group by grouping sets((a, b+1), (a)) +order by a, b+1; + QUERY PLAN +---------------------------------------------------------------------- + Unique + -> Sort + Sort Key: "*VALUES*".column1, (("*VALUES*".column2 + 1)) + -> HashAggregate + Hash Key: "*VALUES*".column1, ("*VALUES*".column2 + 1) + Hash Key: "*VALUES*".column1 + -> Values Scan on "*VALUES*" + Filter: (column1 = (column2 + 1)) +(8 rows) + +select distinct on (a, b+1) a, b+1 +from (values (1, 0), (2, 1)) as t (a, b) where a = b+1 +group by grouping sets((a, b+1), (a)) +order by a, b+1; + a | ?column? +---+---------- + 1 | 1 + 1 | + 2 | 2 + 2 | +(4 rows) + +explain (costs off) +select a, b +from (values (1, 1), (2, 2)) as t (a, b) where a = b +group by grouping sets((a, b), (a)) +order by a, b nulls first; + QUERY PLAN +---------------------------------------------------------------- + Sort + Sort Key: "*VALUES*".column1, "*VALUES*".column2 NULLS FIRST + -> HashAggregate + Hash Key: "*VALUES*".column1, "*VALUES*".column2 + Hash Key: "*VALUES*".column1 + -> Values Scan on "*VALUES*" + Filter: (column1 = column2) +(7 rows) + +select a, b +from (values (1, 1), (2, 2)) as t (a, b) where a = b +group by grouping sets((a, b), (a)) +order by a, b nulls first; + a | b +---+--- + 1 | + 1 | 1 + 2 | + 2 | 2 +(4 rows) + +explain (costs off) +select 1 as one group by rollup(one) order by one nulls first; + QUERY PLAN +----------------------------- + Sort + Sort Key: (1) NULLS FIRST + -> MixedAggregate + Hash Key: 1 + Group Key: () + -> Result +(6 rows) + +select 1 as one group by rollup(one) order by one nulls first; + one +----- + + 1 +(2 rows) + -- end diff --git a/src/test/regress/sql/groupingsets.sql b/src/test/regress/sql/groupingsets.sql index 90ba27257a..c6216edbdc 100644 --- a/src/test/regress/sql/groupingsets.sql +++ b/src/test/regress/sql/groupingsets.sql @@ -589,4 +589,42 @@ explain (costs off) select (select grouping(v1)) from (values ((select 1))) v(v1) group by v1; select (select grouping(v1)) from (values ((select 1))) v(v1) group by v1; +-- expressions nullable by grouping sets +explain (costs off) +select distinct on (a, b) a, b +from (values (1, 1), (2, 2)) as t (a, b) where a = b +group by grouping sets((a, b), (a)) +order by a, b; + +select distinct on (a, b) a, b +from (values (1, 1), (2, 2)) as t (a, b) where a = b +group by grouping sets((a, b), (a)) +order by a, b; + +explain (costs off) +select distinct on (a, b+1) a, b+1 +from (values (1, 0), (2, 1)) as t (a, b) where a = b+1 +group by grouping sets((a, b+1), (a)) +order by a, b+1; + +select distinct on (a, b+1) a, b+1 +from (values (1, 0), (2, 1)) as t (a, b) where a = b+1 +group by grouping sets((a, b+1), (a)) +order by a, b+1; + +explain (costs off) +select a, b +from (values (1, 1), (2, 2)) as t (a, b) where a = b +group by grouping sets((a, b), (a)) +order by a, b nulls first; + +select a, b +from (values (1, 1), (2, 2)) as t (a, b) where a = b +group by grouping sets((a, b), (a)) +order by a, b nulls first; + +explain (costs off) +select 1 as one group by rollup(one) order by one nulls first; +select 1 as one group by rollup(one) order by one nulls first; + -- end -- 2.31.0