On Mon, Jun 12, 2023 at 12:06 PM David Rowley <dgrowle...@gmail.com> wrote:
> On Fri, 9 Jun 2023 at 20:57, Richard Guo <guofengli...@gmail.com> wrote: > > On Fri, Jun 9, 2023 at 8:13 AM David Rowley <dgrowle...@gmail.com> > wrote: > >> It might be possible to make adjustments in nodeWindowAgg.c to have > >> the equality checks come out as true when there is no ORDER BY. > >> update_frameheadpos() is one location that would need to be adjusted. > >> It would need further study to ensure we don't accidentally break > >> anything. I've not done that study, so won't be adjusting the patch > >> for now. > > > > I'm also not sure if doing that is safe in all cases. Hmm, do you think > > we can instead check wc->frameOptions to see if it is the RANGE OFFSET > > case in make_pathkeys_for_window(), and decide to not remove or remove > > redundant ORDER BY items according to whether it is or not RANGE OFFSET? > > I think ideally, we'd not have to add special cases to the planner to > disable the optimisation for certain cases. I'd much rather see > adjustments in the executor to handle cases where we've removed ORDER > BY columns (e.g adjust update_frameheadpos() to assume rows are equal > when there are no order by columns.) That of course would require > that there are no cases where removing ORDER BY columns would change > the actual query results. I can't currently think of any reason why > the results would change, but I'm not overly familiar with the RANGE > option, so I'd need to spend a bit longer looking at it than I have > done so far to feel confident in making the patch process ORDER BY > columns too. > > I'm ok with just doing the PARTITION BY stuff as step one. The ORDER > BY stuff is more complex and risky which seems like a good reason to > tackle separately. I see your point. Agreed that the ORDER BY stuff might be better to be done in a separate patch. So now the v2 patch looks good to me. Thanks Richard
