Обсуждение: Fix a reference error for window functions: In the function 'find_window_functions', the deduplication logic should be removed

On Sun, 25 Jan 2026 at 17:09, Meng Zhang <mza117jc@gmail.com> wrote:
> The deduplication logic won't cause an error when the result of this function is only used in
`select_active_windows`.
> But when the result is used in `optimize_window_clauses`, it will cause the `winref` field of a certain window
functionto not be modified in the new window.
 

Thanks for the report. I'll have a look.

David

Hi，all

David Rowley <dgrowleyml@gmail.com> 于2026年1月25日周日 15:05写道：
>
> On Sun, 25 Jan 2026 at 17:09, Meng Zhang <mza117jc@gmail.com> wrote:
> > The deduplication logic won't cause an error when the result of this function is only used in
`select_active_windows`.
> > But when the result is used in `optimize_window_clauses`, it will cause the `winref` field of a certain window
functionto not be modified in the new window. 
>
> Thanks for the report. I'll have a look.
>
> David
>
>
I did some analysis, and I found this issue was introduced by this
commit ed1a88d:
Allow window functions to adjust their frameOptions

In optimize_window_clauses(), the list wc->winref = 2 was set to NIL,
so "window 2" would not be in the activewindow lists.
We only have one windowagg node, in ExecEndWindowAgg(), when we
process the second "ROW_NUMBER() OVER window2"
which its winref was still 2 because in
find_window_functions_walker(), it was skipped due to duplicates.

The attached patch seems workable. But I have one question.  If we
implement the attached patch, would it introduce redundant
computation?
Maybe optimize_window_clauses() would optimize the repeated
computation. I don't understand this much.

--
Thanks,
Tender Wang

On Sun, 25 Jan 2026 at 20:05, David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Sun, 25 Jan 2026 at 17:09, Meng Zhang <mza117jc@gmail.com> wrote:
> > The deduplication logic won't cause an error when the result of this function is only used in
`select_active_windows`.
> > But when the result is used in `optimize_window_clauses`, it will cause the `winref` field of a certain window
functionto not be modified in the new window.
 
>
> Thanks for the report. I'll have a look.

I initially didn't think your patch was correct as it was getting rid
of the deduplication logic, but after looking a bit deeper, the
deduplication logic no longer works. I assume it did work when Tom
wrote the comment in 2016 (51c0f63e4), but in 2017 Andres changed the
expression evaluation code for projections (b8d7f053c). The expression
evaluation code goes through the targetlist and will find the
WindowFuncs again, all 3 of them, in this case, and since the
deduplication code didn't put them all in the WindowFuncLists lists,
one still has the unmodified winref because optimize_window_clauses()
only operates on the non-duplicate WindowFuncs.

The problem now is if we just delete the code as your patch does, then
the costing code will newly include costs for any duplicate
WindowFuncs, and that could result in the dreaded plan changes in the
backbranches problem. Yes, really we should be including these extra
costs as we *do* evaluate the WindowFuncs separately, so in master, I
think what you have is fine. We just probably will need to do
something to maintain the costs in the backbranches.

I came up with the attached for that. I did write the list_uniquify()
before I realised your fix is ok for master. That function might be
misplaced just in the backbranches, and it might be better to just
foreach and if (!list_member()) directly in optimize_window_clauses()
to get rid of the duplicates. That's probably safer too.

I'll park the attached patch here for a bit to see if anyone else has
any thoughts.

David

On Sun, 25 Jan 2026 at 23:14, David Rowley <dgrowleyml@gmail.com> wrote:
> I came up with the attached for that. I did write the list_uniquify()
> before I realised your fix is ok for master. That function might be
> misplaced just in the backbranches, and it might be better to just
> foreach and if (!list_member()) directly in optimize_window_clauses()
> to get rid of the duplicates. That's probably safer too.

I pushed the deduplication code removal to master and adjusted the
backpatch version to do the foreach -> if (!list_memeber()) as
mentioned above.

David