Introduce an RTE for the grouping step
authorRichard Guo <rguo@postgresql.org>
Tue, 10 Sep 2024 03:35:34 +0000 (12:35 +0900)
committerRichard Guo <rguo@postgresql.org>
Tue, 10 Sep 2024 03:35:34 +0000 (12:35 +0900)
If there are subqueries in the grouping expressions, each of these
subqueries in the targetlist and HAVING clause is expanded into
distinct SubPlan nodes.  As a result, only one of these SubPlan nodes
would be converted to reference to the grouping key column output by
the Agg node; others would have to get evaluated afresh.  This is not
efficient, and with grouping sets this can cause wrong results issues
in cases where they should go to NULL because they are from the wrong
grouping set.  Furthermore, during re-evaluation, these SubPlan nodes
might use nulled column values from grouping sets, which is not
correct.

This issue is not limited to subqueries.  For other types of
expressions that are part of grouping items, if they are transformed
into another form during preprocessing, they may fail to match lower
target items.  This can also lead to wrong results with grouping sets.

To fix this issue, we introduce a new kind of RTE representing the
output of the grouping step, with columns that are the Vars or
expressions being grouped on.  In the parser, we replace the grouping
expressions in the targetlist and HAVING clause with Vars referencing
this new RTE, so that the output of the parser directly expresses the
semantic requirement that the grouping expressions be gotten from the
grouping output rather than computed some other way.  In the planner,
we first preprocess all the columns of this new RTE and then replace
any Vars in the targetlist and HAVING clause that reference this new
RTE with the underlying grouping expressions, so that we will have
only one instance of a SubPlan node for each subquery contained in the
grouping expressions.

Bump catversion because this changes the querytree produced by the
parser.

Thanks to Tom Lane for the idea to invent a new kind of RTE.

Per reports from Geoff Winkless, Tobias Wendorff, Richard Guo from
various threads.

Author: Richard Guo
Reviewed-by: Ashutosh Bapat, Sutou Kouhei
Discussion: https://postgr.es/m/CAMbWs4_dp7e7oTwaiZeBX8+P1rXw4ThkZxh1QG81rhu9Z47VsQ@mail.gmail.com

25 files changed:
src/backend/commands/explain.c
src/backend/nodes/nodeFuncs.c
src/backend/nodes/outfuncs.c
src/backend/nodes/print.c
src/backend/nodes/readfuncs.c
src/backend/optimizer/path/allpaths.c
src/backend/optimizer/plan/planner.c
src/backend/optimizer/plan/setrefs.c
src/backend/optimizer/prep/prepjointree.c
src/backend/optimizer/util/var.c
src/backend/parser/parse_agg.c
src/backend/parser/parse_relation.c
src/backend/parser/parse_target.c
src/backend/utils/adt/ruleutils.c
src/include/catalog/catversion.h
src/include/commands/explain.h
src/include/nodes/nodeFuncs.h
src/include/nodes/parsenodes.h
src/include/nodes/pathnodes.h
src/include/optimizer/optimizer.h
src/include/parser/parse_node.h
src/include/parser/parse_relation.h
src/test/regress/expected/groupingsets.out
src/test/regress/sql/groupingsets.sql
src/tools/pgindent/typedefs.list

