Re: [HACKERS] 0x1A in control file on Windows

Magnus Hagander wrote:
> Heikki Linnakangas wrote:
>> ITAGAKI Takahiro wrote:
>>> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>
>>>> ITAGAKI Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> writes:
>>>>> We probably need to add PG_BINARY when we open control files
>>>>> because 0x1A is an end-of-file marker on Windows.
>>>> Well, why is that a bug? If the platform is so silly as to define text
>>>> files that way, who are we to argue?
>>> Google says it is for for backward compatibility with CP/M
>>> http://en.wikipedia.org/wiki/End-of-file
>>> and adding O_BINARY is the answer.
>>>
>>> http://codenewbie.com/forum/standard-c-c/1208-binary-i-o-file-reading-0x1a-trouble.html
>>>
>> Yes, apparently that's exactly why we have PG_BINARY, see c.h:
>>
>>> /*
>>> * NOTE: this is also used for opening text files.
>>> * WIN32 treats Control-Z as EOF in files opened in text mode.
>>> * Therefore, we open files in binary mode on Win32 so we can read
>>> * literal control-Z. The other affect is that we see CRLF, but
>>> * that is OK because we can already handle those cleanly.
>>> */
>>> #if defined(WIN32) || defined(__CYGWIN__)
>>> #define PG_BINARY O_BINARY
>>> #define PG_BINARY_A "ab"
>>> #define PG_BINARY_R "rb"
>>> #define PG_BINARY_W "wb"
>>> #else
>>> #define PG_BINARY 0
>>> #define PG_BINARY_A "a"
>>> #define PG_BINARY_R "r"
>>> #define PG_BINARY_W "w"
>>> #endif
>> I don't see anything wrong with the patch, but I wonder if there's more
>> open() calls that need the same treatment? Like the one in
>> pg_resetxlog.c/ReadControlFile().
>
> Agreed, and I think that one would also need it - and pg_resetxlog
> already does this when it writes the file. A quick look doesn't show any
> other places, but I may have missed some?

I had a chat with Heikki about this, and the proper way to fix it.

Should there actually be any reason not to *always* open our files with
O_BINARY? That seems to be what should mimic what Unix does, which would
be what we expect, no?

If that is so, then I propose we do that for 8.4, and just backpatch the
O_BINARY flag to these two locations for 8.3 and 8.2. Thoughts?

//Magnus