Re: plpgsql: numeric assignment to an integer variable errors out

The following plpgsql function errors out with cvs head:

CREATE function test_assign() returns void
AS
$$ declare x int;
BEGIN
x := 9E3/2;
END
$$ LANGUAGE 'plpgsql';

postgres=# select test_assign();
ERROR: invalid input syntax for integer: "4500.0000000000000000"
CONTEXT: PL/pgSQL function "test_assign" line 3 at assignment

We do have an existing cast from numeric to type integer. But here basically
we convert the value to string in exec_cast_value before calling int4in. And
then use of strtol in pg_atoi leads to this complaint. Guess converting the
value to string is not always a good strategy.

Regards,
Nikhils
--
http://www.enterprisedb.com

Hi,

<nikhil(dot)sontakke(at)enterprisedb(dot)com> wrote:

> The following plpgsql function errors out with cvs head:
>
> CREATE function test_assign() returns void
> AS
> $$ declare x int;
> BEGIN
> x := 9E3/2;
> END
> $$ LANGUAGE 'plpgsql';
>
> postgres=# select test_assign();
> ERROR: invalid input syntax for integer: "4500.0000000000000000"
> CONTEXT: PL/pgSQL function "test_assign" line 3 at assignment
>
> We do have an existing cast from numeric to type integer. But here
> basically we convert the value to string in exec_cast_value before calling
> int4in. And then use of strtol in pg_atoi leads to this complaint. Guess
> converting the value to string is not always a good strategy.
>

PFA, patch which uses find_coercion_pathway to find a direct
COERCION_PATH_FUNC function and uses that if it is available. Or is there a
better approach? Seems to handle the above issue with this patch.

Regards,
Nikhils
--
http://www.enterprisedb.com

2008/12/29 Nikhil Sontakke <nikhil(dot)sontakke(at)enterprisedb(dot)com>:
> Hi,
>
> <nikhil(dot)sontakke(at)enterprisedb(dot)com> wrote:
>>
>> The following plpgsql function errors out with cvs head:
>>
>> CREATE function test_assign() returns void
>> AS
>> $$ declare x int;
>> BEGIN
>> x := 9E3/2;
>> END
>> $$ LANGUAGE 'plpgsql';
>>
>> postgres=# select test_assign();
>> ERROR: invalid input syntax for integer: "4500.0000000000000000"
>> CONTEXT: PL/pgSQL function "test_assign" line 3 at assignment
>>
>> We do have an existing cast from numeric to type integer. But here
>> basically we convert the value to string in exec_cast_value before calling
>> int4in. And then use of strtol in pg_atoi leads to this complaint. Guess
>> converting the value to string is not always a good strategy.
>
> PFA, patch which uses find_coercion_pathway to find a direct
> COERCION_PATH_FUNC function and uses that if it is available. Or is there a
> better approach? Seems to handle the above issue with this patch.

+1

I thing, so some values should by cached, current patch could by slow.

Regards
Pavel Stehule

>
> Regards,
> Nikhils
> --
> http://www.enterprisedb.com
>
>
> --
> 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
>
>

Hi,

> >
> >> The following plpgsql function errors out with cvs head:
> >>
> >> CREATE function test_assign() returns void
> >> AS
> >> $$ declare x int;
> >> BEGIN
> >> x := 9E3/2;
> >> END
> >> $$ LANGUAGE 'plpgsql';
> >>
> >> postgres=# select test_assign();
> >> ERROR: invalid input syntax for integer: "4500.0000000000000000"
> >> CONTEXT: PL/pgSQL function "test_assign" line 3 at assignment
> >>
> >> We do have an existing cast from numeric to type integer. But here
> >> basically we convert the value to string in exec_cast_value before
> calling
> >> int4in. And then use of strtol in pg_atoi leads to this complaint. Guess
> >> converting the value to string is not always a good strategy.
> >
> > PFA, patch which uses find_coercion_pathway to find a direct
> > COERCION_PATH_FUNC function and uses that if it is available. Or is there
> a
> > better approach? Seems to handle the above issue with this patch.
>
> +1
>
> I thing, so some values should by cached, current patch could by slow.

