Re: Add proper planner support for ORDER BY / DISTINCT aggregates
David Rowley <dgrowleyml@gmail.com>
From: David Rowley <dgrowleyml@gmail.com>
To: Ronan Dunklau <ronan.dunklau@aiven.io>
Cc: PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>,
Ranier Vilela <ranier.vf@gmail.com>
Date: 2022-07-20T05:26:39Z
Lists: pgsql-hackers
Attachments
- v7-0001-Add-planner-support-for-ORDER-BY-aggregates.patch (text/plain) patch v7-0001
On Thu, 4 Nov 2021 at 20:59, Ronan Dunklau <ronan.dunklau@aiven.io> wrote: > I took some time to toy with this again. > > At first I thought that charging a discount in foreign grouping paths for > Aggref targets (since they are computed remotely) would be a good idea, > similar to what is done for the grouping keys. I've been working on this patch again. There was a bit of work to do to rebase it atop db0d67db2. The problem there was that since this patch appends pathkeys to suit ORDER BY / DISTINCT aggregates to the query's group_pathkeys, db0d67db2 goes and tries to rearrange those, but fails to find the SortGroupClause corresponding to the PathKey in group_pathkeys. I wish the code I came up with to make that work was a bit nicer, but what's there at least seems to work. There are a few more making copies of Lists than I'd like. I've also went and added LLVM support to make JIT work with the new DISTINCT expression evaluation step types. Also, James mentioned in [1] about the Merge Join plan change that this patch was causing in an existing test. I looked into that and found the cause. The plan change is not really the fault of this patch, so I've proposed a fix for to make that work more efficiently in [2]. The basics there are that select_outer_pathkeys_for_merge() pre-dates Incremental Sorts and didn't consider prefixes of the query_pathkeys after matching all the join quals. The patch on that thread relaxes that rule and makes this patch produce an Incremental Sort plan for the query in question. Another annoying part of this patch is that I've added an "aggpresorted" field to Aggref, which the planner sets. That's a parse node type and it would be nicer not to have the planner mess around with those. We maybe could wrap up the Aggrefs in some planner struct and pass those to the executor instead. That would require a bit more churn than what I've got in the attached. I've attached the v7 patch. David [1] https://www.postgresql.org/message-id/CAAaqYe-yxXkXVPJkRw1nDA=CJBw28jvhACRyDcU10dAOcdSj=Q@mail.gmail.com [2] https://www.postgresql.org/message-id/CAApHDvrtZu0PHVfDPFM4Yx3jNR2Wuwosv+T2zqa7LrhhBr2rRg@mail.gmail.com
Commits
-
Don't presort ORDER BY/DISTINCT Aggrefs with volatile functions
- da5800d5fa63 16.0 landed
-
Add enable_presorted_aggregate GUC
- 3226f47282a0 16.0 landed
-
Remove pessimistic cost penalization from Incremental Sort
- 4a29eabd1d91 16.0 landed
-
Fix hypothetical problem passing the wrong GROUP BY pathkeys
- af7d270dd3c7 16.0 landed
-
Remove unused fields from ExprEvalStep
- 9fc1776dda9f 16.0 landed
-
Improve performance of ORDER BY / DISTINCT aggregates
- 1349d2790bf4 16.0 landed
-
Refactor function parse_subscription_options.
- 8aafb0261675 15.0 cited