BUG #16688: psql removes only LF without CR at end of backquotes on windows.

Started by PG Bug reporting formover 5 years ago5 messagesbugs
Jump to latest
#1PG Bug reporting form
noreply@postgresql.org

The following bug has been logged on the website:

Bug reference: 16688
Logged by: Kensuke Okamura
Email address: kensuke.okamura@kantei.co.jp
PostgreSQL version: 9.5.5
Operating system: Windows
Description:

psql removes only LF without CR at end of backquotes on windows.

example:
# \echo x`ECHO abc`x
return:
xabc\rx

#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: PG Bug reporting form (#1)
Re: BUG #16688: psql removes only LF without CR at end of backquotes on windows.

PG Bug reporting form <noreply@postgresql.org> writes:

psql removes only LF without CR at end of backquotes on windows.

Hmm, seems like b654714f9 missed this. psqlscanslash.l has

fd = popen(cmd, PG_BINARY_R);
...
/* strip any trailing newline */
if (cmd_output.len > 0 &&
cmd_output.data[cmd_output.len - 1] == '\n')
cmd_output.len--;

But rather than mess with that newline-chomping code, I'm
inclined to wonder why we're using PG_BINARY_R for input
that we clearly expect to be textual. Most of our other
popen's do not do that.

Bruce, this seems to date to 98e9775a3 ... don't suppose
you remember that? I see the point about control-Z in text
files, but I wonder how plausible that is for popen cases.

regards, tom lane

#3Bruce Momjian
bruce@momjian.us
In reply to: Tom Lane (#2)
Re: BUG #16688: psql removes only LF without CR at end of backquotes on windows.

On Tue, Oct 27, 2020 at 11:27:05PM -0400, Tom Lane wrote:

PG Bug reporting form <noreply@postgresql.org> writes:

psql removes only LF without CR at end of backquotes on windows.

Hmm, seems like b654714f9 missed this. psqlscanslash.l has

fd = popen(cmd, PG_BINARY_R);
...
/* strip any trailing newline */
if (cmd_output.len > 0 &&
cmd_output.data[cmd_output.len - 1] == '\n')
cmd_output.len--;

But rather than mess with that newline-chomping code, I'm
inclined to wonder why we're using PG_BINARY_R for input
that we clearly expect to be textual. Most of our other
popen's do not do that.

Bruce, this seems to date to 98e9775a3 ... don't suppose
you remember that? I see the point about control-Z in text
files, but I wonder how plausible that is for popen cases.

It seems safe for popen to use TEXT mode, and it might help with
encoding conversion. I don't think I made a popen distinction when I
was working in this area.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com

The usefulness of a cup is in its emptiness, Bruce Lee

#4Tom Lane
tgl@sss.pgh.pa.us
In reply to: Bruce Momjian (#3)
Re: BUG #16688: psql removes only LF without CR at end of backquotes on windows.

Bruce Momjian <bruce@momjian.us> writes:

On Tue, Oct 27, 2020 at 11:27:05PM -0400, Tom Lane wrote:

But rather than mess with that newline-chomping code, I'm
inclined to wonder why we're using PG_BINARY_R for input
that we clearly expect to be textual. Most of our other
popen's do not do that.

Bruce, this seems to date to 98e9775a3 ... don't suppose
you remember that? I see the point about control-Z in text
files, but I wonder how plausible that is for popen cases.

It seems safe for popen to use TEXT mode, and it might help with
encoding conversion. I don't think I made a popen distinction when I
was working in this area.

I double-checked, and verified that our only other popen() calls
that use binary mode are dealing with COPY data. It seems appropriate
to do so in that case, partly because the data might indeed be binary
and partly because we have adequate newline recognition logic in copy.c.
But it does seem best to uniformly use plain "r" or "w" for other
popen's. So I've fixed this that way.

regards, tom lane

#5Bruce Momjian
bruce@momjian.us
In reply to: Tom Lane (#4)
Re: BUG #16688: psql removes only LF without CR at end of backquotes on windows.

On Wed, Oct 28, 2020 at 02:39:39PM -0400, Tom Lane wrote:

Bruce Momjian <bruce@momjian.us> writes:

On Tue, Oct 27, 2020 at 11:27:05PM -0400, Tom Lane wrote:

But rather than mess with that newline-chomping code, I'm
inclined to wonder why we're using PG_BINARY_R for input
that we clearly expect to be textual. Most of our other
popen's do not do that.

Bruce, this seems to date to 98e9775a3 ... don't suppose
you remember that? I see the point about control-Z in text
files, but I wonder how plausible that is for popen cases.

It seems safe for popen to use TEXT mode, and it might help with
encoding conversion. I don't think I made a popen distinction when I
was working in this area.

I double-checked, and verified that our only other popen() calls
that use binary mode are dealing with COPY data. It seems appropriate
to do so in that case, partly because the data might indeed be binary
and partly because we have adequate newline recognition logic in copy.c.
But it does seem best to uniformly use plain "r" or "w" for other
popen's. So I've fixed this that way.

Thanks.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com

The usefulness of a cup is in its emptiness, Bruce Lee