Re: [HACKERS] incompatible pointer types with newer zlib

2012-03-19 Thread Robert Haas
On Sat, Mar 17, 2012 at 3:58 AM, Peter Eisentraut pete...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] incompatible pointer types with newer zlib

2012-03-19 Thread Tom Lane
Robert Haas robertmh...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] incompatible pointer types with newer zlib

2012-03-19 Thread Andrew Dunstan



On 03/19/2012 02:53 PM, Tom Lane wrote:

Robert Haasrobertmh...@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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] incompatible pointer types with newer zlib

2012-03-17 Thread Peter Eisentraut
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?

diff --git i/src/bin/pg_dump/pg_backup_files.c w/src/bin/pg_dump/pg_backup_files.c
index a7fd91d..32b2a32 100644
--- i/src/bin/pg_dump/pg_backup_files.c
+++ w/src/bin/pg_dump/pg_backup_files.c
@@ -293,27 +293,32 @@ _PrintFileData(ArchiveHandle *AH, char *filename, RestoreOptions *ropt)
 {
 	char		buf[4096];
 	size_t		cnt;
+#ifdef HAVE_LIBZ
+	gzFile		fh;
+#else
+	FILE	   *fh;
+#endif
 
 	if (!filename)
 		return;
 
 #ifdef HAVE_LIBZ
-	AH-FH = gzopen(filename, rb);
+	fh = gzopen(filename, rb);
 #else
-	AH-FH = fopen(filename, PG_BINARY_R);
+	fh = fopen(filename, PG_BINARY_R);
 #endif
 
-	if (AH-FH == NULL)
+	if (!fh)
 		die_horribly(AH, modulename, could not open input file \%s\: %s\n,
 	 filename, strerror(errno));
 
-	while ((cnt = GZREAD(buf, 1, 4095, AH-FH))  0)
+	while ((cnt = GZREAD(buf, 1, 4095, fh))  0)
 	{
 		buf[cnt] = '\0';
 		ahwrite(buf, 1, cnt, AH);
 	}
 
-	if (GZCLOSE(AH-FH) != 0)
+	if (GZCLOSE(fh) != 0)
 		die_horribly(AH, modulename, could not close data file after reading\n);
 }
 

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] incompatible pointer types with newer zlib

2012-03-01 Thread Peter Eisentraut
On fre, 2012-02-24 at 11:10 -0500, Tom Lane wrote:
 Peter Eisentraut pete...@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.



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] incompatible pointer types with newer zlib

2012-02-24 Thread Peter Eisentraut
On tor, 2012-02-23 at 10:17 -0500, Tom Lane wrote:
 Peter Eisentraut pete...@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.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] incompatible pointer types with newer zlib

2012-02-24 Thread Tom Lane
Peter Eisentraut pete...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] incompatible pointer types with newer zlib

2012-02-23 Thread Tom Lane
Peter Eisentraut pete...@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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers