public inbox for [email protected]
help / color / mirror / Atom feedFrom: Andrei Lepikhov <[email protected]>
To: Tom Lane <[email protected]>
To: Alexander Korotkov <[email protected]>
Cc: Richard Guo <[email protected]>
Cc: Pavel Borisov <[email protected]>
Cc: vignesh C <[email protected]>
Cc: PostgreSQL Developers <[email protected]>
Cc: Tomas Vondra <[email protected]>
Cc: Teodor Sigaev <[email protected]>
Cc: David Rowley <[email protected]>
Cc: a.rybakina <[email protected]>
Subject: Re: POC: GROUP BY optimization
Date: Tue, 16 Apr 2024 12:59:35 +0700
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <CA+q6zcVRrd-z4YZ4M43ccst7aGL9==w5r1fionRWhP9ot6mybQ@mail.gmail.com>
<CAMbWs4-7oUGax4qWUwdiuyPKYKDO-DtKrcPedmjbPZyC-uYYrg@mail.gmail.com>
<CAPpHfdsfHn89cAFEa6-6Xm1sB_xf8Rf2WNab6DQzb8Rvs0yD8g@mail.gmail.com>
<CAPpHfdtNnX9PYmgEsh9DXYws2Px6UJhH5kYUxsg3hu-zJTYuWw@mail.gmail.com>
<[email protected]>
<CAMbWs4_NF0stupM8guasC_yCJxdTvRqm3a+JLarjvAxcH314TQ@mail.gmail.com>
<CAMbWs4-NKLa+Ss+X=WR6h0x=T07YBJoAz70ZGHzc-2zcHUHb0A@mail.gmail.com>
<[email protected]>
<CAMbWs49oMEFUEy6RRaenx5ak4AuZzuDEDzi0otEBYwJzBBYz7Q@mail.gmail.com>
<CAPpHfdtMD=RhmB80DMRejs8s72Nr_y0OprUD9YiJFGgPVMdwXw@mail.gmail.com>
<CAMbWs48MSejYST6pX8q-rEJMmRwo+fycDpc5vkkDusc=KF5y7Q@mail.gmail.com>
<[email protected]>
<CAMbWs4-wAwJgxAnUNjCz8Qgzgjwn9t_NN9=i46hVPdaOT6Jmmg@mail.gmail.com>
<d0265! [email protected]>
<[email protected]>
On 4/12/24 06:44, Tom Lane wrote:
> * The very first hunk causes get_eclass_for_sort_expr to have
> side-effects on existing EquivalenceClass data structures.
> What is the argument that that's not going to cause problems?
> At the very least it introduces asymmetry, in that the first
> caller wins the chance to change the EC's ec_sortref and later
> callers won't be able to.
Let me resolve issues bit by bit.
Addressing your first note, 'asymmetry,' I discovered this part of the
code. Before the 8d83a5d, it could cause some artefacts, but since then,
a kind of asymmetry has been introduced into the GROUP-BY clause list.
As you can see in the attached patch, GROUP-BY reordering works even
when the asymmetry of the 8d83a5d chooses different keys.
At the same time, I agree that implicitly setting anything in an
exporting function, which should just look up for EC, is a questionable
coding style. To avoid possible usage issues (in extensions, for
example), I propose setting it up afterwards, explicitly forcing this
action by input parameter - see my attempt in the attachment.
--
regards,
Andrei Lepikhov
Postgres Professional
Attachments:
[text/x-patch] group_by_asymmetry.diff (7.8K, ../[email protected]/2-group_by_asymmetry.diff)
download | inline diff:
diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c
index a619ff9177..bc08dfadaf 100644
--- a/src/backend/optimizer/path/equivclass.c
+++ b/src/backend/optimizer/path/equivclass.c
@@ -652,18 +652,7 @@ get_eclass_for_sort_expr(PlannerInfo *root,
if (opcintype == cur_em->em_datatype &&
equal(expr, cur_em->em_expr))
- {
- /*
- * Match!
- *
- * Copy the sortref if it wasn't set yet. That may happen if
- * the ec was constructed from a WHERE clause, i.e. it doesn't
- * have a target reference at all.
- */
- if (cur_ec->ec_sortref == 0 && sortref > 0)
- cur_ec->ec_sortref = sortref;
- return cur_ec;
- }
+ return cur_ec; /* Match! */
}
}
diff --git a/src/backend/optimizer/path/pathkeys.c b/src/backend/optimizer/path/pathkeys.c
index 1d61881a6b..5d9597adcd 100644
--- a/src/backend/optimizer/path/pathkeys.c
+++ b/src/backend/optimizer/path/pathkeys.c
@@ -1355,7 +1355,8 @@ make_pathkeys_for_sortclauses(PlannerInfo *root,
&sortclauses,
tlist,
false,
- &sortable);
+ &sortable,
+ false);
/* It's caller error if not all clauses were sortable */
Assert(sortable);
return result;
@@ -1379,13 +1380,16 @@ make_pathkeys_for_sortclauses(PlannerInfo *root,
* to remove any clauses that can be proven redundant via the eclass logic.
* Even though we'll have to hash in that case, we might as well not hash
* redundant columns.)
+ * *set_ec_sortref is true if caller wants to set up ec_sortref field in
+ * the pathkey Equivalence Class, if it have not initialized beforehand.
*/
List *
make_pathkeys_for_sortclauses_extended(PlannerInfo *root,
List **sortclauses,
List *tlist,
bool remove_redundant,
- bool *sortable)
+ bool *sortable,
+ bool set_ec_sortref)
{
List *pathkeys = NIL;
ListCell *l;
@@ -1409,6 +1413,13 @@ make_pathkeys_for_sortclauses_extended(PlannerInfo *root,
sortcl->nulls_first,
sortcl->tleSortGroupRef,
true);
+ if (pathkey->pk_eclass->ec_sortref == 0 && set_ec_sortref)
+ /*
+ * Copy the sortref if it wasn't set yet. That may happen if
+ * the ec was constructed from a WHERE clause, i.e. it doesn't
+ * have a target reference at all.
+ */
+ pathkey->pk_eclass->ec_sortref = sortcl->tleSortGroupRef;
/* Canonical form eliminates redundant ordering keys */
if (!pathkey_is_redundant(pathkey, pathkeys))
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 5320da51a0..dee98c9a90 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -3391,7 +3391,7 @@ standard_qp_callback(PlannerInfo *root, void *extra)
/*
* With a plain GROUP BY list, we can remove any grouping items that
* are proven redundant by EquivalenceClass processing. For example,
- * we can remove y given "WHERE x = y GROUP BY x, y". These aren't
+ * we can remove y given "WHERE x get_eclass_for_sort_expr= y GROUP BY x, y". These aren't
* especially common cases, but they're nearly free to detect. Note
* that we remove redundant items from processed_groupClause but not
* the original parse->groupClause.
@@ -3403,7 +3403,8 @@ standard_qp_callback(PlannerInfo *root, void *extra)
&root->processed_groupClause,
tlist,
true,
- &sortable);
+ &sortable,
+ true);
if (!sortable)
{
/* Can't sort; no point in considering aggregate ordering either */
@@ -3453,7 +3454,8 @@ standard_qp_callback(PlannerInfo *root, void *extra)
&root->processed_distinctClause,
tlist,
true,
- &sortable);
+ &sortable,
+ false);
if (!sortable)
root->distinct_pathkeys = NIL;
}
@@ -3479,7 +3481,8 @@ standard_qp_callback(PlannerInfo *root, void *extra)
&groupClauses,
tlist,
false,
- &sortable);
+ &sortable,
+ false);
if (!sortable)
root->setop_pathkeys = NIL;
}
@@ -6027,7 +6030,8 @@ make_pathkeys_for_window(PlannerInfo *root, WindowClause *wc,
&wc->partitionClause,
tlist,
true,
- &sortable);
+ &sortable,
+ false);
Assert(sortable);
}
diff --git a/src/include/optimizer/paths.h b/src/include/optimizer/paths.h
index 39ba461548..fff4b69dfd 100644
--- a/src/include/optimizer/paths.h
+++ b/src/include/optimizer/paths.h
@@ -242,7 +242,8 @@ extern List *make_pathkeys_for_sortclauses_extended(PlannerInfo *root,
List **sortclauses,
List *tlist,
bool remove_redundant,
- bool *sortable);
+ bool *sortable,
+ bool set_ec_sortref);
extern void initialize_mergeclause_eclasses(PlannerInfo *root,
RestrictInfo *restrictinfo);
extern void update_mergeclause_eclasses(PlannerInfo *root,
diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out
index 50695d83a2..1b06f9ca97 100644
--- a/src/test/regress/expected/aggregates.out
+++ b/src/test/regress/expected/aggregates.out
@@ -2886,6 +2886,53 @@ GROUP BY c1.w, c1.z;
5.0000000000000000
(2 rows)
+-- Pathkeys, built in subtree, can be used to optimize GROUP-BY clause ordering.
+-- Also, here we check that it doesn't depend on the initial clause order in the
+-- GROUP-BY list.
+EXPLAIN (COSTS OFF)
+SELECT c1.y,c1.x FROM group_agg_pk c1
+ JOIN group_agg_pk c2
+ ON c1.x = c2.x
+GROUP BY c1.y,c1.x,c2.x;
+ QUERY PLAN
+-----------------------------------------------------
+ Group
+ Group Key: c1.x, c1.y
+ -> Incremental Sort
+ Sort Key: c1.x, c1.y
+ Presorted Key: c1.x
+ -> Merge Join
+ Merge Cond: (c1.x = c2.x)
+ -> Sort
+ Sort Key: c1.x
+ -> Seq Scan on group_agg_pk c1
+ -> Sort
+ Sort Key: c2.x
+ -> Seq Scan on group_agg_pk c2
+(13 rows)
+
+EXPLAIN (COSTS OFF)
+SELECT c1.y,c1.x FROM group_agg_pk c1
+ JOIN group_agg_pk c2
+ ON c1.x = c2.x
+GROUP BY c1.y,c2.x,c1.x;
+ QUERY PLAN
+-----------------------------------------------------
+ Group
+ Group Key: c2.x, c1.y
+ -> Incremental Sort
+ Sort Key: c2.x, c1.y
+ Presorted Key: c2.x
+ -> Merge Join
+ Merge Cond: (c1.x = c2.x)
+ -> Sort
+ Sort Key: c1.x
+ -> Seq Scan on group_agg_pk c1
+ -> Sort
+ Sort Key: c2.x
+ -> Seq Scan on group_agg_pk c2
+(13 rows)
+
RESET enable_nestloop;
RESET enable_hashjoin;
DROP TABLE group_agg_pk;
diff --git a/src/test/regress/sql/aggregates.sql b/src/test/regress/sql/aggregates.sql
index 2905848ead..0be9c48483 100644
--- a/src/test/regress/sql/aggregates.sql
+++ b/src/test/regress/sql/aggregates.sql
@@ -1253,6 +1253,20 @@ SELECT avg(c1.f ORDER BY c1.x, c1.y)
FROM group_agg_pk c1 JOIN group_agg_pk c2 ON c1.x = c2.x
GROUP BY c1.w, c1.z;
+-- Pathkeys, built in subtree, can be used to optimize GROUP-BY clause ordering.
+-- Also, here we check that it doesn't depend on the initial clause order in the
+-- GROUP-BY list.
+EXPLAIN (COSTS OFF)
+SELECT c1.y,c1.x FROM group_agg_pk c1
+ JOIN group_agg_pk c2
+ ON c1.x = c2.x
+GROUP BY c1.y,c1.x,c2.x;
+EXPLAIN (COSTS OFF)
+SELECT c1.y,c1.x FROM group_agg_pk c1
+ JOIN group_agg_pk c2
+ ON c1.x = c2.x
+GROUP BY c1.y,c2.x,c1.x;
+
RESET enable_nestloop;
RESET enable_hashjoin;
DROP TABLE group_agg_pk;
view thread (54+ messages) latest in thread
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: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: POC: GROUP BY optimization
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox