Re: Wrong results with grouping sets

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+741nSA(at)mail(dot)gmail(dot)com>
"Re: Wrong results with grouping sets" on Wed, 10 Jul 2024 09:22:54 +0800,
Richard Guo <guofenglinux(at)gmail(dot)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