summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorAndrew Gierth <rhodiumtoad@postgresql.org>2018-03-21 11:34:09 +0000
committerAndrew Gierth <rhodiumtoad@postgresql.org>2018-03-21 11:41:53 +0000
commitcf21c46495897cf3a59f2b1230b522a969a17bea (patch)
treeda205122a9899cbb9549c27f4c6ccfe3036caa39
parent5b1b7286c9e12dfe6aad6d722b704d3e948a3d03 (diff)
Repair crash with unsortable grouping sets.
If there were multiple grouping sets, none of them empty, all of which were unsortable, then an oversight in consider_groupingsets_paths led to a null pointer dereference. Fix, and add a regression test for this case. Per report from Dang Minh Huong, though I didn't use their patch. Backpatch to 10.x where hashed grouping sets were added.
-rw-r--r--src/backend/optimizer/plan/planner.c23
-rw-r--r--src/test/regress/expected/groupingsets.out12
-rw-r--r--src/test/regress/sql/groupingsets.sql5
3 files changed, 39 insertions, 1 deletions
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 4dae405bd56..1946f9ef968 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -4200,7 +4200,28 @@ consider_groupingsets_paths(PlannerInfo *root,
Assert(can_hash);
- if (pathkeys_contained_in(root->group_pathkeys, path->pathkeys))
+ /*
+ * If the input is coincidentally sorted usefully (which can happen
+ * even if is_sorted is false, since that only means that our caller
+ * has set up the sorting for us), then save some hashtable space by
+ * making use of that. But we need to watch out for degenerate cases:
+ *
+ * 1) If there are any empty grouping sets, then group_pathkeys might
+ * be NIL if all non-empty grouping sets are unsortable. In this case,
+ * there will be a rollup containing only empty groups, and the
+ * pathkeys_contained_in test is vacuously true; this is ok.
+ *
+ * XXX: the above relies on the fact that group_pathkeys is generated
+ * from the first rollup. If we add the ability to consider multiple
+ * sort orders for grouping input, this assumption might fail.
+ *
+ * 2) If there are no empty sets and only unsortable sets, then the
+ * rollups list will be empty (and thus l_start == NULL), and
+ * group_pathkeys will be NIL; we must ensure that the vacuously-true
+ * pathkeys_contain_in test doesn't cause us to crash.
+ */
+ if (l_start != NULL &&
+ pathkeys_contained_in(root->group_pathkeys, path->pathkeys))
{
unhashed_rollup = lfirst(l_start);
exclude_groups = unhashed_rollup->numGroups;
diff --git a/src/test/regress/expected/groupingsets.out b/src/test/regress/expected/groupingsets.out
index d21a494a9dd..c7deec2ff40 100644
--- a/src/test/regress/expected/groupingsets.out
+++ b/src/test/regress/expected/groupingsets.out
@@ -1018,6 +1018,18 @@ explain (costs off)
-> Values Scan on "*VALUES*"
(9 rows)
+-- unsortable cases
+select unsortable_col, count(*)
+ from gstest4 group by grouping sets ((unsortable_col),(unsortable_col))
+ order by unsortable_col::text;
+ unsortable_col | count
+----------------+-------
+ 1 | 4
+ 1 | 4
+ 2 | 4
+ 2 | 4
+(4 rows)
+
-- mixed hashable/sortable cases
select unhashable_col, unsortable_col,
grouping(unhashable_col, unsortable_col),
diff --git a/src/test/regress/sql/groupingsets.sql b/src/test/regress/sql/groupingsets.sql
index eb680286030..c32d23b8d72 100644
--- a/src/test/regress/sql/groupingsets.sql
+++ b/src/test/regress/sql/groupingsets.sql
@@ -292,6 +292,11 @@ explain (costs off)
select a, b, grouping(a,b), array_agg(v order by v)
from gstest1 group by cube(a,b);
+-- unsortable cases
+select unsortable_col, count(*)
+ from gstest4 group by grouping sets ((unsortable_col),(unsortable_col))
+ order by unsortable_col::text;
+
-- mixed hashable/sortable cases
select unhashable_col, unsortable_col,
grouping(unhashable_col, unsortable_col),