From: Andrei Lepikhov <lepihov@gmail.com>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Junwang Zhao <zhjwpku@gmail.com>
Cc: pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
Cc: Tomas Vondra <tomas@vondra.me>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Subject: Re: Incorrect calculation of path fraction value in MergeAppend
Date: Wed, 7 May 2025 20:42:16 +0200
Message-ID: <3c83c594-6beb-414b-8059-f57e9a9eb0d7@gmail.com> (raw)
In-Reply-To: <CAEG8a3K3CgS_cK6jQHjmxsJpMwyTx3r3eLEC+8b2FACFWv_fjg@mail.gmail.com>
References: <3ca271fa-ca5c-458c-8934-eb148622b270@gmail.com>
<CAEG8a3K3CgS_cK6jQHjmxsJpMwyTx3r3eLEC+8b2FACFWv_fjg@mail.gmail.com>
On 7/5/2025 12:45, Álvaro Herrera wrote:
> On 2025-May-07, Andrei Lepikhov wrote:
>> The magic is how it finds a link to the thread. This time it doesn't
> Weird, that doesn't seem to work very well if you try to search for
> stuff. But if you just specify the Message-Id without clicking
> "search" then it works. So here you go:
> https://commitfest.postgresql.org/patch/5742/
Thanks! I think this memo should be pinned at the top of the page.
On 7/5/2025 17:35, Junwang Zhao wrote:
> + /* Convert absolute limit to a path fraction */
> + if (path_fraction >= 1.)
> + path_fraction /= childrel->rows;
>
> maybe add `&& childrel->rows > 0.` to the if condition to
> avoid potential crash. I'm not sure if `childrel->rows` will
> be 0 but I see get_cheapest_fractional_path did this.
You may look at the 76281aa for the reason. Also, looking into the code,
I see that it is impossible now to find more places with rows == 0
except for the Dummy result node. Moreover, it just doesn't make sense
in the case of append nodes: why should the optimiser spend resources
and consider paths if it is impossible to pull any tuples from the subplan?
So, I think checking assertion instead would be the better option. See
new version in the attachment.
--
regards, Andrei Lepikhov
From 2bc5f5348283c6f2535d0da9d2bbfd4942f87736 Mon Sep 17 00:00:00 2001
From: "Andrei V. Lepikhov" <lepihov@gmail.com>
Date: Tue, 29 Apr 2025 12:54:59 +0200
Subject: [PATCH v1] Fix tuple_fraction calculation for merge append fractional
path
---
src/backend/optimizer/path/allpaths.c | 9 ++++++++-
src/test/regress/expected/partition_join.out | 18 ++++++++++++++++++
src/test/regress/sql/partition_join.sql | 3 +++
3 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.cindex 905250b3325..63730695375 100644--- a/src/backend/optimizer/path/allpaths.c+++ b/src/backend/optimizer/path/allpaths.c@@ -1891,7 +1891,14 @@ generate_orderedappend_paths(PlannerInfo *root, RelOptInfo *rel,
*/
if (root->tuple_fraction > 0)
{
- double path_fraction = (1.0 / root->tuple_fraction);+ double path_fraction = root->tuple_fraction;++ /* Merge Append considers only live childrels */+ Assert(childrel->rows > 0);++ /* Convert absolute limit to a path fraction */+ if (path_fraction >= 1.0)+ path_fraction /= childrel->rows;
cheapest_fractional =
get_cheapest_fractional_path_for_pathkeys(childrel->pathlist,
diff --git a/src/test/regress/expected/partition_join.out b/src/test/regress/expected/partition_join.outindex 6101c8c7cf1..d5368186caa 100644--- a/src/test/regress/expected/partition_join.out+++ b/src/test/regress/expected/partition_join.out@@ -5260,6 +5260,24 @@ SELECT x.id, y.id FROM fract_t x LEFT JOIN fract_t y USING (id) ORDER BY x.id DE
Index Cond: (id = x_2.id)
(11 rows)
+EXPLAIN (COSTS OFF) -- Should use NestLoop with parameterised inner scan+SELECT x.id, y.id FROM fract_t x LEFT JOIN fract_t y USING (id)+ORDER BY x.id DESC LIMIT 2;+ QUERY PLAN +--------------------------------------------------------------------------------+ Limit+ -> Merge Append+ Sort Key: x.id DESC+ -> Nested Loop Left Join+ -> Index Only Scan Backward using fract_t0_pkey on fract_t0 x_1+ -> Index Only Scan using fract_t0_pkey on fract_t0 y_1+ Index Cond: (id = x_1.id)+ -> Nested Loop Left Join+ -> Index Only Scan Backward using fract_t1_pkey on fract_t1 x_2+ -> Index Only Scan using fract_t1_pkey on fract_t1 y_2+ Index Cond: (id = x_2.id)+(11 rows)+
--
-- Test Append's fractional paths
--
diff --git a/src/test/regress/sql/partition_join.sql b/src/test/regress/sql/partition_join.sqlindex b76c5451001..30f15ee9acb 100644--- a/src/test/regress/sql/partition_join.sql+++ b/src/test/regress/sql/partition_join.sql@@ -1224,6 +1224,9 @@ SELECT x.id, y.id FROM fract_t x LEFT JOIN fract_t y USING (id) ORDER BY x.id AS
EXPLAIN (COSTS OFF)
SELECT x.id, y.id FROM fract_t x LEFT JOIN fract_t y USING (id) ORDER BY x.id DESC LIMIT 10;
+EXPLAIN (COSTS OFF) -- Should use NestLoop with parameterised inner scan+SELECT x.id, y.id FROM fract_t x LEFT JOIN fract_t y USING (id)+ORDER BY x.id DESC LIMIT 2;
--
-- Test Append's fractional paths
--
2.49.0
Attachments:
[text/plain] v1-0001-Fix-tuple_fraction-calculation-for-merge-append-f.patch (3.2K, ../3c83c594-6beb-414b-8059-f57e9a9eb0d7@gmail.com/2-v1-0001-Fix-tuple_fraction-calculation-for-merge-append-f.patch)
download | inline diff:
From 2bc5f5348283c6f2535d0da9d2bbfd4942f87736 Mon Sep 17 00:00:00 2001
From: "Andrei V. Lepikhov" <lepihov@gmail.com>
Date: Tue, 29 Apr 2025 12:54:59 +0200
Subject: [PATCH v1] Fix tuple_fraction calculation for merge append fractional
path
---
src/backend/optimizer/path/allpaths.c | 9 ++++++++-
src/test/regress/expected/partition_join.out | 18 ++++++++++++++++++
src/test/regress/sql/partition_join.sql | 3 +++
3 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.cindex 905250b3325..63730695375 100644--- a/src/backend/optimizer/path/allpaths.c+++ b/src/backend/optimizer/path/allpaths.c@@ -1891,7 +1891,14 @@ generate_orderedappend_paths(PlannerInfo *root, RelOptInfo *rel,
*/
if (root->tuple_fraction > 0)
{
- double path_fraction = (1.0 / root->tuple_fraction);+ double path_fraction = root->tuple_fraction;++ /* Merge Append considers only live childrels */+ Assert(childrel->rows > 0);++ /* Convert absolute limit to a path fraction */+ if (path_fraction >= 1.0)+ path_fraction /= childrel->rows;
cheapest_fractional =
get_cheapest_fractional_path_for_pathkeys(childrel->pathlist,
diff --git a/src/test/regress/expected/partition_join.out b/src/test/regress/expected/partition_join.outindex 6101c8c7cf1..d5368186caa 100644--- a/src/test/regress/expected/partition_join.out+++ b/src/test/regress/expected/partition_join.out@@ -5260,6 +5260,24 @@ SELECT x.id, y.id FROM fract_t x LEFT JOIN fract_t y USING (id) ORDER BY x.id DE
Index Cond: (id = x_2.id)
(11 rows)
+EXPLAIN (COSTS OFF) -- Should use NestLoop with parameterised inner scan+SELECT x.id, y.id FROM fract_t x LEFT JOIN fract_t y USING (id)+ORDER BY x.id DESC LIMIT 2;+ QUERY PLAN +--------------------------------------------------------------------------------+ Limit+ -> Merge Append+ Sort Key: x.id DESC+ -> Nested Loop Left Join+ -> Index Only Scan Backward using fract_t0_pkey on fract_t0 x_1+ -> Index Only Scan using fract_t0_pkey on fract_t0 y_1+ Index Cond: (id = x_1.id)+ -> Nested Loop Left Join+ -> Index Only Scan Backward using fract_t1_pkey on fract_t1 x_2+ -> Index Only Scan using fract_t1_pkey on fract_t1 y_2+ Index Cond: (id = x_2.id)+(11 rows)+
--
-- Test Append's fractional paths
--
diff --git a/src/test/regress/sql/partition_join.sql b/src/test/regress/sql/partition_join.sqlindex b76c5451001..30f15ee9acb 100644--- a/src/test/regress/sql/partition_join.sql+++ b/src/test/regress/sql/partition_join.sql@@ -1224,6 +1224,9 @@ SELECT x.id, y.id FROM fract_t x LEFT JOIN fract_t y USING (id) ORDER BY x.id AS
EXPLAIN (COSTS OFF)
SELECT x.id, y.id FROM fract_t x LEFT JOIN fract_t y USING (id) ORDER BY x.id DESC LIMIT 10;
+EXPLAIN (COSTS OFF) -- Should use NestLoop with parameterised inner scan+SELECT x.id, y.id FROM fract_t x LEFT JOIN fract_t y USING (id)+ORDER BY x.id DESC LIMIT 2;
--
-- Test Append's fractional paths
--
2.49.0
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-bugs@postgresql.org
Cc: lepihov@gmail.com, alvherre@alvh.no-ip.org, zhjwpku@gmail.com, pgsql-bugs@lists.postgresql.org, tomas@vondra.me, tgl@sss.pgh.pa.us
Subject: Re: Incorrect calculation of path fraction value in MergeAppend
In-Reply-To: <3c83c594-6beb-414b-8059-f57e9a9eb0d7@gmail.com>
* 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