Обсуждение: Summary: changes needed in function defaults behavior

So to summarize what I think we agreed to yesterday:

* CREATE OR REPLACE FUNCTION has to prevent reducing the pronargdefaults
value of an existing function (ie, you can add more defaults but not
remove any).  This ensures a function that matched a given call before
will continue to do so.

* CREATE OR REPLACE FUNCTION also has to prevent a change in the actual
datatype of the default for any polymorphic parameter.  This ensures
that we won't come to a different conclusion about the result type or
the coerced type of any non-defaulted parameter.

* Variadic parameters should be allowed to have defaults (where the
default would typically be an empty array of the right type, though
other possibilities are imaginable).  Requiring the zero-element
case to be treated as a default makes the default and variadic features
act independently, and it avoids a problematic case for variadic
anyarray.

* Two functions that could match a given call after adding defaults
are considered ambiguous only if they would add the same number of
defaults; otherwise we prefer the one with fewer parameters.  This
generalizes the rule that an exact match (no defaults) is preferred
over one that requires adding defaults.

* The parser should *not* add the values of the default expressions
into the parse tree, because that would lock down possibly-obsolete
values into stored views.  Instead the same work has to be repeated
at plan time.  (It's okay for the planner to do it since we already
have a mechanism to invalidate cached plans when the functions they
call are changed.)  We are willing to tolerate some performance loss
in planning speed for this feature.

Barring objections I'll get started on making these changes.
        regards, tom lane

2008/12/17 Tom Lane <tgl@sss.pgh.pa.us>:
> So to summarize what I think we agreed to yesterday:
>
> * CREATE OR REPLACE FUNCTION has to prevent reducing the pronargdefaults
> value of an existing function (ie, you can add more defaults but not
> remove any).  This ensures a function that matched a given call before
> will continue to do so.
>
> * CREATE OR REPLACE FUNCTION also has to prevent a change in the actual
> datatype of the default for any polymorphic parameter.  This ensures
> that we won't come to a different conclusion about the result type or
> the coerced type of any non-defaulted parameter.
>
> * Variadic parameters should be allowed to have defaults (where the
> default would typically be an empty array of the right type, though
> other possibilities are imaginable).  Requiring the zero-element
> case to be treated as a default makes the default and variadic features
> act independently, and it avoids a problematic case for variadic
> anyarray.
>
> * Two functions that could match a given call after adding defaults
> are considered ambiguous only if they would add the same number of
> defaults; otherwise we prefer the one with fewer parameters.  This
> generalizes the rule that an exact match (no defaults) is preferred
> over one that requires adding defaults.
>
> * The parser should *not* add the values of the default expressions
> into the parse tree, because that would lock down possibly-obsolete
> values into stored views.  Instead the same work has to be repeated
> at plan time.  (It's okay for the planner to do it since we already
> have a mechanism to invalidate cached plans when the functions they
> call are changed.)  We are willing to tolerate some performance loss
> in planning speed for this feature.
>
> Barring objections I'll get started on making these changes.
>
>                        regards, tom lane
>

it's look well

regards
Pavel Stehule

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

I wrote:
> * Two functions that could match a given call after adding defaults
> are considered ambiguous only if they would add the same number of
> defaults; otherwise we prefer the one with fewer parameters.  This
> generalizes the rule that an exact match (no defaults) is preferred
> over one that requires adding defaults.

Experimenting with the revised code, I found a curious case that might
be worth worrying about.  Consider the example that started all this:

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

The patch I've got correctly reverse-lists v1 as "select foo(11)".
Now suppose we add

create function foo(f1 int, f2 int = 42) ...

or even

create function foo(f1 int) ...

The view is still gonna reverse-list as "select foo(11)" --- in fact,
we really haven't got much choice about that.  However, if dumped and
reloaded along with one of these shorter-argument-list functions, the
view will be reconstituted as a reference to the shorter function instead
of the original 3-argument function.

I'm not sure how critical this is, since you'd have to be pretty dumb to
put together a set of functions like this that didn't work compatibly.
Still, this is the first instance I know of in which dump/reload isn't
going to be guaranteed to match the same function as was being called
in the dumped database.

If we think this is critical enough to be worth sacrificing something
for, what I'd suggest is that we abandon the concept that shorter
argument lists are allowed to win over longer ones.  This would mean
that
foo(f1)foo(f1 int, f2 int = 42)foo(f1 int, f2 int = 42, f3 int = 43)

