pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedFrom: Alena Rybakina <a.rybakina@postgrespro.ru>
To: Andrei Lepikhov <a.lepikhov@postgrespro.ru>
To: Richard Guo <guofenglinux@gmail.com>
To: Alexander Korotkov <aekorotkov@gmail.com>
Cc: Pavel Borisov <pashkin.elfe@gmail.com>
Cc: vignesh C <vignesh21@gmail.com>
Cc: PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Cc: Tomas Vondra <tomas.vondra@enterprisedb.com>
Cc: Teodor Sigaev <teodor@sigaev.ru>
Cc: David Rowley <dgrowleyml@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Subject: Re: POC: GROUP BY optimization
Date: Mon, 15 Jan 2024 16:21:05 +0300
Message-ID: <df09eb50-b4ae-4662-9247-73296410e8dc@postgrespro.ru> (raw)
In-Reply-To: <57478722-f891-46cb-b3fe-d140fedd4ebe@postgrespro.ru>
References: <CA+q6zcVRrd-z4YZ4M43ccst7aGL9==w5r1fionRWhP9ot6mybQ@mail.gmail.com>
<60610df1-c32f-ebdf-e58c-7a664431f452@enterprisedb.com>
<9acd27e9-3372-4d78-b9bc-73e407b8a007@postgrespro.ru>
<dfd75679-088a-4905-9564-67c5f6421a19@postgrespro.ru>
<CAPpHfdtzaVa7S4onKy3YvttF2rrH5hQNHx9HtcSTLbpjx+MJ+w@mail.gmail.com>
<add93f4d-07d8-4b95-b9be-0d84652a5943@postgrespro.ru>
<CALDaNm2CP2XQGTdNYKHnETeQZUV_TUBM-S297-298VqiwrFrLg@mail.gmail.com>
<138cb2df-1a0d-48c0-be80-48aaf62e8aeb@postgrespro.ru>
<CALT9ZEFbBHxjaFwRxhqf1tj2M5wmGiiZwUY5ts8eq3jwxFJ_mg@mail.gmail.com>
<CAPpHfdsrEfOqJfNuObNvM+-3D4sVzsCkmJXLLNZKf8t04dUgsg@mail.gmail.com>
<d1254c2a-2da7-4290-a81a-ca09c99ebb49@postgrespro.ru>
<CAPpHfdscoK4u4PhR354Swso3Tx0cFOjJKcbG02mi98jTwRxW9A@mail.gmail.com>
<f0ea5a6e-3e9b-48b2-b122-9196ee450cbd@postgrespro.ru>
<CAPpHfdtWiQk587H3F-SHm4VUrb26rU7kBF1GtzV1CKOzz+GcOw@mail.gmail.com>
<CAMbWs48jZKgqnCuWKzaxrzgthDW5MfJTfesnTGUOs+oWCn+uEw@mail.gmail.com>
<57478722-f891-46cb-b3fe-d140fedd4ebe@postgrespro.ru>
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
>> <aekorotkov@gmail.com <mailto:aekorotkov@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
Attachments:
[text/x-patch] group_by.diff (4.1K, ../df09eb50-b4ae-4662-9247-73296410e8dc@postgrespro.ru/2-group_by.diff)
download | inline diff:
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);
}
/*
view thread (171+ messages) latest in thread
Message-ID: <df09eb50-b4ae-4662-9247-73296410e8dc@postgrespro.ru>
Permalink: ../df09eb50-b4ae-4662-9247-73296410e8dc@postgrespro.ru/
Also on: postgresql.org/message-id/df09eb50-b4ae-4662-9247-73296410e8dc@postgrespro.ru
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: a.rybakina@postgrespro.ru, a.lepikhov@postgrespro.ru, guofenglinux@gmail.com, aekorotkov@gmail.com, pashkin.elfe@gmail.com, vignesh21@gmail.com, pgsql-hackers@lists.postgresql.org, tomas.vondra@enterprisedb.com, teodor@sigaev.ru, dgrowleyml@gmail.com, tgl@sss.pgh.pa.us
Subject: Re: POC: GROUP BY optimization
In-Reply-To: <df09eb50-b4ae-4662-9247-73296410e8dc@postgrespro.ru>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox