Thread: BUG #1643: dbf2pg broken + quick fix
The following bug has been logged online: Bug reference: 1643 Logged by: Boris van Schooten Email address: schooten@cs.utwente.nl PostgreSQL version: 7.4.5, 8.0.2 Operating system: FreeBSD Description: dbf2pg broken + quick fix Details: As several people on the mailing lists have already noted, dbf2pg is broken. It usually fails to enter the numeric values from the dbf file in the sql tables. If you enable verbose output (-vv) you see "Illegal numeric value found" errors. As it turns out, the integer checking code (contrib/dbase/dbf2pg.c, function isinteger) is broken. It appears the function reports 0 in case it finds a space in the dbf rather than a digit. I disabled it (always made it return 1). Now, my dbf files import fine.
"Boris van Schooten" <schooten@cs.utwente.nl> writes:
> As it turns out, the integer checking code (contrib/dbase/dbf2pg.c, function
> isinteger) is broken. It appears the function reports 0 in case it finds a
> space in the dbf rather than a digit. I disabled it (always made it return
> 1). Now, my dbf files import fine.
Hmm. I know nothing about dbase ... but if the test has any use at all,
I guess it must be to handle NULL values. How does dbase represent a
NULL? Why is this code only checking this for integer columns?
regards, tom lane
Boris van Schooten <schooten@cs.utwente.nl> writes: > See the spec. > http://www.dbase.com/KnowledgeBase/int/db7_file_fmt.htm Thanks for the link. As far as can be told from this, dbase hasn't got nulls at all -- is that correct, or are they just omitting a ton of relevant information? It looks to me like we should just remove the special case for integer fields altogether. The special case for date fields is wrong in detail as well: as coded it will accept "date" fields with a leading sign, which surely is not intentional. regards, tom lane
It only does the integer check for numbers of type integer (when # of decimals = 0), noninteger numbers are not checked in any way. If isinteger returns 0, the program does give a warning, suggesting it thinks it detected an illegal field rather than a null field. Contrary to the isinteger check on dates, which does not give a warning, though it does enter a null. On dates the check makes sense because dates are always 8 digits long and have no blank padding like numbers. See the spec. http://www.dbase.com/KnowledgeBase/int/db7_file_fmt.htm Don't know anything about nulls in dbf though. I am not a dbase expert, I just run into dbfs often when trying to enter gis data into postgis. Kind regards, Boris van Schooten On Tue, 3 May 2005, Tom Lane wrote: > "Boris van Schooten" <schooten@cs.utwente.nl> writes: > > As it turns out, the integer checking code (contrib/dbase/dbf2pg.c, function > > isinteger) is broken. It appears the function reports 0 in case it finds a > > space in the dbf rather than a digit. I disabled it (always made it return > > 1). Now, my dbf files import fine. > > Hmm. I know nothing about dbase ... but if the test has any use at all, > I guess it must be to handle NULL values. How does dbase represent a > NULL? Why is this code only checking this for integer columns? > > regards, tom lane >
I can't find anything about dbase nulls either. Never seen one. I'll report it when I find nulls (or other bugs in the code). I'm using some 5-10 different db formats, and noticed most of the converters need some work (as usual). The isinteger seems to be made to do double duty, but it doesn't exactly do it well. The code is dated +/-1995, and the version number is unclear, but I noticed there have been recent changes. Maybe cvs will give more info? Tell me when you got your patch ready, then I'll test the newest version. There was this funny thing with the bug report form on the postgres site. I got mails which suggest that the message bounced. Later I sent a mail to Bruce Momjian. I never knew if you replied to the bug report or to the mail to Bruce. Do you know what happened here? Do bug reports get bounced? Kind regards, Boris van Schooten On Tue, 3 May 2005, Tom Lane wrote: > Boris van Schooten <schooten@cs.utwente.nl> writes: > > See the spec. > > > http://www.dbase.com/KnowledgeBase/int/db7_file_fmt.htm > > Thanks for the link. As far as can be told from this, dbase hasn't got > nulls at all -- is that correct, or are they just omitting a ton of > relevant information? > > It looks to me like we should just remove the special case for integer > fields altogether. The special case for date fields is wrong in detail > as well: as coded it will accept "date" fields with a leading sign, > which surely is not intentional. > > regards, tom lane >
Boris van Schooten <schooten@cs.utwente.nl> writes:
> Don't know anything about nulls in dbf though. I am not a dbase expert, I
> just run into dbfs often when trying to enter gis data into postgis.
I'm considering the following patch, which turns around the test: check
for an empty string and if so believe it's a null, otherwise just insert
the value as-is. I dunno if the check for null is actually meaningful,
but I doubt this will break any cases that worked before. Comments anyone?
regards, tom lane
Index: contrib/dbase/dbf2pg.c
===================================================================
RCS file: /cvsroot/pgsql/contrib/dbase/dbf2pg.c,v
retrieving revision 1.21
diff -c -r1.21 dbf2pg.c
*** contrib/dbase/dbf2pg.c 14 Sep 2004 03:28:28 -0000 1.21
--- contrib/dbase/dbf2pg.c 4 May 2005 17:55:29 -0000
***************
*** 63,93 ****
char *convert_charset(char *string);
#endif
void usage(void);
- unsigned int isinteger(char *);
-
- unsigned int
- isinteger(char *buff)
- {
- char *i = buff;
-
- while (*i != '\0')
- {
- if (i == buff)
- if ((*i == '-') ||
- (*i == '+'))
- {
- i++;
- continue;
- }
- if (!isdigit((unsigned char) *i))
- return 0;
- i++;
- }
- return 1;
- }
-
static inline void
strtoupper(char *string)
{
--- 63,70 ----
***************
*** 471,478 ****
/* handle the date first - liuk */
if (fields[h].db_type == 'D')
{
! if ((strlen(foo) == 8) && isinteger(foo))
{
snprintf(pgdate, 11, "%c%c%c%c-%c%c-%c%c",
foo[0], foo[1], foo[2], foo[3],
foo[4], foo[5], foo[6], foo[7]);
--- 448,462 ----
/* handle the date first - liuk */
if (fields[h].db_type == 'D')
{
! if (strlen(foo) == 0)
{
+ /* assume empty string means a NULL */
+ strcat(query, "\\N");
+ }
+ else if (strlen(foo) == 8 &&
+ strspn(foo, "0123456789") == 8)
+ {
+ /* transform YYYYMMDD to Postgres style */
snprintf(pgdate, 11, "%c%c%c%c-%c%c-%c%c",
foo[0], foo[1], foo[2], foo[3],
foo[4], foo[5], foo[6], foo[7]);
***************
*** 480,505 ****
}
else
{
! /*
! * empty field must be inserted as NULL value in
! * this way
! */
! strcat(query, "\\N");
}
}
! else if ((fields[h].db_type == 'N') &&
! (fields[h].db_dec == 0))
{
! if (isinteger(foo))
! strcat(query, foo);
! else
{
strcat(query, "\\N");
- if (verbose)
- fprintf(stderr, "Illegal numeric value found "
- "in record %d, field \"%s\"\n",
- i, fields[h].db_name);
}
}
else
{
--- 464,482 ----
}
else
{
! /* try to insert it as-is */
! strcat(query, foo);
}
}
! else if (fields[h].db_type == 'N')
{
! if (strlen(foo) == 0)
{
+ /* assume empty string means a NULL */
strcat(query, "\\N");
}
+ else
+ strcat(query, foo);
}
else
{
Looks good to me. I'd prefer to have a warning message (if (verbose) fprintf stderr) for each of the exceptional conditions though. I'm even so paranoid I prefer to have the verbose switch on by default. Kinds regards, Boris van Schooten On Wed, 4 May 2005, Tom Lane wrote: > Boris van Schooten <schooten@cs.utwente.nl> writes: > > Don't know anything about nulls in dbf though. I am not a dbase expert, I > > just run into dbfs often when trying to enter gis data into postgis. > > I'm considering the following patch, which turns around the test: check > for an empty string and if so believe it's a null, otherwise just insert > the value as-is. I dunno if the check for null is actually meaningful, > but I doubt this will break any cases that worked before. Comments anyone? > > regards, tom lane
Boris van Schooten <schooten@cs.utwente.nl> writes:
> Looks good to me. I'd prefer to have a warning message (if (verbose)
> fprintf stderr) for each of the exceptional conditions though. I'm even
> so paranoid I prefer to have the verbose switch on by default.
Don't really see the need for it. What we are doing here is trusting to
the backend to error-check the input, rather than making a half-baked
attempt to do error checking in dbf2pg.
regards, tom lane
I don't see this as half baked error checking, but rather, as error reporting during a "half baked conversion". I prefer to know when the converter encounters something it doesn't understand, rather than having it silently enter nulls into the db. It was the original error reporting that helped me find this bug, I suppose. Kind regards, Boris van Schooten On Thu, 5 May 2005, Tom Lane wrote: > Boris van Schooten <schooten@cs.utwente.nl> writes: > > Looks good to me. I'd prefer to have a warning message (if (verbose) > > fprintf stderr) for each of the exceptional conditions though. I'm even > > so paranoid I prefer to have the verbose switch on by default. > > Don't really see the need for it. What we are doing here is trusting to > the backend to error-check the input, rather than making a half-baked > attempt to do error checking in dbf2pg. > > regards, tom lane >
Tom, where are we on this patch? Seems we need to do something.
Here is the thread:
http://archives.postgresql.org/pgsql-bugs/2005-05/msg00008.php
---------------------------------------------------------------------------
Tom Lane wrote:
> Boris van Schooten <schooten@cs.utwente.nl> writes:
> > Don't know anything about nulls in dbf though. I am not a dbase expert, I
> > just run into dbfs often when trying to enter gis data into postgis.
>
> I'm considering the following patch, which turns around the test: check
> for an empty string and if so believe it's a null, otherwise just insert
> the value as-is. I dunno if the check for null is actually meaningful,
> but I doubt this will break any cases that worked before. Comments anyone?
>
> regards, tom lane
>
>
> Index: contrib/dbase/dbf2pg.c
> ===================================================================
> RCS file: /cvsroot/pgsql/contrib/dbase/dbf2pg.c,v
> retrieving revision 1.21
> diff -c -r1.21 dbf2pg.c
> *** contrib/dbase/dbf2pg.c 14 Sep 2004 03:28:28 -0000 1.21
> --- contrib/dbase/dbf2pg.c 4 May 2005 17:55:29 -0000
> ***************
> *** 63,93 ****
> char *convert_charset(char *string);
> #endif
> void usage(void);
> - unsigned int isinteger(char *);
>
>
> -
> - unsigned int
> - isinteger(char *buff)
> - {
> - char *i = buff;
> -
> - while (*i != '\0')
> - {
> - if (i == buff)
> - if ((*i == '-') ||
> - (*i == '+'))
> - {
> - i++;
> - continue;
> - }
> - if (!isdigit((unsigned char) *i))
> - return 0;
> - i++;
> - }
> - return 1;
> - }
> -
> static inline void
> strtoupper(char *string)
> {
> --- 63,70 ----
> ***************
> *** 471,478 ****
> /* handle the date first - liuk */
> if (fields[h].db_type == 'D')
> {
> ! if ((strlen(foo) == 8) && isinteger(foo))
> {
> snprintf(pgdate, 11, "%c%c%c%c-%c%c-%c%c",
> foo[0], foo[1], foo[2], foo[3],
> foo[4], foo[5], foo[6], foo[7]);
> --- 448,462 ----
> /* handle the date first - liuk */
> if (fields[h].db_type == 'D')
> {
> ! if (strlen(foo) == 0)
> {
> + /* assume empty string means a NULL */
> + strcat(query, "\\N");
> + }
> + else if (strlen(foo) == 8 &&
> + strspn(foo, "0123456789") == 8)
> + {
> + /* transform YYYYMMDD to Postgres style */
> snprintf(pgdate, 11, "%c%c%c%c-%c%c-%c%c",
> foo[0], foo[1], foo[2], foo[3],
> foo[4], foo[5], foo[6], foo[7]);
> ***************
> *** 480,505 ****
> }
> else
> {
> ! /*
> ! * empty field must be inserted as NULL value in
> ! * this way
> ! */
> ! strcat(query, "\\N");
> }
> }
> ! else if ((fields[h].db_type == 'N') &&
> ! (fields[h].db_dec == 0))
> {
> ! if (isinteger(foo))
> ! strcat(query, foo);
> ! else
> {
> strcat(query, "\\N");
> - if (verbose)
> - fprintf(stderr, "Illegal numeric value found "
> - "in record %d, field \"%s\"\n",
> - i, fields[h].db_name);
> }
> }
> else
> {
> --- 464,482 ----
> }
> else
> {
> ! /* try to insert it as-is */
> ! strcat(query, foo);
> }
> }
> ! else if (fields[h].db_type == 'N')
> {
> ! if (strlen(foo) == 0)
> {
> + /* assume empty string means a NULL */
> strcat(query, "\\N");
> }
> + else
> + strcat(query, foo);
> }
> else
> {
>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: you can get off all lists at once with the unregister command
> (send "unregister YourEmailAddressHere" to majordomo@postgresql.org)
>
--
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 19073
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Tom, where are we on this patch? Seems we need to do something.
It's in my to-do pile. I was debating whether it's OK to commit to
8.0 branch, or should I just put it in HEAD.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > Tom, where are we on this patch? Seems we need to do something. > > It's in my to-do pile. I was debating whether it's OK to commit to > 8.0 branch, or should I just put it in HEAD. I am thinking just HEAD. If someone really needs it we can point them to CVS HEAD and they can test. -- 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 19073