bug in COPY
This behavior doesn't look right:
nconway=# create table foo (a int default 50, b int default 100);
CREATE TABLE
nconway=# copy foo from stdin;
Enter data to be copied followed by a newline.
End with a backslash and a period on a line by itself.
\.
nconway=# select * from foo;
a | b
---+---
0 |
(1 row)
(The first line of the COPY input is blank: i.e. just a newline)
The problem appears to be in both 7.2.1 and current CVS.
Cheers,
Neil
--
Neil Conway <neilconway@rogers.com>
PGP Key ID: DB3C29FC
nconway@klamath.dyndns.org (Neil Conway) writes:
This behavior doesn't look right:
It's not, but I believe the correct point of view is that the input
data is defective and should be rejected. See past discussions
leading up to the TODO item that mentions rejecting COPY input rows
with the wrong number of fields (rather than silently filling with
NULLs as we do now).
A subsidiary point here is that pg_atoi() explicitly takes a zero-length
string as valid input of value 0. I think this is quite bogus myself,
but I don't know why that behavior was put in or whether we'd be breaking
anything if we tightened it up.
regards, tom lane
On Wed, Jul 24, 2002 at 04:23:56PM -0400, Tom Lane wrote:
nconway@klamath.dyndns.org (Neil Conway) writes:
This behavior doesn't look right:
It's not, but I believe the correct point of view is that the input
data is defective and should be rejected. See past discussions
leading up to the TODO item that mentions rejecting COPY input rows
with the wrong number of fields (rather than silently filling with
NULLs as we do now).
Yeah, I was thinking that too. Now that we have column lists in
COPY, there is no need to keep this functionality around: if the
user wants to load data that is missing a column, they can just
omit the column from the column list and have the column default
inserted (which is a lot more sensible than inserting NULL).
Unfortunately, I think that removing this properly will require
refactoring some of the COPY code. I'll take a look at implementing
this...
Cheers,
Neil
--
Neil Conway <neilconway@rogers.com>
PGP Key ID: DB3C29FC
nconway@klamath.dyndns.org (Neil Conway) writes:
leading up to the TODO item that mentions rejecting COPY input rows
with the wrong number of fields (rather than silently filling with
NULLs as we do now).
Yeah, I was thinking that too. Now that we have column lists in
COPY, there is no need to keep this functionality around: if the
user wants to load data that is missing a column, they can just
omit the column from the column list and have the column default
inserted (which is a lot more sensible than inserting NULL).
Right, that was the thinking exactly.
Unfortunately, I think that removing this properly will require
refactoring some of the COPY code. I'll take a look at implementing
this...
I thought it could be hacked in pretty easily, but if you want to
clean up the structure while you're in there, go for it.
regards, tom lane
Tom Lane wrote:
nconway@klamath.dyndns.org (Neil Conway) writes:
This behavior doesn't look right:
It's not, but I believe the correct point of view is that the input
data is defective and should be rejected. See past discussions
leading up to the TODO item that mentions rejecting COPY input rows
with the wrong number of fields (rather than silently filling with
NULLs as we do now).A subsidiary point here is that pg_atoi() explicitly takes a zero-length
string as valid input of value 0. I think this is quite bogus myself,
but I don't know why that behavior was put in or whether we'd be breaking
anything if we tightened it up.
Yea, I found the atoi zero-length behavior when I tried to clean up the
use of strtol() recently. If you remove that behavior, the regression
tests fail.
--
Bruce Momjian | http://candle.pha.pa.us
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 19026Tom Lane wrote:
A subsidiary point here is that pg_atoi() explicitly takes a zero-length
string as valid input of value 0. I think this is quite bogus myself,
but I don't know why that behavior was put in or whether we'd be breaking
anything if we tightened it up.
I have attached a patch the throws an error if pg_atoi() is passed a
zero-length string, and have included regression diffs showing the
effects of the patch.
Seems the new code catches a few places that were bad, like defineing {}
for an array of 0,0. The copy2.out change is because pg_atoi catches
the problem before COPY does.
The tightening up of pg_atoi seems safe and makes sense to me.
If no adverse comments, I will apply and fix up the regression results.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073Bruce Momjian <pgman@candle.pha.pa.us> writes:
else if (*s == 0)
! elog(ERROR, "pg_atoi: zero-length string!");
The exclamation point seems inappropriate. Perhaps "zero-length input"
would be better than "string" also.
Otherwise this seems like a reasonable thing to do.
regards, tom lane
Tom Lane wrote:
Bruce Momjian <pgman@candle.pha.pa.us> writes:
else if (*s == 0)
! elog(ERROR, "pg_atoi: zero-length string!");The exclamation point seems inappropriate. Perhaps "zero-length input"
would be better than "string" also.
I copied the other test case:
if (s == (char *) NULL)
elog(ERROR, "pg_atoi: NULL pointer!");
I removed them both '!'.
Otherwise this seems like a reasonable thing to do.
OK, will apply now.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073Bruce Momjian <pgman@candle.pha.pa.us> writes:
Tom Lane wrote:
The exclamation point seems inappropriate. Perhaps "zero-length input"
would be better than "string" also.
I copied the other test case:
if (s == (char *) NULL)
elog(ERROR, "pg_atoi: NULL pointer!");
Well, the NULL-pointer test might equally well be coded as an Assert:
it's to catch backend coding errors, not cases of incorrect user input.
So the exclamation point there didn't bother me.
I removed them both '!'.
If you like. But the two conditions are not comparable.
regards, tom lane
Tom Lane wrote:
Bruce Momjian <pgman@candle.pha.pa.us> writes:
Tom Lane wrote:
The exclamation point seems inappropriate. Perhaps "zero-length input"
would be better than "string" also.I copied the other test case:
if (s == (char *) NULL)
elog(ERROR, "pg_atoi: NULL pointer!");Well, the NULL-pointer test might equally well be coded as an Assert:
it's to catch backend coding errors, not cases of incorrect user input.
So the exclamation point there didn't bother me.
You really think it should be Assert? I don't mind opening the chance
to throw an error for bad input, but are you sure that a NULL should
never get there?
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073Patch applied.
---------------------------------------------------------------------------
Tom Lane wrote:
Bruce Momjian <pgman@candle.pha.pa.us> writes:
Tom Lane wrote:
The exclamation point seems inappropriate. Perhaps "zero-length input"
would be better than "string" also.I copied the other test case:
if (s == (char *) NULL)
elog(ERROR, "pg_atoi: NULL pointer!");Well, the NULL-pointer test might equally well be coded as an Assert:
it's to catch backend coding errors, not cases of incorrect user input.
So the exclamation point there didn't bother me.I removed them both '!'.
If you like. But the two conditions are not comparable.
regards, tom lane
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073Attachments:
/bjm/difftext/plainDownload+10-10
Bruce Momjian <pgman@candle.pha.pa.us> writes:
Tom Lane wrote:
Well, the NULL-pointer test might equally well be coded as an Assert:
You really think it should be Assert?
I don't feel a need to change it, no. I was just pointing out that
there shouldn't be any way for a user to cause that condition to occur.
regards, tom lane