Re: Another issue in default-values patch: defaults expanded too soon

Consider

create function foo(f1 int, f2 int = 42, f2 int = 43) ...
create view v1 as select foo(11);

In CVS HEAD this gives

regression=# \d v1
View "public.v1"
Column | Type | Modifiers
--------+---------+-----------
foo | integer |
View definition:
SELECT foo(11, 42, 43) AS foo;

which is an accurate representation of the truth: if you change the
defaults for function foo, v1 will keep on calling it with the old
default values.

Does anyone think this is either unsurprising or desirable?

I'm not sure we can do much to fix it, though. It'd probably be
possible to have the rewriter or planner insert the default expressions,
instead of the parser; but that creates its own issues. Suppose I had
v1 defined as above and then did

create or replace function foo(f1 int, f2 int, f2 int = 43) ...

ie, remove one or more default expressions. *This function definition
no longer matches the original call*. If we did plan-time insertion of
defaults we'd have little choice but to fail when v1 is executed,
because there'd be no principled way to insert a default for f2.
Treating the defaults as being inserted at parse time at least ensures
that v1's call to foo still works.

This at least needs documentation, I think.

Comments?

regards, tom lane

On Dec 16, 2008, at 9:25 PM, Tom Lane wrote:

> Consider
>
> create function foo(f1 int, f2 int = 42, f2 int = 43) ...
> create view v1 as select foo(11);
>
> In CVS HEAD this gives
>
> regression=# \d v1
> View "public.v1"
> Column | Type | Modifiers
> --------+---------+-----------
> foo | integer |
> View definition:
> SELECT foo(11, 42, 43) AS foo;
>
> which is an accurate representation of the truth: if you change the
> defaults for function foo, v1 will keep on calling it with the old
> default values.

Ooh. Ow.

> Does anyone think this is either unsurprising or desirable?

Not I!

> I'm not sure we can do much to fix it, though. It'd probably be
> possible to have the rewriter or planner insert the default
> expressions,
> instead of the parser; but that creates its own issues.

Would the same thing happen for a prepared statement that calls the
function? Or another function?

> Suppose I had v1 defined as above and then did
>
> create or replace function foo(f1 int, f2 int, f2 int = 43) ...
>
> ie, remove one or more default expressions. *This function definition
> no longer matches the original call*. If we did plan-time insertion
> of
> defaults we'd have little choice but to fail when v1 is executed,
> because there'd be no principled way to insert a default for f2.

That seems like it'd be the reasonable thing to do.

> Treating the defaults as being inserted at parse time at least ensures
> that v1's call to foo still works.

That leads to mysterious action-at-a-distance bugs, though. Too weird.

> This at least needs documentation, I think.
>
> Comments?

Documentation at least, yes, but it'd be better, I think, if the
planner inserted the defaults.

Best,

David

"David E. Wheeler" <david(at)kineticode(dot)com> writes:
> On Dec 16, 2008, at 9:25 PM, Tom Lane wrote:
>> I'm not sure we can do much to fix it, though. It'd probably be
>> possible to have the rewriter or planner insert the default
>> expressions, instead of the parser; but that creates its own issues.

> Would the same thing happen for a prepared statement that calls the
> function? Or another function?

Prepared statements and functions don't have such a problem, because
they don't have a long-lived parsetree representation. If you change
the defaults attached to an existing function, that will result in an
invalidation on all plans referencing the function (thanks to some
changes that went in a commitfest or two back), and things should
pretty much "just work". A view or rule is a bigger problem because
it will have a hard binding to a particular function OID.

>> Suppose I had v1 defined as above and then did
>>
>> create or replace function foo(f1 int, f2 int, f2 int = 43) ...
>>
>> ie, remove one or more default expressions. *This function definition
>> no longer matches the original call*. If we did plan-time insertion of
>> defaults we'd have little choice but to fail when v1 is executed,
>> because there'd be no principled way to insert a default for f2.

> That seems like it'd be the reasonable thing to do.

I'm not too thrilled about it. One thing to consider is that with the
default gone, it might be that there is some other function that matches
the textual call better than this one. But we can't really change the
view to reference that other function. So it's going to work
differently than the replan-from-source case, no matter what.

