Thread: BUG #1643: dbf2pg broken + quick fix

BUG #1643: dbf2pg broken + quick fix

From
"Boris van Schooten"
Date:
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.

Re: BUG #1643: dbf2pg broken + quick fix

From
Tom Lane
Date:
"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

Re: BUG #1643: dbf2pg broken + quick fix

From
Tom Lane
Date:
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

Re: BUG #1643: dbf2pg broken + quick fix

From
Boris van Schooten
Date:
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
>

Re: BUG #1643: dbf2pg broken + quick fix

From
Boris van Schooten
Date:
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
>

Re: BUG #1643: dbf2pg broken + quick fix

From
Tom Lane
Date:
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
                  {

Re: BUG #1643: dbf2pg broken + quick fix

From
Boris van Schooten
Date:
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

Re: BUG #1643: dbf2pg broken + quick fix

From
Tom Lane
Date:
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

Re: BUG #1643: dbf2pg broken + quick fix

From
Boris van Schooten
Date:
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
>

Re: BUG #1643: dbf2pg broken + quick fix

From
Bruce Momjian
Date:
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

Re: BUG #1643: dbf2pg broken + quick fix

From
Tom Lane
Date:
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

Re: BUG #1643: dbf2pg broken + quick fix

From
Bruce Momjian
Date:
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