Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit c01f6ef

Browse filesBrowse files
akorotkovakorotkov
committed
Fix usage of aggregate pathkeys in group_keys_reorder_by_pathkeys()
group_keys_reorder_by_pathkeys() function searched for matching pathkeys within root->group_pathkeys. That could lead to picking an aggregate pathkey and using its pathkey->pk_eclass->ec_sortref as an argument of get_sortgroupref_clause_noerr(). Given that ec_sortref of an aggregate pathkey references aggregate targetlist not query targetlist, this leads to incorrect query optimization. Fix this by looking for matching pathkeys only within the first num_groupby_pathkeys pathkeys. Reported-by: David G. Johnston Discussion: https://postgr.es/m/CAKFQuwY3Ek%3DcLThgd8FdaSc5JRDVt0FaV00gMcWra%2BTAR4gGUw%40mail.gmail.com Author: Andrei Lepikhov, Alexander Korotkov
1 parent 6743c5a commit c01f6ef
Copy full SHA for c01f6ef

File tree

Expand file treeCollapse file tree

3 files changed

+64
-4
lines changed
Filter options
Expand file treeCollapse file tree

3 files changed

+64
-4
lines changed

‎src/backend/optimizer/path/pathkeys.c

Copy file name to clipboardExpand all lines: src/backend/optimizer/path/pathkeys.c
+21-4Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -355,10 +355,14 @@ pathkeys_contained_in(List *keys1, List *keys2)
355355

356356
/*
357357
* group_keys_reorder_by_pathkeys
358-
* Reorder GROUP BY keys to match the input pathkeys.
358+
* Reorder GROUP BY pathkeys and clauses to match the input pathkeys.
359359
*
360-
* Function returns new lists (pathkeys and clauses), original GROUP BY lists
361-
* stay untouched.
360+
* 'pathkeys' is an input list of pathkeys
361+
* '*group_pathkeys' and '*group_clauses' are pathkeys and clauses lists to
362+
* reorder. The pointers are redirected to new lists, original lists
363+
* stay untouched.
364+
* 'num_groupby_pathkeys' is the number of first '*group_pathkeys' items to
365+
* search matching pathkeys.
362366
*
363367
* Returns the number of GROUP BY keys with a matching pathkey.
364368
*/
@@ -369,12 +373,24 @@ group_keys_reorder_by_pathkeys(List *pathkeys, List **group_pathkeys,
369373
{
370374
List *new_group_pathkeys = NIL,
371375
*new_group_clauses = NIL;
376+
List *grouping_pathkeys;
372377
ListCell *lc;
373378
int n;
374379

375380
if (pathkeys == NIL || *group_pathkeys == NIL)
376381
return 0;
377382

383+
/*
384+
* We're going to search within just the first num_groupby_pathkeys of
385+
* *group_pathkeys. The thing is that root->group_pathkeys is passed as
386+
* *group_pathkeys containing grouping pathkeys altogether with aggregate
387+
* pathkeys. If we process aggregate pathkeys we could get an invalid
388+
* result of get_sortgroupref_clause_noerr(), because their
389+
* pathkey->pk_eclass->ec_sortref doesn't referece query targetlist. So,
390+
* we allocate a separate list of pathkeys for lookups.
391+
*/
392+
grouping_pathkeys = list_copy_head(*group_pathkeys, num_groupby_pathkeys);
393+
378394
/*
379395
* Walk the pathkeys (determining ordering of the input path) and see if
380396
* there's a matching GROUP BY key. If we find one, we append it to the
@@ -395,7 +411,7 @@ group_keys_reorder_by_pathkeys(List *pathkeys, List **group_pathkeys,
395411
* there is no sortclause reference for some reason.
396412
*/
397413
if (foreach_current_index(lc) >= num_groupby_pathkeys ||
398-
!list_member_ptr(*group_pathkeys, pathkey) ||
414+
!list_member_ptr(grouping_pathkeys, pathkey) ||
399415
pathkey->pk_eclass->ec_sortref == 0)
400416
break;
401417

@@ -429,6 +445,7 @@ group_keys_reorder_by_pathkeys(List *pathkeys, List **group_pathkeys,
429445
*group_clauses = list_concat_unique_ptr(new_group_clauses,
430446
*group_clauses);
431447

448+
list_free(grouping_pathkeys);
432449
return n;
433450
}
434451

‎src/test/regress/expected/aggregates.out

Copy file name to clipboardExpand all lines: src/test/regress/expected/aggregates.out
+24Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2874,6 +2874,30 @@ SELECT y,x,array_agg(distinct w) FROM btg WHERE y < 0 GROUP BY x,y;
28742874

28752875
RESET enable_incremental_sort;
28762876
DROP TABLE btg;
2877+
-- Check we don't pick aggregate path key instead of grouping path key
2878+
CREATE TABLE group_agg_pk AS SELECT
2879+
i % 10 AS x,
2880+
i % 2 AS y,
2881+
i % 2 AS z,
2882+
2 AS w,
2883+
i % 10 AS f
2884+
FROM generate_series(1,100) AS i;
2885+
ANALYZE group_agg_pk;
2886+
SET enable_nestloop = off;
2887+
SET enable_hashjoin = off;
2888+
SELECT
2889+
c1.z, c1.w, string_agg(''::text, repeat(''::text, c1.f) ORDER BY c1.x,c1.y)
2890+
FROM group_agg_pk c1 JOIN group_agg_pk c2 ON (c1.x = c2.f)
2891+
GROUP BY c1.w, c1.z;
2892+
z | w | string_agg
2893+
---+---+------------
2894+
0 | 2 |
2895+
1 | 2 |
2896+
(2 rows)
2897+
2898+
RESET enable_nestloop;
2899+
RESET enable_hashjoin;
2900+
DROP TABLE group_agg_pk;
28772901
-- The case, when scanning sort order correspond to aggregate sort order but
28782902
-- can not be found in the group-by list
28792903
CREATE TABLE agg_sort_order (c1 int PRIMARY KEY, c2 int);

‎src/test/regress/sql/aggregates.sql

Copy file name to clipboardExpand all lines: src/test/regress/sql/aggregates.sql
+19Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1231,6 +1231,25 @@ RESET enable_incremental_sort;
12311231

12321232
DROP TABLE btg;
12331233

1234+
-- Check we don't pick aggregate path key instead of grouping path key
1235+
CREATE TABLE group_agg_pk AS SELECT
1236+
i % 10 AS x,
1237+
i % 2 AS y,
1238+
i % 2 AS z,
1239+
2 AS w,
1240+
i % 10 AS f
1241+
FROM generate_series(1,100) AS i;
1242+
ANALYZE group_agg_pk;
1243+
SET enable_nestloop = off;
1244+
SET enable_hashjoin = off;
1245+
SELECT
1246+
c1.z, c1.w, string_agg(''::text, repeat(''::text, c1.f) ORDER BY c1.x,c1.y)
1247+
FROM group_agg_pk c1 JOIN group_agg_pk c2 ON (c1.x = c2.f)
1248+
GROUP BY c1.w, c1.z;
1249+
RESET enable_nestloop;
1250+
RESET enable_hashjoin;
1251+
DROP TABLE group_agg_pk;
1252+
12341253
-- The case, when scanning sort order correspond to aggregate sort order but
12351254
-- can not be found in the group-by list
12361255
CREATE TABLE agg_sort_order (c1 int PRIMARY KEY, c2 int);

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.