In some ways this is analogous to the problem of "when do you expand *
in a SELECT list?". The SQL spec is clear that it's expanded at CREATE
VIEW time; but there are certainly lots of cases where people wish it
didn't work like that.

regards, tom lane

On Dec 16, 2008, at 10:15 PM, Tom Lane wrote:

>> Would the same thing happen for a prepared statement that calls the
>> function? Or another function?
>
> Prepared statements and functions don't have such a problem, because
> they don't have a long-lived parsetree representation. If you change
> the defaults attached to an existing function, that will result in an
> invalidation on all plans referencing the function (thanks to some
> changes that went in a commitfest or two back), and things should
> pretty much "just work". A view or rule is a bigger problem because
> it will have a hard binding to a particular function OID.

Well, that's good, except that the behavior then becomes inconsistent
in rules and views vs. everything else. I understand the underlying
reasons for this (thanks to your reply here), but those not familiar
with the architecture will find it mystifying. Documentation, at the
very least, is certainly in order.

>> That seems like it'd be the reasonable thing to do.
>
> I'm not too thrilled about it. One thing to consider is that with the
> default gone, it might be that there is some other function that
> matches
> the textual call better than this one. But we can't really change the
> view to reference that other function. So it's going to work
> differently than the replan-from-source case, no matter what.

Bleh.

> In some ways this is analogous to the problem of "when do you expand *
> in a SELECT list?". The SQL spec is clear that it's expanded at
> CREATE
> VIEW time; but there are certainly lots of cases where people wish it
> didn't work like that.

Yeah, function default values aren't specified by the spec, are they?

Best,

David

"David E. Wheeler" <david(at)kineticode(dot)com> writes:
> On Dec 16, 2008, at 10:15 PM, Tom Lane wrote:
>> I'm not too thrilled about it. One thing to consider is that with the
>> default gone, it might be that there is some other function that
>> matches
>> the textual call better than this one. But we can't really change the
>> view to reference that other function. So it's going to work
>> differently than the replan-from-source case, no matter what.

> Bleh.

I haven't really thought through the consequences of this, but one thing
we could consider doing to tamp down the damage is to prohibit changing
the number of defaultable parameters of an existing function, ie treat
pronargdefaults as not allowed to change during CREATE OR REPLACE
FUNCTION. The point here would be to ensure that function replacement
couldn't change the parser's decisions about whether a function matches
a call or not; which is the case in existing releases, but has been
falsified by this patch.

If that's acceptable, then we could insert default expressions at plan
time with confidence that no defaults we need have disappeared under us.

(It might be enough to not allow pronargdefaults to decrease. Not sure
about that though. It's certainly possible that adding a default could
make a call ambiguous when it was not before.)

There's another slightly annoying issue here, which is that in at least
some cases, inserting defaults at plan time would require an additional
traversal of the parsetree. This isn't a huge deal probably, but it
would result in some performance loss in long-but-simple queries.

regards, tom lane

On Dec 16, 2008, at 10:36 PM, Tom Lane wrote:

> I haven't really thought through the consequences of this, but one
> thing
> we could consider doing to tamp down the damage is to prohibit
> changing
> the number of defaultable parameters of an existing function, ie treat
> pronargdefaults as not allowed to change during CREATE OR REPLACE
> FUNCTION. The point here would be to ensure that function replacement
> couldn't change the parser's decisions about whether a function
> matches
> a call or not; which is the case in existing releases, but has been
> falsified by this patch.
>
> If that's acceptable, then we could insert default expressions at plan
> time with confidence that no defaults we need have disappeared under
> us.

Wouldn't you still have the problem if you still allow the default
values to be changed? And I would hope that they could be changed!

> There's another slightly annoying issue here, which is that in at
> least
> some cases, inserting defaults at plan time would require an
> additional
> traversal of the parsetree. This isn't a huge deal probably, but it
> would result in some performance loss in long-but-simple queries.

Yes, and it avoids the principal of least surprise.

Best,

David

