Обсуждение: I think we need an explicit parsetree node for CAST
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
> 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, Pennsylvania19026
> 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, Pennsylvania19026
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
> 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; continueSELECTtest=>\d test Table "test" Attribute | Type | Extra -----------+---------+------- fred | unknown| --------------------------------------------------------------------------- test=> select 'x'::varchar as fred into test ;SELECTtest=> \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 ;SELECTtest=> \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, Pennsylvania19026
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,
Pennsylvania19026
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
> 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, Pennsylvania19026
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 Serviceand other goodies at http://donb.photo.net.
>> 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
> > 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,
Pennsylvania19026