Re: polymorphic types - enforce casting to most common type automatically

Hello

now a functions with more than one polymorphic arguments are relative
fragile due missing casting to most common type. Some our "functions" like
"coalesce" can do it, so it is surprising for our users.

our custom polymorphic function foo(anyelement, anyelement) working well for

foo(10,20) or foo(10.1, 20.1), but not for foo(10, 20.1)

I am thinking, so we can add a searching most common type stage without
breaking to backing compatibility.

What do you think about it?

Regards

Pavel

Hi

I am sending a proof concept. Current implementation is not suboptimal - I
wrote this code for demonstration of current issues, and checking possible
side effects of changes in this patch.

The basic problem is strong restrictive implementation of polymorphic types
- now these types doesn't allow any cast although it is possible. It can be
changed relatively simply I though (after we implemented variadic
functions).

CREATE OR REPLACE FUNCTION public.foo1(anyelement, anyelement)
RETURNS anyelement
LANGUAGE sql
AS $function$
SELECT $1 + $2;
$function$

CREATE OR REPLACE FUNCTION public.foo2(anyelement, anyelement)
RETURNS anyarray
LANGUAGE sql
AS $function$
SELECT ARRAY[$1, $2]
$function$

Now, polymorphic functions don't allow some natively expected calls:

postgres=# select foo1(1,1);
foo1
------
2
(1 row)

postgres=# select foo1(1,1.1);
ERROR: function foo1(integer, numeric) does not exist
LINE 1: select foo1(1,1.1);
^
HINT: No function matches the given name and argument types. You might
need to add explicit type casts.

postgres=# select foo2(1,1);
foo2
-------
{1,1}
(1 row)

postgres=# select foo2(1,1.1);
ERROR: function foo2(integer, numeric) does not exist
LINE 1: select foo2(1,1.1);
^
HINT: No function matches the given name and argument types. You might
need to add explicit type casts.

CREATE OR REPLACE FUNCTION public.foo3(VARIADIC anyarray)
RETURNS anyelement
LANGUAGE sql
AS $function$
SELECT min(v) FROM unnest($1) g(v)
$function$

postgres=# SELECT foo3(1,2,3);
foo3
------
1
(1 row)

postgres=# SELECT foo3(1,2,3.1);
ERROR: function foo3(integer, integer, numeric) does not exist
LINE 1: SELECT foo3(1,2,3.1);
^
HINT: No function matches the given name and argument types. You might
need to add explicit type casts.

Some our functions like COALESCE are not too restrictive and allow to use
types from same category.

postgres=# select coalesce(1,1.1);
coalesce
----------
1
(1 row)

With attached patch the polymorphic functions use same mechanism as our
buildin functions. It is applied on ANYARRAY, ANYELEMENT types only.

postgres=# select foo1(1,1.1), foo2(1,1.1), foo3(1.1,2,3.1);
foo1 | foo2 | foo3
------+---------+------
2.1 | {1,1.1} | 1.1
(1 row)

Comments, notices, ... ?

Regards

Pavel

2014-11-24 20:52 GMT+01:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:

> Hello
>
> now a functions with more than one polymorphic arguments are relative
> fragile due missing casting to most common type. Some our "functions" like
> "coalesce" can do it, so it is surprising for our users.
>
> our custom polymorphic function foo(anyelement, anyelement) working well
> for
>
> foo(10,20) or foo(10.1, 20.1), but not for foo(10, 20.1)
>
> I am thinking, so we can add a searching most common type stage without
> breaking to backing compatibility.
>
> What do you think about it?
>
> Regards
>
> Pavel
>

2015-03-08 19:31 GMT+01:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:

> Hi
>
> I am sending a proof concept. Current implementation is not suboptimal - I
> wrote this code for demonstration of current issues, and checking possible
> side effects of changes in this patch.
>

I am sorry - typo " Current implementation (patch) ***is*** suboptimal"

Best regards

Pavel

