Re: pg_upgrade diffs on WIndows - Mailing list pgsql-hackers

On Tue, Sep  4, 2012 at 03:44:35PM -0400, Andrew Dunstan wrote:
> 
> On 09/04/2012 03:09 PM, Andrew Dunstan wrote:
> >I realized this morning that I might have been a bit cavalier in
> >using dos2unix to smooth away differences in the dumpfiles
> >produced by pg_upgrade. Attached is a dump of the diff if this
> >isn't done,  with Carriage Returns printed as '*' to make them
> >visible. As can be seen, in function bodies dump2 has the Carriage
> >Returns doubled. I have not had time to delve into how this comes
> >about, and I need to attend to some income-producing activity for
> >a bit, but I'd like to get it cleaned up ASAP. We are under the
> >hammer for 9.2, so any help other people can give on this would be
> >appreciated.
> >
> 
> 
> Actually, I have the answer - it's quite simple. We just need to
> open the output files in binary mode when we split the dumpall file.
> The attached patch fixes it. I think we should backpatch the first
> part to 9.0.

> diff --git a/contrib/pg_upgrade/dump.c b/contrib/pg_upgrade/dump.c
> index b905ab0..0a96dde 100644
> --- a/contrib/pg_upgrade/dump.c
> +++ b/contrib/pg_upgrade/dump.c
> @@ -62,10 +62,10 @@ split_old_dump(void)
>      if ((all_dump = fopen(filename, "r")) == NULL)
>          pg_log(PG_FATAL, "Could not open dump file \"%s\": %s\n", filename, getErrorText(errno));
>      snprintf(filename, sizeof(filename), "%s", GLOBALS_DUMP_FILE);
> -    if ((globals_dump = fopen_priv(filename, "w")) == NULL)
> +    if ((globals_dump = fopen_priv(filename, PG_BINARY_W)) == NULL)
>          pg_log(PG_FATAL, "Could not write to dump file \"%s\": %s\n", filename, getErrorText(errno));
>      snprintf(filename, sizeof(filename), "%s", DB_DUMP_FILE);
> -    if ((db_dump = fopen_priv(filename, "w")) == NULL)
> +    if ((db_dump = fopen_priv(filename, PG_BINARY_W)) == NULL)
>          pg_log(PG_FATAL, "Could not write to dump file \"%s\": %s\n", filename, getErrorText(errno));
>  
>      current_output = globals_dump;
> diff --git a/contrib/pg_upgrade/test.sh b/contrib/pg_upgrade/test.sh
> index d411ac6..3899600 100644
> --- a/contrib/pg_upgrade/test.sh
> +++ b/contrib/pg_upgrade/test.sh
> @@ -128,10 +128,6 @@ else
>      sh ./delete_old_cluster.sh
>  fi
>  
> -if [ $testhost = Msys ] ; then
> -       dos2unix "$temp_root"/dump1.sql "$temp_root"/dump2.sql
> -fi
> -
>  if diff -q "$temp_root"/dump1.sql "$temp_root"/dump2.sql; then
>      echo PASSED
>      exit 0

I reviewed this idea and supports this patch's inclusion in 9.2.  I was
unclear why it was needed, but I see pg_dumpall, which is the file
pg_upgrade splits apart, as also using binary mode to write this file:
        OPF = fopen(filename, PG_BINARY_W);

I agree with Tom that pg_upgrade needs some quiet time.  ;-)  Andrew,
have a sufficient number of buildfarm members verified our recent
patches that this can be added.  My patch from last night was mostly C
comments so isn't something that needs testing.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + It's impossible for everything to be true. +