pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedIncorrect calculation of path fraction value in MergeAppend
8+ messages / 5 participants
[nested] [flat]
* Incorrect calculation of path fraction value in MergeAppend
@ 2025-04-29 11:14 Andrei Lepikhov <lepihov@gmail.com>
0 siblings, 1 reply; 8+ messages in thread
From: Andrei Lepikhov @ 2025-04-29 11:14 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>; Tomas Vondra <tomas@vondra.me>
Hi,
Commit 6b94e7a has added a "fractional" strategy to the merge join path
search scope. But as I see it incorrectly calculates tuple_fraction
value. Let's see one of the regression tests' queries:
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;
It uses NestedLoop with parameterised inner scan as a subplan of
MergeAppend. Overall cost is:
Limit (cost=1.11..4.86 rows=10 width=16)
Let's reduce the LIMIT a little:
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 2;
There, the query plan uses plain NestLoop node and overall cost
significantly increases:
Limit (cost=0.56..30.70 rows=2 width=16)
The origin problem lies in the following code line:
path_fraction = (1.0 / root->tuple_fraction);
Building startup, total and fractional MergeAppends, the optimiser puts
off the 'fractional' way because it has built for a 50% limit (uses
MergeJoin with high startup cost) and chooses a startup-optimal path
(built upon plain NestLoops) that is better than the total one.
Instead, the path_fraction value must be corrected when
root->tuple_fraction contains the absolute value of tuples. See the
patch in the attachment.
I report this issue into the bugs mailing list because it might cause
some 'enigmatic' performance slowdowns I have heard about (but have no
proofs) and may need back-patching.
--
regards, Andrei Lepikhov
Attachments:
[text/x-patch] v0-0001-Fix-tuple_fraction-calculation-for-merge-append-f.patch (3.1K, ../../3ca271fa-ca5c-458c-8934-eb148622b270@gmail.com/2-v0-0001-Fix-tuple_fraction-calculation-for-merge-append-f.patch)
download | inline diff:
From 5abcae937f3b9e9cc5ea10721ce0d0a61f917e84 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 v0] Fix tuple_fraction calculation for merge append fractional
path
---
src/backend/optimizer/path/allpaths.c | 6 +++++-
src/test/regress/expected/partition_join.out | 18 ++++++++++++++++++
src/test/regress/sql/partition_join.sql | 3 +++
3 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 905250b3325..543550a9378 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -1891,7 +1891,11 @@ 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;
+
+ /* Convert absolute limit to a path fraction */
+ if (path_fraction >= 1.)
+ 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.out
index 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.sql
index 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.39.5
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Incorrect calculation of path fraction value in MergeAppend
@ 2025-05-07 15:35 Junwang Zhao <zhjwpku@gmail.com>
parent: Andrei Lepikhov <lepihov@gmail.com>
0 siblings, 1 reply; 8+ messages in thread
From: Junwang Zhao @ 2025-05-07 15:35 UTC (permalink / raw)
To: Andrei Lepikhov <lepihov@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>; Tomas Vondra <tomas@vondra.me>
Hi Andrei,
On Tue, Apr 29, 2025 at 7:14 PM Andrei Lepikhov <lepihov@gmail.com> wrote:
>
> Hi,
>
> Commit 6b94e7a has added a "fractional" strategy to the merge join path
> search scope. But as I see it incorrectly calculates tuple_fraction
> value. Let's see one of the regression tests' queries:
>
> 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;
>
> It uses NestedLoop with parameterised inner scan as a subplan of
> MergeAppend. Overall cost is:
> Limit (cost=1.11..4.86 rows=10 width=16)
>
> Let's reduce the LIMIT a little:
>
> 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 2;
>
> There, the query plan uses plain NestLoop node and overall cost
> significantly increases:
> Limit (cost=0.56..30.70 rows=2 width=16)
>
> The origin problem lies in the following code line:
> path_fraction = (1.0 / root->tuple_fraction);
>
> Building startup, total and fractional MergeAppends, the optimiser puts
> off the 'fractional' way because it has built for a 50% limit (uses
> MergeJoin with high startup cost) and chooses a startup-optimal path
> (built upon plain NestLoops) that is better than the total one.
>
> Instead, the path_fraction value must be corrected when
> root->tuple_fraction contains the absolute value of tuples. See the
> patch in the attachment.
>
> I report this issue into the bugs mailing list because it might cause
> some 'enigmatic' performance slowdowns I have heard about (but have no
> proofs) and may need back-patching.
>
> --
> regards, Andrei Lepikhov
I've apply the patch and the tests passed, one small nitpick:
+ double path_fraction = root->tuple_fraction;
+
+ /* 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.
/* Convert absolute # of tuples to a fraction; no need to clamp to 0..1 */
if (tuple_fraction >= 1.0 && best_path->rows > 0)
tuple_fraction /= best_path->rows;
--
Regards
Junwang Zhao
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Incorrect calculation of path fraction value in MergeAppend
@ 2025-05-07 18:42 Andrei Lepikhov <lepihov@gmail.com>
parent: Junwang Zhao <zhjwpku@gmail.com>
0 siblings, 1 reply; 8+ messages in thread
From: Andrei Lepikhov @ 2025-05-07 18:42 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; Junwang Zhao <zhjwpku@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>; Tomas Vondra <tomas@vondra.me>; Tom Lane <tgl@sss.pgh.pa.us>
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.c
index 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.out
index 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.sql
index 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.c
index 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.out
index 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.sql
index 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
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Incorrect calculation of path fraction value in MergeAppend
@ 2025-05-07 23:58 Junwang Zhao <zhjwpku@gmail.com>
parent: Andrei Lepikhov <lepihov@gmail.com>
0 siblings, 1 reply; 8+ messages in thread
From: Junwang Zhao @ 2025-05-07 23:58 UTC (permalink / raw)
To: Andrei Lepikhov <lepihov@gmail.com>; +Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>; Tomas Vondra <tomas@vondra.me>; Tom Lane <tgl@sss.pgh.pa.us>
On Thu, May 8, 2025 at 2:42 AM Andrei Lepikhov <lepihov@gmail.com> wrote:
>
> 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.
Agree, assertion is better, the new version LGTM.
>
> --
> regards, Andrei Lepikhov
--
Regards
Junwang Zhao
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Incorrect calculation of path fraction value in MergeAppend
@ 2025-05-18 11:03 Alexander Korotkov <aekorotkov@gmail.com>
parent: Junwang Zhao <zhjwpku@gmail.com>
0 siblings, 1 reply; 8+ messages in thread
From: Alexander Korotkov @ 2025-05-18 11:03 UTC (permalink / raw)
To: Junwang Zhao <zhjwpku@gmail.com>; +Cc: Andrei Lepikhov <lepihov@gmail.com>; Alvaro Herrera <alvherre@alvh.no-ip.org>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>; Tomas Vondra <tomas@vondra.me>; Tom Lane <tgl@sss.pgh.pa.us>
On Thu, May 8, 2025 at 2:58 AM Junwang Zhao <zhjwpku@gmail.com> wrote:
>
> On Thu, May 8, 2025 at 2:42 AM Andrei Lepikhov <lepihov@gmail.com> wrote:
> >
> > 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.
>
> Agree, assertion is better, the new version LGTM.
I've slightly revised the patch: commit message and one comment. I
think this is clearly a bugfix, and it should be backpatched. I'm
going to push and backpatch it if no objections.
------
Regards,
Alexander Korotkov
Supabase
Attachments:
[application/octet-stream] v2-0001-Fix-tuple_fraction-calculation-in-generate_ordere.patch (3.9K, ../../CAPpHfduJYDs7SNgkmuWLR=EU=X1zuxxXTxX8zBeKXZ2WLoSfWA@mail.gmail.com/2-v2-0001-Fix-tuple_fraction-calculation-in-generate_ordere.patch)
download | inline diff:
From c26e5ac62d9e4f8a967d1e76f54dae66b037941a Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <akorotkov@postgresql.org>
Date: Sun, 18 May 2025 14:01:21 +0300
Subject: [PATCH v2] Fix tuple_fraction calculation in
generate_orderedappend_paths()
6b94e7a6da adjusted generate_orderedappend_paths() to consider fractional
paths. However, it didn't manage to interpret the tuple_fraction value
correctly. According to the header comment of grouping_planner(), the
tuple_fraction >= 1 specifies the absolute number of expected tuples. That
number must be divided by the expected total number of tuples to get the
actual fraction.
Reported-by: Andrei Lepikhov <lepihov@gmail.com>
Discussion: https://postgr.es/m/3ca271fa-ca5c-458c-8934-eb148622b270%40gmail.com
Author: Andrei Lepikhov <lepihov@gmail.com>
Reviewed-by: Junwang Zhao <zhjwpku@gmail.com>
Backpatch-through: 15
---
src/backend/optimizer/path/allpaths.c | 12 +++++++++++-
src/test/regress/expected/partition_join.out | 18 ++++++++++++++++++
src/test/regress/sql/partition_join.sql | 3 +++
3 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 905250b3325..6cc6966b060 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -1891,7 +1891,17 @@ 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 children relations. Dummy
+ * relations must be filtered out before.
+ */
+ 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.out
index 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.sql
index 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.39.5 (Apple Git-154)
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Incorrect calculation of path fraction value in MergeAppend
@ 2025-05-18 12:23 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: Alexander Korotkov <aekorotkov@gmail.com>
0 siblings, 1 reply; 8+ messages in thread
From: Alvaro Herrera @ 2025-05-18 12:23 UTC (permalink / raw)
To: Alexander Korotkov <aekorotkov@gmail.com>; +Cc: Junwang Zhao <zhjwpku@gmail.com>; Andrei Lepikhov <lepihov@gmail.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>; Tomas Vondra <tomas@vondra.me>; Tom Lane <tgl@sss.pgh.pa.us>
On 2025-May-18, Alexander Korotkov wrote:
> I've slightly revised the patch: commit message and one comment. I
> think this is clearly a bugfix, and it should be backpatched. I'm
> going to push and backpatch it if no objections.
I think we mostly avoid backpatching changes that can cause significant
planning changes, because the consequences for existing systems could be
catastrophic -- even if the known plan changes are beneficial. IMO you
need to spend a lot more effort in showing that this can't possibly harm
anything, before backpatching.
Personally I can see getting this in 18beta2 without too much additional
arguing for it. But for 15-17 you'd need a lot more support than is
evident in this thread.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Incorrect calculation of path fraction value in MergeAppend
@ 2025-05-18 14:28 Tom Lane <tgl@sss.pgh.pa.us>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 1 reply; 8+ messages in thread
From: Tom Lane @ 2025-05-18 14:28 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: Alexander Korotkov <aekorotkov@gmail.com>; Junwang Zhao <zhjwpku@gmail.com>; Andrei Lepikhov <lepihov@gmail.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>; Tomas Vondra <tomas@vondra.me>
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Personally I can see getting this in 18beta2 without too much additional
> arguing for it. But for 15-17 you'd need a lot more support than is
> evident in this thread.
+1. The bar for causing plan changes in back branches is very high.
Unless you can show that the buggy behavior is causing wrong answers,
it's usually best to leave it alone.
regards, tom lane
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Incorrect calculation of path fraction value in MergeAppend
@ 2025-05-18 20:47 Alexander Korotkov <aekorotkov@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 8+ messages in thread
From: Alexander Korotkov @ 2025-05-18 20:47 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>; Junwang Zhao <zhjwpku@gmail.com>; Andrei Lepikhov <lepihov@gmail.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>; Tomas Vondra <tomas@vondra.me>
On Sun, May 18, 2025 at 5:29 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > Personally I can see getting this in 18beta2 without too much additional
> > arguing for it. But for 15-17 you'd need a lot more support than is
> > evident in this thread.
>
> +1. The bar for causing plan changes in back branches is very high.
> Unless you can show that the buggy behavior is causing wrong answers,
> it's usually best to leave it alone.
Makes sense. Thank you for the clarification. I'll push this to the
master only.
------
Regards,
Alexander Korotkov
Supabase
^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2025-05-18 20:47 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-04-29 11:14 Incorrect calculation of path fraction value in MergeAppend Andrei Lepikhov <lepihov@gmail.com>
2025-05-07 15:35 ` Junwang Zhao <zhjwpku@gmail.com>
2025-05-07 18:42 ` Andrei Lepikhov <lepihov@gmail.com>
2025-05-07 23:58 ` Junwang Zhao <zhjwpku@gmail.com>
2025-05-18 11:03 ` Alexander Korotkov <aekorotkov@gmail.com>
2025-05-18 12:23 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2025-05-18 14:28 ` Tom Lane <tgl@sss.pgh.pa.us>
2025-05-18 20:47 ` Alexander Korotkov <aekorotkov@gmail.com>
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