>
> The basic problem is strong restrictive implementation of polymorphic
> types - now these types doesn't allow any cast although it is possible. It
> can be changed relatively simply I though (after we implemented variadic
> functions).
>
> CREATE OR REPLACE FUNCTION public.foo1(anyelement, anyelement)
> RETURNS anyelement
> LANGUAGE sql
> AS $function$
> SELECT $1 + $2;
> $function$
>
> CREATE OR REPLACE FUNCTION public.foo2(anyelement, anyelement)
> RETURNS anyarray
> LANGUAGE sql
> AS $function$
> SELECT ARRAY[$1, $2]
> $function$
>
> Now, polymorphic functions don't allow some natively expected calls:
>
> postgres=# select foo1(1,1);
> foo1
> ------
> 2
> (1 row)
>
> postgres=# select foo1(1,1.1);
> ERROR: function foo1(integer, numeric) does not exist
> LINE 1: select foo1(1,1.1);
> ^
> HINT: No function matches the given name and argument types. You might
> need to add explicit type casts.
>
> postgres=# select foo2(1,1);
> foo2
> -------
> {1,1}
> (1 row)
>
> postgres=# select foo2(1,1.1);
> ERROR: function foo2(integer, numeric) does not exist
> LINE 1: select foo2(1,1.1);
> ^
> HINT: No function matches the given name and argument types. You might
> need to add explicit type casts.
>
>
> CREATE OR REPLACE FUNCTION public.foo3(VARIADIC anyarray)
> RETURNS anyelement
> LANGUAGE sql
> AS $function$
> SELECT min(v) FROM unnest($1) g(v)
> $function$
>
> postgres=# SELECT foo3(1,2,3);
> foo3
> ------
> 1
> (1 row)
>
> postgres=# SELECT foo3(1,2,3.1);
> ERROR: function foo3(integer, integer, numeric) does not exist
> LINE 1: SELECT foo3(1,2,3.1);
> ^
> HINT: No function matches the given name and argument types. You might
> need to add explicit type casts.
>
>
> Some our functions like COALESCE are not too restrictive and allow to use
> types from same category.
>
> postgres=# select coalesce(1,1.1);
> coalesce
> ----------
> 1
> (1 row)
>
> With attached patch the polymorphic functions use same mechanism as our
> buildin functions. It is applied on ANYARRAY, ANYELEMENT types only.
>
> postgres=# select foo1(1,1.1), foo2(1,1.1), foo3(1.1,2,3.1);
> foo1 | foo2 | foo3
> ------+---------+------
> 2.1 | {1,1.1} | 1.1
> (1 row)
>
> Comments, notices, ... ?
>
> Regards
>
> Pavel
>
>
> 2014-11-24 20:52 GMT+01:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
>
>> Hello
>>
>> now a functions with more than one polymorphic arguments are relative
>> fragile due missing casting to most common type. Some our "functions" like
>> "coalesce" can do it, so it is surprising for our users.
>>
>> our custom polymorphic function foo(anyelement, anyelement) working well
>> for
>>
>> foo(10,20) or foo(10.1, 20.1), but not for foo(10, 20.1)
>>
>> I am thinking, so we can add a searching most common type stage without
>> breaking to backing compatibility.
>>
>> What do you think about it?
>>
>> Regards
>>
>> Pavel
>>
>
>

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> now a functions with more than one polymorphic arguments are relative
> fragile due missing casting to most common type. Some our "functions" like
> "coalesce" can do it, so it is surprising for our users.

> our custom polymorphic function foo(anyelement, anyelement) working well for
> foo(10,20) or foo(10.1, 20.1), but not for foo(10, 20.1)

> I am thinking, so we can add a searching most common type stage without
> breaking to backing compatibility.

> What do you think about it?

I see nobody's replied to this, still, so ...

I think this is simply a bad idea, for a couple of reasons:

1. It will reduce predictability of type resolution.

