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: Thu, 22 Jul 2021 10:42:49 +0200
Message-ID: <4493999.1py09z8qHF@aivenronan>
Organization: aiven
In-Reply-To: 
 <CAApHDvrXnUFNiYQiHv4Eu_rtbMPsKeVT-KG93_dD-ZYs9G5OnA@mail.gmail.com>
References: 
 <CAApHDvpHzfo92=R4W0+xVua3BUYCKMckWAmo-2t_KiXN-wYH=w@mail.gmail.com>
 <5137746.gn36MdYUqo@aivenronan>
 <CAApHDvrXnUFNiYQiHv4Eu_rtbMPsKeVT-KG93_dD-ZYs9G5OnA@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/4493999.1py09z8qHF%40aivenronan>
Precedence: bulk

Le jeudi 22 juillet 2021, 09:38:50 CEST David Rowley a =E9crit :
> On Thu, 22 Jul 2021 at 02:01, Ronan Dunklau <ronan.dunklau@aiven.io> wrot=
e:
> > I tested the 0001 patch against both HEAD and my proposed bugfix for
> > postgres_fdw.
> >=20
> > There is a problem that the ordered aggregate is not pushed down anymor=
e.
> > The underlying Sort node is correctly pushed down though.
> >=20
> > This comes from the fact that postgres_fdw grouping path never contains
> > any
> > pathkey. Since the cost is fuzzily the same between the pushed-down
> > aggregate and the locally performed one, the tie is broken against the
> > pathkeys.
> I think this might be more down to a lack of any penalty cost for
> fetching foreign tuples. Looking at create_foreignscan_path(), I don't
> see anything that adds anything to account for fetching the tuples
> from the foreign server. If there was something like that then there'd
> be more of a preference to perform the remote aggregation so that
> fewer rows must arrive from the remote server.
>=20
> I tested by adding: total_cost +=3D cpu_tuple_cost * rows * 100; in
> create_foreignscan_path() and I got the plan with the remote
> aggregation. That's a fairly large penalty of 1.0 per row. Much bigger
> than parallel_tuple_cost's default value.
>=20
> I'm a bit undecided on how much this patch needs to get involved in
> adjusting foreign scan costs.  The problem is that we've given the
> executor a new path to consider and nobody has done any proper
> costings for the foreign scan so that it properly prefers paths that
> have to pull fewer foreign tuples.  This is a pretty similar problem
> to what parallel_tuple_cost aims to fix. Also similar to how we had to
> add APPEND_CPU_COST_MULTIPLIER to have partition-wise aggregates
> prefer grouping at the partition level rather than at the partitioned
> table level.
>=20
> > Ideally we would add the group pathkeys to the grouping path, but this
> > would add an additional ORDER BY expression matching the GROUP BY.
> > Moreover, some triaging of the pathkeys would be necessary since we now
> > mix the sort-in- aggref pathkeys with the group_pathkeys.
>=20
> I think you're talking about passing pathkeys into
> create_foreign_upper_path in add_foreign_grouping_paths. If so, I
> don't really see how it would be safe to add pathkeys to the foreign
> grouping path. What if the foreign server did a Hash Aggregate?  The
> rows might come back in any random order.

Yes, I was suggesting to add a new path with the pathkeys factored in, whic=
h=20
if chosen over the non-ordered path would result in an additional ORDER BY=
=20
clause to prevent a HashAggregate. But that doesn't seem a good idea after=
=20
all.

>=20
> I kinda think that to fix this properly would need a new foreign
> server option such as foreign_tuple_cost. I'd feel better about
> something like that with some of the people with a vested interest in
> the FDW code were watching more closely. So far we've not managed to
> entice any of them with the bug report yet, but it's maybe early days
> yet.

We already have that in the form of fdw_tuple_cost as a server option if I'=
m=20
not mistaken ? It works as expected when the number of tuples is notably=20
reduced by the foreign group by.

The problem arise when the cardinality of the groups is equal to the input'=
s=20
cardinality. I think even in that case we should try to use a remote aggreg=
ate=20
since it's a computation that will not happen on the local server. I also=20
think we're more likely to have up to date statistics remotely than the one=
s=20
collected locally on the foreign tables, and the estimated number of groups=
=20
would be more accurate on the remote side than the local one.

=2D-=20
Ronan Dunklau