Re: [BUGS] [HACKERS] 0x1A in control file on Windows
Magnus Hagander wrote: Andrew Dunstan wrote: Tom Lane wrote: Bruce Momjian [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [BUGS] [HACKERS] 0x1A in control file on Windows
Magnus Hagander [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [BUGS] [HACKERS] 0x1A in control file on Windows
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [BUGS] [HACKERS] 0x1A in control file on Windows
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 0x1A in control file on Windows
Tom Lane wrote: ITAGAKI Takahiro [EMAIL PROTECTED] 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 [EMAIL PROTECTED]http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 0x1A in control file on Windows
Bruce Momjian [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 0x1A in control file on Windows
Tom Lane wrote: Bruce Momjian [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 0x1A in control file on Windows
Andrew Dunstan [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 0x1A in control file on Windows
Andrew Dunstan wrote: Tom Lane wrote: Bruce Momjian [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [BUGS] [HACKERS] 0x1A in control file on Windows
Magnus Hagander wrote: Heikki Linnakangas wrote: ITAGAKI Takahiro wrote: Tom Lane [EMAIL PROTECTED] wrote: ITAGAKI Takahiro [EMAIL PROTECTED] 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_BINARYO_BINARY #define PG_BINARY_A ab #define PG_BINARY_R rb #define PG_BINARY_W wb #else #define PG_BINARY0 #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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [BUGS] [HACKERS] 0x1A in control file on Windows
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [BUGS] [HACKERS] 0x1A in control file on Windows
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 0x1A in control file on Windows
Tom Lane [EMAIL PROTECTED] wrote: ITAGAKI Takahiro [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [BUGS] [HACKERS] 0x1A in control file on Windows
ITAGAKI Takahiro wrote: Tom Lane [EMAIL PROTECTED] wrote: ITAGAKI Takahiro [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [BUGS] [HACKERS] 0x1A in control file on Windows
Heikki Linnakangas wrote: ITAGAKI Takahiro wrote: Tom Lane [EMAIL PROTECTED] wrote: ITAGAKI Takahiro [EMAIL PROTECTED] 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_BINARYO_BINARY #define PG_BINARY_A ab #define PG_BINARY_R rb #define PG_BINARY_W wb #else #define PG_BINARY0 #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? /Magnus -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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_crc32crc;
- 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
odds
Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 0x1A in control file on Windows
ITAGAKI Takahiro [EMAIL PROTECTED] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