index 11df4a04d430e2d6793eaf2ea08544b7845a42ae..14cd36c87c5a00604ce959e17c3005d683581341 100644 (file)
@@ -879,6 +879,7 @@ ExplainPrintPlan(ExplainState *es, QueryDesc *queryDesc)
 {
    Bitmapset  *rels_used = NULL;
    PlanState  *ps;
+   ListCell   *lc;
 
    /* Set up ExplainState fields associated with this plan tree */
    Assert(queryDesc->plannedstmt != NULL);
@@ -889,6 +890,17 @@ ExplainPrintPlan(ExplainState *es, QueryDesc *queryDesc)
    es->deparse_cxt = deparse_context_for_plan_tree(queryDesc->plannedstmt,
                                                    es->rtable_names);
    es->printed_subplans = NULL;
+   es->rtable_size = list_length(es->rtable);
+   foreach(lc, es->rtable)
+   {
+       RangeTblEntry *rte = lfirst_node(RangeTblEntry, lc);
+
+       if (rte->rtekind == RTE_GROUP)
+       {
+           es->rtable_size--;
+           break;
+       }
+   }
 
    /*
     * Sometimes we mark a Gather node as "invisible", which means that it's
@@ -2474,7 +2486,7 @@ show_plan_tlist(PlanState *planstate, List *ancestors, ExplainState *es)
    context = set_deparse_context_plan(es->deparse_cxt,
                                       plan,
                                       ancestors);
-   useprefix = list_length(es->rtable) > 1;
+   useprefix = es->rtable_size > 1;
 
    /* Deparse each result column (we now include resjunk ones) */
    foreach(lc, plan->targetlist)
@@ -2558,7 +2570,7 @@ show_upper_qual(List *qual, const char *qlabel,
 {
    bool        useprefix;
 
-   useprefix = (list_length(es->rtable) > 1 || es->verbose);
+   useprefix = (es->rtable_size > 1 || es->verbose);
    show_qual(qual, qlabel, planstate, ancestors, useprefix, es);
 }
 
@@ -2648,7 +2660,7 @@ show_grouping_sets(PlanState *planstate, Agg *agg,
    context = set_deparse_context_plan(es->deparse_cxt,
                                       planstate->plan,
                                       ancestors);
-   useprefix = (list_length(es->rtable) > 1 || es->verbose);
+   useprefix = (es->rtable_size > 1 || es->verbose);
 
    ExplainOpenGroup("Grouping Sets", "Grouping Sets", false, es);
 
@@ -2788,7 +2800,7 @@ show_sort_group_keys(PlanState *planstate, const char *qlabel,
    context = set_deparse_context_plan(es->deparse_cxt,
                                       plan,
                                       ancestors);
-   useprefix = (list_length(es->rtable) > 1 || es->verbose);
+   useprefix = (es->rtable_size > 1 || es->verbose);
 
    for (keyno = 0; keyno < nkeys; keyno++)
    {
@@ -2900,7 +2912,7 @@ show_tablesample(TableSampleClause *tsc, PlanState *planstate,
    context = set_deparse_context_plan(es->deparse_cxt,
                                       planstate->plan,
                                       ancestors);
-   useprefix = list_length(es->rtable) > 1;
+   useprefix = es->rtable_size > 1;
 
    /* Get the tablesample method name */
    method_name = get_func_name(tsc->tsmhandler);
@@ -3386,7 +3398,7 @@ show_memoize_info(MemoizeState *mstate, List *ancestors, ExplainState *es)
     * It's hard to imagine having a memoize node with fewer than 2 RTEs, but
     * let's just keep the same useprefix logic as elsewhere in this file.
     */
-   useprefix = list_length(es->rtable) > 1 || es->verbose;
+   useprefix = es->rtable_size > 1 || es->verbose;
 
    /* Set up deparsing context */
    context = set_deparse_context_plan(es->deparse_cxt,
index d2e2af4f8116d410f39c1d31aac56d12969edb1e..0d00e029f32230d027b9853ca1799ac457063ca1 100644 (file)
@@ -2854,6 +2854,11 @@ range_table_entry_walker_impl(RangeTblEntry *rte,
        case RTE_RESULT:
            /* nothing to do */
            break;
+       case RTE_GROUP:
+           if (!(flags & QTW_IGNORE_GROUPEXPRS))
+               if (WALK(rte->groupexprs))
+                   return true;
+           break;
    }
 
    if (WALK(rte->securityQuals))
@@ -3891,6 +3896,15 @@ range_table_mutator_impl(List *rtable,
            case RTE_RESULT:
                /* nothing to do */
                break;
+           case RTE_GROUP:
+               if (!(flags & QTW_IGNORE_GROUPEXPRS))
+                   MUTATE(newrte->groupexprs, rte->groupexprs, List *);
+               else
+               {
+                   /* else, copy grouping exprs as-is */
+                   newrte->groupexprs = copyObject(rte->groupexprs);
+               }
+               break;
        }
        MUTATE(newrte->securityQuals, rte->securityQuals, List *);
        newrt = lappend(newrt, newrte);
index 3337b77ae6d7a1b8cdf96cebfb682e29475aafc6..9827cf16be4607d1b89e46cb90608f21d425ae04 100644 (file)
@@ -562,6 +562,9 @@ _outRangeTblEntry(StringInfo str, const RangeTblEntry *node)
        case RTE_RESULT:
            /* no extra fields */
            break;
+       case RTE_GROUP:
+           WRITE_NODE_FIELD(groupexprs);
+           break;
        default:
            elog(ERROR, "unrecognized RTE kind: %d", (int) node->rtekind);
            break;
index 02798f4482d04e9552d255493f5a0fa485b9ad53..03416e8f4a1f826ad5a451cfce87228676ac60c0 100644 (file)
@@ -300,6 +300,10 @@ print_rt(const List *rtable)
                printf("%d\t%s\t[result]",
                       i, rte->eref->aliasname);
                break;
+           case RTE_GROUP:
+               printf("%d\t%s\t[group]",
+                      i, rte->eref->aliasname);
+               break;
            default:
                printf("%d\t%s\t[unknown rtekind]",
                       i, rte->eref->aliasname);
index b47950764a43a06bd6044f5f99780ce7ca0ccba2..be5f19dd7f6459b9b13d41e5ed33cd369b5f37e4 100644 (file)
@@ -422,6 +422,9 @@ _readRangeTblEntry(void)
        case RTE_RESULT:
            /* no extra fields */
            break;
+       case RTE_GROUP:
+           READ_NODE_FIELD(groupexprs);
+           break;
        default:
            elog(ERROR, "unrecognized RTE kind: %d",
                 (int) local_node->rtekind);
index 057b4b79ebb8dd702035cef7eb6de4605bf9fc87..172edb643a428fa540dc5986bd9d9bbf590fc1b5 100644 (file)
@@ -731,6 +731,10 @@ set_rel_consider_parallel(PlannerInfo *root, RelOptInfo *rel,
        case RTE_RESULT:
            /* RESULT RTEs, in themselves, are no problem. */
            break;
+       case RTE_GROUP:
+           /* Shouldn't happen; we're only considering baserels here. */
+           Assert(false);
+           return;
    }
 
    /*
index 62b2354f0041c28cfb20fa58b327ded4943c80c8..bd4b652f7a3492b06038858c5c87c28286b0a039 100644 (file)
@@ -88,6 +88,7 @@ create_upper_paths_hook_type create_upper_paths_hook = NULL;
 #define EXPRKIND_ARBITER_ELEM      10
 #define EXPRKIND_TABLEFUNC         11
 #define EXPRKIND_TABLEFUNC_LATERAL 12
+#define EXPRKIND_GROUPEXPR         13
 
 /*
  * Data specific to grouping sets
@@ -748,6 +749,7 @@ subquery_planner(PlannerGlobal *glob, Query *parse, PlannerInfo *parent_root,
     */
    root->hasJoinRTEs = false;
    root->hasLateralRTEs = false;
+   root->group_rtindex = 0;
    hasOuterJoins = false;
    hasResultRTEs = false;
    foreach(l, parse->rtable)
@@ -781,6 +783,10 @@ subquery_planner(PlannerGlobal *glob, Query *parse, PlannerInfo *parent_root,
            case RTE_RESULT:
                hasResultRTEs = true;
                break;
+           case RTE_GROUP:
+               Assert(parse->hasGroupRTE);
+               root->group_rtindex = list_cell_number(parse->rtable, l) + 1;
+               break;
            default:
                /* No work here for other RTE types */
                break;
@@ -836,10 +842,6 @@ subquery_planner(PlannerGlobal *glob, Query *parse, PlannerInfo *parent_root,
        preprocess_expression(root, (Node *) parse->targetList,
                              EXPRKIND_TARGET);
 
-   /* Constant-folding might have removed all set-returning functions */
-   if (parse->hasTargetSRFs)
-       parse->hasTargetSRFs = expression_returns_set((Node *) parse->targetList);
-
    newWithCheckOptions = NIL;
    foreach(l, parse->withCheckOptions)
    {
@@ -969,6 +971,13 @@ subquery_planner(PlannerGlobal *glob, Query *parse, PlannerInfo *parent_root,
            rte->values_lists = (List *)
                preprocess_expression(root, (Node *) rte->values_lists, kind);
        }
+       else if (rte->rtekind == RTE_GROUP)
+       {
+           /* Preprocess the groupexprs list fully */
+           rte->groupexprs = (List *)
+               preprocess_expression(root, (Node *) rte->groupexprs,
+                                     EXPRKIND_GROUPEXPR);
+       }
 
        /*
         * Process each element of the securityQuals list as if it were a
@@ -1005,6 +1014,27 @@ subquery_planner(PlannerGlobal *glob, Query *parse, PlannerInfo *parent_root,
        }
    }
 
+   /*
+    * Replace any Vars in the subquery's targetlist and havingQual that
+    * reference GROUP outputs with the underlying grouping expressions.
+    *
+    * Note that we need to perform this replacement after we've preprocessed
+    * the grouping expressions.  This is to ensure that there is only one
+    * instance of SubPlan for each SubLink contained within the grouping
+    * expressions.
+    */
+   if (parse->hasGroupRTE)
+   {
+       parse->targetList = (List *)
+           flatten_group_exprs(root, root->parse, (Node *) parse->targetList);
+       parse->havingQual =
+           flatten_group_exprs(root, root->parse, parse->havingQual);
+   }
+
+   /* Constant-folding might have removed all set-returning functions */
+   if (parse->hasTargetSRFs)
+       parse->hasTargetSRFs = expression_returns_set((Node *) parse->targetList);
+
    /*
     * In some cases we may want to transfer a HAVING clause into WHERE. We
     * cannot do so if the HAVING clause contains aggregates (obviously) or
@@ -1032,6 +1062,16 @@ subquery_planner(PlannerGlobal *glob, Query *parse, PlannerInfo *parent_root,
     * don't emit a bogus aggregated row. (This could be done better, but it
     * seems not worth optimizing.)
     *
+    * Note that a HAVING clause may contain expressions that are not fully
+    * preprocessed.  This can happen if these expressions are part of
+    * grouping items.  In such cases, they are replaced with GROUP Vars in
+    * the parser and then replaced back after we've done with expression
+    * preprocessing on havingQual.  This is not an issue if the clause
+    * remains in HAVING, because these expressions will be matched to lower
+    * target items in setrefs.c.  However, if the clause is moved or copied
+    * into WHERE, we need to ensure that these expressions are fully
+    * preprocessed.
+    *
     * Note that both havingQual and parse->jointree->quals are in
     * implicitly-ANDed-list form at this point, even though they are declared
     * as Node *.
@@ -1051,16 +1091,28 @@ subquery_planner(PlannerGlobal *glob, Query *parse, PlannerInfo *parent_root,
        }
        else if (parse->groupClause && !parse->groupingSets)
        {
-           /* move it to WHERE */
+           Node       *whereclause;
+
+           /* Preprocess the HAVING clause fully */
+           whereclause = preprocess_expression(root, havingclause,
+                                               EXPRKIND_QUAL);
+           /* ... and move it to WHERE */
            parse->jointree->quals = (Node *)
-               lappend((List *) parse->jointree->quals, havingclause);
+               list_concat((List *) parse->jointree->quals,
+                           (List *) whereclause);
        }
        else
        {
-           /* put a copy in WHERE, keep it in HAVING */
+           Node       *whereclause;
+
+           /* Preprocess the HAVING clause fully */
+           whereclause = preprocess_expression(root, copyObject(havingclause),
+                                               EXPRKIND_QUAL);
+           /* ... and put a copy in WHERE */
            parse->jointree->quals = (Node *)
-               lappend((List *) parse->jointree->quals,
-                       copyObject(havingclause));
+               list_concat((List *) parse->jointree->quals,
+                           (List *) whereclause);
+           /* ... and also keep it in HAVING */
            newHaving = lappend(newHaving, havingclause);
        }
    }
index 7aed84584c67050fe74ee46f370e8ecae732a657..8caf094f7d6ed86f384706b08109e5ebbeabb65e 100644 (file)
@@ -557,6 +557,7 @@ add_rte_to_flat_rtable(PlannerGlobal *glob, List *rteperminfos,
    newrte->coltypes = NIL;
    newrte->coltypmods = NIL;
    newrte->colcollations = NIL;
+   newrte->groupexprs = NIL;
    newrte->securityQuals = NIL;
 
    glob->finalrtable = lappend(glob->finalrtable, newrte);
index 34fbf8ee2378fbaf591b266689f78d2274b572ea..a70404558ff488f01229fdfa167e1b9c7b9cd327 100644 (file)
@@ -1235,6 +1235,7 @@ pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte,
                case RTE_CTE:
                case RTE_NAMEDTUPLESTORE:
                case RTE_RESULT:
+               case RTE_GROUP:
                    /* these can't contain any lateral references */
                    break;
            }
@@ -2218,7 +2219,8 @@ perform_pullup_replace_vars(PlannerInfo *root,
    }
 
    /*
-    * Replace references in the joinaliasvars lists of join RTEs.
+    * Replace references in the joinaliasvars lists of join RTEs and the
+    * groupexprs list of group RTE.
     */
    foreach(lc, parse->rtable)
    {
@@ -2228,6 +2230,10 @@ perform_pullup_replace_vars(PlannerInfo *root,
            otherrte->joinaliasvars = (List *)
                pullup_replace_vars((Node *) otherrte->joinaliasvars,
                                    rvcontext);
+       else if (otherrte->rtekind == RTE_GROUP)
+           otherrte->groupexprs = (List *)
+               pullup_replace_vars((Node *) otherrte->groupexprs,
+                                   rvcontext);
    }
 }
 
@@ -2293,6 +2299,7 @@ replace_vars_in_jointree(Node *jtnode,
                    case RTE_CTE:
                    case RTE_NAMEDTUPLESTORE:
                    case RTE_RESULT:
+                   case RTE_GROUP:
                        /* these shouldn't be marked LATERAL */
                        Assert(false);
                        break;
index 844fc30978b21a5e438150b3561ed3bcba645c3f..b189185fca2d35340d88548abf18179e523b229b 100644 (file)
@@ -81,6 +81,8 @@ static bool pull_var_clause_walker(Node *node,
                                   pull_var_clause_context *context);
 static Node *flatten_join_alias_vars_mutator(Node *node,
                                             flatten_join_alias_vars_context *context);
+static Node *flatten_group_exprs_mutator(Node *node,
+                                        flatten_join_alias_vars_context *context);
 static Node *add_nullingrels_if_needed(PlannerInfo *root, Node *newnode,
                                       Var *oldvar);
 static bool is_standard_join_alias_expression(Node *newnode, Var *oldvar);
@@ -893,6 +895,7 @@ flatten_join_alias_vars_mutator(Node *node,
    }
    /* Already-planned tree not supported */
    Assert(!IsA(node, SubPlan));
+   Assert(!IsA(node, AlternativeSubPlan));
    /* Shouldn't need to handle these planner auxiliary nodes here */
    Assert(!IsA(node, SpecialJoinInfo));
    Assert(!IsA(node, PlaceHolderInfo));
@@ -902,6 +905,141 @@ flatten_join_alias_vars_mutator(Node *node,
                                   (void *) context);
 }
 
+/*
+ * flatten_group_exprs
+ *   Replace Vars that reference GROUP outputs with the underlying grouping
+ *   expressions.
+ */
+Node *
+flatten_group_exprs(PlannerInfo *root, Query *query, Node *node)
+{
+   flatten_join_alias_vars_context context;
+
+   /*
+    * We do not expect this to be applied to the whole Query, only to
+    * expressions or LATERAL subqueries.  Hence, if the top node is a Query,
+    * it's okay to immediately increment sublevels_up.
+    */
+   Assert(node != (Node *) query);
+
+   context.root = root;
+   context.query = query;
+   context.sublevels_up = 0;
+   /* flag whether grouping expressions could possibly contain SubLinks */
+   context.possible_sublink = query->hasSubLinks;
+   /* if hasSubLinks is already true, no need to work hard */
+   context.inserted_sublink = query->hasSubLinks;
+
+   return flatten_group_exprs_mutator(node, &context);
+}
+
+static Node *
+flatten_group_exprs_mutator(Node *node,
+                           flatten_join_alias_vars_context *context)
+{
+   if (node == NULL)
+       return NULL;
+   if (IsA(node, Var))
+   {
+       Var        *var = (Var *) node;
+       RangeTblEntry *rte;
+       Node       *newvar;
+
+       /* No change unless Var belongs to the GROUP of the target level */
+       if (var->varlevelsup != context->sublevels_up)
+           return node;        /* no need to copy, really */
+       rte = rt_fetch(var->varno, context->query->rtable);
+       if (rte->rtekind != RTE_GROUP)
+           return node;
+
+       /* Expand group exprs reference */
+       Assert(var->varattno > 0);
+       newvar = (Node *) list_nth(rte->groupexprs, var->varattno - 1);
+       Assert(newvar != NULL);
+       newvar = copyObject(newvar);
+
+       /*
+        * If we are expanding an expr carried down from an upper query, must
+        * adjust its varlevelsup fields.
+        */
+       if (context->sublevels_up != 0)
+           IncrementVarSublevelsUp(newvar, context->sublevels_up, 0);
+
+       /* Preserve original Var's location, if possible */
+       if (IsA(newvar, Var))
+           ((Var *) newvar)->location = var->location;
+
+       /* Detect if we are adding a sublink to query */
+       if (context->possible_sublink && !context->inserted_sublink)
+           context->inserted_sublink = checkExprHasSubLink(newvar);
+
+       return newvar;
+   }
+
+   if (IsA(node, Aggref))
+   {
+       Aggref     *agg = (Aggref *) node;
+
+       if ((int) agg->agglevelsup == context->sublevels_up)
+       {
+           /*
+            * If we find an aggregate call of the original level, do not
+            * recurse into its normal arguments, ORDER BY arguments, or
+            * filter; there are no grouped vars there.  But we should check
+            * direct arguments as though they weren't in an aggregate.
+            */
+           agg = copyObject(agg);
+           agg->aggdirectargs = (List *)
+               flatten_group_exprs_mutator((Node *) agg->aggdirectargs, context);
+
+           return (Node *) agg;
+       }
+
+       /*
+        * We can skip recursing into aggregates of higher levels altogether,
+        * since they could not possibly contain Vars of concern to us (see
+        * transformAggregateCall).  We do need to look at aggregates of lower
+        * levels, however.
+        */
+       if ((int) agg->agglevelsup > context->sublevels_up)
+           return node;
+   }
+
+   if (IsA(node, GroupingFunc))
+   {
+       GroupingFunc *grp = (GroupingFunc *) node;
+
+       /*
+        * If we find a GroupingFunc node of the original or higher level, do
+        * not recurse into its arguments; there are no grouped vars there.
+        */
+       if ((int) grp->agglevelsup >= context->sublevels_up)
+           return node;