From: Ronan Dunklau <ronan.dunklau@aiven.io>
To: David Rowley <dgrowleyml@gmail.com>
Cc: PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>,
 Ranier Vilela <ranier.vf@gmail.com>
Subject: Re: Add proper planner support for ORDER BY / DISTINCT aggregates
Date: Wed, 21 Jul 2021 16:01:03 +0200
Message-ID: <5137746.gn36MdYUqo@aivenronan>
Organization: aiven
In-Reply-To: 
 <CAApHDvogabtJzP8K6s75ruSXJAOfCvUJtUVWBS=2C-49kRqvCQ@mail.gmail.com>
References: 
 <CAApHDvpHzfo92=R4W0+xVua3BUYCKMckWAmo-2t_KiXN-wYH=w@mail.gmail.com>
 <4285468.TQqHX9kgJM@aivenronan>
 <CAApHDvogabtJzP8K6s75ruSXJAOfCvUJtUVWBS=2C-49kRqvCQ@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/5137746.gn36MdYUqo%40aivenronan>
Precedence: bulk

Le mercredi 21 juillet 2021, 04:52:43 CEST David Rowley a =E9crit :
> Thanks for finding this.  I've made a few changes to make this case
> work as you describe. Please see attached v6 patches.
>=20
> Because I had to add additional code to take the GROUP BY pathkeys
> into account when choosing the best ORDER BY agg pathkeys, the
> function that does that became a little bigger.  To try to reduce the
> complexity of it, I got rid of all the processing from the initial
> loop and instead it now uses the logic from the 2nd loop to find the
> best pathkeys.  The reason I'd not done that in the first place was
> because I'd thought I could get away without building an additional
> Bitmapset for simple cases, but that's probably fairly cheap compared
> to building Pathkeys.   With the additional complexity for the GROUP
> BY pathkeys, the extra code seemed not worth it.
>=20
> The 0001 patch is the ORDER BY aggregate code.  0002 is to fix up some
> broken regression tests in postgres_fdw that 0001 caused.  It appears
> that 0001 uncovered a bug in the postgres_fdw code.  I've reported
> that in [1]. If that turns out to be a bug then it'll need to be fixed
> before this can progress.

I tested the 0001 patch against both HEAD and my proposed bugfix for=20
postgres_fdw.

There is a problem that the ordered aggregate is not pushed down anymore. T=
he=20
underlying Sort node is correctly pushed down though.=20

This comes from the fact that postgres_fdw grouping path never contains any=
=20
pathkey. Since the cost is fuzzily the same between the pushed-down aggrega=
te=20
and the locally performed one, the tie is broken against the pathkeys.

Ideally we would add the group pathkeys to the grouping path, but this woul=
d=20
add an additional ORDER BY expression matching the GROUP BY. Moreover, some=
=20
triaging of the pathkeys would be necessary since we now mix the sort-in-
aggref pathkeys with the group_pathkeys.

=2D-=20
Ronan Dunklau