From: Michael Paquier <michael@paquier.xyz>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Jorge Gustavo Rocha <jgr@geomaster.pt>
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15827: Unable to connect on Windows using pg_services.conf using Python psycopg2
Date: Fri, 28 Jun 2019 12:10:56 +0900
Message-ID: <20190628031056.GA1797@paquier.xyz> (raw)
In-Reply-To: <25605.1561652435@sss.pgh.pa.us>
References: <fcb5c34c-cf5f-6dc9-c68e-6b297f073b79@geomaster.pt>
<CA+mi_8aBnLKypX2Ttksg8JEhApBo=JpF6P_STQGp-FOvDpn6Yw@mail.gmail.com>
<81d9d421-6947-7111-a1dc-cfd99cd3ae53@geomaster.pt>
<23215.1560977098@sss.pgh.pa.us>
<20190620031218.GC2660@paquier.xyz>
<20190620070319.GH2660@paquier.xyz>
<0a9fcd30-5ae2-7e5f-ac1d-60d14a2ebb63@geomaster.pt>
<20190621004233.GA1626@paquier.xyz>
<3383.1561078113@sss.pgh.pa.us>
<25605.1561652435@sss.pgh.pa.us>
On Thu, Jun 27, 2019 at 12:20:35PM -0400, Tom Lane wrote:
> Patch 0001 attached responds to point (1), ie it uses isspace()
> tests to get rid of \r and \n and any trailing whitespace in
> parseServiceFile(). I think we should do this in HEAD, but there's
> perhaps an argument to be made that this is a behavior change and
> it'd be better to use Michael's patch in the back branches.
Yeah, I am not really convinced to add the filtering of lines with
only spaces on back-branches. Nobody has complained about that being
a problem either for years. No objections for HEAD.
> For point (2), I looked through all other fgets() callers in our code.
> Not all of them have newline-chomping logic, but I made all the ones
> that do have such do it the same way (except for those that use the
> isspace() method, which is fine). I'm not sure if this is fixing any
> live bugs --- most of these places are reading popen() output, and
> it's unclear to me whether we can rely on that to suppress \r on
> Windows. The Windows-specific code in pipe_read_line seems to think
> not (but if its test were dead code we wouldn't know it); yet if this
> were a hazard you'd think we'd have gotten complaints about at least
> one of these places. Still, I dislike code that has three or four
> randomly different ways of doing the exact same thing, especially when
> some of them are gratuitously inefficient.
InitPostmasterChild() initializes _setmode() to binary, which reacts
on popen() as well, so there is no magic conversion LF => CRLF like
what text does, so your patch looks good to me as we want to be able
to handle the case of external files written in text mode (like the
SSL passphrase case, good catch!).
The part for pg_resetwal does not seem necessary to me. The file gets
written by initdb in binary mode, so there should never be a CR,
right? Or is it worth worrying about custom tools writing the file,
which makes no actual sense normally?
> Note that I standardized on a loop that chomps trailing \r and \n
> indiscriminately, not the "if chomp \n then chomp \r" approach we
> had in some places. I think that approach does have a corner-case
> bug: if the input is long enough that the \r fits into the buffer
> but the \n doesn't, it'd fail to chomp the \r.
That would basically break a bunch of cases where \r is used in
strings, no, like passwords for single_prompt()? I really think that
we should stick with the approach of only removing \r when it is
followed by \n as we basically want to be able to counter the text
mode of Windows when something external wrote files read by our code,
where \n has been magically transformed to \r\n.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../20190628031056.GA1797@paquier.xyz/2-signature.asc)
download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: michael@paquier.xyz, tgl@sss.pgh.pa.us, jgr@geomaster.pt, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15827: Unable to connect on Windows using pg_services.conf using Python psycopg2
In-Reply-To: <20190628031056.GA1797@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox