Обсуждение: 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
immutablestrict 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
On Fri, Aug 30, 2013 at 6:14 PM, Tom Lane <tgl@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 immutablestrict 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
Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Aug 30, 2013 at 6:14 PM, Tom Lane <tgl@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
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
immutablestrict 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.
- */
- 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/test/regress/expected/window.out b/src/test/regress/expected/window.out
index 7b31d13..1e6365b 100644
*** a/src/test/regress/expected/window.out
--- b/src/test/regress/expected/window.out
*************** FROM empsalary GROUP BY depname;
*** 1035,1037 ****
--- 1035,1072 ----
-- cleanup
DROP TABLE empsalary;
+ -- test user-defined window function with named args and default args
+ CREATE FUNCTION nth_value_def(val anyelement, n integer = 1) RETURNS anyelement
+ LANGUAGE internal WINDOW IMMUTABLE STRICT AS 'window_nth_value';
+ SELECT nth_value_def(n := 2, val := ten) OVER (PARTITION BY four), ten, four
+ FROM (SELECT * FROM tenk1 WHERE unique2 < 10 ORDER BY four, ten) s;
+ nth_value_def | ten | four
+ ---------------+-----+------
+ 0 | 0 | 0
+ 0 | 0 | 0
+ 0 | 4 | 0
+ 1 | 1 | 1
+ 1 | 1 | 1
+ 1 | 7 | 1
+ 1 | 9 | 1
+ | 0 | 2
+ 3 | 1 | 3
+ 3 | 3 | 3
+ (10 rows)
+
+ SELECT nth_value_def(ten) OVER (PARTITION BY four), ten, four
+ FROM (SELECT * FROM tenk1 WHERE unique2 < 10 ORDER BY four, ten) s;
+ nth_value_def | ten | four
+ ---------------+-----+------
+ 0 | 0 | 0
+ 0 | 0 | 0
+ 0 | 4 | 0
+ 1 | 1 | 1
+ 1 | 1 | 1
+ 1 | 7 | 1
+ 1 | 9 | 1
+ 0 | 0 | 2
+ 1 | 1 | 3
+ 1 | 3 | 3
+ (10 rows)
+
diff --git a/src/test/regress/sql/window.sql b/src/test/regress/sql/window.sql
index 6ee3696..7297e62 100644
*** a/src/test/regress/sql/window.sql
--- b/src/test/regress/sql/window.sql
*************** FROM empsalary GROUP BY depname;
*** 274,276 ****
--- 274,286 ----
-- cleanup
DROP TABLE empsalary;
+
+ -- test user-defined window function with named args and default args
+ CREATE FUNCTION nth_value_def(val anyelement, n integer = 1) RETURNS anyelement
+ LANGUAGE internal WINDOW IMMUTABLE STRICT AS 'window_nth_value';
+
+ SELECT nth_value_def(n := 2, val := ten) OVER (PARTITION BY four), ten, four
+ FROM (SELECT * FROM tenk1 WHERE unique2 < 10 ORDER BY four, ten) s;
+
+ SELECT nth_value_def(ten) OVER (PARTITION BY four), ten, four
+ FROM (SELECT * FROM tenk1 WHERE unique2 < 10 ORDER BY four, ten) s;
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, NamedArgExpr));
argtypes[nargs] = exprType(arg);
nargs++;
}
appendStringInfo(buf, "%s(",
generate_function_name(wfunc->winfnoid, nargs,
! NIL, argtypes,
false, NULL));
/* winstar can be set only in zero-argument aggregates */
if (wfunc->winstar)
--- 7469,7488 ----
(errcode(ERRCODE_TOO_MANY_ARGUMENTS),
errmsg("too many arguments")));
nargs = 0;
+ argnames = NIL;
foreach(l, wfunc->args)
{
Node *arg = (Node *) lfirst(l);
! if (IsA(arg, NamedArgExpr))
! argnames = lappend(argnames, ((NamedArgExpr *) arg)->name);
argtypes[nargs] = exprType(arg);
nargs++;
}
appendStringInfo(buf, "%s(",
generate_function_name(wfunc->winfnoid, nargs,
! argnames, argtypes,
false, NULL));
/* winstar can be set only in zero-argument aggregates */
if (wfunc->winstar)
diff --git a/src/test/regress/expected/window.out b/src/test/regress/expected/window.out
index 7b31d13..1e6365b 100644
*** a/src/test/regress/expected/window.out
--- b/src/test/regress/expected/window.out
*************** FROM empsalary GROUP BY depname;
*** 1035,1037 ****
--- 1035,1072 ----
-- cleanup
DROP TABLE empsalary;
+ -- test user-defined window function with named args and default args
+ CREATE FUNCTION nth_value_def(val anyelement, n integer = 1) RETURNS anyelement
+ LANGUAGE internal WINDOW IMMUTABLE STRICT AS 'window_nth_value';
+ SELECT nth_value_def(n := 2, val := ten) OVER (PARTITION BY four), ten, four
+ FROM (SELECT * FROM tenk1 WHERE unique2 < 10 ORDER BY four, ten) s;
+ nth_value_def | ten | four
+ ---------------+-----+------
+ 0 | 0 | 0
+ 0 | 0 | 0
+ 0 | 4 | 0
+ 1 | 1 | 1
+ 1 | 1 | 1
+ 1 | 7 | 1
+ 1 | 9 | 1
+ | 0 | 2
+ 3 | 1 | 3
+ 3 | 3 | 3
+ (10 rows)
+
+ SELECT nth_value_def(ten) OVER (PARTITION BY four), ten, four
+ FROM (SELECT * FROM tenk1 WHERE unique2 < 10 ORDER BY four, ten) s;
+ nth_value_def | ten | four
+ ---------------+-----+------
+ 0 | 0 | 0
+ 0 | 0 | 0
+ 0 | 4 | 0
+ 1 | 1 | 1
+ 1 | 1 | 1
+ 1 | 7 | 1
+ 1 | 9 | 1
+ 0 | 0 | 2
+ 1 | 1 | 3
+ 1 | 3 | 3
+ (10 rows)
+
diff --git a/src/test/regress/sql/window.sql b/src/test/regress/sql/window.sql
index 6ee3696..7297e62 100644
*** a/src/test/regress/sql/window.sql
--- b/src/test/regress/sql/window.sql
*************** FROM empsalary GROUP BY depname;
*** 274,276 ****
--- 274,286 ----
-- cleanup
DROP TABLE empsalary;
+
+ -- test user-defined window function with named args and default args
+ CREATE FUNCTION nth_value_def(val anyelement, n integer = 1) RETURNS anyelement
+ LANGUAGE internal WINDOW IMMUTABLE STRICT AS 'window_nth_value';
+
+ SELECT nth_value_def(n := 2, val := ten) OVER (PARTITION BY four), ten, four
+ FROM (SELECT * FROM tenk1 WHERE unique2 < 10 ORDER BY four, ten) s;
+
+ SELECT nth_value_def(ten) OVER (PARTITION BY four), ten, four
+ FROM (SELECT * FROM tenk1 WHERE unique2 < 10 ORDER BY four, ten) s;