would all be considered equally good matches for a call foo(11)
and so you'd get an "ambiguous function" failure.  While that doesn't
prevent you getting into this sort of trouble, what it would do is
ensure that the dump reload gives an error instead of silently picking
the wrong function.  Also, you'd most likely have gotten a few failures
and thus been shown the error of your ways before you dumped the old
DB at all.

Thoughts?
        regards, tom lane

2008/12/17 Tom Lane <tgl@sss.pgh.pa.us>:
> I wrote:
>> * Two functions that could match a given call after adding defaults
>> are considered ambiguous only if they would add the same number of
>> defaults; otherwise we prefer the one with fewer parameters.  This
>> generalizes the rule that an exact match (no defaults) is preferred
>> over one that requires adding defaults.
>
> Experimenting with the revised code, I found a curious case that might
> be worth worrying about.  Consider the example that started all this:
>
> create function foo(f1 int, f2 int = 42, f3 int = 43) ...
> create view v1 as select foo(11);
>
> The patch I've got correctly reverse-lists v1 as "select foo(11)".
> Now suppose we add
>
> create function foo(f1 int, f2 int = 42) ...
>
> or even
>
> create function foo(f1 int) ...

do you remember on request for using "default" keyword in funccall?
This should be solution. In view, you don't store select foo(11), but
you have to store select foo(11, default, default).

regards
Pavel Stehule



>
> The view is still gonna reverse-list as "select foo(11)" --- in fact,
> we really haven't got much choice about that.  However, if dumped and
> reloaded along with one of these shorter-argument-list functions, the
> view will be reconstituted as a reference to the shorter function instead
> of the original 3-argument function.
>
> I'm not sure how critical this is, since you'd have to be pretty dumb to
> put together a set of functions like this that didn't work compatibly.
> Still, this is the first instance I know of in which dump/reload isn't
> going to be guaranteed to match the same function as was being called
> in the dumped database.
>
> If we think this is critical enough to be worth sacrificing something
> for, what I'd suggest is that we abandon the concept that shorter
> argument lists are allowed to win over longer ones.  This would mean
> that
>
>        foo(f1)
>        foo(f1 int, f2 int = 42)
>        foo(f1 int, f2 int = 42, f3 int = 43)
>
> would all be considered equally good matches for a call foo(11)
> and so you'd get an "ambiguous function" failure.  While that doesn't
> prevent you getting into this sort of trouble, what it would do is
> ensure that the dump reload gives an error instead of silently picking
> the wrong function.  Also, you'd most likely have gotten a few failures
> and thus been shown the error of your ways before you dumped the old
> DB at all.
>
> Thoughts?
>
>                        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
>

"Pavel Stehule" <pavel.stehule@gmail.com> writes:
> 2008/12/17 Tom Lane <tgl@sss.pgh.pa.us>:
>> Experimenting with the revised code, I found a curious case that might
>> be worth worrying about.  Consider the example that started all this:

> do you remember on request for using "default" keyword in funccall?
> This should be solution. In view, you don't store select foo(11), but
> you have to store select foo(11, default, default).

Seems pretty ugly; keep in mind you'd be looking at that notation
constantly (in \d, EXPLAIN, etc), not just in dumps.

I think the most conservative thing to do is to treat varying numbers of
defaults as ambiguous for now.  We can relax that later without breaking
working code, but we couldn't go the other way if something else comes
up.
        regards, tom lane

2008/12/18 Tom Lane <tgl@sss.pgh.pa.us>:
> "Pavel Stehule" <pavel.stehule@gmail.com> writes:
>> 2008/12/17 Tom Lane <tgl@sss.pgh.pa.us>:
>>> Experimenting with the revised code, I found a curious case that might
>>> be worth worrying about.  Consider the example that started all this:
>
>> do you remember on request for using "default" keyword in funccall?
>> This should be solution. In view, you don't store select foo(11), but
>> you have to store select foo(11, default, default).
>
> Seems pretty ugly; keep in mind you'd be looking at that notation
> constantly (in \d, EXPLAIN, etc), not just in dumps.
>

