plpgsql: numeric assignment to an integer variable errors out

Started by Nikhil Sontakkeabout 17 years ago11 messages

nikhil.sontakke@enterprisedb.com

about 17 years ago

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

Nikhil Sontakke

nikhil.sontakke@enterprisedb.com

about 17 years ago

In reply to: Nikhil Sontakke (#1)

1 attachment(s)

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

Hi,

<nikhil.sontakke@enterprisedb.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

Attachments:

plpgsql_numeric_bug.patchtext/x-diff; name=plpgsql_numeric_bug.patchDownload

Index: src/pl/plpgsql/src/pl_exec.c
===================================================================
RCS file: /repositories/postgreshome/cvs/pgsql/src/pl/plpgsql/src/pl_exec.c,v
retrieving revision 1.225
diff -c -r1.225 pl_exec.c
*** src/pl/plpgsql/src/pl_exec.c	20 Nov 2008 15:36:22 -0000	1.225
--- src/pl/plpgsql/src/pl_exec.c	29 Dec 2008 15:23:49 -0000
***************
*** 24,29 ****
--- 24,30 ----
  #include "funcapi.h"
  #include "nodes/nodeFuncs.h"
  #include "parser/scansup.h"
+ #include "parser/parse_coerce.h"
  #include "tcop/tcopprot.h"
  #include "utils/array.h"
  #include "utils/builtins.h"
***************
*** 4732,4750 ****
  	{
  		if (!isnull)
  		{
! 			char	   *extval;
  
! 			extval = convert_value_to_string(value, valtype);
  
! 			/* Allow input function to use SPI ... see notes above */
! 			SPI_push();
  
! 			value = InputFunctionCall(reqinput, extval,
! 									  reqtypioparam, reqtypmod);
  
! 			SPI_pop();
  
! 			pfree(extval);
  		}
  		else
  		{
--- 4733,4777 ----
  	{
  		if (!isnull)
  		{
! 			/*
! 			 * find out if there is a coercion pathway via a function
! 			 */
! 			CoercionPathType pathtype;
! 			Oid				 castfunc;
  
! 			pathtype = find_coercion_pathway(reqtype, valtype,
! 											 COERCION_ASSIGNMENT,
! 											 &castfunc);
! 
! 			if (pathtype == COERCION_PATH_FUNC)
! 			{
! 				FmgrInfo	cast_func_finfo;
! 
! 				fmgr_info_cxt(castfunc, &cast_func_finfo,
! 							  CurrentMemoryContext);
! 				cast_func_finfo.fn_oid = InvalidOid;
! 			
! 				/* do we need reqtypmod for cast functions? */
! 				value = FunctionCall2(&cast_func_finfo,
! 										 value,
! 										 reqtypmod);
! 			}
! 			else
! 			{
! 				char	   *extval;
  
! 				extval = convert_value_to_string(value, valtype);
  
! 				/* Allow input function to use SPI ... see notes above */
! 				SPI_push();
  
! 				value = InputFunctionCall(reqinput, extval,
! 										  reqtypioparam, reqtypmod);
  
! 				SPI_pop();
! 
! 				pfree(extval);
! 			}
  		}
  		else
  		{

Pavel Stehule

pavel.stehule@gmail.com

about 17 years ago

In reply to: Nikhil Sontakke (#2)

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

2008/12/29 Nikhil Sontakke <nikhil.sontakke@enterprisedb.com>:

Hi,

<nikhil.sontakke@enterprisedb.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.

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

Regards
Pavel Stehule

Show quoted text

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

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

Nikhil Sontakke

nikhil.sontakke@enterprisedb.com

about 17 years ago

In reply to: Pavel Stehule (#3)

1 attachment(s)

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

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

Attachments:

plpgsql_numeric_bug_v2.0.patchtext/x-diff; name=plpgsql_numeric_bug_v2.0.patchDownload

Index: src/pl/plpgsql/src/pl_exec.c
===================================================================
RCS file: /repositories/postgreshome/cvs/pgsql/src/pl/plpgsql/src/pl_exec.c,v
retrieving revision 1.225
diff -c -r1.225 pl_exec.c
*** src/pl/plpgsql/src/pl_exec.c	20 Nov 2008 15:36:22 -0000	1.225
--- src/pl/plpgsql/src/pl_exec.c	30 Dec 2008 07:31:36 -0000
***************
*** 20,29 ****
--- 20,31 ----
  #include "access/transam.h"
  #include "catalog/pg_proc.h"
  #include "catalog/pg_type.h"
+ #include "catalog/pg_cast.h"
  #include "executor/spi_priv.h"
  #include "funcapi.h"
  #include "nodes/nodeFuncs.h"
  #include "parser/scansup.h"
+ #include "parser/parse_coerce.h"
  #include "tcop/tcopprot.h"
  #include "utils/array.h"
  #include "utils/builtins.h"
***************
*** 199,204 ****
--- 201,208 ----
  static void free_params_data(PreparedParamsData *ppd);
  static Portal exec_dynquery_with_params(PLpgSQL_execstate *estate,
  										PLpgSQL_expr *query, List *params);
+ static CoercionPathType find_assignment_coercion_pathway(Oid targetTypeId,
+ 											Oid sourceTypeId, Oid *funcid);
  
  
  /* ----------
***************
*** 4732,4750 ****
  	{
  		if (!isnull)
  		{
! 			char	   *extval;
  
! 			extval = convert_value_to_string(value, valtype);
  
! 			/* Allow input function to use SPI ... see notes above */
! 			SPI_push();
  
! 			value = InputFunctionCall(reqinput, extval,
! 									  reqtypioparam, reqtypmod);
  
! 			SPI_pop();
  
! 			pfree(extval);
  		}
  		else
  		{
--- 4736,4779 ----
  	{
  		if (!isnull)
  		{
! 			/*
! 			 * find out if there is a coercion pathway via a function
! 			 */
! 			CoercionPathType pathtype;
! 			Oid				 castfunc;
  
! 			pathtype = find_assignment_coercion_pathway(reqtype, valtype,
! 											 &castfunc);
  
! 			if (pathtype == COERCION_PATH_FUNC)
! 			{
! 				FmgrInfo	cast_func_finfo;
! 
! 				fmgr_info_cxt(castfunc, &cast_func_finfo,
! 							  CurrentMemoryContext);
! 				cast_func_finfo.fn_oid = InvalidOid;
! 			
! 				/* do we need reqtypmod for cast functions? */
! 				value = FunctionCall2(&cast_func_finfo,
! 										 value,
! 										 reqtypmod);
! 			}
! 			else
! 			{
! 				char	   *extval;
  
! 				extval = convert_value_to_string(value, valtype);
  
! 				/* Allow input function to use SPI ... see notes above */
! 				SPI_push();
  
! 				value = InputFunctionCall(reqinput, extval,
! 										  reqtypioparam, reqtypmod);
! 
! 				SPI_pop();
! 
! 				pfree(extval);
! 			}
  		}
  		else
  		{
***************
*** 5456,5458 ****
--- 5485,5513 ----
  
  	return portal;
  }
+ 
+ static CoercionPathType 
+ find_assignment_coercion_pathway(Oid targetTypeId, Oid sourceTypeId, Oid *funcid)
+ {
+ 	CoercionPathType result = COERCION_PATH_NONE;
+ 	HeapTuple	tuple;
+ 
+ 	/* Look in pg_cast */
+ 	tuple = SearchSysCache(CASTSOURCETARGET,
+ 						   ObjectIdGetDatum(sourceTypeId),
+ 						   ObjectIdGetDatum(targetTypeId),
+ 						   0, 0);
+ 
+ 	if (HeapTupleIsValid(tuple))
+ 	{
+ 		Form_pg_cast castForm = (Form_pg_cast) GETSTRUCT(tuple);
+ 
+ 		*funcid = castForm->castfunc;
+ 		ReleaseSysCache(tuple);
+ 	}
+ 
+ 	if (OidIsValid(*funcid))
+ 		result = COERCION_PATH_FUNC;
+ 
+ 	return result;
+ }

Bruce Momjian

bruce@momjian.us

almost 17 years ago

In reply to: Nikhil Sontakke (#4)

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

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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Pavel Stehule

pavel.stehule@gmail.com

almost 17 years ago

In reply to: Bruce Momjian (#5)

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

2009/1/22 Bruce Momjian <bruce@momjian.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

Show quoted text

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Pavel Stehule

pavel.stehule@gmail.com

almost 17 years ago

In reply to: Bruce Momjian (#5)

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

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@momjian.us>:

Show quoted text

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@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

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

Bruce Momjian

bruce@momjian.us

almost 16 years ago

In reply to: Nikhil Sontakke (#2)

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

Whatever happened to this patch?

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

Nikhil Sontakke wrote:

Hi,

<nikhil.sontakke@enterprisedb.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@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

-- 
  Bruce Momjian  <bruce@momjian.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. +

Tom Lane

tgl@sss.pgh.pa.us

almost 16 years ago

In reply to: Bruce Momjian (#8)

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

Bruce Momjian <bruce@momjian.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

#10

Bruce Momjian

bruce@momjian.us

almost 16 years ago

In reply to: Tom Lane (#9)

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

Tom Lane wrote:

Bruce Momjian <bruce@momjian.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@momjian.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. +

#11

Nikhil Sontakke

nikhil.sontakke@enterprisedb.com

almost 16 years ago

In reply to: Tom Lane (#9)

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

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