On Wed, 11 Jan 2023 at 05:24, David Rowley <dgrowleyml@gmail.com> wrote:
>
> I'm wondering if 1349d279 should have just never opted to presort
> Aggrefs which have volatile functions so that the existing behaviour
> of unordered output is given always and nobody is fooled into thinking
> this works correctly only to be disappointed later when they add some
> other aggregate to their query or if we should fix both. Certainly,
> it seems much easier to do the former.
>
I took a look at this, and I agree that the best solution is probably
to have make_pathkeys_for_groupagg() ignore Aggrefs that contain
volatile functions. Not only is that the simplest solution, preserving
the old behaviour, I think it's required for correctness.
Aside from the fact that I don't think such aggregates would benefit
from the optimisation introduced by 1349d279, I think it would be
incorrect if there was more than one such aggregate having the same
sort expression, because I think that volatile sorting should be
evaluated separately for each aggregate. For example:
SELECT string_agg(a::text, ',' ORDER BY random()),
string_agg(a::text, ',' ORDER BY random())
FROM generate_series(1,3) s(a);
string_agg | string_agg
------------+------------
2,1,3 | 3,2,1
(1 row)
so pre-sorting wouldn't be right (or at least it would change existing
behaviour in a surprising way).
Regards,
Dean
