Fix generate_union_paths for non-sortable types.

The previous logic would fail to set groupList when
grouping_is_sortable() returned false, which could cause queries
to return wrong answers when some columns were not sortable.

David Rowley, reviewed by Heikki Linnakangas and Álvaro Herrera.
Reported by Hubert "depesz" Lubaczewski.

Discussion: http://postgr.es/m/[email protected]
Discussion: http://postgr.es/m/CAApHDvra=c8_zZT0J-TftByWN2Y+OJfnjNJFg4Dfdi2s+QzmqA@mail.gmail.com

diff --git a/src/backend/optimizer/prep/prepunion.c b/src/backend/optimizer/prep/prepunion.c
index 30068c27a1329207becdcc2dad004f0ceb9b9f93..1c69c6e97e80a1975345ed37aa7ce40667f1c413 100644 (file)
--- a/src/backend/optimizer/prep/prepunion.c
+++ b/src/backend/optimizer/prep/prepunion.c
@@ -498,7 +498,7 @@ generate_recursion_path(SetOperationStmt *setOp, PlannerInfo *root,
  * interesting_pathkeys: if not NIL, also include paths that suit these
  * pathkeys, sorting any unsorted paths as required.
  * *pNumGroups: if not NULL, we estimate the number of distinct groups
- *     in the result, and store it there
+ * in the result, and store it there.
  */
 static void
 build_setop_child_paths(PlannerInfo *root, RelOptInfo *rel,
@@ -714,7 +714,7 @@ generate_union_paths(SetOperationStmt *op, PlannerInfo *root,
    List       *groupList = NIL;
    Path       *apath;
    Path       *gpath = NULL;
-   bool        try_sorted;
+   bool        try_sorted = false;
    List       *union_pathkeys = NIL;
 
    /*
@@ -740,18 +740,21 @@ generate_union_paths(SetOperationStmt *op, PlannerInfo *root,
                                  tlist_list, refnames_tlist);
    *pTargetList = tlist;
 
-   /* For for UNIONs (not UNION ALL), try sorting, if sorting is possible */
-   try_sorted = !op->all && grouping_is_sortable(op->groupClauses);
-
-   if (try_sorted)
+   /* For UNIONs (not UNION ALL), try sorting, if sorting is possible */
+   if (!op->all)
    {
        /* Identify the grouping semantics */
        groupList = generate_setop_grouplist(op, tlist);
 
-       /* Determine the pathkeys for sorting by the whole target list */
-       union_pathkeys = make_pathkeys_for_sortclauses(root, groupList, tlist);
+       if (grouping_is_sortable(op->groupClauses))
+       {
+           try_sorted = true;
+           /* Determine the pathkeys for sorting by the whole target list */
+           union_pathkeys = make_pathkeys_for_sortclauses(root, groupList,
+                                                          tlist);
 
-       root->query_pathkeys = union_pathkeys;
+           root->query_pathkeys = union_pathkeys;
+       }
    }
 
    /*

diff --git a/src/test/regress/expected/union.out b/src/test/regress/expected/union.out
index 26b718e9033f75bab9c476a93f57371a6d4f4488..0fd0e1c38b3d7c1836ef3e63db65c025ba8b8bf7 100644 (file)
--- a/src/test/regress/expected/union.out
+++ b/src/test/regress/expected/union.out
@@ -815,6 +815,19 @@ select x from (values (row(1, 2)), (row(1, 3))) _(x) except select x from (value
  (1,3)
 (1 row)
 
+-- non-sortable type
+-- Ensure we get a HashAggregate plan.  Keep enable_hashagg=off to ensure
+-- there's no chance of a sort.
+explain (costs off) select '123'::xid union select '123'::xid;
+        QUERY PLAN         
+---------------------------
+ HashAggregate
+   Group Key: ('123'::xid)
+   ->  Append
+         ->  Result
+         ->  Result
+(5 rows)
+
 reset enable_hashagg;
 --
 -- Mixed types

diff --git a/src/test/regress/sql/union.sql b/src/test/regress/sql/union.sql
index 8afc580c6320139bc56d818c616179edea3d08bc..f8826514e42a6bda17700193512ce0cb690d1072 100644 (file)
--- a/src/test/regress/sql/union.sql
+++ b/src/test/regress/sql/union.sql
@@ -244,6 +244,12 @@ explain (costs off)
 select x from (values (row(1, 2)), (row(1, 3))) _(x) except select x from (values (row(1, 2)), (row(1, 4))) _(x);
 select x from (values (row(1, 2)), (row(1, 3))) _(x) except select x from (values (row(1, 2)), (row(1, 4))) _(x);
 
+-- non-sortable type
+
+-- Ensure we get a HashAggregate plan.  Keep enable_hashagg=off to ensure
+-- there's no chance of a sort.
+explain (costs off) select '123'::xid union select '123'::xid;
+
 reset enable_hashagg;
 
 --