On 13/1/2024 22:00, Alexander Korotkov wrote:
On Sat, Jan 13, 2024 at 11:09 AM Andrei Lepikhov
<a.lepik...@postgrespro.ru> wrote:
On 11/1/2024 18:30, Alexander Korotkov wrote:
On Tue, Jan 9, 2024 at 1:14 PM Pavel Borisov <pashkin.e...@gmail.com> wrote:
Hmm, I don't see this old code in these patches. Resend 0002-* because
of trailing spaces.
AFAIK, cfbot does not seek old versions of patchset parts in previous messages.
So for it to run correctly, a new version of the whole patchset should be sent
even if most patches are unchanged.
Please, find the revised patchset with some refactoring and comments
improvement from me. I'll continue looking into this.
The patch looks better, thanks to your refactoring.
I propose additional comments and tests to make the code more
understandable (see attachment).
I intended to look into this part of the code more, but the tests show a
difference between PostgreSQL v.15 and v.16, which causes changes in the
code of this feature.
Makes sense. I've incorporated your changes into the patchset.
One more improvement. To underpin code change:
- return cur_ec; /* Match! */
+ {
+ /*
+ * Match!
+ *
+ * Copy the sortref if it wasn't set yet. That may happen if
+ * the ec was constructed from 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;
+ }
I propose the test (see attachment). It shows why we introduce this
change: GROUP-BY should juggle not only pathkeys generated by explicit
sort operators but also planner-made, likewise in this example, by
MergeJoin.
--
regards,
Andrei Lepikhov
Postgres Professional
diff --git a/src/test/regress/expected/aggregates.out
b/src/test/regress/expected/aggregates.out
index 0d46e096e5..ca38e78f21 100644
--- a/src/test/regress/expected/aggregates.out
+++ b/src/test/regress/expected/aggregates.out
@@ -2879,6 +2879,37 @@ FROM t1 WHERE c2 < 100 GROUP BY c1 ORDER BY 2;
(10 rows)
DROP TABLE t1 CASCADE;
+-- Check, that GROUP-BY reordering optimization can operate with pathkeys,
built
+-- by planner itself. For example, by MergeJoin.
+SET enable_hashjoin = off;
+SET enable_nestloop = off;
+explain (COSTS OFF)
+SELECT c1.relname,c1.relpages
+FROM pg_class c1 JOIN pg_class c2 ON (c1.relname=c2.relname AND
c1.relpages=c2.relpages)
+GROUP BY c1.reltuples,c1.relpages,c1.relname
+ORDER BY c1.relpages, c1.relname, c1.relpages*c1.relpages;
+ QUERY PLAN
+---------------------------------------------------------------------------------------------
+ Incremental Sort
+ Sort Key: c1.relpages, c1.relname, ((c1.relpages * c1.relpages))
+ Presorted Key: c1.relpages, c1.relname
+ -> Group
+ Group Key: c1.relpages, c1.relname, c1.reltuples
+ -> Incremental Sort
+ Sort Key: c1.relpages, c1.relname, c1.reltuples
+ Presorted Key: c1.relpages, c1.relname
+ -> Merge Join
+ Merge Cond: ((c1.relpages = c2.relpages) AND (c1.relname
= c2.relname))
+ -> Sort
+ Sort Key: c1.relpages, c1.relname
+ -> Seq Scan on pg_class c1
+ -> Sort
+ Sort Key: c2.relpages, c2.relname
+ -> Seq Scan on pg_class c2
+(16 rows)
+
+RESET enable_hashjoin;
+RESET enable_nestloop;
RESET enable_hashagg;
RESET max_parallel_workers;
RESET max_parallel_workers_per_gather;
diff --git a/src/test/regress/sql/aggregates.sql
b/src/test/regress/sql/aggregates.sql
index f99167ac9e..cf87b5d5dd 100644
--- a/src/test/regress/sql/aggregates.sql
+++ b/src/test/regress/sql/aggregates.sql
@@ -1233,6 +1233,18 @@ SELECT array_agg(c1 ORDER BY c2),c2
FROM t1 WHERE c2 < 100 GROUP BY c1 ORDER BY 2;
DROP TABLE t1 CASCADE;
+-- Check, that GROUP-BY reordering optimization can operate with pathkeys,
built
+-- by planner itself. For example, by MergeJoin.
+SET enable_hashjoin = off;
+SET enable_nestloop = off;
+explain (COSTS OFF)
+SELECT c1.relname,c1.relpages
+FROM pg_class c1 JOIN pg_class c2 ON (c1.relname=c2.relname AND
c1.relpages=c2.relpages)
+GROUP BY c1.reltuples,c1.relpages,c1.relname
+ORDER BY c1.relpages, c1.relname, c1.relpages*c1.relpages;
+RESET enable_hashjoin;
+RESET enable_nestloop;
+
RESET enable_hashagg;
RESET max_parallel_workers;
RESET max_parallel_workers_per_gather;
