On 15.01.2024 12:46, Andrei Lepikhov wrote:
On 15/1/2024 13:42, Richard Guo wrote:
On Mon, Jan 15, 2024 at 8:20 AM Alexander Korotkov
<aekorot...@gmail.com <mailto:aekorot...@gmail.com>> wrote:
Thank you for providing the test case relevant for this code change.
The revised patch incorporating this change is attached. Now the
patchset looks good to me. I'm going to push it if there are no
objections.
Seems I'm late for the party. Can we hold for several more days? I'd
like to have a review on this patch.
Get on board! It looks like this feature needs as much review as
possible (likewise SJE).
Hi! Thank you for your work on this issue! I believe that this will help
the scheduler to make a more optimal query plan here and therefore speed
up their execution.
I have reviewed patches and noticed that we can add some code
refactoring. I have attached a diff file (group_by.diff) to this email.
The changes involve spelling corrections, renaming variables and porting
some common parts.
In addition, I have a few questions, since some points in the code
remained unclear to me.
1. I didn't understand why we have a question in the comment next to
the enable_group_by_reordering variable in
src/backend/optimizer/path/pathkeys.c file, I assumed it was spelling
and fixed it in the diff file.
2. Why do we set the variable (path = path_save) here
(add_paths_to_grouping_rel function) if we change its variable below and
we can pass path_save as a parameter?
foreach(lc2, pathkey_orderings)
{
PathKeyInfo *info = (PathKeyInfo *) lfirst(lc2);
/* restore the path (we replace it in the loop) */
path = path_save;
path = make_ordered_path(root,
grouped_rel,
path,
cheapest_path,
info->pathkeys);
if (path == NULL)
continue;
--
Regards,
Alena Rybakina
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
diff --git a/src/backend/optimizer/path/pathkeys.c b/src/backend/optimizer/path/pathkeys.c
index 5aac6d66776..8be58fa2b0e 100644
--- a/src/backend/optimizer/path/pathkeys.c
+++ b/src/backend/optimizer/path/pathkeys.c
@@ -29,7 +29,7 @@
#include "partitioning/partbounds.h"
#include "utils/lsyscache.h"
-/* Consider reordering of GROUP BY keys? */
+/* Consider reordering of GROUP BY keys */
bool enable_group_by_reordering = true;
static bool pathkey_is_redundant(PathKey *new_pathkey, List *pathkeys);
@@ -362,7 +362,7 @@ pathkeys_contained_in(List *keys1, List *keys2)
*
* Returns the number of GROUP BY keys with a matching pathkey.
*/
-static int
+static PathKeyInfo *
group_keys_reorder_by_pathkeys(List *pathkeys, List **group_pathkeys,
List **group_clauses,
int num_groupby_pathkeys)
@@ -421,7 +421,16 @@ group_keys_reorder_by_pathkeys(List *pathkeys, List **group_pathkeys,
*group_clauses = list_concat_unique_ptr(new_group_clauses,
*group_clauses);
- return n;
+ if (n > 0 &&
+ (enable_incremental_sort || n == list_length(*group_pathkeys)))
+ {
+ PathKeyInfo *info = makeNode(PathKeyInfo);
+ info->pathkeys = *group_pathkeys;
+ info->clauses = *group_clauses;
+ return info;
+ }
+
+ return NULL;
}
/*
@@ -436,7 +445,7 @@ group_keys_reorder_by_pathkeys(List *pathkeys, List **group_pathkeys,
*
* - the original ordering, as specified by the GROUP BY clause,
* - GROUP BY keys reordered to match 'path' ordering (as much as possible),
- * - GROUP BY keys to match target ORDER BY clause (as much as possible).
+ * - GROUP BY keys should match the target ORDER BY clause (as much as possible).
*/
List *
get_useful_group_keys_orderings(PlannerInfo *root, Path *path)
@@ -475,20 +484,11 @@ get_useful_group_keys_orderings(PlannerInfo *root, Path *path)
*/
if (path->pathkeys)
{
- int n;
-
- n = group_keys_reorder_by_pathkeys(path->pathkeys, &pathkeys, &clauses,
+ info = group_keys_reorder_by_pathkeys(path->pathkeys, &pathkeys, &clauses,
root->num_groupby_pathkeys);
- if (n > 0 &&
- (enable_incremental_sort || n == list_length(path->pathkeys)))
- {
- info = makeNode(PathKeyInfo);
- info->pathkeys = pathkeys;
- info->clauses = clauses;
-
+ if (info)
infos = lappend(infos, info);
- }
}
/*
@@ -497,21 +497,12 @@ get_useful_group_keys_orderings(PlannerInfo *root, Path *path)
*/
if (root->sort_pathkeys)
{
- int n;
-
- n = group_keys_reorder_by_pathkeys(root->sort_pathkeys, &pathkeys,
+ info = group_keys_reorder_by_pathkeys(root->sort_pathkeys, &pathkeys,
&clauses,
root->num_groupby_pathkeys);
- if (n > 0 &&
- (enable_incremental_sort || n == list_length(path->pathkeys)))
- {
- info = makeNode(PathKeyInfo);
- info->pathkeys = pathkeys;
- info->clauses = clauses;
-
+ if (info)
infos = lappend(infos, info);
- }
}
return infos;
@@ -2163,27 +2154,26 @@ truncate_useless_pathkeys(PlannerInfo *root,
RelOptInfo *rel,
List *pathkeys)
{
- int nuseful;
- int nuseful2;
+ int nuseful_pathkeys;
+ int nuseful_alternative;
+
+ nuseful_pathkeys = pathkeys_useful_for_merging(root, rel, pathkeys);
- nuseful = pathkeys_useful_for_merging(root, rel, pathkeys);
- nuseful2 = pathkeys_useful_for_ordering(root, pathkeys);
- if (nuseful2 > nuseful)
- nuseful = nuseful2;
- nuseful2 = pathkeys_useful_for_grouping(root, pathkeys);
- if (nuseful2 > nuseful)
- nuseful = nuseful2;
+ if((nuseful_alternative = pathkeys_useful_for_ordering(root, pathkeys)) > nuseful_pathkeys)
+ nuseful_pathkeys = nuseful_alternative;
+ if ((nuseful_alternative = pathkeys_useful_for_grouping(root, pathkeys)) > nuseful_pathkeys)
+ nuseful_pathkeys = nuseful_alternative;
/*
* Note: not safe to modify input list destructively, but we can avoid
* copying the list if we're not actually going to change it
*/
- if (nuseful == 0)
+ if (nuseful_pathkeys == 0)
return NIL;
- else if (nuseful == list_length(pathkeys))
+ else if (nuseful_pathkeys == list_length(pathkeys))
return pathkeys;
else
- return list_copy_head(pathkeys, nuseful);
+ return list_copy_head(pathkeys, nuseful_pathkeys);
}
/*
