public inbox for [email protected]  

help / color / mirror / Atom feed

From: David Rowley <[email protected]>
To: Tom Lane <[email protected]>
Cc: Richard Guo <[email protected]>
Cc: Dean Rasheed <[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: Wed, 11 Jan 2023 18:23:50 +1300
Message-ID: <CAApHDvov5upudo+03svepBJZ2o7DGtU7X+biVX12ni6ojUDn9A@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
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]>

On Wed, 11 Jan 2023 at 17:32, Tom Lane <[email protected]> wrote:
>
> David Rowley <[email protected]> writes:
> > I think whatever the fix is here, we should likely ensure that the
> > results are consistent regardless of which Aggrefs are the presorted
> > ones.  Perhaps the easiest way to do that, and to ensure we call the
> > volatile functions are called the same number of times would just be
> > to never choose Aggrefs with volatile functions when doing
> > make_pathkeys_for_groupagg().
>
> There's existing logic in equivclass.c and other places that tries
> to draw very tight lines around what we'll assume about volatile
> sort expressions (pathkeys).  It sounds like there's someplace in
> this recent patch that didn't get that memo.

I'm not sure I did a good job of communicating my thoughts there. What
I mean is, having volatile functions in the aggregate's ORDER BY or
DISTINCT clause didn't seem very well behaved prior to the presorted
aggregates patch. If I go and fix the bug with the missing targetlist
items, then a query such as:

select string_agg(random()::text, ',' order by random()) from
generate_series(1,3);

should start putting the random() numbers in order where it didn't
prior to 1349d279. Perhaps users might be happy that those are in
order, however, if they then go and change the query to:

select sum(a order by a),string_agg(random()::text, ',' order by
random()) from generate_series(1,3);

then they might become unhappy again that their string_agg is not
ordered the way they specified because the planner opted to sort by
"a" rather than "random()" after the initial scan.

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.

David

view thread (7+ messages)  latest in thread

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: <CAApHDvov5upudo+03svepBJZ2o7DGtU7X+biVX12ni6ojUDn9A@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