On 17/7/2024 16:33, Matthias van de Meent wrote:
> On Wed, 17 Jul 2024 at 05:29, Andrei Lepikhov <lepihov@gmail.com> wrote:
>> As I see, the code:
>> aggsortop = fetch_agg_sort_op(aggref->aggfnoid);
>> if (!OidIsValid(aggsortop))
>> return false; /* not a MIN/MAX aggregate */
>>
>> used twice and can be evaluated earlier to avoid duplicated code.
>
> The code is structured like this to make sure we only start accessing
> catalogs once we know that all other reasons to bail out from this
> optimization indicate we can apply the opimization. You'll notice that
> I've tried to put the cheapest checks that only use caller-supplied
> information first, and catalog accesses only after that.
After additional research I think I get the key misunderstanding why you
did so:
As I see, the checks:
if (list_length(aggref->aggorder) > 1)
return false;
if (orderClause->tleSortGroupRef != curTarget->ressortgroupref)
return false;
not needed at all. You already have check:
if (list_length(aggref->args) != 1)
and this tells us, that if we have ordering like MIN(x ORDER BY <smth>),
this <smth> ordering contains only aggregate argument x. Because if it
contained some expression, the transformAggregateCall() would add this
expression to agg->args by calling the transformSortClause() routine.
The tleSortGroupRef is just exactly ressortgroupref - no need to recheck
it one more time. Of course, it is suitable only for MIN/MAX aggregates,
but we discuss only them right now. Am I wrong?
If you want, you can place it as assertions (see the diff in attachment).
--
regards, Andrei Lepikhov