pg.ddx.io  pgsql-bugs@postgresql.org mailing list archive  
help / color / mirror / Atom feed
BUG #19102: Assertion failure in generate_orderedappend_paths with aggregate pushdown
13+ messages / 5 participants
[nested] [flat]

* BUG #19102: Assertion failure in generate_orderedappend_paths with aggregate pushdown
@ 2025-11-03 05:51  PG Bug reporting form <noreply@postgresql.org>
  0 siblings, 1 reply; 13+ messages in thread

From: PG Bug reporting form @ 2025-11-03 05:51 UTC (permalink / raw)
  To: pgsql-bugs@lists.postgresql.org; +Cc: kuntalghosh.2007@gmail.com

The following bug has been logged on the website:

Bug reference:      19102
Logged by:          Kuntal Ghosh
Email address:      kuntalghosh.2007@gmail.com
PostgreSQL version: 18.0
Operating system:   aarch64 GNU/Linux
Description:        

Hi,

The assertion "Assert(childrel->rows > 0)" in generate_orderedappend_paths()
fails when processing queries where aggregates are pushed down below merge
append. In such cases, childrel can be an upperrel (grouping rel) rather
than a base relation, and will have zero row-estimates.

Steps to reproduce the issue:

CREATE TABLE pagg_tab2 (id BIGINT, PRIMARY KEY (id)) PARTITION BY RANGE
(id);
CREATE TABLE pagg_tab2_0 PARTITION OF pagg_tab2 FOR VALUES FROM ('0') TO
('1000');
CREATE TABLE pagg_tab2_1 PARTITION OF pagg_tab2 FOR VALUES FROM ('1000') TO
('2000');

SET enable_partitionwise_aggregate = true;

EXPLAIN (COSTS OFF)  SELECT count(*)  FROM pagg_tab2 x  GROUP BY x.id  ORDER
BY x.id DESC  LIMIT 2;

Server crashes with assertion failure:
TRAP: FailedAssertion("childrel->rows > 0", File: "allpaths.c", Line: 1983)

To fix the issue, we can replace the direct division with
clamp_row_est(childrel->rows) to safely handle zero, and remove the
incorrect assertion:

-   Assert(childrel->rows > 0);
-   path_fraction /= childrel->rows;
+   path_fraction /= clamp_row_est(childrel->rows);

Thanks & regards,
Kuntal Ghosh



^ permalink  raw  reply  [nested|flat] 13+ messages in thread

* Re: BUG #19102: Assertion failure in generate_orderedappend_paths with aggregate pushdown
@ 2025-11-03 05:57  Kuntal Ghosh <kuntalghosh.2007@gmail.com>
  parent: PG Bug reporting form <noreply@postgresql.org>
  0 siblings, 1 reply; 13+ messages in thread

From: Kuntal Ghosh @ 2025-11-03 05:57 UTC (permalink / raw)
  To: kuntalghosh.2007@gmail.com; pgsql-bugs@lists.postgresql.org; +Cc: akorotkov@postgresql.org

On Mon, Nov 3, 2025 at 11:22 AM PG Bug reporting form <
noreply@postgresql.org> wrote:

>
> Server crashes with assertion failure:
> TRAP: FailedAssertion("childrel->rows > 0", File: "allpaths.c", Line: 1983)
>
> To fix the issue, we can replace the direct division with
> clamp_row_est(childrel->rows) to safely handle zero, and remove the
> incorrect assertion:
>
> -   Assert(childrel->rows > 0);
> -   path_fraction /= childrel->rows;
> +   path_fraction /= clamp_row_est(childrel->rows);
>
> Added a patch with the proposed fix and regression test.

-- 
Thanks & Regards,
Kuntal Ghosh

Attachments:

  [application/octet-stream] 0001-Remove-incorrect-assertion-for-childrel-rows-in-orde.patch (4.0K, ../../CAGz5QCKuD54qPVnQAUXEoQzWmpVNty8NNoMNnKzrSE3qBRCJDw@mail.gmail.com/3-0001-Remove-incorrect-assertion-for-childrel-rows-in-orde.patch)
  download | inline diff:
From bb8ecc7e7bac169d0cdc5e4b2bb8a18665b4b936 Mon Sep 17 00:00:00 2001
From: Kuntal Ghosh <kuntalgh@amazon.com>
Date: Mon, 3 Nov 2025 04:45:01 +0000
Subject: [PATCH] Remove incorrect assertion for childrel rows in ordered
 append paths

