On Sun, 25 Jan 2026 at 20:05, David Rowley <[email protected]> wrote:
>
> On Sun, 25 Jan 2026 at 17:09, Meng Zhang <[email protected]> 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 function to 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
diff --git a/src/backend/nodes/list.c b/src/backend/nodes/list.c
index 98fc2b44b50..170d9bdad84 100644
--- a/src/backend/nodes/list.c
+++ b/src/backend/nodes/list.c
@@ -1513,6 +1513,52 @@ list_deduplicate_oid(List *list)
check_list_invariants(list);
}
+/*
+ * list_uniquify
+ * Remove duplicate items from a pointer list according to the item
+ * values.
+ *
+ * Unlike the list_deduplicate* functions, we have no means to sort pointer
+ * lists by their values. Because we don't require a sorted list, this
+ * function does not follow the naming standard of those functions.
+ */
+void
+list_uniquify(List *list)
+{
+ int len;
+
+ Assert(IsPointerList(list));
+ len = list_length(list);
+ if (len > 1)
+ {
+ ListCell *elements = list->elements;
+
+ for (int i = 0; i < len; i++)
+ {
+ for (int j = i + 1; j < len;)
+ {
+ if (equal(elements[i].ptr_value,
elements[j].ptr_value))
+ {
+ /*
+ * Move all elements beyond the j'th
element up 1 place.
+ * XXX this won't be very efficient for
long lists with
+ * many duplicates.
+ */
+ memmove(&list->elements[j],
+ &list->elements[j + 1],
+ (len - j) *
sizeof(ListCell));
+ len--;
+ }
+ else
+ j++;
+ }
+ }
+
+ list->length = len;
+ }
+ check_list_invariants(list);
+}
+
/*
* Free all storage in a list, and optionally the pointed-to elements
*/
diff --git a/src/backend/optimizer/plan/planner.c
b/src/backend/optimizer/plan/planner.c
index 7a6b8b749f2..8b8fae695ec 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -5964,6 +5964,32 @@ optimize_window_clauses(PlannerInfo *root,
WindowFuncLists *wflists)
}
}
}
+
+ /*
+ * XXX remove any duplicate WindowFuncs from each WindowClause. This
has
+ * been done only in the back branches. Previously, the deduplication
was
+ * done in find_window_functions(), but that caused issues with the code
+ * above when moving a WindowFunc to another WindowClause as any
duplicate
+ * WindowFuncs won't receive the adjusted winref when merging
+ * WindowClauses. The deduplication below has been done only so that we
+ * maintain the same cost calculations. As it turns out, the previous
+ * deduplication code thought it was saving effort during execution by
+ * getting rid of duplicates, but that was not true as the expression
+ * evaluation code will evaluate each WindowFunc mentioned in the
+ * targetlist.
+ */
+ foreach(lc, windowClause)
+ {
+ WindowClause *wc = lfirst_node(WindowClause, lc);
+ List *lst = wflists->windowFuncs[wc->winref];
+
+ if (lst == NIL)
+ continue;
+
+ wflists->numWindowFuncs -= list_length(lst);
+ list_uniquify(lst);
+ wflists->numWindowFuncs += list_length(lst);
+ }
}
/*
diff --git a/src/backend/optimizer/util/clauses.c
b/src/backend/optimizer/util/clauses.c
index 39d35827c35..32204776c45 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -261,13 +261,10 @@ find_window_functions_walker(Node *node, WindowFuncLists
*lists)
if (wfunc->winref > lists->maxWinRef)
elog(ERROR, "WindowFunc contains out-of-range winref
%u",
wfunc->winref);
- /* eliminate duplicates, so that we avoid repeated computation
*/
- if (!list_member(lists->windowFuncs[wfunc->winref], wfunc))
- {
- lists->windowFuncs[wfunc->winref] =
- lappend(lists->windowFuncs[wfunc->winref],
wfunc);
- lists->numWindowFuncs++;
- }
+
+ lists->windowFuncs[wfunc->winref] =
+ lappend(lists->windowFuncs[wfunc->winref], wfunc);
+ lists->numWindowFuncs++;
/*
* We assume that the parser checked that there are no window
diff --git a/src/include/nodes/pg_list.h b/src/include/nodes/pg_list.h
index ae80975548f..ad65945177b 100644
--- a/src/include/nodes/pg_list.h
+++ b/src/include/nodes/pg_list.h
@@ -668,6 +668,7 @@ pg_nodiscard extern List *list_concat_unique_int(List
*list1, const List *list2)
pg_nodiscard extern List *list_concat_unique_oid(List *list1, const List
*list2);
extern void list_deduplicate_oid(List *list);
+extern void list_uniquify(List *list);
extern void list_free(List *list);
extern void list_free_deep(List *list);
