Re: Expand applicability of aggregate's sortop optimization

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