public inbox for [email protected]
help / color / mirror / Atom feedFrom: David Rowley <[email protected]>
To: Tom Lane <[email protected]>
Cc: Daniel Farkaš <[email protected]>
Cc: Magnus Hagander <[email protected]>
Cc: PostgreSQL mailing lists <[email protected]>
Subject: Re: BUG #17502: View based on window functions returns wrong results when queried
Date: Mon, 30 May 2022 17:24:33 +1200
Message-ID: <CAApHDvr94vSvxm4j_ykMBapFLXC8H_ntLg7oD5fhqfqR0x-snA@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<CABUevEz7t3OWxaSSVSHOxa5KcYioOk=yRwoL3iMViyZQ8m2wAw@mail.gmail.com>
<CAGckUK2GLF=d9J5ErEWgK5x8ECqCw4equnq3jEzrqtfJw+iHYw@mail.gmail.com>
<CAApHDvoR4BXm7oxSrYhpEAFOZ-qMPkkYnn14y6sxtPf=5w5OOw@mail.gmail.com>
<[email protected]>
<CAApHDvp5dapiG2PBSYz+Yps+LGEwr8bY_NoUr1+=rn9oL+NfPA@mail.gmail.com>
<[email protected]>
<CAApHDvpacAbOTYeTPCgKnafUOvJT0=dFP0q+eVhOhB2Yaa8+QQ@mail.gmail.com>
<[email protected]>
On Mon, 30 May 2022 at 15:02, Tom Lane <[email protected]> wrote:
> I'm not necessarily against adding the prohibition in the back
> branches. However, if this has been wrong since 10.x (if not
> further back) then it seems like few people are tripping over
> the inconsistency.
Yeah, perhaps. But we have received a report from 1 person and I think
it seems strange to adopt a policy that we require multiple bug
reports for the same issue before we consider fixing.
FWIW, just to demonstrate what a fix could look like, I've attached a
patch. I'm not planning to commit it if you really think we shouldn't
be fixing it.
The patch also does nothing for the weirdness that I described in the
DISTINCT ON case earlier in this thread.
David
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 7ac116a791..5dab40f6ff 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -3933,6 +3933,44 @@ recurse_push_qual(Node *setOp, Query *topquery,
* SIMPLIFYING SUBQUERY TARGETLISTS
*****************************************************************************/
+/*
+ * has_set_returning_window_clauses
+ * Returns true if 'node' contains any WindowFuncs which reference a
+ * WindowClause where the PARTITION BY or ORDER BY clause contains any
+ * set returning functions.
+ */
+static bool
+has_set_returning_window_clauses(Query *subquery, Node *node)
+{
+ WindowFuncLists *wflists;
+
+ wflists = find_window_functions(node,
+ list_length(subquery->windowClause));
+
+ if (wflists->numWindowFuncs == 0)
+ return false;
+
+ for (int i = 1; i <= wflists->maxWinRef; i++)
+ {
+ WindowClause *wclause;
+
+ if (wflists->windowFuncs[i] == NIL)
+ continue;
+
+ wclause = (WindowClause *) list_nth(subquery->windowClause, i - 1);
+
+ if (expression_returns_set((Node *) get_sortgrouplist_exprs(wclause->partitionClause,
+ subquery->targetList)))
+ return true;
+
+ if (expression_returns_set((Node *) get_sortgrouplist_exprs(wclause->orderClause,
+ subquery->targetList)))
+ return true;
+ }
+
+ return false;
+}
+
/*
* remove_unused_subquery_outputs
* Remove subquery targetlist items we don't need
@@ -4028,9 +4066,20 @@ remove_unused_subquery_outputs(Query *subquery, RelOptInfo *rel)
* If it contains a set-returning function, we can't remove it since
* that could change the number of rows returned by the subquery.
*/
- if (subquery->hasTargetSRFs &&
- expression_returns_set(texpr))
- continue;
+ if (subquery->hasTargetSRFs)
+ {
+ if (expression_returns_set(texpr))
+ continue;
+
+ /*
+ * Check if the target entry contains any WindowFunc references
+ * which have a WindowClause where the PARTITION BY or ORDER BY
+ * clause contain any SRFs
+ */
+ if (subquery->hasWindowFuncs &&
+ has_set_returning_window_clauses(subquery, (Node *) texpr))
+ continue;
+ }
/*
* If it contains volatile functions, we daren't remove it for fear
Attachments:
[text/plain] dont_remove_srf_windowfuncs.patch (2.2K, ../CAApHDvr94vSvxm4j_ykMBapFLXC8H_ntLg7oD5fhqfqR0x-snA@mail.gmail.com/2-dont_remove_srf_windowfuncs.patch)
download | inline diff:
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 7ac116a791..5dab40f6ff 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -3933,6 +3933,44 @@ recurse_push_qual(Node *setOp, Query *topquery,
* SIMPLIFYING SUBQUERY TARGETLISTS
*****************************************************************************/
+/*
+ * has_set_returning_window_clauses
+ * Returns true if 'node' contains any WindowFuncs which reference a
+ * WindowClause where the PARTITION BY or ORDER BY clause contains any
+ * set returning functions.
+ */
+static bool
+has_set_returning_window_clauses(Query *subquery, Node *node)
+{
+ WindowFuncLists *wflists;
+
+ wflists = find_window_functions(node,
+ list_length(subquery->windowClause));
+
+ if (wflists->numWindowFuncs == 0)
+ return false;
+
+ for (int i = 1; i <= wflists->maxWinRef; i++)
+ {
+ WindowClause *wclause;
+
+ if (wflists->windowFuncs[i] == NIL)
+ continue;
+
+ wclause = (WindowClause *) list_nth(subquery->windowClause, i - 1);
+
+ if (expression_returns_set((Node *) get_sortgrouplist_exprs(wclause->partitionClause,
+ subquery->targetList)))
+ return true;
+
+ if (expression_returns_set((Node *) get_sortgrouplist_exprs(wclause->orderClause,
+ subquery->targetList)))
+ return true;
+ }
+
+ return false;
+}
+
/*
* remove_unused_subquery_outputs
* Remove subquery targetlist items we don't need
@@ -4028,9 +4066,20 @@ remove_unused_subquery_outputs(Query *subquery, RelOptInfo *rel)
* If it contains a set-returning function, we can't remove it since
* that could change the number of rows returned by the subquery.
*/
- if (subquery->hasTargetSRFs &&
- expression_returns_set(texpr))
- continue;
+ if (subquery->hasTargetSRFs)
+ {
+ if (expression_returns_set(texpr))
+ continue;
+
+ /*
+ * Check if the target entry contains any WindowFunc references
+ * which have a WindowClause where the PARTITION BY or ORDER BY
+ * clause contain any SRFs
+ */
+ if (subquery->hasWindowFuncs &&
+ has_set_returning_window_clauses(subquery, (Node *) texpr))
+ continue;
+ }
/*
* If it contains volatile functions, we daren't remove it for fear
view thread (18+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: BUG #17502: View based on window functions returns wrong results when queried
In-Reply-To: <CAApHDvr94vSvxm4j_ykMBapFLXC8H_ntLg7oD5fhqfqR0x-snA@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox