I noticed today that the system drops any "typmod" modifier associated
with a type name being casted to.  For example,

regression=# select '1.23456'::numeric(7,2);
 ?column?
----------
  1.23456			--- should be 1.23
(1 row)

regression=# select CAST ('1234567.89' AS numeric(4,1));
  ?column?
------------
 1234567.89			--- should raise a numeric-overflow error
(1 row)

These particular cases can be fixed with a one-line patch, I think,
because there is storage in an A_Const node to hold a reference to
a Typename, which includes typmod.  parse_expr.c is just forgetting
to pass the typmod to parser_typecast().

BUT: there isn't any equally simple patch when the value being casted
is not a constant.  For instance

	select field1 :: numeric(7,2) from table1;

cannot work properly now, because gram.y transforms it into

	select numeric(field1) from table;

which (a) drops the typmod and (b) bypasses all of the intelligence
that should be used to determine how to coerce the type.

What I think we need is to add a new parsetree node type that explicitly
represents a CAST operator, and then modify parse_expr.c to transform
that node type into an appropriate function call (or, perhaps, nothing
at all if the source value is already the right type).

Comments?

			regards, tom lane

Sat, 15 Jan 2000 21:03:50 -0500
Tom Lane <tgl@sss.pgh.pa.us>
I think we need an explicit parsetree node for CAST
> These particular cases can be fixed with a one-line patch, I think,
> because there is storage in an A_Const node to hold a reference to
> a Typename, which includes typmod.  parse_expr.c is just forgetting
> to pass the typmod to parser_typecast().
> 
> BUT: there isn't any equally simple patch when the value being casted
> is not a constant.  For instance
> 
> 	select field1 :: numeric(7,2) from table1;
> 
> cannot work properly now, because gram.y transforms it into
> 
> 	select numeric(field1) from table;
> 
> which (a) drops the typmod and (b) bypasses all of the intelligence
> that should be used to determine how to coerce the type.
> 
> What I think we need is to add a new parsetree node type that explicitly
> represents a CAST operator, and then modify parse_expr.c to transform
> that node type into an appropriate function call (or, perhaps, nothing
> at all if the source value is already the right type).

I have on the TODO list, and once considered adding more passing around
of atttypmod in the parser to keep such information.  Maybe I shoud do
that first to see what happens and what gets fixed.

-- 
  Bruce Momjian                        |  http://www.op.net/~candle
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Sat, 15 Jan 2000 21:37:06 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: [HACKERS] I think we need an explicit parsetree node for CAST
> These particular cases can be fixed with a one-line patch, I think,
> because there is storage in an A_Const node to hold a reference to
> a Typename, which includes typmod.  parse_expr.c is just forgetting
> to pass the typmod to parser_typecast().
> 
> BUT: there isn't any equally simple patch when the value being casted
> is not a constant.  For instance
> 
> 	select field1 :: numeric(7,2) from table1;
> 
> cannot work properly now, because gram.y transforms it into
> 
> 	select numeric(field1) from table;
> 
> which (a) drops the typmod and (b) bypasses all of the intelligence
> that should be used to determine how to coerce the type.
> 
> What I think we need is to add a new parsetree node type that explicitly
> represents a CAST operator, and then modify parse_expr.c to transform
> that node type into an appropriate function call (or, perhaps, nothing
> at all if the source value is already the right type).

Actually, I think I never made the additional atttypmod changes because
no one had ever reported a problem, and I was confused by that.

-- 
  Bruce Momjian                        |  http://www.op.net/~candle
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Sat, 15 Jan 2000 21:41:09 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: [HACKERS] I think we need an explicit parsetree node for CAST
At 09:03 PM 1/15/00 -0500, Tom Lane wrote:

>What I think we need is to add a new parsetree node type that explicitly
>represents a CAST operator, and then modify parse_expr.c to transform
>that node type into an appropriate function call (or, perhaps, nothing
>at all if the source value is already the right type).
>
>Comments?

Well, I hate to keep popping up wearing my compiler-writer hat, but
yes, this seems obvious.   If the casting notation includes type
modifications like precision information then the simple expression
type_name(expr) can't ever match it.  The casting notation must be 
rejected or properly executed.  Silent errors of this type are simply
unforgivable.  Either an error or a proper transformation (presumably
to a function that takes precision and scale parameters) is fine, but
silent and incorrect execution is a major sin.



- Don Baccus, Portland OR <dhogaza@pacifier.com>
  Nature photos, on-line guides, Pacific Northwest
  Rare Bird Alert Service and other goodies at
  http://donb.photo.net.

Sat, 15 Jan 2000 20:07:30 -0800
Don Baccus <dhogaza@pacifier.com>
Re: [HACKERS] I think we need an explicit parsetree node for
  CAST
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Actually, I think I never made the additional atttypmod changes because
> no one had ever reported a problem, and I was confused by that.

I think that after further discussion, we concluded that it wasn't
really possible to determine an atttypmod value to attach to the
result of most expressions.  However, CAST is a special case because
there *is* a typmod value associated with the Typename node.  The
thing I want to do is make sure we hold onto that value long enough
to use it...

			regards, tom lane

Sat, 15 Jan 2000 23:22:42 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: [HACKERS] I think we need an explicit parsetree node for CAST 
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > Actually, I think I never made the additional atttypmod changes because
> > no one had ever reported a problem, and I was confused by that.
> 
> I think that after further discussion, we concluded that it wasn't
> really possible to determine an atttypmod value to attach to the
> result of most expressions.  However, CAST is a special case because
> there *is* a typmod value associated with the Typename node.  The
> thing I want to do is make sure we hold onto that value long enough
> to use it...

