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

Le mercredi 21 juillet 2021, 04:52:43 CEST David Rowley a écrit :
> Thanks for finding this. I've made a few changes to make this case
> work as you describe. Please see attached v6 patches.
>
> 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.
>
> 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
postgres_fdw.

There is a problem that the ordered aggregate is not pushed down anymore. The
underlying Sort node is correctly pushed down though.

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.

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.

--
Ronan Dunklau