Agreed, it can slow things down a bit especially since we are only
interested in the COERCION_PATH_FUNC case. What we need is a much simpler
pathway function which searches in the SysCache and returns back with the
valid/invalid castfunc immediately.

PFA, version 2.0 of this patch with these changes in place. I could have
added a generic function in parse_coerce.c, but thought the use case was
restricted to plpgsql and hence I have kept it within pl_exec.c for now.

Regards,
Nikhils
--
http://www.enterprisedb.com

Nikhil Sontakke wrote:
> > > PFA, patch which uses find_coercion_pathway to find a direct
> > > COERCION_PATH_FUNC function and uses that if it is available. Or is there
> > a
> > > better approach? Seems to handle the above issue with this patch.
> >
> > +1
> >
> > I thing, so some values should by cached, current patch could by slow.
>
>
> Agreed, it can slow things down a bit especially since we are only
> interested in the COERCION_PATH_FUNC case. What we need is a much simpler
> pathway function which searches in the SysCache and returns back with the
> valid/invalid castfunc immediately.
>
> PFA, version 2.0 of this patch with these changes in place. I could have
> added a generic function in parse_coerce.c, but thought the use case was
> restricted to plpgsql and hence I have kept it within pl_exec.c for now.

Where are we on this? 8.5?

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

2009/1/22 Bruce Momjian <bruce(at)momjian(dot)us>:
> Nikhil Sontakke wrote:
>> > > PFA, patch which uses find_coercion_pathway to find a direct
>> > > COERCION_PATH_FUNC function and uses that if it is available. Or is there
>> > a
>> > > better approach? Seems to handle the above issue with this patch.
>> >
>> > +1
>> >
>> > I thing, so some values should by cached, current patch could by slow.
>>
>>
>> Agreed, it can slow things down a bit especially since we are only
>> interested in the COERCION_PATH_FUNC case. What we need is a much simpler
>> pathway function which searches in the SysCache and returns back with the
>> valid/invalid castfunc immediately.
>>
>> PFA, version 2.0 of this patch with these changes in place. I could have
>> added a generic function in parse_coerce.c, but thought the use case was
>> restricted to plpgsql and hence I have kept it within pl_exec.c for now.
>
> Where are we on this? 8.5?
>

I hope, so this patch will be in next commitfest

Pavel

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

Hello

I tested patch v2.0, and I thing, so this patch should be used as bug
fix. It has same or little bit better speed than current and solve
some problems with numeric's implicit casting in plpgsql. But this is
only an half solution.

The core of problem is in lazy casting of plpgsql. We need to modify
execution plan for pl expression when result is different than
destination type. For almost expression we know destination type.

there is some sample of effectiveness:
postgres=# create or replace function test1()
returns int as $$
declare s int := 0;
begin
for i in 1..100000 loop
s := 4e3; -- numeric
end loop;
return s;
end;
$$ language plpgsql immutable;
CREATE FUNCTION
Time: 5,140 ms
postgres=# create or replace function test2()
returns int as $$
declare s int := 0;
begin
for i in 1..100000 loop
s := 4e3::int;
end loop;
return s;
end;
$$ language plpgsql immutable;
CREATE FUNCTION
Time: 416,240 ms
postgres=# select test1();
test1
-------
4000
(1 row)

Time: 161,048 ms
postgres=# select test2();
test2
-------
4000
(1 row)

Time: 68,110 ms
postgres=# select test1();
test1
-------
4000
(1 row)

Time: 171,020 ms
postgres=# select test2();
test2
-------
4000
(1 row)

Time: 61,771 ms
postgres=#

Regards
Pavel Stehule

