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

On Fri, Aug 30, 2013 at 6:14 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Aug 30, 2013 at 6:14 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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 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

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