Horton

Re: Wrong results with grouping sets

Sutou Kouhei <kou@clear-code.com>

View thread
From: Sutou Kouhei <kou@clear-code.com>
To: guofenglinux@gmail.com
Cc: ashutosh.bapat.oss@gmail.com, tgl@sss.pgh.pa.us, pgsql-hackers@postgresql.org
Date: 2024-07-15T08:38:22Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Remove the RTE_GROUP RTE if we drop the groupClause

  2. Mark expressions nullable by grouping sets

  3. Introduce an RTE for the grouping step

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