Re: Remove WindowClause PARTITION BY items belonging to redundant pathkeys

On Mon, Jun 12, 2023 at 12:06 PM David Rowley <dgrowleyml(at)gmail(dot)com> wrote:

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