Re: [HACKERS] Window functions can be created with defaults, but they don't work
I wrote:
I noticed this while poking at the variadic-aggregates issue:
regression=# create function nth_value_def(anyelement, integer = 1) returns
anyelement language internal window immutable strict as 'window_nth_value';
CREATE FUNCTION
regression=# SELECT nth_value_def(ten) OVER (PARTITION BY four), ten, four
FROM (SELECT * FROM tenk1 WHERE unique2 10 ORDER BY four, ten)s;
The connection to the server was lost. Attempting reset: Failed.
Attached is a proposed patch against HEAD that fixes this by supporting
default arguments properly for window functions. In passing, it also
allows named-argument notation in window function calls, since that's
free once the other thing works (because the same subroutine fixes up
both things).
The reason this crashes is that the planner doesn't apply
default-insertion to WindowFunc nodes, only to FuncExprs. We could make
it do that, probably, but that seems to me like a feature addition.
I think a more reasonable approach for back-patching is to fix function
creation to disallow attaching defaults to window functions (or
aggregates, for that matter, which would have the same issue if CREATE
AGGREGATE had the syntax option to specify defaults). ProcedureCreate
seems like an appropriate place, since it already contains a lot of sanity
checks of this sort.
Having now done the patch to fix it properly, I'm more inclined to think
that maybe we should just back-patch this rather than inserting an error
check. It seems pretty low-risk.
Comments?
regards, tom lane
diff --git a/doc/src/sgml/syntax.sgml b/doc/src/sgml/syntax.sgml
index e3dbc4b..16aade4 100644
*** a/doc/src/sgml/syntax.sgml
--- b/doc/src/sgml/syntax.sgml
*** SELECT concat_lower_or_upper('Hello', 'W
*** 2527,2534
note
para
! Named and mixed call notations can currently be used only with regular
! functions, not with aggregate functions or window functions.
/para
/note
/sect2
--- 2527,2535
note
para
! Named and mixed call notations currently cannot be used when calling an
! aggregate function (but they do work when an aggregate function is used
! as a window function).
/para
/note
/sect2
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 76c032c..4fa73a9 100644
*** a/src/backend/optimizer/util/clauses.c
--- b/src/backend/optimizer/util/clauses.c
*** eval_const_expressions_mutator(Node *nod
*** 2251,2256
--- 2251,2306
*/
return (Node *) copyObject(param);
}
+ case T_WindowFunc:
+ {
+ WindowFunc *expr = (WindowFunc *) node;
+ Oid funcid = expr-winfnoid;
+ List *args;
+ Expr *aggfilter;
+ HeapTuple func_tuple;
+ WindowFunc *newexpr;
+
+ /*
+ * We can't really simplify a WindowFunc node, but we mustn't
+ * just fall through to the default processing, because we
+ * have to apply expand_function_arguments to its argument
+ * list. That takes care of inserting default arguments and
+ * expanding named-argument notation.
+ */
+ func_tuple = SearchSysCache1(PROCOID, ObjectIdGetDatum(funcid));
+ if (!HeapTupleIsValid(func_tuple))
+ elog(ERROR, cache lookup failed for function %u, funcid);
+
+ args = expand_function_arguments(expr-args, expr-wintype,
+ func_tuple);
+
+ ReleaseSysCache(func_tuple);
+
+ /* Now, recursively simplify the args (which are a List) */
+ args = (List *)
+ expression_tree_mutator((Node *) args,
+ eval_const_expressions_mutator,
+ (void *) context);
+ /* ... and the filter expression, which isn't */
+ aggfilter = (Expr *)
+ eval_const_expressions_mutator((Node *) expr-aggfilter,
+ context);
+
+ /* And build the replacement WindowFunc node */
+ newexpr = makeNode(WindowFunc);
+ newexpr-winfnoid = expr-winfnoid;
+ newexpr-wintype = expr-wintype;
+ newexpr-wincollid = expr-wincollid;
+ newexpr-inputcollid = expr-inputcollid;
+ newexpr-args = args;
+ newexpr-aggfilter = aggfilter;
+ newexpr-winref = expr-winref;
+ newexpr-winstar = expr-winstar;
+ newexpr-winagg = expr-winagg;
+ newexpr-location = expr-location;
+
+ return (Node *) newexpr;
+ }
case T_FuncExpr:
{
FuncExpr *expr = (FuncExpr *) node;
diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index 2bd24c8..ede36d1 100644
*** a/src/backend/parser/parse_func.c
--- b/src/backend/parser/parse_func.c
*** ParseFuncOrColumn(ParseState *pstate, Li
*** 537,553
errmsg(window functions cannot return sets),
parser_errposition(pstate, location)));
- /*
- * We might want to support this later, but for now reject it because
- * the planner and executor wouldn't cope with NamedArgExprs in a
- * WindowFunc node.
-
Re: [HACKERS] Window functions can be created with defaults, but they don't work
I wrote:
Attached is a proposed patch against HEAD that fixes this by supporting
default arguments properly for window functions. In passing, it also
allows named-argument notation in window function calls, since that's
free once the other thing works (because the same subroutine fixes up
both things).
Hm, well, almost free --- turns out ruleutils.c was assuming WindowFuncs
couldn't contain named arguments. Updated patch attached.
regards, tom lane
diff --git a/doc/src/sgml/syntax.sgml b/doc/src/sgml/syntax.sgml
index e3dbc4b..16aade4 100644
*** a/doc/src/sgml/syntax.sgml
--- b/doc/src/sgml/syntax.sgml
*** SELECT concat_lower_or_upper('Hello', 'W
*** 2527,2534
note
para
! Named and mixed call notations can currently be used only with regular
! functions, not with aggregate functions or window functions.
/para
/note
/sect2
--- 2527,2535
note
para
! Named and mixed call notations currently cannot be used when calling an
! aggregate function (but they do work when an aggregate function is used
! as a window function).
/para
/note
/sect2
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 76c032c..4fa73a9 100644
*** a/src/backend/optimizer/util/clauses.c
--- b/src/backend/optimizer/util/clauses.c
*** eval_const_expressions_mutator(Node *nod
*** 2251,2256
--- 2251,2306
*/
return (Node *) copyObject(param);
}
+ case T_WindowFunc:
+ {
+ WindowFunc *expr = (WindowFunc *) node;
+ Oid funcid = expr-winfnoid;
+ List *args;
+ Expr *aggfilter;
+ HeapTuple func_tuple;
+ WindowFunc *newexpr;
+
+ /*
+ * We can't really simplify a WindowFunc node, but we mustn't
+ * just fall through to the default processing, because we
+ * have to apply expand_function_arguments to its argument
+ * list. That takes care of inserting default arguments and
+ * expanding named-argument notation.
+ */
+ func_tuple = SearchSysCache1(PROCOID, ObjectIdGetDatum(funcid));
+ if (!HeapTupleIsValid(func_tuple))
+ elog(ERROR, cache lookup failed for function %u, funcid);
+
+ args = expand_function_arguments(expr-args, expr-wintype,
+ func_tuple);
+
+ ReleaseSysCache(func_tuple);
+
+ /* Now, recursively simplify the args (which are a List) */
+ args = (List *)
+ expression_tree_mutator((Node *) args,
+ eval_const_expressions_mutator,
+ (void *) context);
+ /* ... and the filter expression, which isn't */
+ aggfilter = (Expr *)
+ eval_const_expressions_mutator((Node *) expr-aggfilter,
+ context);
+
+ /* And build the replacement WindowFunc node */
+ newexpr = makeNode(WindowFunc);
+ newexpr-winfnoid = expr-winfnoid;
+ newexpr-wintype = expr-wintype;
+ newexpr-wincollid = expr-wincollid;
+ newexpr-inputcollid = expr-inputcollid;
+ newexpr-args = args;
+ newexpr-aggfilter = aggfilter;
+ newexpr-winref = expr-winref;
+ newexpr-winstar = expr-winstar;
+ newexpr-winagg = expr-winagg;
+ newexpr-location = expr-location;
+
+ return (Node *) newexpr;
+ }
case T_FuncExpr:
{
FuncExpr *expr = (FuncExpr *) node;
diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index 2bd24c8..ede36d1 100644
*** a/src/backend/parser/parse_func.c
--- b/src/backend/parser/parse_func.c
*** ParseFuncOrColumn(ParseState *pstate, Li
*** 537,553
errmsg(window functions cannot return sets),
parser_errposition(pstate, location)));
- /*
- * We might want to support this later, but for now reject it because
- * the planner and executor wouldn't cope with NamedArgExprs in a
- * WindowFunc node.
- */
- if (argnames != NIL)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg(window functions cannot use named arguments),
- parser_errposition(pstate, location)));
-
/* parse_agg.c does additional window-func-specific processing */
transformWindowFuncCall(pstate, wfunc, over);
--- 537,542
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 04b1c4f..915fb7a 100644
*** a/src/backend/utils/adt/ruleutils.c
--- b/src/backend/utils/adt/ruleutils.c
*** get_windowfunc_expr(WindowFunc *wfunc, d
*** 7461,7466
--- 7461,7467
StringInfo buf = context-buf;
Oid argtypes[FUNC_MAX_ARGS];
int nargs;
+ List *argnames;
ListCell *l;
if (list_length(wfunc-args) FUNC_MAX_ARGS)
*** get_windowfunc_expr(WindowFunc *wfunc, d
*** 7468,7485
(errcode(ERRCODE_TOO_MANY_ARGUMENTS),
errmsg(too many arguments)));
nargs = 0;
foreach(l, wfunc-args)
{
Node *arg = (Node *) lfirst(l);
! Assert(!IsA(arg,
[HACKERS] Window functions can be created with defaults, but they don't work
I noticed this while poking at the variadic-aggregates issue: regression=# create function nth_value_def(anyelement, integer = 1) returns anyelement language internal window immutable strict as 'window_nth_value'; CREATE FUNCTION regression=# SELECT nth_value_def(ten) OVER (PARTITION BY four), ten, four FROM (SELECT * FROM tenk1 WHERE unique2 10 ORDER BY four, ten)s; The connection to the server was lost. Attempting reset: Failed. The reason this crashes is that the planner doesn't apply default-insertion to WindowFunc nodes, only to FuncExprs. We could make it do that, probably, but that seems to me like a feature addition. I think a more reasonable approach for back-patching is to fix function creation to disallow attaching defaults to window functions (or aggregates, for that matter, which would have the same issue if CREATE AGGREGATE had the syntax option to specify defaults). ProcedureCreate seems like an appropriate place, since it already contains a lot of sanity checks of this sort. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Window functions can be created with defaults, but they don't work
On Fri, Aug 30, 2013 at 6:14 PM, Tom Lane t...@sss.pgh.pa.us wrote: I noticed this while poking at the variadic-aggregates issue: regression=# create function nth_value_def(anyelement, integer = 1) returns anyelement language internal window immutable strict as 'window_nth_value'; CREATE FUNCTION regression=# SELECT nth_value_def(ten) OVER (PARTITION BY four), ten, four FROM (SELECT * FROM tenk1 WHERE unique2 10 ORDER BY four, ten)s; The connection to the server was lost. Attempting reset: Failed. The reason this crashes is that the planner doesn't apply default-insertion to WindowFunc nodes, only to FuncExprs. We could make it do that, probably, but that seems to me like a feature addition. I think a more reasonable approach for back-patching is to fix function creation to disallow attaching defaults to window functions (or aggregates, for that matter, which would have the same issue if CREATE AGGREGATE had the syntax option to specify defaults). ProcedureCreate seems like an appropriate place, since it already contains a lot of sanity checks of this sort. I'm not sure I agree. Under that approach, any functions that have already been created like that will still crash the server. A malicious user could create a function like this now and wait to crontab it until the day he's leaving the company. Or there are more accidental scenarios as well. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Window functions can be created with defaults, but they don't work
Robert Haas robertmh...@gmail.com writes: On Fri, Aug 30, 2013 at 6:14 PM, Tom Lane t...@sss.pgh.pa.us wrote: The reason this crashes is that the planner doesn't apply default-insertion to WindowFunc nodes, only to FuncExprs. I'm not sure I agree. Under that approach, any functions that have already been created like that will still crash the server. A malicious user could create a function like this now and wait to crontab it until the day he's leaving the company. Or there are more accidental scenarios as well. The crash is only possible because the underlying internal-language function doesn't sanity-check its input enough to catch the case of too few arguments. As such, it's not that different from hundreds of other cases where a superuser can cause a crash by misdeclaring the arguments to an internal-language function. So I don't find your argument compelling. I'd even say this was user error, except that it's not obvious that this case shouldn't work. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