iThe assertion that childrel->rows > 0 is incorrect when aggregates are
pushed down below merge append, as childrel may be an upperrel in such
cases. Use clamp_row_est() to safely handle zero or near-zero row
estimates when calculating path fractions.
---
 src/backend/optimizer/path/allpaths.c            |  8 +-------
 .../regress/expected/partition_aggregate.out     | 16 ++++++++++++++++
 src/test/regress/sql/partition_aggregate.sql     |  9 +++++++++
 3 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 9c6436eb72f..f9eff4d45c6 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -1976,15 +1976,9 @@ generate_orderedappend_paths(PlannerInfo *root, RelOptInfo *rel,
 			{
 				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;
+					path_fraction /= clamp_row_est(childrel->rows);
 
 				cheapest_fractional =
 					get_cheapest_fractional_path_for_pathkeys(childrel->pathlist,
diff --git a/src/test/regress/expected/partition_aggregate.out b/src/test/regress/expected/partition_aggregate.out
index fc84929a002..992d016407f 100644
--- a/src/test/regress/expected/partition_aggregate.out
+++ b/src/test/regress/expected/partition_aggregate.out
@@ -930,6 +930,22 @@ SELECT a, c, sum(b), avg(c), count(*) FROM pagg_tab_m GROUP BY (a+b)/2, 2, 1 HAV
  20 | 40 |  50 | 40.0000000000000000 |     5
 (6 rows)
 
+-- Test with relation having interesting order
+CREATE TABLE pagg_tab3 (id BIGINT, PRIMARY KEY (id)) PARTITION BY RANGE (id);
+CREATE TABLE pagg_tab3_0 PARTITION OF pagg_tab3 FOR VALUES FROM ('0') TO ('1000');
+CREATE TABLE pagg_tab3_1 PARTITION OF pagg_tab3 FOR VALUES FROM ('1000') TO ('2000');
+SET enable_partitionwise_aggregate = true;
+EXPLAIN (COSTS OFF)  SELECT count(*)  FROM pagg_tab3 x  GROUP BY x.id  ORDER BY x.id DESC  LIMIT 2;
+                                      QUERY PLAN                                      
+--------------------------------------------------------------------------------------
+ Limit
+   ->  GroupAggregate
+         Group Key: x.id
+         ->  Append
+               ->  Index Only Scan Backward using pagg_tab3_1_pkey on pagg_tab3_1 x_2
+               ->  Index Only Scan Backward using pagg_tab3_0_pkey on pagg_tab3_0 x_1
+(6 rows)
+
 -- Test with multi-level partitioning scheme
 CREATE TABLE pagg_tab_ml (a int, b int, c text) PARTITION BY RANGE(a);
 CREATE TABLE pagg_tab_ml_p1 PARTITION OF pagg_tab_ml FOR VALUES FROM (0) TO (12);
diff --git a/src/test/regress/sql/partition_aggregate.sql b/src/test/regress/sql/partition_aggregate.sql
index 124cc260461..31030c9b76f 100644
--- a/src/test/regress/sql/partition_aggregate.sql
+++ b/src/test/regress/sql/partition_aggregate.sql
@@ -202,6 +202,15 @@ EXPLAIN (COSTS OFF)
 SELECT a, c, sum(b), avg(c), count(*) FROM pagg_tab_m GROUP BY (a+b)/2, 2, 1 HAVING sum(b) = 50 AND avg(c) > 25 ORDER BY 1, 2, 3;
 SELECT a, c, sum(b), avg(c), count(*) FROM pagg_tab_m GROUP BY (a+b)/2, 2, 1 HAVING sum(b) = 50 AND avg(c) > 25 ORDER BY 1, 2, 3;
 
+-- Test with relation having interesting order
+CREATE TABLE pagg_tab3 (id BIGINT, PRIMARY KEY (id)) PARTITION BY RANGE (id);
+CREATE TABLE pagg_tab3_0 PARTITION OF pagg_tab3 FOR VALUES FROM ('0') TO ('1000');
+CREATE TABLE pagg_tab3_1 PARTITION OF pagg_tab3 FOR VALUES FROM ('1000') TO ('2000');
+
+SET enable_partitionwise_aggregate = true;
+
+EXPLAIN (COSTS OFF)  SELECT count(*)  FROM pagg_tab3 x  GROUP BY x.id  ORDER BY x.id DESC  LIMIT 2;
+
 
 -- Test with multi-level partitioning scheme
 
-- 
2.47.3



^ permalink  raw  reply  [nested|flat] 13+ messages in thread

* Re: BUG #19102: Assertion failure in generate_orderedappend_paths with aggregate pushdown
@ 2025-11-03 10:09  Richard Guo <guofenglinux@gmail.com>
  parent: Kuntal Ghosh <kuntalghosh.2007@gmail.com>
  0 siblings, 1 reply; 13+ messages in thread

From: Richard Guo @ 2025-11-03 10:09 UTC (permalink / raw)
  To: Kuntal Ghosh <kuntalghosh.2007@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org; akorotkov@postgresql.org

On Mon, Nov 3, 2025 at 2:58 PM Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote:
> On Mon, Nov 3, 2025 at 11:22 AM PG Bug reporting form <noreply@postgresql.org> wrote:
>> Server crashes with assertion failure:
>> TRAP: FailedAssertion("childrel->rows > 0", File: "allpaths.c", Line: 1983)
>>
>> To fix the issue, we can replace the direct division with
>> clamp_row_est(childrel->rows) to safely handle zero, and remove the
>> incorrect assertion:
>>
>> -   Assert(childrel->rows > 0);
>> -   path_fraction /= childrel->rows;
>> +   path_fraction /= clamp_row_est(childrel->rows);
>>
> Added a patch with the proposed fix and regression test.

Thanks for the report -- that's a good catch.  However, I don't think
we should use childrel->rows to calculate the fraction in the first
place.  It would be better to use cheapest_total->rows instead.

- Richard





^ permalink  raw  reply  [nested|flat] 13+ messages in thread

* Re: BUG #19102: Assertion failure in generate_orderedappend_paths with aggregate pushdown
@ 2025-11-03 13:09  Richard Guo <guofenglinux@gmail.com>
  parent: Richard Guo <guofenglinux@gmail.com>
  0 siblings, 1 reply; 13+ messages in thread

From: Richard Guo @ 2025-11-03 13:09 UTC (permalink / raw)
  To: Kuntal Ghosh <kuntalghosh.2007@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org; akorotkov@postgresql.org

On Mon, Nov 3, 2025 at 7:09 PM Richard Guo <guofenglinux@gmail.com> wrote:
> Thanks for the report -- that's a good catch.  However, I don't think
> we should use childrel->rows to calculate the fraction in the first
> place.  It would be better to use cheapest_total->rows instead.

Reading generate_orderedappend_paths(), I noticed a couple of other
issues:

1. This function considers the cheapest-fractional case in addition to
the cheapest-startup and cheapest-total cases, but the function's
comment does not mention this.

 * We consider both cheapest-startup and cheapest-total cases, ie, for each
 * interesting ordering, collect all the cheapest startup subpaths and all the
 * cheapest total paths, and build a suitable path for each case.

2. The function does not handle the case where the paths in
total_subpaths and fractional_subpaths are identical.  This is not a
rare scenario, and as a result, it could create two exactly identical
ordered append paths.

3. The comment for startup_neq_total is also out of date, since we may
call create_append_path(), not just create_merge_append_path(), to
generate ordered append paths.

Here is a WIP patch to address these issues.

- Richard

Attachments:

  [application/octet-stream] v1-0001-Fix-generate_orderedappend_paths.patch (4.0K, ../../CAMbWs4-OYsgA75tGGiBARt87G0y_z_GBTSLrzudcJxAzndYkYw@mail.gmail.com/2-v1-0001-Fix-generate_orderedappend_paths.patch)
  download | inline diff:
From 7e7648759078ec4a7b024e5ed29771c45e6d1b9f Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Mon, 3 Nov 2025 21:43:28 +0900
Subject: [PATCH v1] Fix generate_orderedappend_paths()

---
 src/backend/optimizer/path/allpaths.c | 29 ++++++++++++++++++---------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 9c6436eb72f..ad813f3c6d2 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -1810,9 +1810,11 @@ add_paths_to_append_rel(PlannerInfo *root, RelOptInfo *rel,
  * We generate a path for each ordering (pathkey list) appearing in
  * all_child_pathkeys.
  *
- * We consider both cheapest-startup and cheapest-total cases, ie, for each
- * interesting ordering, collect all the cheapest startup subpaths and all the
- * cheapest total paths, and build a suitable path for each case.
+ * We consider the cheapest-startup and cheapest-total cases, and also the
+ * cheapest-fractional case when not all tuples need to be retrieved.  For each
+ * interesting ordering, we collect all the cheapest startup subpaths, all the
+ * cheapest total paths, and, if applicable, all the cheapest fractional paths,
+ * and build a suitable path for each case.
  *
  * We don't currently generate any parameterized ordered paths here.  While
  * it would not take much more code here to do so, it's very unclear that it
@@ -1875,6 +1877,7 @@ generate_orderedappend_paths(PlannerInfo *root, RelOptInfo *rel,
 		List	   *total_subpaths = NIL;
 		List	   *fractional_subpaths = NIL;
 		bool		startup_neq_total = false;
+		bool		fraction_neq_total = false;
 		bool		match_partition_order;
 		bool		match_partition_order_desc;
 		int			end_index;
@@ -1980,11 +1983,11 @@ generate_orderedappend_paths(PlannerInfo *root, RelOptInfo *rel,
 				 * Merge Append considers only live children relations.  Dummy
 				 * relations must be filtered out before.
 				 */
-				Assert(childrel->rows > 0);
+				Assert(cheapest_total->rows > 0);
 
 				/* Convert absolute limit to a path fraction */
 				if (path_fraction >= 1.0)
-					path_fraction /= childrel->rows;
+					path_fraction /= cheapest_total->rows;
 
 				cheapest_fractional =
 					get_cheapest_fractional_path_for_pathkeys(childrel->pathlist,
@@ -1999,15 +2002,21 @@ generate_orderedappend_paths(PlannerInfo *root, RelOptInfo *rel,
 				 * XXX We might consider partially sorted paths too (with an
 				 * incremental sort on top). But we'd have to build all the
 				 * incremental paths, do the costing etc.
+				 *
+				 * Also, notice whether we actually have different paths for
+				 * the "fractional" and "total" cases.  This helps avoid
+				 * generating two identical ordered append paths.
 				 */
-				if (!cheapest_fractional)
+				if (cheapest_fractional == NULL)
 					cheapest_fractional = cheapest_total;
+				else if (cheapest_fractional != cheapest_total)
+					fraction_neq_total = true;
 			}
 
 			/*
 			 * Notice whether we actually have different paths for the
-			 * "cheapest" and "total" cases; frequently there will be no point
-			 * in two create_merge_append_path() calls.
+			 * "cheapest" and "total" cases.  This helps avoid generating two
+			 * identical ordered append paths.
 			 */
 			if (cheapest_startup != cheapest_total)
 				startup_neq_total = true;
@@ -2078,7 +2087,7 @@ generate_orderedappend_paths(PlannerInfo *root, RelOptInfo *rel,
 														  false,
 														  -1));
 
-			if (fractional_subpaths)
+			if (fractional_subpaths && fraction_neq_total)
 				add_path(rel, (Path *) create_append_path(root,
 														  rel,
 														  fractional_subpaths,
@@ -2104,7 +2113,7 @@ generate_orderedappend_paths(PlannerInfo *root, RelOptInfo *rel,
 																pathkeys,
 																NULL));
 
-			if (fractional_subpaths)
+			if (fractional_subpaths && fraction_neq_total)
 				add_path(rel, (Path *) create_merge_append_path(root,
 																rel,
 																fractional_subpaths,
-- 
2.39.5 (Apple Git-154)



^ permalink  raw  reply  [nested|flat] 13+ messages in thread

* Re: BUG #19102: Assertion failure in generate_orderedappend_paths with aggregate pushdown
@ 2025-11-04 07:44  Richard Guo <guofenglinux@gmail.com>
  parent: Richard Guo <guofenglinux@gmail.com>
  0 siblings, 3 replies; 13+ messages in thread

From: Richard Guo @ 2025-11-04 07:44 UTC (permalink / raw)
  To: Kuntal Ghosh <kuntalghosh.2007@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org; akorotkov@postgresql.org

On Mon, Nov 3, 2025 at 10:09 PM Richard Guo <guofenglinux@gmail.com> wrote:
> Reading generate_orderedappend_paths(), I noticed a couple of other
> issues:
>
> 1. This function considers the cheapest-fractional case in addition to
> the cheapest-startup and cheapest-total cases, but the function's
> comment does not mention this.
>
>  * We consider both cheapest-startup and cheapest-total cases, ie, for each
>  * interesting ordering, collect all the cheapest startup subpaths and all the
>  * cheapest total paths, and build a suitable path for each case.
>
> 2. The function does not handle the case where the paths in
> total_subpaths and fractional_subpaths are identical.  This is not a
> rare scenario, and as a result, it could create two exactly identical
> ordered append paths.
>
> 3. The comment for startup_neq_total is also out of date, since we may
> call create_append_path(), not just create_merge_append_path(), to
> generate ordered append paths.
>
> Here is a WIP patch to address these issues.

Here are the updated patches.  I split the changes into two: 0001
fixes the Assert failure and updates the out-of-date comment for
generate_orderedappend_paths(), while 0002 addresses the second and
third issues I described upthread.

The assertion failure also exists in v18, so I think it would be best
to get 0001 pushed and backpatched before the code freeze.  I'm not
sure whether 0002 should be backpatched.  Before that, I'd like to
know whether these two patches make sense.

Any thoughts?

- Richard

Attachments:

  [application/octet-stream] v2-0001-Fix-assertion-failure-in-generate_orderedappend_p.patch (5.7K, ../../CAMbWs49ONLr4-_X3xe5nMgy-Jaqju_Cn0osaj3RMSsV-9Pd2Cg@mail.gmail.com/2-v2-0001-Fix-assertion-failure-in-generate_orderedappend_p.patch)
  download | inline diff:
From ad8f65c7353c6ec62a027e82f2b38e98d706fb23 Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Mon, 3 Nov 2025 21:43:28 +0900
Subject: [PATCH v2 1/2] Fix assertion failure in
 generate_orderedappend_paths()

In generate_orderedappend_paths(), there is an assumption that a child
relation's row estimate is always greater than zero.  There is an
Assert verifying this assumption, and the estimate is also used to
convert an absolute tuple count into a fraction.

However, this assumption is not always valid -- for example, upper
relations can have their row estimates unset, resulting in a value of
zero.  This can cause an assertion failure in debug builds or lead to
the tuple fraction being computed as infinity in production builds.

To fix, use the row estimate from the cheapest_total path to compute
the tuple fraction.  The row estimate in this path should already have
been forced to a valid value.

In passing, update the comment for generate_orderedappend_paths() to
note that the function also considers the cheapest-fractional case
when not all tuples need to be retrieved.  That is, it collects all
the cheapest fractional paths and builds an ordered append path for
each interesting ordering.
---
 src/backend/optimizer/path/allpaths.c         | 20 +++++++-----
 .../regress/expected/partition_aggregate.out  | 31 +++++++++++++++++++
 src/test/regress/sql/partition_aggregate.sql  |  5 +++
 3 files changed, 49 insertions(+), 7 deletions(-)

diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 9c6436eb72f..41233b98373 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -1810,9 +1810,11 @@ add_paths_to_append_rel(PlannerInfo *root, RelOptInfo *rel,
  * We generate a path for each ordering (pathkey list) appearing in
  * all_child_pathkeys.
  *
- * We consider both cheapest-startup and cheapest-total cases, ie, for each
- * interesting ordering, collect all the cheapest startup subpaths and all the
- * cheapest total paths, and build a suitable path for each case.
+ * We consider the cheapest-startup and cheapest-total cases, and also the
+ * cheapest-fractional case when not all tuples need to be retrieved.  For each
+ * interesting ordering, we collect all the cheapest startup subpaths, all the
+ * cheapest total paths, and, if applicable, all the cheapest fractional paths,
+ * and build a suitable path for each case.
  *
  * We don't currently generate any parameterized ordered paths here.  While
  * it would not take much more code here to do so, it's very unclear that it
@@ -1977,14 +1979,18 @@ generate_orderedappend_paths(PlannerInfo *root, RelOptInfo *rel,
 				double		path_fraction = root->tuple_fraction;
 
 				/*
-				 * Merge Append considers only live children relations.  Dummy
-				 * relations must be filtered out before.
+				 * We should not have a dummy child relation here.  However,
+				 * we cannot use childrel->rows to compute the tuple fraction,
+				 * as childrel can be an upper relation with an unset row
+				 * estimate.  Instead, we use the row estimate from the
+				 * cheapest_total path, which should already have been forced
+				 * to a sane value.
 				 */
-				Assert(childrel->rows > 0);
+				Assert(cheapest_total->rows > 0);
 
 				/* Convert absolute limit to a path fraction */
 				if (path_fraction >= 1.0)
-					path_fraction /= childrel->rows;
+					path_fraction /= cheapest_total->rows;
 
 				cheapest_fractional =
 					get_cheapest_fractional_path_for_pathkeys(childrel->pathlist,
diff --git a/src/test/regress/expected/partition_aggregate.out b/src/test/regress/expected/partition_aggregate.out
index fc84929a002..c30304b99c7 100644
--- a/src/test/regress/expected/partition_aggregate.out
+++ b/src/test/regress/expected/partition_aggregate.out
@@ -339,6 +339,37 @@ SELECT a FROM pagg_tab WHERE a < 3 GROUP BY a ORDER BY 1;
  2
 (3 rows)
 
+-- Test partitionwise aggregation with ordered append path built from fractional paths
+EXPLAIN (COSTS OFF)
+SELECT count(*) FROM pagg_tab GROUP BY c ORDER BY c LIMIT 1;
+                         QUERY PLAN                         
+------------------------------------------------------------
+ Limit
+   ->  Merge Append
+         Sort Key: pagg_tab.c
+         ->  GroupAggregate
+               Group Key: pagg_tab.c
+               ->  Sort
+                     Sort Key: pagg_tab.c
+                     ->  Seq Scan on pagg_tab_p1 pagg_tab
+         ->  GroupAggregate
+               Group Key: pagg_tab_1.c
+               ->  Sort
+                     Sort Key: pagg_tab_1.c
+                     ->  Seq Scan on pagg_tab_p2 pagg_tab_1
+         ->  GroupAggregate
+               Group Key: pagg_tab_2.c
+               ->  Sort
+                     Sort Key: pagg_tab_2.c
+                     ->  Seq Scan on pagg_tab_p3 pagg_tab_2
+(18 rows)
+
+SELECT count(*) FROM pagg_tab GROUP BY c ORDER BY c LIMIT 1;
+ count 
+-------
+   250
+(1 row)
+
 RESET enable_hashagg;
 -- ROLLUP, partitionwise aggregation does not apply
 EXPLAIN (COSTS OFF)
diff --git a/src/test/regress/sql/partition_aggregate.sql b/src/test/regress/sql/partition_aggregate.sql
index 124cc260461..7c725e2663a 100644
--- a/src/test/regress/sql/partition_aggregate.sql
+++ b/src/test/regress/sql/partition_aggregate.sql
@@ -76,6 +76,11 @@ EXPLAIN (COSTS OFF)
 SELECT a FROM pagg_tab WHERE a < 3 GROUP BY a ORDER BY 1;
 SELECT a FROM pagg_tab WHERE a < 3 GROUP BY a ORDER BY 1;
 
+-- Test partitionwise aggregation with ordered append path built from fractional paths
+EXPLAIN (COSTS OFF)
+SELECT count(*) FROM pagg_tab GROUP BY c ORDER BY c LIMIT 1;
+SELECT count(*) FROM pagg_tab GROUP BY c ORDER BY c LIMIT 1;
+
 RESET enable_hashagg;
 
 -- ROLLUP, partitionwise aggregation does not apply
-- 
2.39.5 (Apple Git-154)



  [application/octet-stream] v2-0002-Avoid-creating-duplicate-ordered-append-paths.patch (3.0K, ../../CAMbWs49ONLr4-_X3xe5nMgy-Jaqju_Cn0osaj3RMSsV-9Pd2Cg@mail.gmail.com/3-v2-0002-Avoid-creating-duplicate-ordered-append-paths.patch)
  download | inline diff:
From a41f31d1e5341e93f41d7c467c32a20c2e2d958e Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Tue, 4 Nov 2025 16:01:41 +0900
Subject: [PATCH v2 2/2] Avoid creating duplicate ordered append paths

In generate_orderedappend_paths(), the function does not handle the
case where the paths in total_subpaths and fractional_subpaths are
identical.  This situation is not uncommon, and as a result, it may
generate two exactly identical ordered append paths.

Fix by checking whether total_subpaths and fractional_subpaths contain
the same paths, and skipping creation of the ordered append path for
the fractional case when they are identical.
---
 src/backend/optimizer/path/allpaths.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 41233b98373..4c43fd0b19b 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -1877,6 +1877,7 @@ generate_orderedappend_paths(PlannerInfo *root, RelOptInfo *rel,
 		List	   *total_subpaths = NIL;
 		List	   *fractional_subpaths = NIL;
 		bool		startup_neq_total = false;
+		bool		fraction_neq_total = false;
 		bool		match_partition_order;
 		bool		match_partition_order_desc;
 		int			end_index;
@@ -2005,15 +2006,21 @@ generate_orderedappend_paths(PlannerInfo *root, RelOptInfo *rel,
 				 * XXX We might consider partially sorted paths too (with an
 				 * incremental sort on top). But we'd have to build all the
 				 * incremental paths, do the costing etc.
+				 *
+				 * Also, notice whether we actually have different paths for
+				 * the "fractional" and "total" cases.  This helps avoid
+				 * generating two identical ordered append paths.
 				 */
-				if (!cheapest_fractional)
+				if (cheapest_fractional == NULL)
 					cheapest_fractional = cheapest_total;
+				else if (cheapest_fractional != cheapest_total)
+					fraction_neq_total = true;
 			}
 
 			/*
 			 * Notice whether we actually have different paths for the
-			 * "cheapest" and "total" cases; frequently there will be no point
-			 * in two create_merge_append_path() calls.
+			 * "cheapest" and "total" cases.  This helps avoid generating two
+			 * identical ordered append paths.
 			 */
 			if (cheapest_startup != cheapest_total)
 				startup_neq_total = true;
@@ -2084,7 +2091,7 @@ generate_orderedappend_paths(PlannerInfo *root, RelOptInfo *rel,
 														  false,
 														  -1));
 
-			if (fractional_subpaths)
+			if (fractional_subpaths && fraction_neq_total)
 				add_path(rel, (Path *) create_append_path(root,
 														  rel,
 														  fractional_subpaths,
@@ -2110,7 +2117,7 @@ generate_orderedappend_paths(PlannerInfo *root, RelOptInfo *rel,
 																pathkeys,
 																NULL));
 
-			if (fractional_subpaths)
+			if (fractional_subpaths && fraction_neq_total)
 				add_path(rel, (Path *) create_merge_append_path(root,
 																rel,
 																fractional_subpaths,
-- 
2.39.5 (Apple Git-154)



^ permalink  raw  reply  [nested|flat] 13+ messages in thread

* Re: BUG #19102: Assertion failure in generate_orderedappend_paths with aggregate pushdown
@ 2025-11-04 09:51  Kuntal Ghosh <kuntalghosh.2007@gmail.com>
  parent: Richard Guo <guofenglinux@gmail.com>
  2 siblings, 0 replies; 13+ messages in thread

From: Kuntal Ghosh @ 2025-11-04 09:51 UTC (permalink / raw)
  To: Richard Guo <guofenglinux@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org; akorotkov@postgresql.org

On Tue, Nov 4, 2025 at 1:14 PM Richard Guo <guofenglinux@gmail.com> wrote:

> Here are the updated patches.  I split the changes into two: 0001
> fixes the Assert failure and updates the out-of-date comment for
> generate_orderedappend_paths(), while 0002 addresses the second and
> third issues I described upthread.
>
> The assertion failure also exists in v18, so I think it would be best
> to get 0001 pushed and backpatched before the code freeze.  I'm not
> sure whether 0002 should be backpatched.  Before that, I'd like to
> know whether these two patches make sense.
>
Thanks for the patch.

The fix for the assert failure looks good to me. And, yes, it would be good
to backpatch this before the code freeze.

-- 
Thanks & Regards,
Kuntal Ghosh

^ permalink  raw  reply  [nested|flat] 13+ messages in thread

* Re: BUG #19102: Assertion failure in generate_orderedappend_paths with aggregate pushdown
@ 2025-11-04 13:43  Andrei Lepikhov <lepihov@gmail.com>
  parent: Richard Guo <guofenglinux@gmail.com>
  2 siblings, 1 reply; 13+ messages in thread

From: Andrei Lepikhov @ 2025-11-04 13:43 UTC (permalink / raw)
  To: Richard Guo <guofenglinux@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org, akorotkov@postgresql.org, Kuntal Ghosh <kuntalghosh.2007@gmail.com>

On 4/11/2025 08:44, Richard Guo wrote:
> Any thoughts?The first patch looks good, but I still have a couple of questions.
1. We don't use parameterised paths in MergeAppend yet. I wonder if it 
could be nudged by spreading the use of partitioned tables with foreign 
partitions. Do you think, in such a case, the usage of 
cheapest_total->rows will stay correct? It seems that the parameterised 
path has much less estimation than the RelOptInfo...

2. I understand why the upper relation has unset nrows. However, it may 
be more accurate to set row estimation for a pushing-down upper 
RelOptInfo. Or, at least, describe in comments why this is desirable 
behaviour. It would be profitable, at least, for extension developers.

I also support the second patch. With many partitions, it allows us to 
save a significant amount of CPU cycles.

-- 
regards, Andrei Lepikhov,
pgEdge





^ permalink  raw  reply  [nested|flat] 13+ messages in thread

* Re: BUG #19102: Assertion failure in generate_orderedappend_paths with aggregate pushdown
@ 2025-11-04 23:41  Alexander Korotkov <aekorotkov@gmail.com>
  parent: Andrei Lepikhov <lepihov@gmail.com>
  0 siblings, 1 reply; 13+ messages in thread

From: Alexander Korotkov @ 2025-11-04 23:41 UTC (permalink / raw)
  To: Andrei Lepikhov <lepihov@gmail.com>; +Cc: Richard Guo <guofenglinux@gmail.com>; pgsql-bugs@lists.postgresql.org, akorotkov@postgresql.org, Kuntal Ghosh <kuntalghosh.2007@gmail.com>

On Tue, Nov 4, 2025 at 3:43 PM Andrei Lepikhov <lepihov@gmail.com> wrote:
> On 4/11/2025 08:44, Richard Guo wrote:
> > Any thoughts?The first patch looks good, but I still have a couple of questions.
> 1. We don't use parameterised paths in MergeAppend yet. I wonder if it
> could be nudged by spreading the use of partitioned tables with foreign
> partitions. Do you think, in such a case, the usage of
> cheapest_total->rows will stay correct? It seems that the parameterised
> path has much less estimation than the RelOptInfo...
>
> 2. I understand why the upper relation has unset nrows. However, it may
> be more accurate to set row estimation for a pushing-down upper
> RelOptInfo. Or, at least, describe in comments why this is desirable
> behaviour. It would be profitable, at least, for extension developers.

I wonder if get_cheapest_fractional_path_for_pathkeys() should start
the same as get_cheapest_fractional_path() with calculation of the
tuple fraction.  We could change its first argument to RelOptInfo,
since the both callers get pathlist from RelOptInfo.  See attached
draft patch implementing this.

> I also support the second patch. With many partitions, it allows us to
> save a significant amount of CPU cycles.

+1

------
Regards,
Alexander Korotkov
Supabase

Attachments:

  [application/octet-stream] v3-0001-Fix-assertion-failure-in-generate_orderedappend_p.patch (6.1K, ../../CAPpHfduu=L-KW65rzjATySgUFh0-smBjj8PSQ_=pb-ayD9Gzjg@mail.gmail.com/2-v3-0001-Fix-assertion-failure-in-generate_orderedappend_p.patch)
  download | inline diff:
From c10fa47d30783412edb5d26e6f9f77d994755ed9 Mon Sep 17 00:00:00 2001
From: Alexander Korotkov <akorotkov@postgresql.org>
Date: Wed, 5 Nov 2025 01:40:03 +0200
Subject: [PATCH v3] Fix assertion failure in generate_orderedappend_paths()

Reported-by:
Bug:
Discussion:
Author:
Co-authored-by:
Reviewed-by:
Tested-by:
Backpatch-through:
---
 src/backend/optimizer/path/allpaths.c         | 16 ++--------
 src/backend/optimizer/path/pathkeys.c         |  9 ++++--
 src/backend/optimizer/plan/planagg.c          |  2 +-
 src/include/optimizer/paths.h                 |  2 +-
 .../regress/expected/partition_aggregate.out  | 31 +++++++++++++++++++
 src/test/regress/sql/partition_aggregate.sql  |  5 +++
 6 files changed, 47 insertions(+), 18 deletions(-)

diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 9c6436eb72f..b93e00f6cb3 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -1974,23 +1974,11 @@ generate_orderedappend_paths(PlannerInfo *root, RelOptInfo *rel,
 			 */
 			if (root->tuple_fraction > 0)
 			{
-				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,
+					get_cheapest_fractional_path_for_pathkeys(childrel,
 															  pathkeys,
 															  NULL,
-															  path_fraction);
+															  root->tuple_fraction);
 
 				/*
 				 * If we found no path with matching pathkeys, use the
diff --git a/src/backend/optimizer/path/pathkeys.c b/src/backend/optimizer/path/pathkeys.c
index 139fa1f875a..630aa598747 100644
--- a/src/backend/optimizer/path/pathkeys.c
+++ b/src/backend/optimizer/path/pathkeys.c
@@ -663,15 +663,20 @@ get_cheapest_path_for_pathkeys(List *paths, List *pathkeys,
  * 'fraction' is the fraction of the total tuples expected to be retrieved
  */
 Path *
-get_cheapest_fractional_path_for_pathkeys(List *paths,
+get_cheapest_fractional_path_for_pathkeys(RelOptInfo *rel,
 										  List *pathkeys,
 										  Relids required_outer,
 										  double fraction)
 {
+	Path	   *best_path = rel->cheapest_total_path;
 	Path	   *matched_path = NULL;
 	ListCell   *l;
 
-	foreach(l, paths)
+	/* Convert absolute # of tuples to a fraction; no need to clamp to 0..1 */
+	if (fraction >= 1.0 && best_path->rows > 0)
+		fraction /= best_path->rows;
+
+	foreach(l, rel->pathlist)
 	{
 		Path	   *path = (Path *) lfirst(l);
 
diff --git a/src/backend/optimizer/plan/planagg.c b/src/backend/optimizer/plan/planagg.c
index a2ac58d246e..0fa91cd6b5f 100644
--- a/src/backend/optimizer/plan/planagg.c
+++ b/src/backend/optimizer/plan/planagg.c
@@ -443,7 +443,7 @@ build_minmax_path(PlannerInfo *root, MinMaxAggInfo *mminfo,
 		path_fraction = 1.0;
 
 	sorted_path =
-		get_cheapest_fractional_path_for_pathkeys(final_rel->pathlist,
+		get_cheapest_fractional_path_for_pathkeys(final_rel,
 												  subroot->query_pathkeys,
 												  NULL,
 												  path_fraction);
diff --git a/src/include/optimizer/paths.h b/src/include/optimizer/paths.h
index f6a62df0b43..23f65176eb4 100644
--- a/src/include/optimizer/paths.h
+++ b/src/include/optimizer/paths.h
@@ -223,7 +223,7 @@ extern Path *get_cheapest_path_for_pathkeys(List *paths, List *pathkeys,
 											Relids required_outer,
 											CostSelector cost_criterion,
 											bool require_parallel_safe);
-extern Path *get_cheapest_fractional_path_for_pathkeys(List *paths,
+extern Path *get_cheapest_fractional_path_for_pathkeys(RelOptInfo *rel,
 													   List *pathkeys,
 													   Relids required_outer,
 													   double fraction);
diff --git a/src/test/regress/expected/partition_aggregate.out b/src/test/regress/expected/partition_aggregate.out
index fc84929a002..c30304b99c7 100644
--- a/src/test/regress/expected/partition_aggregate.out
+++ b/src/test/regress/expected/partition_aggregate.out
@@ -339,6 +339,37 @@ SELECT a FROM pagg_tab WHERE a < 3 GROUP BY a ORDER BY 1;
  2
 (3 rows)
 
+-- Test partitionwise aggregation with ordered append path built from fractional paths
+EXPLAIN (COSTS OFF)
+SELECT count(*) FROM pagg_tab GROUP BY c ORDER BY c LIMIT 1;
+                         QUERY PLAN                         
+------------------------------------------------------------
+ Limit
+   ->  Merge Append
+         Sort Key: pagg_tab.c
+         ->  GroupAggregate
+               Group Key: pagg_tab.c
+               ->  Sort
+                     Sort Key: pagg_tab.c
+                     ->  Seq Scan on pagg_tab_p1 pagg_tab
+         ->  GroupAggregate
+               Group Key: pagg_tab_1.c
+               ->  Sort
+                     Sort Key: pagg_tab_1.c
+                     ->  Seq Scan on pagg_tab_p2 pagg_tab_1
+         ->  GroupAggregate
+               Group Key: pagg_tab_2.c
+               ->  Sort
+                     Sort Key: pagg_tab_2.c
+                     ->  Seq Scan on pagg_tab_p3 pagg_tab_2
+(18 rows)
+
+SELECT count(*) FROM pagg_tab GROUP BY c ORDER BY c LIMIT 1;
+ count 
+-------
+   250
+(1 row)
+
 RESET enable_hashagg;
 -- ROLLUP, partitionwise aggregation does not apply
 EXPLAIN (COSTS OFF)
diff --git a/src/test/regress/sql/partition_aggregate.sql b/src/test/regress/sql/partition_aggregate.sql
index 124cc260461..7c725e2663a 100644
--- a/src/test/regress/sql/partition_aggregate.sql
+++ b/src/test/regress/sql/partition_aggregate.sql
@@ -76,6 +76,11 @@ EXPLAIN (COSTS OFF)
 SELECT a FROM pagg_tab WHERE a < 3 GROUP BY a ORDER BY 1;
 SELECT a FROM pagg_tab WHERE a < 3 GROUP BY a ORDER BY 1;
 
+-- Test partitionwise aggregation with ordered append path built from fractional paths
+EXPLAIN (COSTS OFF)
+SELECT count(*) FROM pagg_tab GROUP BY c ORDER BY c LIMIT 1;
+SELECT count(*) FROM pagg_tab GROUP BY c ORDER BY c LIMIT 1;
+
 RESET enable_hashagg;
 
 -- ROLLUP, partitionwise aggregation does not apply
-- 
2.39.5 (Apple Git-154)



^ permalink  raw  reply  [nested|flat] 13+ messages in thread

* Re: BUG #19102: Assertion failure in generate_orderedappend_paths with aggregate pushdown
@ 2025-11-05 01:36  Richard Guo <guofenglinux@gmail.com>
  parent: Alexander Korotkov <aekorotkov@gmail.com>
  0 siblings, 1 reply; 13+ messages in thread

From: Richard Guo @ 2025-11-05 01:36 UTC (permalink / raw)
  To: Alexander Korotkov <aekorotkov@gmail.com>; +Cc: Andrei Lepikhov <lepihov@gmail.com>; pgsql-bugs@lists.postgresql.org, akorotkov@postgresql.org, Kuntal Ghosh <kuntalghosh.2007@gmail.com>

On Wed, Nov 5, 2025 at 8:41 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> I wonder if get_cheapest_fractional_path_for_pathkeys() should start
> the same as get_cheapest_fractional_path() with calculation of the
> tuple fraction.  We could change its first argument to RelOptInfo,
> since the both callers get pathlist from RelOptInfo.  See attached
> draft patch implementing this.

No, I don't think your patch is correct.  With your changes, the
meaning of the fraction parameter in
get_cheapest_fractional_path_for_pathkeys() becomes quite ambiguous.

In the build_minmax_path() case, this parameter represents the
fraction of tuples we want to retrieve, and thus converting the
fraction again within get_cheapest_fractional_path_for_pathkeys() is
incorrect.  However, in the generate_orderedappend_paths() case, the
parameter is interpreted the same way as in grouping_planner().  I
don't think it's a good design choice for the same function parameter
to be interpreted differently depending on where it is called.

In addition, your patch doesn't update this function's comment to
provide a correct explanation of the fraction parameter.

- Richard





^ permalink  raw  reply  [nested|flat] 13+ messages in thread

* Re: BUG #19102: Assertion failure in generate_orderedappend_paths with aggregate pushdown
@ 2025-11-05 01:50  Richard Guo <guofenglinux@gmail.com>
  parent: Richard Guo <guofenglinux@gmail.com>
  2 siblings, 1 reply; 13+ messages in thread

From: Richard Guo @ 2025-11-05 01:50 UTC (permalink / raw)
  To: Kuntal Ghosh <kuntalghosh.2007@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org; akorotkov@postgresql.org

On Tue, Nov 4, 2025 at 4:44 PM Richard Guo <guofenglinux@gmail.com> wrote:
> The assertion failure also exists in v18, so I think it would be best
> to get 0001 pushed and backpatched before the code freeze.  I'm not
> sure whether 0002 should be backpatched.  Before that, I'd like to
> know whether these two patches make sense.

With the code freeze just a couple of days away, I plan to push and
backpatch v2-0001 shortly, barring any objections.

- Richard





^ permalink  raw  reply  [nested|flat] 13+ messages in thread

* Re: BUG #19102: Assertion failure in generate_orderedappend_paths with aggregate pushdown
@ 2025-11-05 06:04  Kuntal Ghosh <kuntalghosh.2007@gmail.com>
  parent: Richard Guo <guofenglinux@gmail.com>
  0 siblings, 0 replies; 13+ messages in thread

From: Kuntal Ghosh @ 2025-11-05 06:04 UTC (permalink / raw)
  To: Richard Guo <guofenglinux@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org; akorotkov@postgresql.org

On Wed, Nov 5, 2025 at 7:20 AM Richard Guo <guofenglinux@gmail.com> wrote:

> On Tue, Nov 4, 2025 at 4:44 PM Richard Guo <guofenglinux@gmail.com> wrote:
> > The assertion failure also exists in v18, so I think it would be best
> > to get 0001 pushed and backpatched before the code freeze.  I'm not
> > sure whether 0002 should be backpatched.  Before that, I'd like to
> > know whether these two patches make sense.
>
> With the code freeze just a couple of days away, I plan to push and
> backpatch v2-0001 shortly, barring any objections.
>
LGTM

-- 
Thanks & Regards,
Kuntal Ghosh

^ permalink  raw  reply  [nested|flat] 13+ messages in thread

* Re: BUG #19102: Assertion failure in generate_orderedappend_paths with aggregate pushdown
@ 2025-11-05 08:30  Alexander Korotkov <aekorotkov@gmail.com>
  parent: Richard Guo <guofenglinux@gmail.com>
  0 siblings, 1 reply; 13+ messages in thread

From: Alexander Korotkov @ 2025-11-05 08:30 UTC (permalink / raw)
  To: Richard Guo <guofenglinux@gmail.com>; +Cc: Andrei Lepikhov <lepihov@gmail.com>; pgsql-bugs@lists.postgresql.org, akorotkov@postgresql.org, Kuntal Ghosh <kuntalghosh.2007@gmail.com>

Hi, Richard!

On Wed, Nov 5, 2025 at 3:36 AM Richard Guo <guofenglinux@gmail.com> wrote:
>
> On Wed, Nov 5, 2025 at 8:41 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > I wonder if get_cheapest_fractional_path_for_pathkeys() should start
> > the same as get_cheapest_fractional_path() with calculation of the
> > tuple fraction.  We could change its first argument to RelOptInfo,
> > since the both callers get pathlist from RelOptInfo.  See attached
> > draft patch implementing this.
>
> No, I don't think your patch is correct.  With your changes, the
> meaning of the fraction parameter in
> get_cheapest_fractional_path_for_pathkeys() becomes quite ambiguous.
>
> In the build_minmax_path() case, this parameter represents the
> fraction of tuples we want to retrieve, and thus converting the
> fraction again within get_cheapest_fractional_path_for_pathkeys() is
> incorrect.  However, in the generate_orderedappend_paths() case, the
> parameter is interpreted the same way as in grouping_planner().  I
> don't think it's a good design choice for the same function parameter
> to be interpreted differently depending on where it is called.
>
> In addition, your patch doesn't update this function's comment to
> provide a correct explanation of the fraction parameter.

Hmm... I don't quite get the point, because with my patch
get_cheapest_fractional_path_for_pathkeys() would allow passing tuple
fraction as either fraction of tuples or absolute number of tuples in
the same way as grouping_planner() (see its header comment).

But given we need to backpatch this, we should avoid changing
functions signatures.  So, please, go ahead pushing your patch.

------
Regards,
Alexander Korotkov
Supabase





^ permalink  raw  reply  [nested|flat] 13+ messages in thread

* Re: BUG #19102: Assertion failure in generate_orderedappend_paths with aggregate pushdown
@ 2025-11-05 10:19  Richard Guo <guofenglinux@gmail.com>
  parent: Alexander Korotkov <aekorotkov@gmail.com>
  0 siblings, 0 replies; 13+ messages in thread

From: Richard Guo @ 2025-11-05 10:19 UTC (permalink / raw)
  To: Alexander Korotkov <aekorotkov@gmail.com>; +Cc: Andrei Lepikhov <lepihov@gmail.com>; pgsql-bugs@lists.postgresql.org, akorotkov@postgresql.org, Kuntal Ghosh <kuntalghosh.2007@gmail.com>

On Wed, Nov 5, 2025 at 5:30 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> Hmm... I don't quite get the point, because with my patch
> get_cheapest_fractional_path_for_pathkeys() would allow passing tuple
> fraction as either fraction of tuples or absolute number of tuples in
> the same way as grouping_planner() (see its header comment).

Now I see what you mean.  However, I'm still not sure this is a better
approach, especially since the tuple fraction could end up being
calculated twice in the build_minmax_path() case.  Also, I still don't
think the comment in your patch is correct.  The comment for
get_cheapest_fractional_path_for_pathkeys() states:

 * See compare_fractional_path_costs() for the interpretation of the fraction
 * parameter.

However, in cases where the fraction is greater than 1,
compare_fractional_path_costs() interprets it as 1, whereas the
get_cheapest_fractional_path_for_pathkeys() function in your patch
interprets it as the absolute number of tuples to be retrieved.

> But given we need to backpatch this, we should avoid changing
> functions signatures.  So, please, go ahead pushing your patch.

I've pushed and backpatched v2-0001.  Thanks to everyone for the
reviews, and to Kuntal for the report and reproduction script.

I've also pushed 0002, but only to master.  I'm a bit hesitant to
backpatch it given the lack of field complaints.  If it later turns
out that a backpatch is needed, we can do that then.

- Richard





^ permalink  raw  reply  [nested|flat] 13+ messages in thread


end of thread, other threads:[~2025-11-05 10:19 UTC | newest]

Thread overview: 13+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-11-03 05:51 BUG #19102: Assertion failure in generate_orderedappend_paths with aggregate pushdown PG Bug reporting form <noreply@postgresql.org>
2025-11-03 05:57 ` Kuntal Ghosh <kuntalghosh.2007@gmail.com>
2025-11-03 10:09   ` Richard Guo <guofenglinux@gmail.com>
2025-11-03 13:09     ` Richard Guo <guofenglinux@gmail.com>
2025-11-04 07:44       ` Richard Guo <guofenglinux@gmail.com>
2025-11-04 09:51         ` Kuntal Ghosh <kuntalghosh.2007@gmail.com>
2025-11-04 13:43         ` Andrei Lepikhov <lepihov@gmail.com>
2025-11-04 23:41           ` Alexander Korotkov <aekorotkov@gmail.com>
2025-11-05 01:36             ` Richard Guo <guofenglinux@gmail.com>
2025-11-05 08:30               ` Alexander Korotkov <aekorotkov@gmail.com>
2025-11-05 10:19                 ` Richard Guo <guofenglinux@gmail.com>
2025-11-05 01:50         ` Richard Guo <guofenglinux@gmail.com>
2025-11-05 06:04           ` Kuntal Ghosh <kuntalghosh.2007@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