Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix assertion failure in generate_orderedappend_paths()

  2. Avoid creating duplicate ordered append paths

  1. BUG #19102: Assertion failure in generate_orderedappend_paths with aggregate pushdown

    PG Bug reporting form <noreply@postgresql.org> — 2025-11-03T05:51:09Z

    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

  2. Re: BUG #19102: Assertion failure in generate_orderedappend_paths with aggregate pushdown

    Kuntal Ghosh <kuntalghosh.2007@gmail.com> — 2025-11-03T05:57:51Z

    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

  3. Re: BUG #19102: Assertion failure in generate_orderedappend_paths with aggregate pushdown

    Richard Guo <guofenglinux@gmail.com> — 2025-11-03T10:09:25Z

    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

  4. Re: BUG #19102: Assertion failure in generate_orderedappend_paths with aggregate pushdown

    Richard Guo <guofenglinux@gmail.com> — 2025-11-03T13:09:44Z

    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

  5. Re: BUG #19102: Assertion failure in generate_orderedappend_paths with aggregate pushdown

    Richard Guo <guofenglinux@gmail.com> — 2025-11-04T07:44:42Z

    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

  6. Re: BUG #19102: Assertion failure in generate_orderedappend_paths with aggregate pushdown

    Kuntal Ghosh <kuntalghosh.2007@gmail.com> — 2025-11-04T09:51:56Z

    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

  7. Re: BUG #19102: Assertion failure in generate_orderedappend_paths with aggregate pushdown

    Andrei Lepikhov <lepihov@gmail.com> — 2025-11-04T13:43:05Z

    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

  8. Re: BUG #19102: Assertion failure in generate_orderedappend_paths with aggregate pushdown

    Alexander Korotkov <aekorotkov@gmail.com> — 2025-11-04T23:41:13Z

    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

  9. Re: BUG #19102: Assertion failure in generate_orderedappend_paths with aggregate pushdown

    Richard Guo <guofenglinux@gmail.com> — 2025-11-05T01:36:38Z

    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

  10. Re: BUG #19102: Assertion failure in generate_orderedappend_paths with aggregate pushdown

    Richard Guo <guofenglinux@gmail.com> — 2025-11-05T01:50:15Z

    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

  11. Re: BUG #19102: Assertion failure in generate_orderedappend_paths with aggregate pushdown

    Kuntal Ghosh <kuntalghosh.2007@gmail.com> — 2025-11-05T06:04:11Z

    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

  12. Re: BUG #19102: Assertion failure in generate_orderedappend_paths with aggregate pushdown

    Alexander Korotkov <aekorotkov@gmail.com> — 2025-11-05T08:30:06Z

    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

  13. Re: BUG #19102: Assertion failure in generate_orderedappend_paths with aggregate pushdown

    Richard Guo <guofenglinux@gmail.com> — 2025-11-05T10:19:15Z

    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