Thread

1. incompatible pointer types with newer zlib

   Peter Eisentraut <peter_e@gmx.net> — 2012-02-23T09:16:11Z

   According to me update logs, somewhere between zlib versions 1.2.3.4 and
   1.2.6, the definition of the gzFile type was changed from void * to
   struct gzFile_s *, an opaque struct.  Note that gzFile is already the
   pointer in either case.  Our code has assumed, however, that you use
   gzFile like FILE, namely that the APIs take a pointer to the type, but
   that is not the case.  So code like
   
   gzFile *handle = gzopen(...)
   
   is wrong.
   
   This used to pass silently because you can assign a void* to a void**,
   but with the newer definition you get a bunch of warnings like
   
   pg_backup_files.c: In function ‘_StartData’:
   pg_backup_files.c:256:11: warning: assignment from incompatible pointer type [enabled by default]
   pg_backup_files.c: In function ‘_WriteData’:
   pg_backup_files.c:271:2: warning: passing argument 1 of ‘gzwrite’ from incompatible pointer type [enabled by default]
   /usr/include/zlib.h:1318:12: note: expected ‘gzFile’ but argument is of type ‘struct gzFile_s **’
   
   Affected are pg_dump and pg_basebackup.
   
   Fixing most of this is not difficult, see attached patch.  The only
   ugliness is in pg_backup_archiver.h
   
   FILE       *FH;             /* General purpose file handle */
   
   which is used throughout pg_dump as sometimes a real FILE* and sometimes
   a gzFile handle.  There are also some fileno() calls on this, so just
   replacing this with an #ifdef isn't going to work.  This might need some
   more restructuring to make the code truly type-safe.  My quick patch
   replaces the type with void*, thus sort of restoring the original
   situation that allowed this to work.
   
   Note that these are only warnings, so we probably don't need to worry
   about backpatching this in a hurry.
   
   

2. Re: incompatible pointer types with newer zlib

   Tom Lane <tgl@sss.pgh.pa.us> — 2012-02-23T15:17:28Z

   Peter Eisentraut <peter_e@gmx.net> writes:
   > Fixing most of this is not difficult, see attached patch.  The only
   > ugliness is in pg_backup_archiver.h
   
   > FILE       *FH;             /* General purpose file handle */
   
   > which is used throughout pg_dump as sometimes a real FILE* and sometimes
   > a gzFile handle.  There are also some fileno() calls on this, so just
   > replacing this with an #ifdef isn't going to work.  This might need some
   > more restructuring to make the code truly type-safe.  My quick patch
   > replaces the type with void*, thus sort of restoring the original
   > situation that allowed this to work.
   
   void * seems entirely reasonable given the two different usages, but
   I would be happier if the patch added explicit casts whereever FH is
   set to or used as one of these two types.
   
   			regards, tom lane
   
   

3. Re: incompatible pointer types with newer zlib

   Peter Eisentraut <peter_e@gmx.net> — 2012-02-24T10:01:13Z

   On tor, 2012-02-23 at 10:17 -0500, Tom Lane wrote:
   > Peter Eisentraut <peter_e@gmx.net> writes:
   > > Fixing most of this is not difficult, see attached patch.  The only
   > > ugliness is in pg_backup_archiver.h
   > 
   > > FILE       *FH;             /* General purpose file handle */
   > 
   > > which is used throughout pg_dump as sometimes a real FILE* and sometimes
   > > a gzFile handle.  There are also some fileno() calls on this, so just
   > > replacing this with an #ifdef isn't going to work.  This might need some
   > > more restructuring to make the code truly type-safe.  My quick patch
   > > replaces the type with void*, thus sort of restoring the original
   > > situation that allowed this to work.
   > 
   > void * seems entirely reasonable given the two different usages, but
   > I would be happier if the patch added explicit casts whereever FH is
   > set to or used as one of these two types.
   
   That would add about 70 casts all over the place.  I don't think that
   will make things clearer or more robust.  I think we either leave it as
   my first patch for now or find a more robust solution with a union or
   something.
   
   
   

4. Re: incompatible pointer types with newer zlib

   Tom Lane <tgl@sss.pgh.pa.us> — 2012-02-24T16:10:16Z

   Peter Eisentraut <peter_e@gmx.net> writes:
   > On tor, 2012-02-23 at 10:17 -0500, Tom Lane wrote:
   >> void * seems entirely reasonable given the two different usages, but
   >> I would be happier if the patch added explicit casts whereever FH is
   >> set to or used as one of these two types.
   
   > That would add about 70 casts all over the place.  I don't think that
   > will make things clearer or more robust.  I think we either leave it as
   > my first patch for now or find a more robust solution with a union or
   > something.
   
   Hm.  Could we just create two separate fields?  It's not real clear to
   me that forcing both these usages into a generic pointer buys much.
   
   			regards, tom lane
   
   