"David E. Wheeler" <david(at)kineticode(dot)com> writes:
> On Dec 16, 2008, at 10:36 PM, Tom Lane wrote:
>> ... The point here would be to ensure that function replacement
>> couldn't change the parser's decisions about whether a function matches
>> a call or not; which is the case in existing releases, but has been
>> falsified by this patch.
>>
>> If that's acceptable, then we could insert default expressions at plan
>> time with confidence that no defaults we need have disappeared under
>> us.

> Wouldn't you still have the problem if you still allow the default
> values to be changed? And I would hope that they could be changed!

No, you could change the *values* of the default expressions. What
you'd not be allowed to do is remove a default entirely. (Or, perhaps,
add one; I'm less sure about that.) The point here is that adding or
removing a default changes the set of calls a function could possibly
match, just as changing the list of parameter types changes what it
can match. We don't allow the latter and I'm thinking we shouldn't
allow the former either.

regards, tom lane

And while we're on the subject ... as the patch stands, it lets you
provide defaults for polymorphic parameters, as in

regression=# create function eq (f1 anyelement, f2 anyelement default 42) returns bool as 'select $1 = $2' language sql;
CREATE FUNCTION
regression=# select eq(now(),now());
eq
----
t
(1 row)

regression=# select eq(now());
ERROR: arguments declared "anyelement" are not all alike
DETAIL: timestamp with time zone versus integer
regression=# select eq(11,12);
eq
----
f
(1 row)

regression=# select eq(11);
eq
----
f
(1 row)

regression=# select eq(42);
eq
----
t
(1 row)

The reason this is pretty wacko is that changing the default can change the
set of calls the function can match:

regression=# create or replace function eq (f1 anyelement, f2 anyelement default now()) returns bool as 'select $1 = $2' language sql;
CREATE FUNCTION
regression=# select eq(now());
eq
----
t
(1 row)

regression=# select eq(42);
ERROR: arguments declared "anyelement" are not all alike
DETAIL: integer versus timestamp with time zone

In fact, it's worse than that: changing the default can change the
resolved output type of the function.

regression=# create function dupl (f1 anyelement default 42) returns anyelement
as 'select $1' language sql;
CREATE FUNCTION
regression=# select dupl();
dupl
------
42
(1 row)

regression=# create or replace function dupl (f1 anyelement default now()) returns anyelement
as 'select $1' language sql;
CREATE FUNCTION
regression=# select dupl();
dupl
-------------------------------
2008-12-16 17:39:41.418412-05
(1 row)

Isn't *that* special. Needless to say, this would utterly break any
view or rule referencing the function.

I'm inclined to think we should forbid defaults for polymorphic
parameters, and wondered if anyone can come up with an actually useful
use-case that'd require it. If we were going to allow it, we'd at least
have to restrict things enough so that the resolved output type couldn't
change.