yes, it's not perfect - and I have to agree, prepared statements,
views should by (and it is) problem. I didn't expect it. On second
hand (practical view) most of functions with defaults or variadic will
not be overloaded (it's not argument), so we could be more strict in
checking.

regards
Pavel Stehule

> I think the most conservative thing to do is to treat varying numbers of
> defaults as ambiguous for now.  We can relax that later without breaking
> working code, but we couldn't go the other way if something else comes
> up.
>
>                        regards, tom lane
>

2008/12/18 Pavel Stehule <pavel.stehule@gmail.com>:
> 2008/12/18 Tom Lane <tgl@sss.pgh.pa.us>:
>> "Pavel Stehule" <pavel.stehule@gmail.com> writes:
>>> 2008/12/17 Tom Lane <tgl@sss.pgh.pa.us>:
>>>> Experimenting with the revised code, I found a curious case that might
>>>> be worth worrying about.  Consider the example that started all this:
>>
>>> do you remember on request for using "default" keyword in funccall?
>>> This should be solution. In view, you don't store select foo(11), but
>>> you have to store select foo(11, default, default).
>>
>> Seems pretty ugly; keep in mind you'd be looking at that notation
>> constantly (in \d, EXPLAIN, etc), not just in dumps.
>>
>
> yes, it's not perfect - and I have to agree, prepared statements,
> views should by (and it is) problem. I didn't expect it. On second
> hand (practical view) most of functions with defaults or variadic will
> not be overloaded (it's not argument), so we could be more strict in
> checking.

some primitive idea - we could to disallow defaults params for views.

Pavel

>
> regards
> Pavel Stehule
>
>> I think the most conservative thing to do is to treat varying numbers of
>> defaults as ambiguous for now.  We can relax that later without breaking
>> working code, but we couldn't go the other way if something else comes
>> up.
>>
>>                        regards, tom lane
>>
>

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

Le 18 déc. 08 à 00:56, Tom Lane a écrit :

> "Pavel Stehule" <pavel.stehule@gmail.com> writes:
>> do you remember on request for using "default" keyword in funccall?
>> This should be solution. In view, you don't store select foo(11), but
>> you have to store select foo(11, default, default).
>
> Seems pretty ugly; keep in mind you'd be looking at that notation
> constantly (in \d, EXPLAIN, etc), not just in dumps.

Well, considering it could solve the issue at hand without semantics
revisiting, it doesn't seem that ugly to me. Forcing the users of
default arguments to see they are using them all over the places is
fine by me.

And being able to put in default explicitly when calling the function
could also be a nice feature for people auto-generating calling code.  From what we get on IRC it's far easier for
peopleto special case   
DEFAULT rather than special case list constructions, as already
possible in INSERT statements.

> I think the most conservative thing to do is to treat varying
> numbers of
> defaults as ambiguous for now.  We can relax that later without
> breaking
> working code, but we couldn't go the other way if something else comes
> up.

If a user vote is worth anything, I'd vote for explicit default
notation please.

- -
dim
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Darwin)

iEYEARECAAYFAklKE8EACgkQlBXRlnbh1blAUwCeOPUglnybmjL1DKCoq56mB5Te
12MAn3zLhPJihx8LTVN8luELQrHMEQrJ
=+htG
-----END PGP SIGNATURE-----

Tom Lane wrote:
> So to summarize what I think we agreed to yesterday:
> 
> * CREATE OR REPLACE FUNCTION has to prevent reducing the pronargdefaults
> value of an existing function (ie, you can add more defaults but not
> remove any).  This ensures a function that matched a given call before
> will continue to do so.
> 
> * CREATE OR REPLACE FUNCTION also has to prevent a change in the actual
> datatype of the default for any polymorphic parameter.  This ensures
> that we won't come to a different conclusion about the result type or
> the coerced type of any non-defaulted parameter.

I assume there will be some documentation explaining why these
restrictions exist because I am not sure it will be obvious.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +

I would agree with making it stricter. It would force people to do less stupid things.
Our main use case for default parameter will be getting rid of all the old versions of functions with shorter parameter lists by just creating new versions of old functions with additional default parameters.

We don't use views much but all the fuss and restrictions that surround them gives me a feeling that there might be something to be improved in how they are implemented/hacked into the PostgreSQL.

What might be the use case for
       foo(f1 int)
       foo(f1 int, f2 int = 42)
       foo(f1 int, f2 int = 42, f3 int = 43)
?

When i have function in database
     foo(f1 int)
and do create or replace
     foo(f1 int, f2 int = 42)
I would expect foo to get replaced.

Current implementation seems to make us go through drop create sequence.

regards,
Asko

PS. Any chance for lifting the restriction for changing function return type without dropping the function.