0x1A in control file on Windows
I found a bug that pg_controldata ends with error if control files
contain 0x1A (Ctrl+Z) on Windows.
We probably need to add PG_BINARY when we open control files
because 0x1A is an end-of-file marker on Windows.
This fix needs to be applied in back versions (8.2, 8.3 and HEAD).
Index: src/bin/pg_controldata/pg_controldata.c
===================================================================
--- src/bin/pg_controldata/pg_controldata.c (head)
+++ src/bin/pg_controldata/pg_controldata.c (pg_control_0x1A)
@@ -107,7 +107,7 @@snprintf(ControlFilePath, MAXPGPATH, "%s/global/pg_control", DataDir);
- if ((fd = open(ControlFilePath, O_RDONLY, 0)) == -1)
+ if ((fd = open(ControlFilePath, O_RDONLY | PG_BINARY, 0)) == -1)
{
fprintf(stderr, _("%s: could not open file \"%s\" for reading: %s\n"),
progname, ControlFilePath, strerror(errno));
Index: src/bin/pg_resetxlog/pg_resetxlog.c
===================================================================
--- src/bin/pg_resetxlog/pg_resetxlog.c (head)
+++ src/bin/pg_resetxlog/pg_resetxlog.c (pg_control_0x1A)
@@ -373,7 +373,7 @@
char *buffer;
pg_crc32 crc;- if ((fd = open(XLOG_CONTROL_FILE, O_RDONLY, 0)) < 0)
+ if ((fd = open(XLOG_CONTROL_FILE, O_RDONLY | PG_BINARY, 0)) < 0)
{
/*
* If pg_control is not there at all, or we can't read it, the oddsRegards,
---
ITAGAKI Takahiro
NTT Open Source Software Center
ITAGAKI Takahiro <itagaki.takahiro@oss.ntt.co.jp> writes:
I found a bug that pg_controldata ends with error if control files
contain 0x1A (Ctrl+Z) on Windows.
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?
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> wrote:
ITAGAKI Takahiro <itagaki.takahiro@oss.ntt.co.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
Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center
ITAGAKI Takahiro wrote:
Tom Lane <tgl@sss.pgh.pa.us> wrote:
ITAGAKI Takahiro <itagaki.takahiro@oss.ntt.co.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().
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
Heikki Linnakangas wrote:
ITAGAKI Takahiro wrote:
Tom Lane <tgl@sss.pgh.pa.us> wrote:
ITAGAKI Takahiro <itagaki.takahiro@oss.ntt.co.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"
#endifI 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?
/Magnus
Magnus Hagander wrote:
Heikki Linnakangas wrote:
ITAGAKI Takahiro wrote:
Tom Lane <tgl@sss.pgh.pa.us> wrote:
ITAGAKI Takahiro <itagaki.takahiro@oss.ntt.co.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"
#endifI 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
Magnus Hagander wrote:
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?
ISTR there are a few places where we want CRLF translation (config files?)
I'd be fairly conservative about making changes like this.
cheers
andrew
Andrew Dunstan wrote:
Magnus Hagander wrote:
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?ISTR there are a few places where we want CRLF translation (config files?)
These seem to be using fopen() (through AllocateFile()), which wouldn't
be affected by this.
I'd be fairly conservative about making changes like this.
This is why my proposal would be not to backpatch such a change, but to
only do it for 8.4.
//Magnus
Tom Lane wrote:
ITAGAKI Takahiro <itagaki.takahiro@oss.ntt.co.jp> writes:
I found a bug that pg_controldata ends with error if control files
contain 0x1A (Ctrl+Z) on Windows.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?
The problem is that our pg_controldata might have binary values that
contain 0x1a that will be confused by the operating system as
end-of-file.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Bruce Momjian <bruce@momjian.us> writes:
Tom Lane wrote:
Well, why is that a bug? If the platform is so silly as to define text
files that way, who are we to argue?
The problem is that our pg_controldata might have binary values that
contain 0x1a that will be confused by the operating system as
end-of-file.
pg_controldata is certainly already being read as binary. The
discussion here is about *text* files, particularly configuration
files. Why should we not adhere to the platform standard about
what a text file is?
If you need a positive reason why this might be a bad idea, consider the
idea that someone is examining postgresql.conf with a text editor that
stops reading at control-Z. He might not be able to see items that the
postmaster is treating as valid.
regards, tom lane
Tom Lane wrote:
Bruce Momjian <bruce@momjian.us> writes:
Tom Lane wrote:
Well, why is that a bug? If the platform is so silly as to define text
files that way, who are we to argue?The problem is that our pg_controldata might have binary values that
contain 0x1a that will be confused by the operating system as
end-of-file.pg_controldata is certainly already being read as binary.
Umm, no, it is in the backend I believe but not in the utilities. Hence
the original bug report. We need to add the binary flag in
pg_controldata.c and pg_resetxlog.c.
The
discussion here is about *text* files, particularly configuration
files. Why should we not adhere to the platform standard about
what a text file is?If you need a positive reason why this might be a bad idea, consider the
idea that someone is examining postgresql.conf with a text editor that
stops reading at control-Z. He might not be able to see items that the
postmaster is treating as valid.
Yes, exactly right. We certainly can't just open everything in binary
mode. Magnus did say that all the current config files are opened in
text mode as far as he could see.
cheers
andrew
Andrew Dunstan <andrew@dunslane.net> writes:
Tom Lane wrote:
pg_controldata is certainly already being read as binary.
Umm, no, it is in the backend I believe but not in the utilities. Hence
the original bug report. We need to add the binary flag in
pg_controldata.c and pg_resetxlog.c.
Ah, okay, that's surely a bug. But I think the discussion here was
about adding PG_BINARY to *all* open requests. That I don't like.
regards, tom lane
Andrew Dunstan wrote:
Tom Lane wrote:
Bruce Momjian <bruce@momjian.us> writes:
Tom Lane wrote:
Well, why is that a bug? If the platform is so silly as to define text
files that way, who are we to argue?The problem is that our pg_controldata might have binary values that
contain 0x1a that will be confused by the operating system as
end-of-file.pg_controldata is certainly already being read as binary.
Umm, no, it is in the backend I believe but not in the utilities. Hence
the original bug report. We need to add the binary flag in
pg_controldata.c and pg_resetxlog.c.
Right.
I'll go ahead and put that part in (I find two locations - the one in
the original patch, and the extra one Heikki noticed).
The
discussion here is about *text* files, particularly configuration
files. Why should we not adhere to the platform standard about
what a text file is?If you need a positive reason why this might be a bad idea, consider the
idea that someone is examining postgresql.conf with a text editor that
stops reading at control-Z. He might not be able to see items that the
postmaster is treating as valid.Yes, exactly right. We certainly can't just open everything in binary
mode. Magnus did say that all the current config files are opened in
text mode as far as he could see.
The point being that the config files are opened with AllocateFile(),
which in turn calls fopen(). It doesn't use open(). The proposal was
only to make all *open()* calls do it binary. I was under the impression
that on Unix, that's what open() did, so we should behave the same?
//Magnus
Magnus Hagander wrote:
Andrew Dunstan wrote:
Tom Lane wrote:
Bruce Momjian <bruce@momjian.us> writes:
Tom Lane wrote:
Well, why is that a bug? If the platform is so silly as to define text
files that way, who are we to argue?The problem is that our pg_controldata might have binary values that
contain 0x1a that will be confused by the operating system as
end-of-file.pg_controldata is certainly already being read as binary.
Umm, no, it is in the backend I believe but not in the utilities. Hence
the original bug report. We need to add the binary flag in
pg_controldata.c and pg_resetxlog.c.Right.
I'll go ahead and put that part in (I find two locations - the one in
the original patch, and the extra one Heikki noticed).
Eh, both were in the original patch, I just didn't scroll far enough :-)
Applied to HEAD and backpatched back to 8.2 - since it only affects
Windows, we don't go further.
//Magnus
Magnus Hagander <magnus@hagander.net> writes:
Andrew Dunstan wrote:
Tom Lane wrote:
If you need a positive reason why this might be a bad idea, consider the
idea that someone is examining postgresql.conf with a text editor that
stops reading at control-Z. He might not be able to see items that the
postmaster is treating as valid.Yes, exactly right. We certainly can't just open everything in binary
mode. Magnus did say that all the current config files are opened in
text mode as far as he could see.
The point being that the config files are opened with AllocateFile(),
which in turn calls fopen(). It doesn't use open(). The proposal was
only to make all *open()* calls do it binary. I was under the impression
that on Unix, that's what open() did, so we should behave the same?
That seems just weird. I do not think there's any correlation between
whether we use open or fopen and whether the file is text or binary.
Even if it happens to be true right now, depending on it would be
fragile.
regards, tom lane
Tom Lane wrote:
The point being that the config files are opened with AllocateFile(),
which in turn calls fopen(). It doesn't use open(). The proposal was
only to make all *open()* calls do it binary. I was under the impression
that on Unix, that's what open() did, so we should behave the same?That seems just weird. I do not think there's any correlation between
whether we use open or fopen and whether the file is text or binary.
Even if it happens to be true right now, depending on it would be
fragile.
I agree. If you really want something like that you should invent
OpenConfigFile() or some such. But it hardly seems worth it.
cheers
andrew
Andrew Dunstan wrote:
Tom Lane wrote:
The point being that the config files are opened with AllocateFile(),
which in turn calls fopen(). It doesn't use open(). The proposal was
only to make all *open()* calls do it binary. I was under the impression
that on Unix, that's what open() did, so we should behave the same?That seems just weird. I do not think there's any correlation between
whether we use open or fopen and whether the file is text or binary.
Even if it happens to be true right now, depending on it would be
fragile.I agree. If you really want something like that you should invent
OpenConfigFile() or some such. But it hardly seems worth it.
Well, the AllocateFile() API already has the ability to specify if it's
a text file or not (using the fopen syntax).
And given that we have a way to specify it for open (PG_BINARY), we can
just leave it that way I guess. Even if we end up always specifying
PG_BINARY, that may keep things less fragile. As long as we remember to
specify it. (PG_BINARY is a no-op on all other platforms than Windows,
btw, because there is no way to specify anything other than binary mode
for open() on Unix)
//Magnus