(The reason I stumbled across this was that the current behavior is an
artifact of inserting the default expressions at parse time; in fact,
before resolving polymorphic types. It would get very much more painful
to support any sort of behavior along this line if we don't do that.)

regards, tom lane

Tom Lane wrote:
> Consider
>
> create function foo(f1 int, f2 int = 42, f2 int = 43) ...
> create view v1 as select foo(11);
>
> In CVS HEAD this gives
>
> regression=# \d v1
> View "public.v1"
> Column | Type | Modifiers
> --------+---------+-----------
> foo | integer |
> View definition:
> SELECT foo(11, 42, 43) AS foo;
>
> which is an accurate representation of the truth: if you change the
> defaults for function foo, v1 will keep on calling it with the old
> default values.
>
> Does anyone think this is either unsurprising or desirable?

Huh? Shouldn't changing a function which a view depends on require a
rebuild of the view?

That is, I think we should treat changing the defaults the same as we
would changing the number and type of parameters; it kicks off a
dependency check and requires a CASCADE.

--Josh

Josh Berkus <josh(at)agliodbs(dot)com> writes:
> Huh? Shouldn't changing a function which a view depends on require a
> rebuild of the view?

There is no such concept as "a rebuild of the view".

> That is, I think we should treat changing the defaults the same as we
> would changing the number and type of parameters; it kicks off a
> dependency check and requires a CASCADE.

Dream on ... there is no such facility in Postgres and we are not going
to build one in the 8.4 timeframe.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

> Josh Berkus <josh(at)agliodbs(dot)com> writes:
>
>> That is, I think we should treat changing the defaults the same as we
>> would changing the number and type of parameters; it kicks off a
>> dependency check and requires a CASCADE.
>
> Dream on ... there is no such facility in Postgres and we are not going
> to build one in the 8.4 timeframe.

Well there is this:

postgres=# create or replace function foo (text) returns text as 'select 1' language sql;
ERROR: 42P13: cannot change return type of existing function
HINT: Use DROP FUNCTION first.
LOCATION: ProcedureCreate, pg_proc.c:366

We could say that changing the type of a default argument for a polymorphic
argument isn't allowed just like changing the return value.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's PostGIS support!

Gregory Stark <stark(at)enterprisedb(dot)com> writes:
> We could say that changing the type of a default argument for a polymorphic
> argument isn't allowed just like changing the return value.

The point I was trying to make is that allowing defaults for polymorphic
args at all is going to cause a very significant amount of work and
complication, of which enforcing the above check is just a small part.
I wanted to see a plausible use-case for it before expending that work.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

> Gregory Stark <stark(at)enterprisedb(dot)com> writes:
>> We could say that changing the type of a default argument for a polymorphic
>> argument isn't allowed just like changing the return value.
>
> The point I was trying to make is that allowing defaults for polymorphic
> args at all is going to cause a very significant amount of work and
> complication, of which enforcing the above check is just a small part.
> I wanted to see a plausible use-case for it before expending that work.

Well honestly I don't see a terribly compelling use case for default arguments
altogether. Obviously they're just a programmer convenience and don't really
let anyone do anything they couldn't do without them.

So it's not like any use case for default polymorphic arguments is going to be
especially compelling either. But I don't see any reason it'll be any less
useful for polymorphic arguments than any other type.

The fundamental problem with polymorphic parameters and default arguments is
that the type of the argument determines the type of the return value. And the
type of the return value can't change within an existing parse tree -- so it
seems to me that barring changing the type of a default argument for a
polymorphic parameter is precisely targeted to the source and should cover all
problems it causes.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's 24x7 Postgres support!

Greg,

> Well honestly I don't see a terribly compelling use case for default arguments
> altogether. Obviously they're just a programmer convenience and don't really
> let anyone do anything they couldn't do without them.

The primary use case is for people who are porting applications from
other DBMSes. Which don't support polymorphic arguments in the first place.

--Josh

Gregory Stark <stark(at)enterprisedb(dot)com> writes:
> So it's not like any use case for default polymorphic arguments is going to be
> especially compelling either. But I don't see any reason it'll be any less
> useful for polymorphic arguments than any other type.

Well, the reason it seems funny to me is that the point of a polymorphic
argument is to be datatype-agnostic, and as soon as you put in a default
value of a particular type you're letting go of that principle.

The only case I can imagine being actually useful is where the default
is an untyped null, and you have a non-defaultable anyelement argument
that drives determination of what the defaulted one is, ie something
like

create function array2 (f1 anyelement, f2 anyelement = null)
returns anyarray
as 'select array[$1,$2]' language sql;

Although this seems well-defined in principle, I'm not sure if it
actually works with the current patch; and it'd be even trickier
if we try to put in some code that says "the default's type can't
change". A null constant has the same type as an unmarked string
literal (namely UNKNOWN) but IIRC they don't really behave the same
for type resolution purposes.

regards, tom lane

On Tue, Dec 16, 2008 at 3:25 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Does anyone think this is either unsurprising or desirable?

That's horrible.

I wonder whether the whole architecture is wrong here. Perhaps when a
function is created with N arguments of which M have default values,
we should actually create M entries in pg_proc: one for each possible
number of arguments from N-M up through N. The one with N arguments
would be the main entry, and the rest would be dependent entries that
would get dropped if the main entry were dropped (similar to the way
sequences for serial columns depend on the parent table). If one of
the dependent entries conflicted with an existing entry, then CREATE
FUNCTION would simply fail.

I think this would kill all of the problems reported thus far at one
blow. There wouldn't be any need for code to resolve ambiguous
function calls because there wouldn't be any ambiguous function calls,
or at least no more than what already exists in 8.3. The problem you
report here would go away because the view definition would match one
of the dummy entries. Removing a necessary default value would remove
that dummy entry, which would be caught by the existing dependency
stuff. Changing a default would amount to changing the definition of
a dummy entry as if by CREATE OR REPLACE FUNCTION, which would work
just as expected.

...Robert

"Robert Haas" <robertmhaas(at)gmail(dot)com> writes:
> I wonder whether the whole architecture is wrong here. Perhaps when a
> function is created with N arguments of which M have default values,
> we should actually create M entries in pg_proc: one for each possible
> number of arguments from N-M up through N.

That's been considered and rejected before, in the context of the
variadic-function patch which has a lot of the same issues. What it
mostly does is bloat pg_proc.

> I think this would kill all of the problems reported thus far at one
> blow.

No, it doesn't resolve any of them ... particularly not the ones
associated with defaults for polymorphics.

regards, tom lane

On Tue, Dec 16, 2008 at 9:18 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Robert Haas" <robertmhaas(at)gmail(dot)com> writes:
>> I wonder whether the whole architecture is wrong here. Perhaps when a
>> function is created with N arguments of which M have default values,
>> we should actually create M entries in pg_proc: one for each possible
>> number of arguments from N-M up through N.
>
> That's been considered and rejected before, in the context of the
> variadic-function patch which has a lot of the same issues. What it
> mostly does is bloat pg_proc.

Only if you have a large number of functions with a large number of
optional arguments each. That's possible, I suppose, but it hardly
seems likely, or worth worrying about.

>> I think this would kill all of the problems reported thus far at one
>> blow.
>
> No, it doesn't resolve any of them ... particularly not the ones
> associated with defaults for polymorphics.

I think that's hyperbole. You would probably still need to forbid
non-polymorphic defaults for polymorphic parameters (you might be able
to make NULL work, and maybe the empty array for anyarray... not
sure), but I think that most of the other issues you raised would be
addressed by my proposal. You may hate it anyway; I'm OK with that.
:-)

