public inbox for [email protected]
help / color / mirror / Atom feedFrom: Dean Rasheed <[email protected]>
To: David Rowley <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Richard Guo <[email protected]>
Cc: Ronan Dunklau <[email protected]>
Cc: Justin Pryzby <[email protected]>
Cc: Pavel Luzanov <[email protected]>
Cc: [email protected]
Cc: Ranier Vilela <[email protected]>
Subject: Re: Add proper planner support for ORDER BY / DISTINCT aggregates
Date: Tue, 17 Jan 2023 00:16:10 +0000
Message-ID: <CAEZATCWLqfyYR0zmD7Nh7WzPRd4bn2z+JFs3XNETZ-FVsy2-OQ@mail.gmail.com> (raw)
In-Reply-To: <CAApHDvov5upudo+03svepBJZ2o7DGtU7X+biVX12ni6ojUDn9A@mail.gmail.com>
References: <3163474.aeNJFYEL58@aivenronan>
<CAApHDvowz-gfbuq=8iwHebRcy4VpFuJv52B3jCcXVi+CPFrd1A@mail.gmail.com>
<[email protected]>
<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>
<[email protected]>
<CAApHDvov5upudo+03svepBJZ2o7DGtU7X+biVX12ni6ojUDn9A@mail.gmail.com>
On Wed, 11 Jan 2023 at 05:24, David Rowley <[email protected]> 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
view thread (7+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Add proper planner support for ORDER BY / DISTINCT aggregates
In-Reply-To: <CAEZATCWLqfyYR0zmD7Nh7WzPRd4bn2z+JFs3XNETZ-FVsy2-OQ@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox