Re: [BUGS] Improper const-evaluation of HAVING with grouping sets and subquery pullup

I wrote:
>> We should have used ressortgroupref matching to prevent this, but without
>> having checked the code right now, I think that we might only apply that
>> logic to non-Var tlist entries.  If the Agg output tlist had mentioned
>> column 2 not column 1 of the child node, I bet we'd get the right answer.

> Indeed, the attached patch passes all regression tests and produces the
> same answers for both of Heikki's examples:

I did some back-porting work on this.  The patch applies back to 9.5
where grouping sets were introduced, but it only fixes the problem
in 9.6 and later --- in 9.5, you still get the wrong output :-(.

Bisecting says the behavior changed at commit 3fc6e2d7f "Make the upper
part of the planner work by generating and comparing Paths".  Ugh.
We are certainly not back-patching that into 9.5, and I'm disinclined
even to try to identify exactly why that commit changed the behavior.

Given that this is such a weird corner case, and we've not heard
complaints from actual users about it, I'm inclined just to apply
the patch in 9.6 and up and call it good.

            regards, tom lane

diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index 1382b67..fa9a3f0 100644
*** a/src/backend/optimizer/plan/setrefs.c
--- b/src/backend/optimizer/plan/setrefs.c
*************** set_upper_references(PlannerInfo *root,
*** 1744,1751 ****
          TargetEntry *tle = (TargetEntry *) lfirst(l);
          Node       *newexpr;

!         /* If it's a non-Var sort/group item, first try to match by sortref */
!         if (tle->ressortgroupref != 0 && !IsA(tle->expr, Var))
          {
              newexpr = (Node *)
                  search_indexed_tlist_for_sortgroupref(tle->expr,
--- 1744,1751 ----
          TargetEntry *tle = (TargetEntry *) lfirst(l);
          Node       *newexpr;

!         /* If it's a sort/group item, first try to match by sortref */
!         if (tle->ressortgroupref != 0)
          {
              newexpr = (Node *)
                  search_indexed_tlist_for_sortgroupref(tle->expr,
*************** search_indexed_tlist_for_non_var(Expr *n
*** 2113,2119 ****

  /*
   * search_indexed_tlist_for_sortgroupref --- find a sort/group expression
-  *        (which is assumed not to be just a Var)
   *
   * If a match is found, return a Var constructed to reference the tlist item.
   * If no match, return NULL.
--- 2113,2118 ----
diff --git a/src/test/regress/expected/groupingsets.out b/src/test/regress/expected/groupingsets.out
index fd618af..833d515 100644
*** a/src/test/regress/expected/groupingsets.out
--- b/src/test/regress/expected/groupingsets.out
*************** select a, d, grouping(a,b,c)
*** 360,365 ****
--- 360,394 ----
   2 | 2 |        2
  (4 rows)

+ -- check that distinct grouping columns are kept separate
+ -- even if they are equal()
+ explain (costs off)
+ select g as alias1, g as alias2
+   from generate_series(1,3) g
+  group by alias1, rollup(alias2);
+                    QUERY PLAN
+ ------------------------------------------------
+  GroupAggregate
+    Group Key: g, g
+    Group Key: g
+    ->  Sort
+          Sort Key: g
+          ->  Function Scan on generate_series g
+ (6 rows)
+
+ select g as alias1, g as alias2
+   from generate_series(1,3) g
+  group by alias1, rollup(alias2);
+  alias1 | alias2
+ --------+--------
+       1 |      1
+       1 |
+       2 |      2
+       2 |
+       3 |      3
+       3 |
+ (6 rows)
+
  -- simple rescan tests
  select a, b, sum(v.x)
    from (values (1),(2)) v(x), gstest_data(v.x)
diff --git a/src/test/regress/sql/groupingsets.sql b/src/test/regress/sql/groupingsets.sql
index 564ebc9..2b4ab69 100644
*** a/src/test/regress/sql/groupingsets.sql
--- b/src/test/regress/sql/groupingsets.sql
*************** select a, d, grouping(a,b,c)
*** 141,146 ****
--- 141,157 ----
    from gstest3
   group by grouping sets ((a,b), (a,c));

+ -- check that distinct grouping columns are kept separate
+ -- even if they are equal()
+ explain (costs off)
+ select g as alias1, g as alias2
+   from generate_series(1,3) g
+  group by alias1, rollup(alias2);
+
+ select g as alias1, g as alias2
+   from generate_series(1,3) g
+  group by alias1, rollup(alias2);
+
  -- simple rescan tests

  select a, b, sum(v.x)

-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs