On Fri, 9 Jun 2023 at 20:57, Richard Guo <guofenglinux@gmail.com> wrote: > On Fri, Jun 9, 2023 at 8:13 AM David Rowley <dgrowleyml@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.
