BUG #19102: Assertion failure in generate_orderedappend_paths with aggregate pushdown

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

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

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

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

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

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

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

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

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

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

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

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