I found the area you mentioned and fixed it.  Seems the other areas I
remember being a problem were fixed by me or someone else, maybe you.
I remember makeResdom and makeVar having bogus entries sometimes, but I
see now they are all fixed.

I see your issue, and I don't know the code well enough to comment on
it.

I was able to do:

	test=> select 'x' as fred into test ;
	NOTICE:  Attribute 'fred' has an unknown type
	        Relation created; continue
	SELECT
	test=> \d test
	        Table "test"
	 Attribute |  Type   | Extra 
	-----------+---------+-------
	 fred      | unknown | 
	
---------------------------------------------------------------------------

	test=> select 'x'::varchar as fred into test ;
	SELECT
	test=> \d test
	          Table "test"
	 Attribute |    Type    | Extra 
	-----------+------------+-------
	 fred      | varchar(0) | 


Seems we should disallow this.  This last one is the one you want to
fix:

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

	test=> select 'x'::varchar(20) as fred into test ;
	SELECT
	test=> \d test
	          Table "test"
	 Attribute |    Type    | Extra 
	-----------+------------+-------
	 fred      | varchar(0) | 

-- 
  Bruce Momjian                        |  http://www.op.net/~candle
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Sun, 16 Jan 2000 00:39:53 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: [HACKERS] I think we need an explicit parsetree node for CAST
I have applied a patch for this one.

> I noticed today that the system drops any "typmod" modifier associated
> with a type name being casted to.  For example,
> 
> regression=# select '1.23456'::numeric(7,2);
>  ?column?
> ----------
>   1.23456			--- should be 1.23
> (1 row)
> 
> regression=# select CAST ('1234567.89' AS numeric(4,1));
>   ?column?
> ------------
>  1234567.89			--- should raise a numeric-overflow error
> (1 row)
> 
> These particular cases can be fixed with a one-line patch, I think,
> because there is storage in an A_Const node to hold a reference to
> a Typename, which includes typmod.  parse_expr.c is just forgetting
> to pass the typmod to parser_typecast().
> 
> BUT: there isn't any equally simple patch when the value being casted
> is not a constant.  For instance
> 
> 	select field1 :: numeric(7,2) from table1;
> 
> cannot work properly now, because gram.y transforms it into
> 
> 	select numeric(field1) from table;
> 
> which (a) drops the typmod and (b) bypasses all of the intelligence
> that should be used to determine how to coerce the type.
> 
> What I think we need is to add a new parsetree node type that explicitly
> represents a CAST operator, and then modify parse_expr.c to transform
> that node type into an appropriate function call (or, perhaps, nothing
> at all if the source value is already the right type).
> 
> Comments?
> 
> 			regards, tom lane
> 
> ************
> 


-- 
  Bruce Momjian                        |  http://www.op.net/~candle
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Sun, 16 Jan 2000 00:40:49 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: [HACKERS] I think we need an explicit parsetree node for CAST
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I have applied a patch for this one.

Right, you saw the parser_typecast mistake.  But the problem of doing
it properly for non-constant input to the CAST is still open.

			regards, tom lane

Sun, 16 Jan 2000 01:25:58 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: [HACKERS] I think we need an explicit parsetree node for CAST 
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > I have applied a patch for this one.
> 
> Right, you saw the parser_typecast mistake.  But the problem of doing
> it properly for non-constant input to the CAST is still open.
> 

Yes, and constants with cases in SELECT INTO are broken too.


-- 
  Bruce Momjian                        |  http://www.op.net/~candle
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Sun, 16 Jan 2000 01:44:37 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: [HACKERS] I think we need an explicit parsetree node for CAST
>> Right, you saw the parser_typecast mistake.  But the problem of doing
>> it properly for non-constant input to the CAST is still open.

BTW, the strings regress test is currently failing in a couple of
places, because it thinks that casting to "char" won't truncate the
string.  With this patch in place, casting a constant to "char" means
casting to char(1) which indeed truncates to one character.  I think
this is correct behavior, though it may surprise someone somewhere.

There are other places in the strings test that cast non-constant
expressions to "char", and those are going to change behavior as soon
as I finish inventing a parsenode for CAST.  So I am not going to bother
checking in an update for the strings test until the dust settles.

> Yes, and constants with cases in SELECT INTO are broken too.

Huh?  I'm not sure if I follow this or not --- would you give an
example?

			regards, tom lane

Sun, 16 Jan 2000 16:32:38 -0500
Tom Lane <tgl@sss.pgh.pa.us>
Re: [HACKERS] I think we need an explicit parsetree node for CAST 
> > Yes, and constants with cases in SELECT INTO are broken too.
> 
> Huh?  I'm not sure if I follow this or not --- would you give an
> example?

Here is the mail I sent out last night.  It shows a failure:

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

I see your issue, and I don't know the code well enough to comment on
it.

I was able to do:

        test=> select 'x' as fred into test ;
        NOTICE:  Attribute 'fred' has an unknown type
                Relation created; continue
        SELECT
        test=> \d test
                Table "test"
         Attribute |  Type   | Extra
        -----------+---------+-------
         fred      | unknown |

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


        test=> select 'x'::varchar as fred into test ;
        SELECT
        test=> \d test
                  Table "test"
         Attribute |    Type    | Extra
        -----------+------------+-------
         fred      | varchar(0) |


Seems we should disallow this.  This last one is the one you want to
fix:

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


        test=> select 'x'::varchar(20) as fred into test ;
        SELECT
        test=> \d test
                  Table "test"
         Attribute |    Type    | Extra
        -----------+------------+-------
         fred      | varchar(0) |

-
-- 
  Bruce Momjian                        |  http://www.op.net/~candle
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Sun, 16 Jan 2000 17:04:44 -0500 (EST)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: [HACKERS] I think we need an explicit parsetree node for CAST