5. Re: incompatible pointer types with newer zlib

   Peter Eisentraut <peter_e@gmx.net> — 2012-03-01T17:19:54Z

   On fre, 2012-02-24 at 11:10 -0500, Tom Lane wrote:
   > Peter Eisentraut <peter_e@gmx.net> writes:
   > > On tor, 2012-02-23 at 10:17 -0500, Tom Lane wrote:
   > >> void * seems entirely reasonable given the two different usages, but
   > >> I would be happier if the patch added explicit casts whereever FH is
   > >> set to or used as one of these two types.
   > 
   > > That would add about 70 casts all over the place.  I don't think that
   > > will make things clearer or more robust.  I think we either leave it as
   > > my first patch for now or find a more robust solution with a union or
   > > something.
   > 
   > Hm.  Could we just create two separate fields?  It's not real clear to
   > me that forcing both these usages into a generic pointer buys much.
   
   It's used in confusing ways.
   
   In pg_backup_files.c _PrintFileData(), it's a compile-time decision
   whether FH is a FILE or a gzFile:
   
   #ifdef HAVE_LIBZ
       AH->FH = gzopen(filename, "rb");
   #else
       AH->FH = fopen(filename, PG_BINARY_R);
   #endif
   
   But if we changed FH to be gzFile under HAVE_LIBZ, then tons of other
   places will complain that use fread(), fwrite(), fileno(), etc. directly
   on FH.
   
   Considering the volume of who complains in one way versus the other, I
   think _PrintFileData() is at fault.
   
   I think the best fix would be to rearrange _PrintFileData() so that it
   doesn't use FH at all.  Instead, we could define a separate
   ArchiveHandle field IF that works more like OF, and then change
   ahwrite() to use that.
   
   
   
   

6. Re: incompatible pointer types with newer zlib

   Peter Eisentraut <peter_e@gmx.net> — 2012-03-17T07:58:49Z

   On tor, 2012-03-01 at 19:19 +0200, Peter Eisentraut wrote:
   > I think the best fix would be to rearrange _PrintFileData() so that it
   > doesn't use FH at all.  Instead, we could define a separate
   > ArchiveHandle field IF that works more like OF, and then change
   > ahwrite() to use that.
   
   Here is a patch that might fix this.  I haven't been able to test this
   properly, so this is just from tracing the code.  It looks like
   _PrintFileData() doesn't need to use FH at all, so it could use a local
   file handle variable instead.  Could someone verify this please?
   
   

7. Re: incompatible pointer types with newer zlib

   Robert Haas <robertmhaas@gmail.com> — 2012-03-19T16:50:54Z

   On Sat, Mar 17, 2012 at 3:58 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
   > On tor, 2012-03-01 at 19:19 +0200, Peter Eisentraut wrote:
   >> I think the best fix would be to rearrange _PrintFileData() so that it
   >> doesn't use FH at all.  Instead, we could define a separate
   >> ArchiveHandle field IF that works more like OF, and then change
   >> ahwrite() to use that.
   >
   > Here is a patch that might fix this.  I haven't been able to test this
   > properly, so this is just from tracing the code.  It looks like
   > _PrintFileData() doesn't need to use FH at all, so it could use a local
   > file handle variable instead.  Could someone verify this please?
   
   It looks like this code can be used via the undocumented -Ff option to
   pg_dump.  But considering this code was added in 2000 as demonstration
   code and has apparently never been documented, and considering also
   that we now have the "directory" archive format which is presumably
   quite a similar idea but documented and intended for production use,
   maybe we should just rip out pg_backup_files/archFiles altogether.
   pg_dump is crufty enough without supporting undocumented and obsolete
   options for multiple decades.
   
   -- 
   Robert Haas
   EnterpriseDB: http://www.enterprisedb.com
   The Enterprise PostgreSQL Company
   
   

8. Re: incompatible pointer types with newer zlib

   Tom Lane <tgl@sss.pgh.pa.us> — 2012-03-19T18:53:37Z

   Robert Haas <robertmhaas@gmail.com> writes:
   > maybe we should just rip out pg_backup_files/archFiles altogether.
   > pg_dump is crufty enough without supporting undocumented and obsolete
   > options for multiple decades.
   
   +1
   
   			regards, tom lane
   
   

9. Re: incompatible pointer types with newer zlib

   Andrew Dunstan <andrew@dunslane.net> — 2012-03-19T19:06:28Z

   
   On 03/19/2012 02:53 PM, Tom Lane wrote:
   > Robert Haas<robertmhaas@gmail.com>  writes:
   >> maybe we should just rip out pg_backup_files/archFiles altogether.
   >> pg_dump is crufty enough without supporting undocumented and obsolete
   >> options for multiple decades.
   > +1
   >
   > 			
   
   
   Yeah, go for it.
   
   cheers
   
   andrew