MIME-Version: 1.0
References: <3163474.aeNJFYEL58@aivenronan> <CAApHDvowz-gfbuq=8iwHebRcy4VpFuJv52B3jCcXVi+CPFrd1A@mail.gmail.com>
 <9f61ddbf-2989-1536-b31e-6459370a6baa@postgrespro.ru> <2202180.iZASKD2KPV@aivenlaptop>
 <CAApHDvpkbvDEDJPNZrUqCsCLVC57ZY8qrPn8nGpQSrn4RYstZw@mail.gmail.com>
 <CAMbWs4_+5cP+8xjiAc=hds935ZiiG6oepPDeVNovWhQ66DARJw@mail.gmail.com>
 <CAApHDvqiEBY5_fVv4ZwUtWDPk0pJ6TdoYKFMwhhF_Gdkv-xAkg@mail.gmail.com>
 <CAApHDvr1Sm+g9hbv4REOVuvQKeDWXcKUAhmbK5K+dfun0s9CvA@mail.gmail.com>
 <CAApHDvptP-VK+ERLVs6UOAWaCQ7y4bf4_SSfRLCSyKWb-rW5zg@mail.gmail.com>
 <CAApHDvrsmvoJ1GF18TCxLL1wE=QrHKdam4dy73a5UF_QpNKqgQ@mail.gmail.com>
 <CAEZATCWETioXs5kY8vT6BVguY41_wD962VDk=u_Nvd7S1UXzuQ@mail.gmail.com>
 <CAMbWs49sa00xmyw9TfHLYDsHhsud_YEUm3YRkEE4tJrUDqLCgQ@mail.gmail.com>
 <CAApHDvruKdB5kaj6W9-kaM=zhMtgD2LvU5YDO3kFui6XSZC5Tg@mail.gmail.com>
 <416296.1673411524@sss.pgh.pa.us> <CAApHDvov5upudo+03svepBJZ2o7DGtU7X+biVX12ni6ojUDn9A@mail.gmail.com>
In-Reply-To: <CAApHDvov5upudo+03svepBJZ2o7DGtU7X+biVX12ni6ojUDn9A@mail.gmail.com>
From: Dean Rasheed <dean.a.rasheed@gmail.com>
Date: Tue, 17 Jan 2023 00:16:10 +0000
Message-ID: <CAEZATCWLqfyYR0zmD7Nh7WzPRd4bn2z+JFs3XNETZ-FVsy2-OQ@mail.gmail.com>
Subject: Re: Add proper planner support for ORDER BY / DISTINCT aggregates
To: David Rowley <dgrowleyml@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Richard Guo <guofenglinux@gmail.com>, 
	Ronan Dunklau <ronan.dunklau@aiven.io>, Justin Pryzby <pryzby@telsasoft.com>, 
	Pavel Luzanov <p.luzanov@postgrespro.ru>, pgsql-hackers@lists.postgresql.org, 
	Ranier Vilela <ranier.vf@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Archived-At: <https://www.postgresql.org/message-id/CAEZATCWLqfyYR0zmD7Nh7WzPRd4bn2z%2BJFs3XNETZ-FVsy2-OQ%40mail.gmail.com>
Precedence: bulk

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