Hi, I'm reviewing patches in Commitfest 2024-07 from top to bottom: https://commitfest.postgresql.org/48/
This is the 3rd patch: https://commitfest.postgresql.org/48/4583/ FYI: https://commitfest.postgresql.org/48/4681/ is my patch. In <CAMbWs49RNmFhgDzoL=suwjrcsk-wizxa6uvtp0jmz0z+741...@mail.gmail.com> "Re: Wrong results with grouping sets" on Wed, 10 Jul 2024 09:22:54 +0800, Richard Guo <guofengli...@gmail.com> wrote: > Here is an updated version of this patchset. I've added some comments > according to the review feedback, and also tweaked the commit messages > a bit more. I'm not familiar with related codes but here are my comments: 0001: --- diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index 85a62b538e5..8055f4b2b9e 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -1242,6 +1245,12 @@ typedef struct RangeTblEntry /* estimated or actual from caller */ Cardinality enrtuples pg_node_attr(query_jumble_ignore); + /* + * Fields valid for a GROUP RTE (else NULL/zero): + */ + /* list of expressions grouped on */ + List *groupexprs pg_node_attr(query_jumble_ignore); + /* * Fields valid in all RTEs: */ ---- + * Fields valid for a GROUP RTE (else NULL/zero): There is only one field and it's LIST. So how about using the following? * A field valid for a GROUP RTE (else NIL): ---- diff --git a/src/backend/optimizer/util/var.c b/src/backend/optimizer/util/var.c index 844fc30978b..0982f873a42 100644 --- a/src/backend/optimizer/util/var.c +++ b/src/backend/optimizer/util/var.c @@ -902,6 +915,141 @@ flatten_join_alias_vars_mutator(Node *node, ... +Node * +flatten_group_exprs(PlannerInfo *root, Query *query, Node *node) +{ + flatten_join_alias_vars_context context; ... --- If we want to reuse flatten_join_alias_vars_context for flatten_group_exprs(), how about renaming it? flatten_join_alias_vars() only uses flatten_join_alias_vars_context for now. So the name of flatten_join_alias_vars_context is meaningful. But if we want to flatten_join_alias_vars_context for flatten_group_exprs() too. The name of flatten_join_alias_vars_context is strange. ---- diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c index 2f64eaf0e37..69476384252 100644 --- a/src/backend/parser/parse_relation.c +++ b/src/backend/parser/parse_relation.c @@ -2557,6 +2557,79 @@ addRangeTableEntryForENR(ParseState *pstate, ... + char *colname = te->resname ? pstrdup(te->resname) : "unamed_col"; ... ---- Can the "te->resname == NULL" case be happen? If so, how about adding a new test for the case? (BTW, is "unamed_col" intentional name? Is it a typo of "unnamed_col"?) Thanks, -- kou
