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

Hi, Richard!

On Wed, Nov 5, 2025 at 3:36 AM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
>
> On Wed, Nov 5, 2025 at 8:41 AM Alexander Korotkov <aekorotkov(at)gmail(dot)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