Subject: Re: Add proper planner support for ORDER BY / DISTINCT aggregates

From: Richard Guo <[email-masked]>
Date: Wed, 11 Jan 2023 10:45:51 +0800

On Tue, Jan 10, 2023 at 6:12 PM Dean Rasheed <[email-masked]>
wrote:

> While doing some random testing, I noticed that the following is broken in
> HEAD:
>
> SELECT COUNT(DISTINCT random()) FROM generate_series(1,10);
>
> ERROR:  ORDER/GROUP BY expression not found in targetlist
>
> It appears to have been broken by 1349d279, though I haven't looked at
> the details.

Yeah, this is definitely broken.  For this query, we try to sort the
scan/join path by random() before performing the Aggregate, which is an
optimization implemented in 1349d2790b.  However the scan/join plan's
tlist does not contain random(), which I think we need to fix.

Thanks
Richard

---

From: David Rowley <[email-masked]>
Date: Wed, 11 Jan 2023 17:12:45 +1300

On Wed, 11 Jan 2023 at 15:46, Richard Guo <[email-masked]> wrote:
> However the scan/join plan's

I was wondering if that's true and considered that we don't want to
evaluate random() for the sort then again when doing the aggregate
transitions, but I see that does not really work before 1349d279, per:

postgres=# set enable_presorted_aggregate=0;
SET
postgres=# select string_agg(random()::text, ',' order by random())
from generate_series(1,3);
                        string_agg
-----------------------------------------------------------
 0.8659110018246505,0.15612649559563474,0.2022878955613403
(1 row)

I'd have expected those random numbers to be concatenated in ascending order.

Running: select random() from generate_Series(1,3) order by random();
gives me the results in the order I'd have expected.

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().

David

---

From: Tom Lane <[email-masked]>
Date: Tue, 10 Jan 2023 23:32:04 -0500

> 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.

			regards, tom lane

---

From: David Rowley <[email-masked]>
Date: Wed, 11 Jan 2023 18:23:50 +1300

> 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

---

From: Richard Guo <[email-masked]>
Date: Wed, 11 Jan 2023 16:47:05 +0800

> from generate_series(1,3);

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

I traced this query a bit and found that when executing the aggregation
the random() function in the aggregate expression (position 1) and in
the order by clause (position 2) are calculated separately.  And the
sorting is performed based on the function results from the order by
clause.  In the final output, what we see is the function results from
the aggregate expression.  Thus we'll notice the output is not sorted.

I'm not sure if this is expected or broken though.

BTW, if we explicitly add ::text for random() in the order by clause, as

select string_agg(
        random()::text,
        ','
        order by random()::text
        )

The random() function will be calculated only once for each tuple, and
we can get a sorted output.

Thanks
Richard

---

From: Dean Rasheed <[email-masked]>
Date: Tue, 17 Jan 2023 00:16:10 +0000

> 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