...Robert

2008/12/17 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Tue, Dec 16, 2008 at 9:18 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> "Robert Haas" <robertmhaas(at)gmail(dot)com> writes:
>>> I wonder whether the whole architecture is wrong here. Perhaps when a
>>> function is created with N arguments of which M have default values,
>>> we should actually create M entries in pg_proc: one for each possible
>>> number of arguments from N-M up through N.
>>
>> That's been considered and rejected before, in the context of the
>> variadic-function patch which has a lot of the same issues. What it
>> mostly does is bloat pg_proc.
>
> Only if you have a large number of functions with a large number of
> optional arguments each. That's possible, I suppose, but it hardly
> seems likely, or worth worrying about.
>
>>> I think this would kill all of the problems reported thus far at one
>>> blow.
>>
>> No, it doesn't resolve any of them ... particularly not the ones
>> associated with defaults for polymorphics.
>
> I think that's hyperbole. You would probably still need to forbid
> non-polymorphic defaults for polymorphic parameters (you might be able
> to make NULL work, and maybe the empty array for anyarray... not
> sure), but I think that most of the other issues you raised would be
> addressed by my proposal. You may hate it anyway; I'm OK with that.
> :-)

it's not good solution, problem is anywhere else - we raise exception
about ambigonous call to early, when we don't have all knowleadges.

Pavel

>
> ...Robert
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Gregory Stark wrote:
> Well honestly I don't see a terribly compelling use case for default arguments
> altogether. Obviously they're just a programmer convenience and don't really
> let anyone do anything they couldn't do without them.

The real payoff comes with name-based paramter lists (the name => value
busines) and allowing defaults anywhere in the parameter list (not just
at the end). This is required to port many PL/SQL-using applications,
and you can't write it any other way.