On Fri, 16 Jul 2021 at 22:00, David Rowley <dgrowle...@gmail.com> wrote: > > On Fri, 16 Jul 2021 at 18:04, David Rowley <dgrowle...@gmail.com> wrote: > > What we maybe could consider instead would be to pick the first Aggref > > then look for the most sorted derivative of that then tally up the > > number of Aggrefs that can be sorted using those pathkeys, then repeat > > that process for the remaining Aggrefs that didn't have the same > > prefix then use the pathkeys for the set with the most Aggrefs. We > > could still tiebreak on the targetlist position so at least it's not > > random which ones we pick. Now that we have a list of Aggrefs that are > > deduplicated in the planner thanks to 0a2bc5d61e it should be fairly > > easy to do that. > > I've attached a patch which does as I mention above.
Looks like I did a sloppy job of that. I messed up the condition in standard_qp_callback() that sets the ORDER BY aggregate pathkeys so that it accidentally set them when there was an unsortable GROUP BY clause, as highlighted by the postgres_fdw tests failing. I've now added a comment to explain why the condition is the way it is so that I don't forget again. Here's a cleaned-up version that passes make check-world. David
v5-0001-Add-planner-support-for-ORDER-BY-aggregates.patch
Description: Binary data
