On Wed, 11 Jan 2023 at 05:24, David Rowley <dgrowle...@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