Обсуждение: Re: BUG #19102: Assertion failure in generate_orderedappend_paths with aggregate pushdown

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 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