From: Ronan Dunklau <ronan.dunklau@aiven.io>
To: David Rowley <dgrowleyml@gmail.com>
Cc: PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Add proper planner support for ORDER BY / DISTINCT aggregates
Date: Mon, 05 Jul 2021 08:38:28 +0200
Message-ID: <2311666.Sa1gP0IYip@aivenronan>
Organization: aiven
In-Reply-To: 
 <CAApHDvpYZwbRNurFsG+cvbUFBaFaiF6J6VWKBvWdtVsbU-Bfgg@mail.gmail.com>
References: 
 <CAApHDvpHzfo92=R4W0+xVua3BUYCKMckWAmo-2t_KiXN-wYH=w@mail.gmail.com>
 <5348877.kVHUhDTtjH@aivenronan>
 <CAApHDvpYZwbRNurFsG+cvbUFBaFaiF6J6VWKBvWdtVsbU-Bfgg@mail.gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="iso-8859-1"
Archived-At: 
 <https://www.postgresql.org/message-id/2311666.Sa1gP0IYip%40aivenronan>
Precedence: bulk

Le vendredi 2 juillet 2021, 10:39:44 CEST David Rowley a =E9crit :
> On Fri, 2 Jul 2021 at 19:54, Ronan Dunklau <ronan.dunklau@aiven.io> wrote:
> > I don't know if it's acceptable, but in the case where you add both an
> > aggregate with an ORDER BY clause, and another aggregate without the
> > clause, the output for the unordered one will change and use the same
> > ordering, maybe suprising the unsuspecting user. Would that be acceptab=
le
> > ?
>=20
> That's a good question. There was an argument in [1] that mentions
> that there might be a group of people who rely on aggregation being
> done in a certain order where they're not specifying an ORDER BY
> clause in the aggregate.  If that group of people exists, then it's
> possible they might be upset in the scenario that you describe.
>=20
> I also think that it's going to be pretty hard to make significant
> gains in this area if we are too scared to make changes to undefined
> behaviour.  You wouldn't have to look too hard in the pgsql-general
> mailing list to find someone complaining that their query output is in
> the wrong order on some query that does not have an ORDER BY. We
> pretty much always tell those people that the order is undefined
> without an ORDER BY. I'm not too sure why Tom in [1] classes the ORDER
> BY aggregate people any differently. We'll be stuck forever here and
> in many other areas if we're too scared to change the order of
> aggregation. You could argue that something like parallelism has
> changed that for people already. I think the multi-batch Hash
> Aggregate code could also change this.

I would agree with that.

>=20
> > I was curious about the performance implication of that additional
> > transition, and could not reproduce a signifcant difference. I may be
> > doing something wrong: how did you highlight it ?
>=20
> It was pretty basic. I just created a table with two columns and no
> index and did something like SELECT a,SUM(b ORDER BY b) from ab GROUP
> BY 1;  the new code will include a Sort due to lack of any index and
> the old code would have done a sort inside nodeAgg.c. I imagine that
> the overhead comes from the fact that in the patched version nodeAgg.c
> must ask its subnode (nodeSort.c) for the next tuple each time,
> whereas unpatched nodeAgg.c already has all the tuples in a tuplestore
> and can fetch them very quickly in a tight loop.

Ok, I reproduced that case, just not using a group by: by adding the group =
by=20
a sort node is added in both cases (master and your patch), except that wit=
h=20
your patch we sort on both keys and that doesn't really incur a performance=
=20
penalty.=20

I think the overhead occurs because in the ExecAgg case, we use the=20
tuplesort_*_datum API as an optimization when we have a single column as an=
=20
input, which the ExecSort code doesn't. Maybe it would be worth it to try t=
o=20
use that API in sort nodes too, when it can be done.=20


>=20
> David
>=20
> [1] https://www.postgresql.org/message-id/6538.1522096067%40sss.pgh.pa.us


=2D-=20
Ronan Dunklau