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