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

From: David Rowley <[email-masked]>
Date: Tue, 17 Jan 2023 16:39:38 +1300

On Tue, 17 Jan 2023 at 13:16, Dean Rasheed <[email-masked]> wrote:
>
> On Wed, 11 Jan 2023 at 05:24, David Rowley <[email-masked]> 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.

Thanks for giving that some additional thought.  I've just pushed a
fix which adjusts things that way.

David

---

From: Richard Guo <[email-masked]>
Date: Tue, 17 Jan 2023 14:29:56 +0800

> fix which adjusts things that way.

This makes a lot of sense.  I agree that we shouldn't do pre-sorting for
volatile sort expressions, especially when there are multiple aggregates
with the same volatile sort expression.

Not related to this specific issue, but I find sorting by volatile
expression is confusing in different scenarios.  Consider the two
queries given by David

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

Query 2:
select random()::text from generate_series(1,3) order by random();

Considering the targetlist as Aggref->args or Query->targetList, in both
queries we would add an additional TargetEntry (as resjunk column) for
the ORDER BY item 'random()', because it's not present in the existing
targetlist.  Note that the existing TargetEntry for 'random()::text' is
a CoerceViaIO expression which is an explicit cast, so we cannot strip
it and match it to the ORDER BY item.  Thus we would have two random()
FuncExprs in the final targetlist, for both queries.

In query 1 we call random() twice for each tuple, one for the original
TargetEntry 'random()::text', and one for the TargetEntry of the ORDER
BY item 'random()', and do the sorting according to the second call
results.  Thus we would notice the final output is unsorted because it's
from the first random() call.

However, in query 2 we have the ORDER BY item 'random()' in the
scan/join node's targetlist.  And then for the two random() FuncExprs in
the final targetlist, set_plan_references would adjust both of them to
refer to the outputs of the scan/join node.  Thus random() is actually
called only once for each tuple and we would find the final output is
sorted.

It seems we fail to keep consistent about the behavior of sorting by
volatile expression in the two scenarios.

BTW, I wonder if we should have checked CoercionForm before
fix_upper_expr_mutator steps into CoerceViaIO->arg to adjust the expr
there.  It seems parser checks it and only strips implicit coercions
when matching TargetEntry expr to ORDER BY item.

Thanks
Richard

---

From: Tom Lane <[email-masked]>
Date: Tue, 17 Jan 2023 02:05:19 -0500

Richard Guo <[email-masked]> writes:
> there.

I will just quote what it says in primnodes.h:

 * NB: equal() ignores CoercionForm fields, therefore this *must* not carry
 * any semantically significant information.

If you think the planner should act differently for different values of
CoercionForm, you are mistaken.  Maybe this is evidence of some
previous bit of brain-fade, but if so we need to fix that.

			regards, tom lane

---

From: Richard Guo <[email-masked]>
Date: Wed, 18 Jan 2023 17:37:01 +0800

> previous bit of brain-fade, but if so we need to fix that.

According to this comment in primnodes.h, the planner is not supposed to
treat implicit and explicit casts differently.  In this case
set_plan_references is doing its job correctly, to adjust both random()
FuncExprs in targetlist to refer to subplan's output for query 2.  As a
consequence we would get a sorted output.

I'm still confused that when the same scenario happens with ORDER BY in
an aggregate function, like in query 1, the result is different in that
we would get an unsorted output.

I wonder if we should avoid this inconsistent behavior.

Thanks
Richard