2009/1/22 Bruce Momjian <bruce(at)momjian(dot)us>:
> Nikhil Sontakke wrote:
>> > > PFA, patch which uses find_coercion_pathway to find a direct
>> > > COERCION_PATH_FUNC function and uses that if it is available. Or is there
>> > a
>> > > better approach? Seems to handle the above issue with this patch.
>> >
>> > +1
>> >
>> > I thing, so some values should by cached, current patch could by slow.
>>
>>
>> Agreed, it can slow things down a bit especially since we are only
>> interested in the COERCION_PATH_FUNC case. What we need is a much simpler
>> pathway function which searches in the SysCache and returns back with the
>> valid/invalid castfunc immediately.
>>
>> PFA, version 2.0 of this patch with these changes in place. I could have
>> added a generic function in parse_coerce.c, but thought the use case was
>> restricted to plpgsql and hence I have kept it within pl_exec.c for now.
>
> Where are we on this? 8.5?
>
> --
> Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
> EnterpriseDB http://enterprisedb.com
>
> + If your life is a hard drive, Christ can be your backup. +
>

Whatever happened to this patch?

---------------------------------------------------------------------------

Nikhil Sontakke wrote:
> Hi,
>
> <nikhil(dot)sontakke(at)enterprisedb(dot)com> wrote:
>
> > The following plpgsql function errors out with cvs head:
> >
> > CREATE function test_assign() returns void
> > AS
> > $$ declare x int;
> > BEGIN
> > x := 9E3/2;
> > END
> > $$ LANGUAGE 'plpgsql';
> >
> > postgres=# select test_assign();
> > ERROR: invalid input syntax for integer: "4500.0000000000000000"
> > CONTEXT: PL/pgSQL function "test_assign" line 3 at assignment
> >
> > We do have an existing cast from numeric to type integer. But here
> > basically we convert the value to string in exec_cast_value before calling
> > int4in. And then use of strtol in pg_atoi leads to this complaint. Guess
> > converting the value to string is not always a good strategy.
> >
>
> PFA, patch which uses find_coercion_pathway to find a direct
> COERCION_PATH_FUNC function and uses that if it is available. Or is there a
> better approach? Seems to handle the above issue with this patch.
>
> Regards,
> Nikhils
> --
> http://www.enterprisedb.com

[ Attachment, skipping... ]

>
> --
> 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

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com
PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do
+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> Whatever happened to this patch?

I think we bounced it on the grounds that it would represent a
fundamental change in plpgsql behavior and break a whole lot of
applications. People have been relying on plpgsql's coerce-via-IO
assignment behavior for ten years. If you prefer coerce via
cast conversion, you can get that by writing an explicit cast.

Now it is true that a lot of the uses for that were subsumed when
we added coerce-via-IO to the native cast capabilities; but I'm
still quite scared of what this would break, and I don't see any
field demand for a change.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <bruce(at)momjian(dot)us> writes:
> > Whatever happened to this patch?
>
> I think we bounced it on the grounds that it would represent a
> fundamental change in plpgsql behavior and break a whole lot of
> applications. People have been relying on plpgsql's coerce-via-IO
> assignment behavior for ten years. If you prefer coerce via
> cast conversion, you can get that by writing an explicit cast.
>
> Now it is true that a lot of the uses for that were subsumed when
> we added coerce-via-IO to the native cast capabilities; but I'm
> still quite scared of what this would break, and I don't see any
> field demand for a change.

Thanks. Sorry to be asking so many questions but it is the only way I
can be sure we have covered everything.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com
PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do
+ If your life is a hard drive, Christ can be your backup. +

Hi,

> Now it is true that a lot of the uses for that were subsumed when
> we added coerce-via-IO to the native cast capabilities; but I'm
> still quite scared of what this would break, and I don't see any
> field demand for a change.

Well, we have had one of our EDB connectors facing issues because of
this existing conversion mechanism.

I think this is a small patch which tries to do the "right" thing, no?

Regards,
Nikhils
--
http://www.enterprisedb.com