Fix generate_union_paths for non-sortable types.

robertmhaas · robertmhaas · commit c37267162e88 · 2024-05-21T12:54:09.000-04:00
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/Zktzf926vslR35Fv@depesz.com 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
@@ -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
@@ -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
@@ -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;
 
 --