2. It will greatly increase the risk of getting "ambiguous function call"
failures, because of adding more possible ways to match the same call.
(The argument that we'd not break backwards compatibility is thus bogus.)

Worth noting for onlookers is that the submitted patch seems to be using
UNION-style rules to determine a common type for anyelement arguments,
not just counting the "most common" type among the arguments as you might
think from the subject. But that doesn't make things any better.

An example of what would presumably happen if we adopted this sort of rule
(I've not checked whether the patch as written does this, but it would
logically follow) is that appending a float to an integer array would
cause the whole array to be silently promoted to float, with attendant
possible loss of precision for existing array elements. That does not
seem to me to satisfy the principle of least astonishment. Related,
even more astonishing behaviors could ensue from type promotion in
anyrange situations, eg range_contains_elem(anyrange,anyelement).
So I think it's just as well that we make people write a cast to show
what they mean in such cases.

In fact, if you discount cases involving anyarray and anyrange, we do not
have *any* built-in functions for which this patch would do anything,
except for the three-argument forms of lead() and lag(), where I think it
would be rather astonishing to let the default-value argument control the
result type, anyway. This leaves me feeling dubious both about the actual
scope of the use-case for such a change, and about whether "use the UNION
rules" would be a sensible heuristic even if we wanted to do something.
There seem to be too many cases where it's not a great idea to put all the
arguments on exactly equal footing for deciding what common type to
choose.

So I'm inclined to mark this patch as Rejected.

regards, tom lane

Hi

2015-07-10 18:43 GMT+02:00 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:

> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> > now a functions with more than one polymorphic arguments are relative
> > fragile due missing casting to most common type. Some our "functions"
> like
> > "coalesce" can do it, so it is surprising for our users.
>
> > our custom polymorphic function foo(anyelement, anyelement) working well
> for
> > foo(10,20) or foo(10.1, 20.1), but not for foo(10, 20.1)
>
> > I am thinking, so we can add a searching most common type stage without
> > breaking to backing compatibility.
>
> > What do you think about it?
>
> I see nobody's replied to this, still, so ...
>
> I think this is simply a bad idea, for a couple of reasons:
>
> 1. It will reduce predictability of type resolution.
>

I don't think - same mechanism we use - it doesn't introduce some new.

>
> 2. It will greatly increase the risk of getting "ambiguous function call"
> failures, because of adding more possible ways to match the same call.
> (The argument that we'd not break backwards compatibility is thus bogus.)
>

Maybe I not described well my idea.

This can generate new conflicts only when new behave will be different than
old behave. And different old behave is not possible - it fails on error
now. So there is possible, with this patch, some queries can fail on
conflict, but this code fails on "function doesn't exists" now. So if there
is some possibility of breaking compatibility, then one error can be
replaced by different error. It is known best practice to don't mix
polymorphic parameters and function overloading.

Why I need it - the motivation, why I returned to this topic is issue
https://github.com/orafce/orafce/issues/17 and some questions about same
topic on stackoverflow.

There is workaround with "any" type - but I have to repeat lot of work what
core analyzer can do, and the code in extension is longer. And I have to
write extension in C.

>
> Worth noting for onlookers is that the submitted patch seems to be using
> UNION-style rules to determine a common type for anyelement arguments,
> not just counting the "most common" type among the arguments as you might
> think from the subject. But that doesn't make things any better.
>

it is related to only polymorphic types.

>
> An example of what would presumably happen if we adopted this sort of rule
> (I've not checked whether the patch as written does this, but it would
> logically follow) is that appending a float to an integer array would
> cause the whole array to be silently promoted to float, with attendant
> possible loss of precision for existing array elements.

it is based on select_common_type() - so it is use only available implicit
casts.

> That does not
> seem to me to satisfy the principle of least astonishment. Related,
> even more astonishing behaviors could ensue from type promotion in
> anyrange situations, eg range_contains_elem(anyrange,anyelement).
> So I think it's just as well that we make people write a cast to show
> what they mean in such cases.
>

The polymorphic parameters create much bigger space - if somebody needs to
less variability, then he doesn't use polymorphic params.

I understand to some situation, when we prefer strict work with polymorphic
parameters - theoretically we can introduce new option that enforce it.

> In fact, if you discount cases involving anyarray and anyrange, we do not
> have *any* built-in functions for which this patch would do anything,
> except for the three-argument forms of lead() and lag(), where I think it
> would be rather astonishing to let the default-value argument control the
> result type, anyway. This leaves me feeling dubious both about the actual
> scope of the use-case for such a change, and about whether "use the UNION
> rules" would be a sensible heuristic even if we wanted to do something.
> There seem to be too many cases where it's not a great idea to put all the
> arguments on exactly equal footing for deciding what common type to
> choose.
>

Very common problem of polymorphic parameters is mix of numeric and
integers as parameters. It is one known gotcha - and I am trying to solve
it.

Regards

Pavel

>
> So I'm inclined to mark this patch as Rejected.
>
> regards, tom lane
>

2015-07-10 23:19 GMT+02:00 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:

> Hi
>
> 2015-07-10 18:43 GMT+02:00 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>
>> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> > now a functions with more than one polymorphic arguments are relative
>> > fragile due missing casting to most common type. Some our "functions"
>> like
>> > "coalesce" can do it, so it is surprising for our users.
>>
>> > our custom polymorphic function foo(anyelement, anyelement) working
>> well for
>> > foo(10,20) or foo(10.1, 20.1), but not for foo(10, 20.1)
>>
>> > I am thinking, so we can add a searching most common type stage without
>> > breaking to backing compatibility.
>>
>> > What do you think about it?
>>
>> I see nobody's replied to this, still, so ...
>>
>> I think this is simply a bad idea, for a couple of reasons:
>>
>> 1. It will reduce predictability of type resolution.
>>
>
> I don't think - same mechanism we use - it doesn't introduce some new.
>
>
>>
>> 2. It will greatly increase the risk of getting "ambiguous function call"
>> failures, because of adding more possible ways to match the same call.
>> (The argument that we'd not break backwards compatibility is thus bogus.)
>>
>
> Maybe I not described well my idea.
>
> This can generate new conflicts only when new behave will be different
> than old behave. And different old behave is not possible - it fails on
> error now. So there is possible, with this patch, some queries can fail on
> conflict, but this code fails on "function doesn't exists" now. So if there
> is some possibility of breaking compatibility, then one error can be
> replaced by different error. It is known best practice to don't mix
> polymorphic parameters and function overloading.
>
> Why I need it - the motivation, why I returned to this topic is issue
> https://github.com/orafce/orafce/issues/17 and some questions about same
> topic on stackoverflow.
>
> There is workaround with "any" type - but I have to repeat lot of work
> what core analyzer can do, and the code in extension is longer. And I have
> to write extension in C.
>

It worse - workaround with "any" isn't good enough for implementation NVL
function - because I cannot to change result type inside function.

I know, so you dislike parser/analyzer hook, but is there any other
possibility? Some hint in pg_class?

Regards

Pavel

>
>
>>
>> Worth noting for onlookers is that the submitted patch seems to be using
>> UNION-style rules to determine a common type for anyelement arguments,
>> not just counting the "most common" type among the arguments as you might
>> think from the subject. But that doesn't make things any better.
>>
>
> it is related to only polymorphic types.
>
>>
>> An example of what would presumably happen if we adopted this sort of rule
>> (I've not checked whether the patch as written does this, but it would
>> logically follow) is that appending a float to an integer array would
>> cause the whole array to be silently promoted to float, with attendant
>> possible loss of precision for existing array elements.
>
>
> it is based on select_common_type() - so it is use only available implicit
> casts.
>
>
>> That does not
>> seem to me to satisfy the principle of least astonishment. Related,
>> even more astonishing behaviors could ensue from type promotion in
>> anyrange situations, eg range_contains_elem(anyrange,anyelement).
>> So I think it's just as well that we make people write a cast to show
>> what they mean in such cases.
>>
>
> The polymorphic parameters create much bigger space - if somebody needs to
> less variability, then he doesn't use polymorphic params.
>
> I understand to some situation, when we prefer strict work with
> polymorphic parameters - theoretically we can introduce new option that
> enforce it.
>
>
>> In fact, if you discount cases involving anyarray and anyrange, we do not
>> have *any* built-in functions for which this patch would do anything,
>> except for the three-argument forms of lead() and lag(), where I think it
>> would be rather astonishing to let the default-value argument control the
>> result type, anyway. This leaves me feeling dubious both about the actual
>> scope of the use-case for such a change, and about whether "use the UNION
>> rules" would be a sensible heuristic even if we wanted to do something.
>> There seem to be too many cases where it's not a great idea to put all the
>> arguments on exactly equal footing for deciding what common type to
>> choose.
>>
>
> Very common problem of polymorphic parameters is mix of numeric and
> integers as parameters. It is one known gotcha - and I am trying to solve
> it.
>
> Regards
>
> Pavel
>
>
>>
>> So I'm inclined to mark this patch as Rejected.
>>
>> regards, tom lane
>>
>
>

On 07/11/2015 12:19 AM, Pavel Stehule wrote:
> 2015-07-10 18:43 GMT+02:00 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>
>> An example of what would presumably happen if we adopted this sort of rule
>> (I've not checked whether the patch as written does this, but it would
>> logically follow) is that appending a float to an integer array would
>> cause the whole array to be silently promoted to float, with attendant
>> possible loss of precision for existing array elements.
>
> it is based on select_common_type() - so it is use only available implicit
> casts.

Without patch:

postgres=# select pg_typeof(array_append('{1,2,3}'::int[], 1.2::float));
ERROR: function array_append(integer[], double precision) does not exist

With patch:

postgres=# select pg_typeof(array_append('{1,2,3}'::int[], 1.2::float));
pg_typeof
--------------------
double precision[]
(1 row)

Yeah, I agree with Tom that we don't want that change in behaviour. I'll
mark this as rejected.
- Heikki

2015-07-22 10:37 GMT+02:00 Heikki Linnakangas <hlinnaka(at)iki(dot)fi>:

> On 07/11/2015 12:19 AM, Pavel Stehule wrote:
>
>> 2015-07-10 18:43 GMT+02:00 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>>
>> An example of what would presumably happen if we adopted this sort of
>>> rule
>>> (I've not checked whether the patch as written does this, but it would
>>> logically follow) is that appending a float to an integer array would
>>> cause the whole array to be silently promoted to float, with attendant
>>> possible loss of precision for existing array elements.
>>>
>>
>> it is based on select_common_type() - so it is use only available implicit
>> casts.
>>
>
> Without patch:
>
> postgres=# select pg_typeof(array_append('{1,2,3}'::int[], 1.2::float));
> ERROR: function array_append(integer[], double precision) does not exist
>
> With patch:
>
> postgres=# select pg_typeof(array_append('{1,2,3}'::int[], 1.2::float));
> pg_typeof
> --------------------
> double precision[]
> (1 row)
>
>
> Yeah, I agree with Tom that we don't want that change in behaviour. I'll
> mark this as rejected.
>

ok.

I accept it - it introduce incompatible changes.

Still I am sure, so this behave is some what users expect, but it should
not be introduced this way.

Any ideas how this issues can be solved?

Regards

Pavel

> - Heikki
>
>