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

Hi Craig,

On Wed, May 3, 2017 at 10:50 AM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> On 3 May 2017 at 12:32, Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com> wrote:
>> [Adding -hackers mailing list]
>>
>> On Fri, Apr 28, 2017 at 6:28 PM, <henry_boehlert(at)agilent(dot)com> wrote:
>>>
>>> The following bug has been logged on the website:
>>>
>>> Bug reference: 14634
>>> Logged by: Henry Boehlert
>>> Email address: henry_boehlert(at)agilent(dot)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.

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

--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com