Thread: DEFAULT fixed
I have fixed the problem with DEFAULT ''.
test=> create table t1 (str1 char(2) default '',str2 text default
'',str3 text default '' );
CREATE
test=> insert into t1 values ('aa', 'string2', 'string3');
INSERT 18830 1
test=> insert into t1 (str3) values ('string3');
INSERT 18831 1
test=> select * from t1;
str1|str2 |str3
----+-------+-------
aa |string2|string3
| |string3
(2 rows)
The fix is to pass atttypmod into parse_coerce(), and when a bpchar type
is the output type, we pass the atttypmod value into bpcharin, so the
type is properly padded.
The bad news is that many other calls to parse_coerce do not have access
to the column's atttypmod value, so they don't properly pad the
coersion.
Does anyone have an opinion on this? Why does only DEFAULT have this
problem? Does anyone know how inserts of '' into char() fields get
padded with the proper atttypmod value? Do I need to pass atttypmod to
all the functions that call parse_coerce, so I can pass a value for all
cases?
--
Bruce Momjian | http://www.op.net/~candle
maillist@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
? src/Makefile.custom
? src/config.log
? src/log
? src/config.cache
? src/config.status
? src/GNUmakefile
? src/Makefile.global
? src/backend/fmgr.h
? src/backend/parse.h
? src/backend/postgres
? src/backend/global1.bki.source
? src/backend/local1_template1.bki.source
? src/backend/global1.description
? src/backend/local1_template1.description
? src/backend/bootstrap/bootparse.c
? src/backend/bootstrap/bootstrap_tokens.h
? src/backend/bootstrap/bootscanner.c
? src/backend/catalog/genbki.sh
? src/backend/catalog/global1.bki.source
? src/backend/catalog/global1.description
? src/backend/catalog/local1_template1.bki.source
? src/backend/catalog/local1_template1.description
? src/backend/port/Makefile
? src/backend/utils/Gen_fmgrtab.sh
? src/backend/utils/fmgr.h
? src/backend/utils/fmgrtab.c
? src/bin/cleardbdir/cleardbdir
? src/bin/createdb/createdb
? src/bin/createlang/createlang
? src/bin/createuser/createuser
? src/bin/destroydb/destroydb
? src/bin/destroylang/destroylang
? src/bin/destroyuser/destroyuser
? src/bin/initdb/initdb
? src/bin/initlocation/initlocation
? src/bin/ipcclean/ipcclean
? src/bin/pg_dump/Makefile
? src/bin/pg_dump/pg_dump
? src/bin/pg_id/pg_id
? src/bin/pg_passwd/pg_passwd
? src/bin/pg_version/Makefile
? src/bin/pg_version/pg_version
? src/bin/pgtclsh/mkMakefile.tcldefs.sh
? src/bin/pgtclsh/mkMakefile.tkdefs.sh
? src/bin/pgtclsh/Makefile.tkdefs
? src/bin/pgtclsh/Makefile.tcldefs
? src/bin/pgtclsh/pgtclsh
? src/bin/pgtclsh/pgtksh
? src/bin/psql/Makefile
? src/bin/psql/psql
? src/include/version.h
? src/include/config.h
? src/interfaces/ecpg/lib/Makefile
? src/interfaces/ecpg/lib/libecpg.so.3.0.0
? src/interfaces/ecpg/preproc/ecpg
? src/interfaces/libpgtcl/Makefile
? src/interfaces/libpgtcl/libpgtcl.so.2.0
? src/interfaces/libpq/Makefile
? src/interfaces/libpq/libpq.so.2.0
? src/interfaces/libpq++/Makefile
? src/interfaces/libpq++/libpq++.so.2.0
? src/interfaces/odbc/GNUmakefile
? src/interfaces/odbc/Makefile.global
? src/lextest/lex.yy.c
? src/lextest/lextest
? src/pl/plpgsql/src/Makefile
? src/pl/plpgsql/src/mklang.sql
? src/pl/plpgsql/src/pl_gram.c
? src/pl/plpgsql/src/pl.tab.h
? src/pl/plpgsql/src/pl_scan.c
? src/pl/tcl/mkMakefile.tcldefs.sh
? src/pl/tcl/Makefile.tcldefs
? src/test/regress/log
? src/test/regress/log2
Index: src/backend/catalog/heap.c
===================================================================
RCS file: /usr/local/cvsroot/pgsql/src/backend/catalog/heap.c,v
retrieving revision 1.83
diff -c -r1.83 heap.c
*** src/backend/catalog/heap.c 1999/05/21 18:33:12 1.83
--- src/backend/catalog/heap.c 1999/05/22 04:05:41
***************
*** 1538,1563 ****
if (type != atp->atttypid)
{
! /*
! * Though these types are binary compatible, bpchar has a fixed
! * length on the disk, requiring non-bpchar types to be padded
! * before storage in the default table. bjm 1999/05/18
! */
! if (1==0 && atp->atttypid == BPCHAROID &&
! (type == TEXTOID || type == BPCHAROID || type == UNKNOWNOID))
! {
!
! FuncCall *n = makeNode(FuncCall);
!
! n->funcname = typeidTypeName(atp->atttypid);
! n->args = lcons((Node *)expr, NIL);
! expr = transformExpr(NULL, (Node *) n, EXPR_COLUMN_FIRST);
!
! }
! else if (IS_BINARY_COMPATIBLE(type, atp->atttypid))
; /* use without change */
else if (can_coerce_type(1, &(type), &(atp->atttypid)))
! expr = coerce_type(NULL, (Node *)expr, type, atp->atttypid);
else if (IsA(expr, Const))
{
if (*cast != 0)
--- 1538,1548 ----
if (type != atp->atttypid)
{
! if (IS_BINARY_COMPATIBLE(type, atp->atttypid))
; /* use without change */
else if (can_coerce_type(1, &(type), &(atp->atttypid)))
! expr = coerce_type(NULL, (Node *)expr, type, atp->atttypid,
! atp->atttypmod);
else if (IsA(expr, Const))
{
if (*cast != 0)
Index: src/backend/parser/parse_coerce.c
===================================================================
RCS file: /usr/local/cvsroot/pgsql/src/backend/parser/parse_coerce.c,v
retrieving revision 2.14
diff -c -r2.14 parse_coerce.c
*** src/backend/parser/parse_coerce.c 1999/05/22 02:55:57 2.14
--- src/backend/parser/parse_coerce.c 1999/05/22 04:05:45
***************
*** 35,41 ****
* Convert a function argument to a different type.
*/
Node *
! coerce_type(ParseState *pstate, Node *node, Oid inputTypeId, Oid targetTypeId)
{
Node *result = NULL;
Oid infunc;
--- 35,42 ----
* Convert a function argument to a different type.
*/
Node *
! coerce_type(ParseState *pstate, Node *node, Oid inputTypeId, Oid targetTypeId,
! int32 atttypmod)
{
Node *result = NULL;
Oid infunc;
***************
*** 82,92 ****
con->consttype = targetTypeId;
con->constlen = typeLen(typeidType(targetTypeId));
! /* use "-1" for varchar() type */
con->constvalue = (Datum) fmgr(infunc,
val,
typeidTypElem(targetTypeId),
! -1);
con->constisnull = false;
con->constbyval = typeByVal(typeidType(targetTypeId));
con->constisset = false;
--- 83,98 ----
con->consttype = targetTypeId;
con->constlen = typeLen(typeidType(targetTypeId));
! /*
! * Use "-1" for varchar() type.
! * For char(), we need to pad out the type with the proper
! * number of spaces. This was a major problem for
! * DEFAULT string constants to char() types.
! */
con->constvalue = (Datum) fmgr(infunc,
val,
typeidTypElem(targetTypeId),
! (targetTypeId != BPCHAROID) ? -1 : atttypmod);
con->constisnull = false;
con->constbyval = typeByVal(typeidType(targetTypeId));
con->constisset = false;
***************
*** 100,106 ****
result = node;
return result;
! } /* coerce_type() */
/* can_coerce_type()
--- 106,112 ----
result = node;
return result;
! }
/* can_coerce_type()
***************
*** 178,184 ****
}
return true;
! } /* can_coerce_type() */
/* TypeCategory()
--- 184,190 ----
}
return true;
! }
/* TypeCategory()
Index: src/backend/parser/parse_expr.c
===================================================================
RCS file: /usr/local/cvsroot/pgsql/src/backend/parser/parse_expr.c,v
retrieving revision 1.46
diff -c -r1.46 parse_expr.c
*** src/backend/parser/parse_expr.c 1999/05/18 23:40:05 1.46
--- src/backend/parser/parse_expr.c 1999/05/22 04:05:45
***************
*** 417,423 ****
}
else if (can_coerce_type(1, &c->casetype, &ptype))
{
! c->defresult = coerce_type(pstate, c->defresult, c->casetype, ptype);
c->casetype = ptype;
}
else
--- 417,424 ----
}
else if (can_coerce_type(1, &c->casetype, &ptype))
{
! c->defresult = coerce_type(pstate, c->defresult,
! c->casetype, ptype, -1);
c->casetype = ptype;
}
else
***************
*** 439,445 ****
{
if (can_coerce_type(1, &wtype, &ptype))
{
! w->result = coerce_type(pstate, w->result, wtype, ptype);
}
else
{
--- 440,447 ----
{
if (can_coerce_type(1, &wtype, &ptype))
{
! w->result = coerce_type(pstate, w->result, wtype,
! ptype, -1);
}
else
{
Index: src/backend/parser/parse_func.c
===================================================================
RCS file: /usr/local/cvsroot/pgsql/src/backend/parser/parse_func.c,v
retrieving revision 1.44
diff -c -r1.44 parse_func.c
*** src/backend/parser/parse_func.c 1999/05/17 17:03:33 1.44
--- src/backend/parser/parse_func.c 1999/05/22 04:05:53
***************
*** 352,358 ****
}
else
{
-
/*
* Parsing aggregates.
*/
--- 352,357 ----
***************
*** 361,367 ****
int ncandidates;
CandidateList candidates;
-
/*
* the aggregate COUNT is a special case, ignore its base
* type. Treat it as zero
--- 360,365 ----
***************
*** 392,398 ****
type = agg_select_candidate(basetype, candidates);
if (OidIsValid(type))
{
! lfirst(fargs) = coerce_type(pstate
Bruce Momjian <maillist@candle.pha.pa.us> writes:
> Does anyone have an opinion on this? Why does only DEFAULT have this
> problem? Does anyone know how inserts of '' into char() fields get
> padded with the proper atttypmod value? Do I need to pass atttypmod to
> all the functions that call parse_coerce, so I can pass a value for all
> cases?
Possibly DEFAULT is the only case where the constant value created by
the parser will get shoved directly into a tuple with no run-time
coercion? That's strictly a guess. I agree this issue needs to be
looked at more closely.
Now that we know the problem comes from missing atttypmod info, it
seems likely that related failures can occur for NUMERIC and other
types that depend on atttypmod. (Are there any such types? Even
if there aren't now, there will probably be more and more in future.)
It might be best to just bite the bullet and make the parser carry
around both the type's OID and typmod at all times.
regards, tom lane
> Bruce Momjian <maillist@candle.pha.pa.us> writes: > > Does anyone have an opinion on this? Why does only DEFAULT have this > > problem? Does anyone know how inserts of '' into char() fields get > > padded with the proper atttypmod value? Do I need to pass atttypmod to > > all the functions that call parse_coerce, so I can pass a value for all > > cases? > > Possibly DEFAULT is the only case where the constant value created by > the parser will get shoved directly into a tuple with no run-time > coercion? That's strictly a guess. I agree this issue needs to be > looked at more closely. > > Now that we know the problem comes from missing atttypmod info, it > seems likely that related failures can occur for NUMERIC and other > types that depend on atttypmod. (Are there any such types? Even > if there aren't now, there will probably be more and more in future.) > It might be best to just bite the bullet and make the parser carry > around both the type's OID and typmod at all times. That was my guess too, that atttypmod would become more important. -- Bruce Momjian | http://www.op.net/~candle maillist@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
> Now that we know the problem comes from missing atttypmod info, it > seems likely that related failures can occur for NUMERIC and other > types that depend on atttypmod. (Are there any such types? Even > if there aren't now, there will probably be more and more in future.) > It might be best to just bite the bullet and make the parser carry > around both the type's OID and typmod at all times. I will try to add it, but I must not that there are some cases where I don't have access to the atttypmod of the result, so it may not be possible to do it in every case. Perhaps I should just leave this for post 6.5, because we don't have any other bug reports on it. -- Bruce Momjian | http://www.op.net/~candle maillist@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 <maillist@candle.pha.pa.us> writes:
>> It might be best to just bite the bullet and make the parser carry
>> around both the type's OID and typmod at all times.
> I will try to add it, but I must not that there are some cases where I
> don't have access to the atttypmod of the result, so it may not be
> possible to do it in every case. Perhaps I should just leave this for
> post 6.5, because we don't have any other bug reports on it.
After further thought, I think this may be a more difficult and subtle
issue than we've realized. In the current state of the system, there
are many places where you have a value that you can only know the type
OID for, not atttypmod --- specifically, any intermediate expression
result. Barring reworking the entire function-call mechanism to pass
atttypmod around, that's not going to be possible to change.
The only context where you really know atttypmod is where you have
just fetched a value out of a table column or are about to store a
value into a table column. When storing, you need to be prepared to
coerce the given value to the right type if *either* type OID or
atttypmod is different --- but, in general, you don't know atttypmod
for the given value. (In the cases I know of, you can deduce it by
examining the value itself, but this requires type-specific knowledge.)
So on the whole I think this is something that has to be dealt with
at the point of storing data into a tuple. Maybe we need a new
fundamental operation for types that pay attention to atttypmod:
"make this value match the typmod of the target column, which is
thus-and-so". Trying to attack the problem from the source side by
propagating typmod all around the parser is probably doomed to failure,
because there will be many contexts where there's no way to know it.
Since you have a fix for the only symptom reported to date, I'm
inclined to agree that we should leave well enough alone for now;
there are other, bigger, problems that we need to address for 6.5.
But I think we'll have to come back to this issue later.
regards, tom lane
Actually, it's not as fixed as all that...
create table foo1 (a char(5) default '', b int4);
insert into foo1 (b) values (334);
select * from foo1;
a | b
-----+--- |334
(1 row)
Good, the basic case is fixed, but:
create table foo2 (a char(5) default text '', b int4);
insert into foo2 (b) values (334);
select * from foo2;
a| b
-+--|16
(1 row)
Ooops.
What you seem to have done is twiddle the handling of DEFAULT clauses
so that the value stored for the default expression is pre-coerced to the
column type. That's good as far as it goes, but it fails in cases where
the stored value has to be of a different type.
My guess is that what *really* ought to happen here is that
transformInsertStmt should check the type of the value it's gotten from
the default clause and apply coerce_type if necessary.
Unless someone can come up with a less artificial example than the one
above, I'm inclined to leave it alone for 6.5. This is the same code
area that will have to be redone to fix the INSERT ... SELECT problem
I was chasing earlier today: coercion of the values produced by SELECT
will have to wait until the tail end of transformInsertStmt, and we
might as well make wrong-type default constants get fixed in the same
place. So I'm not eager to write some throwaway code to patch a problem
that no one is likely to see in practice. What's your feeling about it?
regards, tom lane
> What's your feeling about it?
(Back from the weekend).
I'd vote for simple-fix-for-now...
- Thomas
--
Thomas Lockhart lockhart@alumni.caltech.edu
South Pasadena, California
> Actually, it's not as fixed as all that... > > create table foo1 (a char(5) default '', b int4); > insert into foo1 (b) values (334); > select * from foo1; > a | b > -----+--- > |334 > (1 row) > > Good, the basic case is fixed, but: > > create table foo2 (a char(5) default text '', b int4); > insert into foo2 (b) values (334); > select * from foo2; > a| b > -+-- > |16 > (1 row) > > Ooops. > > What you seem to have done is twiddle the handling of DEFAULT clauses > so that the value stored for the default expression is pre-coerced to the > column type. That's good as far as it goes, but it fails in cases where > the stored value has to be of a different type. Gee, I didn't know you could specify the type of the default. Look at this: create table kk( x char(20) default bpchar ''); This is going to bypass the coerce_type, so I think this would fail too. > My guess is that what *really* ought to happen here is that > transformInsertStmt should check the type of the value it's gotten from > the default clause and apply coerce_type if necessary. Again, in my example, it will not even get coerced. > Unless someone can come up with a less artificial example than the one > above, I'm inclined to leave it alone for 6.5. This is the same code > area that will have to be redone to fix the INSERT ... SELECT problem > I was chasing earlier today: coercion of the values produced by SELECT > will have to wait until the tail end of transformInsertStmt, and we > might as well make wrong-type default constants get fixed in the same > place. So I'm not eager to write some throwaway code to patch a problem > that no one is likely to see in practice. What's your feeling about it? Yes, I think we will just wait on this one, and add it to our TODO list for the next release. We still have some big items on the list. -- Bruce Momjian | http://www.op.net/~candle maillist@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
> After further thought, I think this may be a more difficult and subtle > issue than we've realized. In the current state of the system, there > are many places where you have a value that you can only know the type > OID for, not atttypmod --- specifically, any intermediate expression > result. Barring reworking the entire function-call mechanism to pass > atttypmod around, that's not going to be possible to change. Yes, I agree this is true, and I am not really sure how we can handle this. The return of a char() field can probably assume that whatever the length identified in the VARLENA length field is the proper length. varchar() is much more complicated. Though the on-disk length doesn't have to match the maximum length specified in the table creation statement, we do need to truncate any overly long strings returned by functions. However, the good news is that there are only a few cases where we care about atttypmod. If we are returning rows to the user from a function, we don't care. They get whatever we produce. The only cases we care are in an INSERT, UPDATE, and now, as we have discovered, a DEFAULT clause on a CREATE TABLE. In all other cases, the atttypmod is not needed. I still need to figure out where INSERT into a char() gets the string padded properly. Time to fire up the ddd debugger, now that I have the 6.5 HISTORY file completed. > The only context where you really know atttypmod is where you have > just fetched a value out of a table column or are about to store a > value into a table column. When storing, you need to be prepared to > coerce the given value to the right type if *either* type OID or > atttypmod is different --- but, in general, you don't know atttypmod > for the given value. (In the cases I know of, you can deduce it by > examining the value itself, but this requires type-specific knowledge.) Yes, I see what you mean. > So on the whole I think this is something that has to be dealt with > at the point of storing data into a tuple. Maybe we need a new > fundamental operation for types that pay attention to atttypmod: > "make this value match the typmod of the target column, which is > thus-and-so". Trying to attack the problem from the source side by > propagating typmod all around the parser is probably doomed to failure, > because there will be many contexts where there's no way to know it. Excellent idea. -- Bruce Momjian | http://www.op.net/~candle maillist@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
Added to TODO: * CREATE TABLE test (a char(5) DEFAULT text '', b int4) fails on INSERT > Actually, it's not as fixed as all that... > > create table foo1 (a char(5) default '', b int4); > insert into foo1 (b) values (334); > select * from foo1; > a | b > -----+--- > |334 > (1 row) > > Good, the basic case is fixed, but: > > create table foo2 (a char(5) default text '', b int4); > insert into foo2 (b) values (334); > select * from foo2; > a| b > -+-- > |16 > (1 row) > > Ooops. > > What you seem to have done is twiddle the handling of DEFAULT clauses > so that the value stored for the default expression is pre-coerced to the > column type. That's good as far as it goes, but it fails in cases where > the stored value has to be of a different type. > > My guess is that what *really* ought to happen here is that > transformInsertStmt should check the type of the value it's gotten from > the default clause and apply coerce_type if necessary. > > Unless someone can come up with a less artificial example than the one > above, I'm inclined to leave it alone for 6.5. This is the same code > area that will have to be redone to fix the INSERT ... SELECT problem > I was chasing earlier today: coercion of the values produced by SELECT > will have to wait until the tail end of transformInsertStmt, and we > might as well make wrong-type default constants get fixed in the same > place. So I'm not eager to write some throwaway code to patch a problem > that no one is likely to see in practice. What's your feeling about it? > > regards, tom lane > -- Bruce Momjian | http://www.op.net/~candle maillist@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