Re: Aggregate node doesn't include cost for sorting - Mailing list pgsql-hackers

Hi David,

Thanks for the explanations and awesome that this was improved on already.
I didn't have this change on my radar.

On 12/8/22 11:40, David Rowley wrote:
>
> This output surely must be from a version of PostgreSQL prior to
> 1349d279?  I can't quite figure out why you've added a "SET
> enable_seqscan = FALSE;". That makes it look like you've used the same
> version of PostgreSQL to produce both of these plans.  The 2nd plan
> you've shown must be post 1349d279.


Both plans were captured on 14.5, which is indeed prior to 1349d279.

I disabled sequential scan to show that there's an alternative plan 
which is superior to the chosen plan: Index Only Scan is more expensive 
and takes longer than the Seq Scan, but the subsequent Aggregate runs 
much faster as it doesn't have to sort, making the plan overall superior.

>
> So, with the assumption that you've used 2 different versions to show
> this output, for post 1349d279, there does not seem to be much choice
> on how the aggregate is executed. What's your concern about the
> costings having to be accurate given there's no other plan choice?

There's another plan choice which is using the index to get pre-sorted 
input rows, see previous comment.

>
> The new pre-sorted aggregate code will always request the sort order
> that suits the largest number of ORDER BY / DISTINCT aggregates.  When
> there are multiple ORDER BY / DISTINCT aggregates and they have
> different sort requirements then there certainly are completing ways
> that the aggregation portion of the plan can be executed.  I opted to
> make the choice just based on the number of aggregates that could
> become presorted.  nodeAgg.c is currently not very smart about sharing
> sorts between multiple aggregates with the same sort requirements.  If
> that was made better, there might be more motivation to have better
> costing code in make_pathkeys_for_groupagg().  However, the reasons
> for the reversion of db0d67db seemed to be mainly around the lack of
> ability to accurately cost multiple competing sort orders. We'd need
> to come up with some better way to do that if we were to want to give
> make_pathkeys_for_groupagg() similar abilities.
>
>> Also, why does the Aggregate node sort itself? Why don't we instead emit
>> an explicit Sort node in the plan so that it's visible to the user what
>> is happening? As soon as there's also a GROUP BY in the query, a Sort
>> node occurs in the plan. This seems inconsistent.
> Post 1349d279 we do that, but it can only do it for 1 sort order.
> There can be any number of aggregate functions which require a sort
> and they don't all have to have the same sort order requirements.  We
> can't do the same as WindowAgg does by chaining nodes together either
> because the aggregate node aggregates the results and we'd need all
> the same input rows to be available at each step.
>
> The only other way would be to have it so an Aggregate node could be
> fed by multiple different input nodes and then it would only work on
> the aggregates that suit the given input before reading the next input
> and doing the other aggregates.  Making it work like that would cause
> quite a bit of additional effort during planning (not to mention the
> executor). We'd have to run the join search once per required order,
> which is one of the slowest parts of planning.  Right now, you could
> probably make that work by just writing the SQL to have a subquery per
> sort requirement.

Thanks for the explanation!

-- 
David Geier
(ServiceNow)