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:
>> Thanks for the job! I guess you could be more brave and push down also
>> FILTER statement.
>
> While probably not impossible, I wasn't planning on changing this code
> with new optimizations; just expanding the applicability of the
> current optimizations.
Got it>> 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.
>
> If the fetch_agg_sort_op clause would be deduplicated, it would either
> increase code complexity to handle both aggref->aggorder paths, or it
> would increase the cost of planning MAX(a ORDER BY b) because of the
> newly added catalog access.
IMO it looks like a micro optimisation. But I agree, it is more about
code style - let the committer decide what is better.>> Also, I'm unsure
about the necessity of looking through the btree
>> classes. Maybe just to check the commutator to the sortop, like in the
>> diff attached? Or could you provide an example to support your approach?
>
> I think it could work, but I'd be hesitant to rely on that, as
> commutator registration is optional (useful, but never required for
> btree operator classes' operators). Looking at the btree operator
> class, which is the definition of sortability in PostgreSQL, seems
> more suitable and correct.
Hm, I dubious about that. Can you provide an example which my variant
will not pass but your does that correctly?
--
regards, Andrei Lepikhov