On Tue, 13 Dec 2022 at 20:53, David Rowley <dgrowleyml@gmail.com> wrote:
> I think what we need to do is: Do our best to give incremental sort
> the most realistic costs we can and accept that it might choose a
> worse plan in some cases. Users can turn it off if they really have no
> other means to convince the planner it's wrong.
>
> Additionally, I think what we also need to add a GUC such as
> enable_presorted_aggregate. People can use that when their Index Scan
> -> Incremental Sort -> Aggregate plan is worse than their previous Seq
> Scan -> Sort -> Aggregate plan that they were getting in < 16.
> Turning off enable_incremental_sort alone won't give them the same
> aggregate plan that they had in pg15 as we always set the
> query_pathkeys to request a sort order that will suit the order by /
> distinct aggregates.
>
> I'll draft up a patch for the enable_presorted_aggregate.
I've attached a patch series for this.
v3-0001 can be ignored here. I've posted about that in [1]. Any
discussion about that patch should take place over there. The patch
is required to get the 0002 patch to pass isolation check
v3-0002 removes the 1.5 x cost pessimism from incremental sort and
also rewrites how we make incremental sort paths. I've now gone
through the remaining places where we create an incremental sort path
to give all those the same treatment that I'd added to
add_paths_to_grouping_rel(). There was a 1 or 2 plan changes in the
regression tests. One was the isolation test change, which I claim to
be a broken test and should be fixed another way. The other was
performing a Sort on the cheapest input path which had presorted keys.
That plan now uses an Incremental Sort to make use of the presorted
keys. I'm happy to see just how much redundant code this removes.
About 200 lines.
v3-0003 adds the enable_presorted_aggregate GUC.
David
[1] https://www.postgresql.org/message-id/CAApHDvrbDhObhLV+=U_K_-t+2Av2av1aL9d+2j_3AO-XndaviA@mail.gmail.com
