Fix possible issue of a WindowFunc being in the wrong WindowClause

ed1a88dda made it so WindowClauses can be merged when all window
functions belonging to the WindowClause can equally well use some
other WindowClause without any behavioral changes.  When that
optimization applies, the WindowFunc's "winref" gets adjusted to
reference the new WindowClause.

That commit does not work well with the deduplication logic in
find_window_functions(), which only added the WindowFunc to the list
when there wasn't already an identical WindowFunc in the list.  That
deduplication logic meant that the duplicate WindowFunc wouldn't get the
"winref" changed when optimize_window_clauses() was able to swap the
WindowFunc to another WindowClause.  This could lead to the following
error in the unlikely event that the deduplication code did something and
the duplicate WindowFunc happened to be moved into another WindowClause.

ERROR:  WindowFunc with winref 2 assigned to WindowAgg with winref 1

As it turns out, the deduplication logic in find_window_functions() is
pretty bogus.  It might have done something when added, as that code
predates b8d7f053c, which changed how projections work.  As it turns
out, at least now we *will* evaluate the duplicate WindowFuncs.  All
that the deduplication code seems to do today is assist in
underestimating the WindowAggPath costs due to not counting the
evaluation costs of duplicate WindowFuncs.

Ideally the fix would be to remove the deduplication code, but that
could result in changes to the plan costs, as duplicate WindowFuncs
would then be costed.  Instead, let's play it safe and shift the
deduplication code so it runs after the other processing in
optimize_window_clauses().

Backpatch only as far as v16 as there doesn't seem to be any other harm
done by the WindowFunc deduplication code before then.  This issue was
fixed in master by 7027dd499.

Reported-by: Meng Zhang <mza117jc@gmail.com>
Author: Meng Zhang <mza117jc@gmail.com>
Author: David Rowley <dgrowleyml@gmail.com>
Discussion: https://postgr.es/m/CAErYLFAuxmW0UVdgrz7iiuNrxGQnFK_OP9hBD5CUzRgjrVrz=Q@mail.gmail.com
Backpatch-through: 16

diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 5e2af9808f6944058e203a906f1cb0bdf60300ed..601d16de63d128146503f180af48af07fdd00995 100644 (file)
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -5913,6 +5913,41 @@ 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);
+               ListCell   *lc2;
+               List       *list = wflists->windowFuncs[wc->winref];
+               List       *newlist = NIL;
+
+               if (list == NIL)
+                       continue;
+
+               foreach(lc2, list)
+               {
+                       if (!list_member(newlist, lfirst(lc2)))
+                               newlist = lappend(newlist, lfirst(lc2));
+                       else
+                               wflists->numWindowFuncs--;
+               }
+               list_free(list);
+
+               wflists->windowFuncs[wc->winref] = newlist;
+       }
 }
 
 /*

diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 125966028aa99c15d992bcb18f4b17699c9d9df2..4d68b50fa207bdbddd7651eb434587ec1466095c 100644 (file)
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -248,13 +248,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