Re: MergeAppend could consider sorting cheapest child path

Andrei Lepikhov писал(а) 2025-05-05 14:38:
> On 4/29/25 19:25, Alexander Pyhalov wrote:
>> Andrei Lepikhov писал(а) 2025-04-29 16:52:
>> But it seems, base_path can't be NULL
> Correct. Fixed.
> 
>> 
>> Also we check base_path for required_outer and require_parallel_safe, 
>> but if cheapest path for pathkeys is NULL, these checks are not 
>> performed.
> Thanks. Fixed.
> 
>> Luckily, they seen to be no-op anyway due to cheapest_total- > 
>> >param_info == NULL and function arguments being NULL (required_outer)
>> and false (require_parallel_safe). Should we do something about this? 
>> Don't know, perhaps, remove these misleading arguments?
> The main reason why I introduced these _ext routines was that this 
> schema may be used somewhere else. And in that case parameterised paths 
> may exist. Who knows, may be parameterised paths will be introduced for 
> merge append too?
> 
>> become no-op? And we do return non-null path from 
>> get_cheapest_fractional_path_for_pathkeys_ext(), as it seems we return 
>> either cheapest_total_path or cheapest fractional path from 
>> get_cheapest_fractional_path_for_pathkeys_ext().
> Ok.
> 
> Let's check next version of the patch in the attachment.

Hi.

Both functions are a bit different:

        Path   *base_path = rel->cheapest_total_path;
        Path   *path;

        path = get_cheapest_path_for_pathkeys(rel->pathlist, pathkeys,
                                                                          
         required_outer, cost_criterion,
                                                                          
         require_parallel_safe);

        /* Stop here if the path doesn't satisfy necessary conditions */
        if ((require_parallel_safe && !base_path->parallel_safe) ||
                !bms_is_subset(PATH_REQ_OUTER(base_path), 
required_outer))
                return path;


Here comment speaks about "the path", and check is performed on 
base_path, could it be misleading?

In get_cheapest_fractional_path_for_pathkeys_ext():

        path = get_cheapest_fractional_path_for_pathkeys(rel->pathlist, 
pathkeys,
                                                                          
                                required_outer, fraction);

        base_path = rel->cheapest_total_path;

        /* Stop here if the path doesn't satisfy necessary conditions */
        if (!bms_is_subset(PATH_REQ_OUTER(base_path), required_outer))
                return path;


Here "the path" also refers to base_path, but here at least it's the 
last path we've just looked at. Should we make base_path initialization 
consistent and fix comment a bit, so that there's no possible ambiguity 
and it's evident that it refers to the base_path?

Also logic a bit differs if path is NULL. In 
get_cheapest_path_for_pathkeys_ext() we explicitly check for path being 
NULL, in get_cheapest_fractional_path_for_pathkeys_ext() only after 
calculating sort cost.

I've tried to fix comments a bit and unified functions definitions.
-- 
Best regards,
Alexander Pyhalov,
Postgres Professional