Re: Is this a bug in pg_current_logfile() on Windows?

[ redirecting to pghackers ]

Thomas Kellerer <shammat(at)gmx(dot)net> writes:
> Tom Lane schrieb am 08.07.2020 um 18:41:
>> Somehow, the reading file is being left in binary mode and thus it's
>> failing to convert \r\n back to plain \n.
>> Now the weird thing about that is I'd have expected "r" and "w" modes
>> to imply Windows text mode already, so that I'd have figured that
>> _setmode call to be a useless no-op. Apparently on some Windows libc
>> implementations, it's not. How was your installation built exactly?

> That's the build from EnterpriseDB

What I'd momentarily forgotten is that we don't use Windows' native
fopen(). On that platform, we use pgwin32_fopen which defaults to
binary mode (because _open_osfhandle does). So the _setmode calls in
syslogger.c are *not* no-ops, and the failure in pg_current_logfile()
is clearly explained by the fact that it's doing nothing to strip
carriage returns.

However ... I put in a test case to try to expose this failure, and
our Windows buildfarm critters remain perfectly happy. So what's up
with that? After some digging around, I believe the reason is that
PostgresNode::psql is stripping the \r from pg_current_logfile()'s
result, here:

$$stdout =~ s/\r//g if $TestLib::windows_os;

I'm slightly tempted to extend the test case by verifying on the
server side that the result ends in ".log" with no extra characters.
More generally, I wonder if the above behavior is really a good idea.
It seems to have been added in commit 33f3bbc6d as a hack to avoid
having to think too hard about mingw's behavior, but now I wonder if
it isn't masking other bugs too. At the very least I think we ought
to tighten the coding to

$$stdout =~ s/\r\n/\n/g if $TestLib::windows_os;

so that it won't strip carriage returns at random.

Meanwhile, back at the ranch, how shall we fix pg_current_logfile()?
I see two credible alternatives:

1. Insert
#ifdef WIN32
_setmode(_fileno(fd), _O_TEXT);
#endif
to make this function match the coding in syslogger.c.

2. Manually strip '\r' if present, independent of platform.

The second alternative would conform to the policy we established in
commit b654714f9, that newline-chomping code should uniformly drop \r.
However, that policy is mainly intended to allow non-Windows builds
to cope with text files that might have been made with a Windows text
editor. Surely we don't need to worry about a cross-platform source
for the log metafile. So I'm leaning a bit to the first alternative,
so as not to add useless overhead and complexity on non-Windows builds.

Thoughts?

regards, tom lane