Обсуждение: Re: pgsql: Allow concurrent-safe open() and fopen() in frontend code for Wi
So we seem to be out of the woods in terms of 0ba06e0bf breaking the regression tests, but I'm not very happy about the whole thing, because that patch wasn't supposed to change the behavior of open/fopen in any way other than allowing concurrent file access. Obviously, it did. After looking at src/port/open.c a bit, it seems like the problem is that our fopen() requires a nonstandard "t" option to select text mode. I'd always supposed that you got binary mode with "b" and text mode otherwise; is there some third possibility on Windows? Anyway, I'm inclined to propose that we ought to do something like the attached in HEAD and v11 in order to reduce the risk that there are other unexpected behavioral changes. This should also let us revert 0ba06e0bf's change in initdb.c. I wonder whether pgwin32_open() also ought to enforce that either O_BINARY or O_TEXT mode gets selected. regards, tom lane diff --git a/src/port/open.c b/src/port/open.c index a3ad946..85ab06e 100644 --- a/src/port/open.c +++ b/src/port/open.c @@ -154,7 +154,7 @@ pgwin32_fopen(const char *fileName, const char *mode) if (strchr(mode, 'b')) openmode |= O_BINARY; - if (strchr(mode, 't')) + else openmode |= O_TEXT; fd = pgwin32_open(fileName, openmode); diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c index b53d6eb..cb8c745 100644 --- a/src/bin/initdb/initdb.c +++ b/src/bin/initdb/initdb.c @@ -491,15 +491,7 @@ readfile(const char *path) char *buffer; int c; -#ifdef WIN32 - /* - * On Windows, we have to open the file in text mode so that carriage - * returns are stripped. - */ - if ((infile = fopen(path, "rt")) == NULL) -#else if ((infile = fopen(path, "r")) == NULL) -#endif { fprintf(stderr, _("%s: could not open file \"%s\" for reading: %s\n"), progname, path, strerror(errno));
On Mon, Sep 17, 2018 at 07:38:24PM -0400, Tom Lane wrote: > So we seem to be out of the woods in terms of 0ba06e0bf breaking the > regression tests, but I'm not very happy about the whole thing, because > that patch wasn't supposed to change the behavior of open/fopen in any > way other than allowing concurrent file access. Obviously, it did. Thanks! I can see that as well. > After looking at src/port/open.c a bit, it seems like the problem is > that our fopen() requires a nonstandard "t" option to select text mode. > I'd always supposed that you got binary mode with "b" and text mode > otherwise; is there some third possibility on Windows? There is a sort of automatic mode... Please see below. > Anyway, I'm inclined to propose that we ought to do something like > the attached in HEAD and v11 in order to reduce the risk that there > are other unexpected behavioral changes. This should also let us > revert 0ba06e0bf's change in initdb.c. > > I wonder whether pgwin32_open() also ought to enforce that either > O_BINARY or O_TEXT mode gets selected. There is no need to go down that road I think, a lot of code paths already call setmode to make sure that binary or text modes are used. > diff --git a/src/port/open.c b/src/port/open.c > index a3ad946..85ab06e 100644 > --- a/src/port/open.c > +++ b/src/port/open.c > @@ -154,7 +154,7 @@ pgwin32_fopen(const char *fileName, const char *mode) > > if (strchr(mode, 'b')) > openmode |= O_BINARY; > - if (strchr(mode, 't')) > + else > openmode |= O_TEXT; Hm. I don't think that this is correct either. The whole logic behind the mode selected depends on how setmode() is being set to. Please see here: https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/setmode?view=vs-2017 And the point is that if neither 'b' nor 't' are set, then open() would use the value set by the process, which is 't' by default if not set. And initdb does not set that, so it would use 't'. miscinit.c sets it to 'b', and your patch would likely cause some backend code to be broken. So it seems to me that if 'b' or 't' are not defined by the caller, then we ought to use what get_fmode() returns: https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/get-fmode?view=vs-2017 What I think I broke is that CreateFile ignores what _fmode uses, which has caused the breakage, while calling directly open() or fopen() does the work. There are also other things assuming that binary mode is used, you can grep for "setmode" and see how miscinit.c or pg_basebackup do the job. -- Michael
Вложения
On Tue, Sep 18, 2018 at 09:11:43AM +0900, Michael Paquier wrote: > What I think I broke is that CreateFile ignores what _fmode uses, which > has caused the breakage, while calling directly open() or fopen() does > the work. There are also other things assuming that binary mode is > used, you can grep for "setmode" and see how miscinit.c or pg_basebackup > do the job. I have been playing with this stuff, and hacked the attached. Now, while TAP tests of initdb and pgbench are happy (I can actually see the past failure as well), pg_dump complains at authentication time when using plain-text mode when using databases with all ASCII characters. That's not something I expected first, but _get_fmode also influences operations like pipe(), which is something that pg_dump uses, and setmode is enforced to binary mode only when adapted. I am getting to wonder if what's present on HEAD represents actually the best deal we could get. Attached is the patch I used for reference. Thoughts? -- Michael
Вложения
Michael Paquier <michael@paquier.xyz> writes: > On Tue, Sep 18, 2018 at 09:11:43AM +0900, Michael Paquier wrote: >> What I think I broke is that CreateFile ignores what _fmode uses, which >> has caused the breakage, while calling directly open() or fopen() does >> the work. There are also other things assuming that binary mode is >> used, you can grep for "setmode" and see how miscinit.c or pg_basebackup >> do the job. > I have been playing with this stuff, and hacked the attached. Now, while > TAP tests of initdb and pgbench are happy (I can actually see the past > failure as well), pg_dump complains at authentication time when using > plain-text mode when using databases with all ASCII characters. That's > not something I expected first, but _get_fmode also influences > operations like pipe(), which is something that pg_dump uses, and > setmode is enforced to binary mode only when adapted. > I am getting to wonder if what's present on HEAD represents actually the > best deal we could get. Attached is the patch I used for reference. > Thoughts? Well, we have to do something. I have a report from EDB's packagers that in 11beta4, "initdb --pwfile" is failing on Windows (ie, one can't connect afterwards using the specified password). It seems nearly certain to me that the reason is that the file is read with FILE *pwf = fopen(pwfilename, "r"); and so the \r isn't getting stripped from what's used as the password. So my opinion is that it's not negotiable that we have to restore the previous behavior in this realm. I don't want to be running around finding other cases one at a time, and I absolutely won't hold still for putting an "#ifdef WIN32" around each such case. (Even if we fixed all our own code, we'd still be breaking 3rd-party code.) What I don't understand yet is why your latest patch doesn't restore the previous behavior. What exactly is still different? In the meantime, we might be well advised to revert this patch in v11 and just continue to work on the problem in HEAD. I see now that this wasn't something to cram in during late beta ... regards, tom lane
Tom Lane wrote: > Well, we have to do something. I have a report from EDB's packagers > that in 11beta4, "initdb --pwfile" is failing on Windows (ie, one can't > connect afterwards using the specified password). It seems nearly > certain to me that the reason is that the file is read with > > FILE *pwf = fopen(pwfilename, "r"); > > and so the \r isn't getting stripped from what's used as the password. Perhaps there is something obvious that I'm missing, but it seems that all the problems we observe are caused by frontend code suddenly defaulting to binary mode when it was text mode before. Would it be an option to have pgwin32_open default to text mode in frontend code and to binary mode in backend code? Yours, Laurenz Albe
Laurenz Albe <laurenz.albe@cybertec.at> writes: > Would it be an option to have pgwin32_open default to text mode in > frontend code and to binary mode in backend code? Well, the question is why Michael's latest proposed patch doesn't accomplish that. regards, tom lane
Tom Lane wrote: > Laurenz Albe <laurenz.albe@cybertec.at> writes: > > Would it be an option to have pgwin32_open default to text mode in > > frontend code and to binary mode in backend code? > > Well, the question is why Michael's latest proposed patch doesn't > accomplish that. I was thinking of something trivial like this: --- a/src/port/open.c +++ b/src/port/open.c @@ -71,6 +71,12 @@ pgwin32_open(const char *fileName, int fileFlags,...) _O_SHORT_LIVED | O_DSYNC | O_DIRECT | (O_CREAT | O_TRUNC | O_EXCL) | (O_TEXT | O_BINARY))) == fileFlags); +#ifdef FRONTEND + /* default to text mode in frontend code */ + if (fileFlags & O_BINARY == 0) + fileFlags |= O_TEXT; +#endif + sa.nLength = sizeof(sa); sa.bInheritHandle = TRUE; sa.lpSecurityDescriptor = NULL; That wouldn't influence pipes, which was what Michael said was a problem for pg_dump. I currently have no Windows system close, so I cannot test... Yours, Laurenz Albe
On Tue, Sep 18, 2018 at 10:45:09AM -0400, Tom Lane wrote: > In the meantime, we might be well advised to revert this patch in > v11 and just continue to work on the problem in HEAD. I see now > that this wasn't something to cram in during late beta ... I can see that you have reverted the change just before I was able to reply. Thanks I was going to do the same. Also, if we cannot find a clear solution for HEAD rather soon, say by tomorrow, let's also remove it there as well and go ack to the blackboard. I still want to test a couple of other solutions first. -- Michael
Вложения
On Tue, Sep 18, 2018 at 06:04:58PM +0200, Laurenz Albe wrote: > That wouldn't influence pipes, which was what Michael said was a > problem for pg_dump. Yeah, the authentication blows up badly on that.. You can see all the tests using user and database names with all ASCII range turning red. > I currently have no Windows system close, so I cannot test... I have spent a large portion of my morning trying to test all the solutions proposed, and a winner shows up. Attached are three patches which present all the solutions mentioned, attached here for visibility: - win32-open-michael.patch, or the solution I was working on. This has led me actually nowhere. Even trying to enforce _fmode to happen only on the frontend has caused breakages of pg_dump for authentication. Trying to be smarter than what other binaries do is nice and consistent with the Windows experience I think, still it looks that this can lead to breakages when a utility uses setmode() for some of the output files. I noticed that pgwin32_open missed to enforce the mode used when calling _fdmode, still that solves nothing. - win32-open-tom.patch, which switches pgwin32_fopen() to use text mode all the time if binary is not specified. While this looked promising, I have noticed that this has been causing the same set of errors as what my previous patch has been doing in pg_dump TAP tests. Anyway, a solution needs also to happen for pgwin32_open() as we want direct calls to it to get the right call. - win32-open-laurenz.patch, which enforces to text mode only if binary mode is not defined, which maps strictly to what pre-11 is doing when calling the system _open or _fopen. And surprisingly, this is proving to pass all the tests I ran: bincheck (including pgbench and pg_dump), upgradecheck, recoverycheck, check, etc. initdb --pwfile is not complaining to me either. So the odds are that Laurenz's solution is what we are looking for. Laurenz, Tom, any thoughts to share? I won't back-patch that ;) -- Michael
Вложения
Michael Paquier <michael@paquier.xyz> writes: > I have spent a large portion of my morning trying to test all the > solutions proposed, and a winner shows up. ... > - win32-open-laurenz.patch, which enforces to text mode only if binary > mode is not defined, which maps strictly to what pre-11 is doing when > calling the system _open or _fopen. And surprisingly, this is proving > to pass all the tests I ran: bincheck (including pgbench and pg_dump), > upgradecheck, recoverycheck, check, etc. initdb --pwfile is not > complaining to me either. I'm OK with this approach. I wonder though what happens if you take away the "#ifdef FRONTEND" and just enforce that one or the other mode is selected always. That would seem like a sensible solution rather than a wart to me ... regards, tom lane
On Wed, Sep 19, 2018 at 11:55:01AM -0400, Tom Lane wrote: > I'm OK with this approach. I wonder though what happens if you take > away the "#ifdef FRONTEND" and just enforce that one or the other mode > is selected always. That would seem like a sensible solution rather > than a wart to me ... Thanks, I have pushed the solution from Laurenz to maintain pure compatibility. The origin of the failures reported by pg_dump actually comes from a mismatch with pg_hba.conf in the way users and/or databases are parsed. I am glad that we have tests for the full range of ASCII characters in TAP so as it is easy to detect regressions at early stages. We could try to manipulate more setmode calls like the one in miscinit.c so as authentication gets the right call. I am not sure of other side effects though... -- Michael
Вложения
Michael Paquier wrote: > Thanks, I have pushed the solution from Laurenz to maintain pure > compatibility. Thanks for all the work! Yours, Laurenz Albe