Re: Re: [BUGS] BUG #14634: On Windows pg_basebackup should write tar to stdout in binary mode

On Wed, May 3, 2017 at 10:44 PM, Ashutosh Sharma <ashu.coek88@gmail.com>
wrote:

> Hi Craig,
>
> On Wed, May 3, 2017 at 10:50 AM, Craig Ringer <craig@2ndquadrant.com>
> wrote:
> > On 3 May 2017 at 12:32, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
> >> [Adding -hackers mailing list]
> >>
> >> On Fri, Apr 28, 2017 at 6:28 PM, <henry_boehlert@agilent.com> wrote:
> >>>
> >>> The following bug has been logged on the website:
> >>>
> >>> Bug reference:      14634
> >>> Logged by:          Henry Boehlert
> >>> Email address:      henry_boehlert@agilent.com
> >>> PostgreSQL version: 9.6.2
> >>> Operating system:   Windows Server 2012 R2 6.3.9600
> >>> Description:
> >>>
> >>> Executing command pg_basebackup -D -F t writes its output to stdout,
> which
> >>> is open in text mode, causing LF to be converted to CR LF thus
> corrupting
> >>> the resulting archive.
> >>>
> >>> To write the tar to stdout, on Windows stdout's mode should be
> temporarily
> >>> switched to binary.
> >>>
> >>> https://msdn.microsoft.com/en-us/library/tw4k6df8.aspx
> >>
> >>
> >> Thanks for reporting the issue.
> >> With the attached patch, I was able to extract the tar file that gets
> >> generated when the tar file is written into stdout. I tested the
> >> the compressed tar also.
> >>
> >> This bug needs to be fixed in back branches also.
> >
> > We should do the same for pg_dump in -Fc mode.
>
> Did you meant -Fp mode. I think we are already setting stdout file to
> binary mode if the output format is custom. Please refer to the
> following code in parseArchiveFormat() and _allocAH() respectively
>
> static ArchiveFormat
> parseArchiveFormat(const char *format, ArchiveMode *mode)
> {
>     ...............
>     ...............
>     else if (pg_strcasecmp(format, "c") == 0)
>         archiveFormat = archCustom;
>     else if (pg_strcasecmp(format, "custom") == 0)
>         archiveFormat = archCustom;
>
>     else if (pg_strcasecmp(format, "p") == 0)
>         archiveFormat = archNull;
>     else if (pg_strcasecmp(format, "plain") == 0)
>         archiveFormat = archNull;
>     ...............
>     ...............
> }
>
> static ArchiveHandle *
> _allocAH(const char *FileSpec, const ArchiveFormat fmt,
>          const int compression, bool dosync, ArchiveMode mode,
>          SetupWorkerPtrType setupWorkerPtr)
> {
>
>     ...............
>     ...............
> #ifdef WIN32
>     if (fmt != archNull &&
>         (AH->fSpec == NULL || strcmp(AH->fSpec, "") == 0))
>     {
>         if (mode == archModeWrite)
>             setmode(fileno(stdout), O_BINARY);
>         else
>             setmode(fileno(stdin), O_BINARY);
>     }
> #endif
>     ..................
>     ..................
> }
>
> Please confirm.
>

I also think it is the plain text mode. There is no problem with text
mode file that contains the CR LF characters.

Meanwhile, I have unit tested the patch submitted by Hari upthread on
> postgresql v10 and it works fine. Below are the steps that i have
> followed to test Hari's patch.
>
> Without patch:
> ==============
> C:\Users\ashu\postgresql\TMP\test\bin>.\pg_basebackup.exe -D - -F t -X
> none > base.tar
> NOTICE:  WAL archiving is not enabled; you must ensure that all required
> WAL seg
> ments are copied through other means to complete the backup
>
> C:\Users\ashu\postgresql\TMP\test\bin>tar -xf base.tar
> tar: Skipping to next header
> tar: Exiting with failure status due to previous errors
>
>
> With patch:
> ===========
> C:\Users\ashu\git-clone-postgresql\postgresql\TMP\
> test\bin>.\pg_basebackup.exe
> -D - -F t -X none > base.tar
> NOTICE:  WAL archiving is not enabled; you must ensure that all required
> WAL seg
> ments are copied through other means to complete the backup
>
> C:\Users\ashu\postgresql\TMP\test\bin>cp base.tar ..\basebakup
>
> C:\Users\ashu\postgresql\TMP\test\basebakup>tar -xf base.tar
>
> C:\Users\ashu\postgresql\TMP\test\basebakup>ls
> PG_VERSION    pg_commit_ts   pg_multixact  pg_stat      pg_wal
> backup_label  pg_dynshmem    pg_notify     pg_stat_tmp  pg_xact
> base          pg_hba.conf    pg_replslot   pg_subtrans
> postgresql.auto.conf
> base.tar      pg_ident.conf  pg_serial     pg_tblspc    postgresql.conf
> global        pg_logical     pg_snapshots  pg_twophase  tablespace_map
>
>
Thanks for the tests to verify the patch.


Regards,
Hari Babu
Fujitsu Australia