Is this a bug in pg_current_logfile() on Windows?
Hello,
I noticed the following strage output when running Postgres 12.3 (not psql) on Windows
postgres=# select pg_current_logfile();
pg_current_logfile
------------------------------------
pg_log/postgresql-2020-07-08.log\r
(1 row)
Note the "\r" at the end of the file name.
This does not happen when running Postgres on Linux.
Is this intended for some strange reason?
Or a bug or a technical limitation?
Regards
Thomas
On 7/8/20 6:05 AM, Thomas Kellerer wrote:
Hello,
I noticed the following strage output when running Postgres 12.3 (not psql) on Windows
postgres=# select pg_current_logfile();
pg_current_logfile
------------------------------------
pg_log/postgresql-2020-07-08.log\r
(1 row)Note the "\r" at the end of the file name.
This does not happen when running Postgres on Linux.
Is this intended for some strange reason?
Or a bug or a technical limitation?
I'm guessing the difference between Unix line ending:
\n
and Windows:
\r\n
Regards
Thomas
--
Adrian Klaver
adrian.klaver@aklaver.com
On 7/8/20 6:45 AM, Adrian Klaver wrote:
On 7/8/20 6:05 AM, Thomas Kellerer wrote:
Hello,
I noticed the following strage output when running Postgres 12.3 (not
psql) on Windowspostgres=# select pg_current_logfile();
pg_current_logfile
------------------------------------
pg_log/postgresql-2020-07-08.log\r
(1 row)Note the "\r" at the end of the file name.
This does not happen when running Postgres on Linux.
Is this intended for some strange reason?
Or a bug or a technical limitation?I'm guessing the difference between Unix line ending:
\n
and Windows:
\r\n
From source(backend/utils/adt/misc.c):
nlpos = strchr(log_filepath, '\n');
if (nlpos == NULL)
{
/* Uh oh. No newline found, so file content is corrupted. */
elog(ERROR,
"missing newline character in \"%s\"",
LOG_METAINFO_DATAFILE);
break;
}
*nlpos = '\0';
if (logfmt == NULL || strcmp(logfmt, log_format) == 0)
{
FreeFile(fd);
PG_RETURN_TEXT_P(cstring_to_text(log_filepath));
}
Regards
Thomas
--
Adrian Klaver
adrian.klaver@aklaver.com
Thomas Kellerer <shammat@gmx.net> writes:
I noticed the following strage output when running Postgres 12.3 (not psql) on Windows
postgres=# select pg_current_logfile();
pg_current_logfile
------------------------------------
pg_log/postgresql-2020-07-08.log\r
(1 row)
Note the "\r" at the end of the file name.
Yeah, that seems like a bug. I think the reason is that syslogger.c
does this when writing the log metafile:
fh = fopen(LOG_METAINFO_DATAFILE_TMP, "w");
...
#ifdef WIN32
/* use CRLF line endings on Windows */
_setmode(_fileno(fh), _O_TEXT);
#endif
while misc.c only does this when reading the file:
fd = AllocateFile(LOG_METAINFO_DATAFILE, "r");
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?
regards, tom lane
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
[ redirecting to pghackers ]
Thomas Kellerer <shammat@gmx.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
On 7/8/20 5:26 PM, Tom Lane wrote:
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.
Seems reasonable. If we rip it out completely we'll have to find all the
places it breaks and fix them. And we'll almost certainly get new
breakage. If it's hiding a real bug we'll have to do that, but I'd be
reluctant unless there's actual proof.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
On 7/8/20 5:26 PM, Tom Lane wrote:
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.
Seems reasonable. If we rip it out completely we'll have to find all the
places it breaks and fix them. And we'll almost certainly get new
breakage. If it's hiding a real bug we'll have to do that, but I'd be
reluctant unless there's actual proof.
Hard to tell. What I propose to do right now is change the \r filters
as shown above, and see if the test I added in 004_logrotate.pl starts
to show failures on Windows. If it does, and no other place does,
I'm willing to be satisfied with that. If we see *other* failures then
that'd prove that the problem is real, no?
regards, tom lane
I wrote:
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
Seems reasonable. If we rip it out completely we'll have to find all the
places it breaks and fix them. And we'll almost certainly get new
breakage. If it's hiding a real bug we'll have to do that, but I'd be
reluctant unless there's actual proof.
Hard to tell. What I propose to do right now is change the \r filters
as shown above, and see if the test I added in 004_logrotate.pl starts
to show failures on Windows. If it does, and no other place does,
I'm willing to be satisfied with that. If we see *other* failures then
that'd prove that the problem is real, no?
So I did that, and the first report is from bowerbird and it's still
green. Unless I'm completely misinterpreting what's happening (always
a possibility), that means we're still managing to remove "data"
occurrences of \r.
The most likely theory about that, I think, is that IPC::Run::run already
translated any \r\n occurrences in the psql command's output to plain \n.
Then, the \r generated by pg_current_logfile() would butt up against the
line-ending \n, allowing the "fix" in sub psql to remove valid data.
One thing I noticed while making 91bdf499b is that some of these
substitutions are conditioned on "if $TestLib::windows_os" while others
are conditioned on "if $Config{osname} eq 'msys'". What is the reason
for this difference? Is it possible that we only really need to do it
in the latter case?
regards, tom lane
On 7/8/20 10:40 PM, Tom Lane wrote:
I wrote:
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
Seems reasonable. If we rip it out completely we'll have to find all the
places it breaks and fix them. And we'll almost certainly get new
breakage. If it's hiding a real bug we'll have to do that, but I'd be
reluctant unless there's actual proof.Hard to tell. What I propose to do right now is change the \r filters
as shown above, and see if the test I added in 004_logrotate.pl starts
to show failures on Windows. If it does, and no other place does,
I'm willing to be satisfied with that. If we see *other* failures then
that'd prove that the problem is real, no?So I did that, and the first report is from bowerbird and it's still
green. Unless I'm completely misinterpreting what's happening (always
a possibility), that means we're still managing to remove "data"
occurrences of \r.The most likely theory about that, I think, is that IPC::Run::run already
translated any \r\n occurrences in the psql command's output to plain \n.
Then, the \r generated by pg_current_logfile() would butt up against the
line-ending \n, allowing the "fix" in sub psql to remove valid data.
It's possible. I do see some mangling of that kind in IPC::Run's
Win32IO.pm and Win32Pump.pm.
Attached for reference is the IPC::Run package I usually use on Windows.
One thing I noticed while making 91bdf499b is that some of these
substitutions are conditioned on "if $TestLib::windows_os" while others
are conditioned on "if $Config{osname} eq 'msys'". What is the reason
for this difference? Is it possible that we only really need to do it
in the latter case?
In general I make the condition for such hacks as restrictive as
possible. I don't guarantee that I have been perfectly consistent about
that, though.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
IPC-Run-Win-0.94.tgzapplication/x-compressed-tar; name=IPC-Run-Win-0.94.tgzDownload
� H��[ �[{s�F������0LH�R���]K�-)�*���#Y��� bH"�\Y����=����d�m���2�l�����~MwO+�����'_�Y��|cC}>[�����
��������6:���m`Hl|Y���%��$�Y��!�}w�^��N������<��������/��Ng�����S��-?`{_bg$�g����uc����g��I������e��&-��,�B>�a��6�������(��Q;���k��������g�O:�X����w�`���+<�3�vFR@�]�@��:�Y�lG�����q;�d��Q�2$��K�#/�8�R/�����f���9����W*B���W�������`"�T�8��\�Rd �)�]��t ����P8�+��s#�#��c�����'r�S=�j*�$�|��z)����s�L����A�J���C���n���#��P�z@t��� ������C�)��]���|����<��@4|y#�D8��8���v���| ���x��+I���D����X���� ��b�$�#��Q����10�a���O������cM�fC������������wR���I�!�l[�CC��� +��Z�6���U��R^5�&W2N ��6�IJ\���.k�t���D���zW����.3iE����KH�\�H"9��
$�.0�;B0�� �c'�'�� �*�8�#f �$3��3v�,^�L[��G��>����f����~(m�b�
���= �L��Vl�`,��'0�j��)-Ir�%��� �������Q��e�<�7��T^0��G�?�8<��/ ��-b��������jm�\d��VWh�R^&����c��uD��C��n��4��n��E-,��IIF��K�u$�0q<k�����| L�v��D�j��� ���%� �:JX������bp���c��R�G/�O���A`�D4@�n����_�,��&��
y���p��y�mQ%QT7��?3/�c]�$�� EmoSp_�� O��-�]��k�� Li/���}+��p"�BwJRT���C�mk��T1�'����Q�����UxCHl�J>�#�����������:�����1)A:aW���/e����l��R�+�M������wv�6������N*��z2�.�c� *�G
�`@vG����n�q8 ��$�`�h!�JU5���,�����`�<<�@�x�����@��TT�R#�Fs����Bo�n��#ED�~�p���]����s�|�w|�ov�?��_���������J.��f�������R��<e�lu��(��N�V�*O��Y���O��k��K?���gF�3.��]���>Q�BP��`������]yJ���e���2i1;K�8��(6���0�#8��R,�N/��p��1�6g~�������N������ �M������?���}�����,�4��65�2��"Bv��V�����y]t�;m�f�L��j4��sG1���R����������C8����9z���)}�@�:��)=6p�l�'��@%���t6+,.�0N{����|m)�Mz�2��f>�z#pu[��/�O�M&S��AF �b�gkMs���]�{������4��m�!A��Eg���\)�#���4������pX�I�N#�Dp�G�\�>����,a�+)�5"W$��'� ��c�F�2�S���y��X�u����.!S�/b�D������y���"���C���$���Y�`f5�C]� ����BO�t��8�m�&�c{�`�������2�%����#��RA�l�� 2!� �yww����J]�[,�s�yJ>�,7�w�Vg������r��^�2������Y�X���~�1K*�o������E����Yzc�����Ji�:fU��@��D��K����kmF���k����6V�<�T�-�����Mh�ga�����d�
S�5���:\;d�"� �6@u�r����:��p_n ��� !�9�$+�L�TR,����J� ��"�MS������0��A�2����<C�V��9������Nr4Nf���a�!$�J���q��Dm��`�?�����1�����?(���s[�hyJ��s�Ey����V��N�0��#����������x��:��Z���9�q���(������nH�I�����k��Ac\�J��#����kEh�3wi�����������<h�����O#�y���W� ��������0�/�[[�(f��g�o���u�����_���I�)��N14kA��vg���x�<x���I�M���ou��4��M�I$vJ�XI7����,��z�~N�N�K'I5K�����fGB2�(��:1�7����,��n>�%��#hA<4O��|N��`���D��}N��`����M���#hm�@�,�B����.�R����T �~��G/I���N��K���t\�����(5���� �:\
�y��{�0��d��B/��m5�}��r���H���������g��V���>����U�/4T���o�� �0 �r����O�f�j�v��w��~��8/�ZvtvV�O�pW�ZEd��'w#�yUTs��x�]������B���3����
RMS�Q�5
j��,`I�]����n�����i��G1P~���o��\�)6#I{A:l�'?������.���68 ��`NyJ�I*��79�)o1���D�}U�4dv�����'����^o
e
����>pT����V��k��0�� ������4��G���*�#�D��T��t��0-�5���&�
]��94�$u� ��8 �M�q�w� }��N�9[3RN����Y�`H�aFf�ZE���+����G�S��LT�c�7V�-���h�p4�������c�7���v��i�8$w��vO�i5��5�;�R���z�MvX�Ra*����k�~�2P,~k���f �K�3�-���[��i����(-��L��J��k��u&�����5\���h�G�Q��V2������Y�n�����ElC��bh��������PGJ�H�>�����s�g���$��e!PY%��s�8MU}��s��te�y�s��Ry���'���L)]����${ ��i�F���Re� ����� ��?_�\ME�������o�_���?�{y���O�����<{�Y]__]��
���X{����k<������>�{biI/��8��_n�h/��x�u����O_�>:�8:o�.D�D�/�S�
�|�e��>@*)N�0�a<q�����^�����R�c��X�.c�Y���+���m��Q_�����Q�����=`�������W��S�FQ� ��W@��U�R��b��b)��\�!AU�Q�"��l �0,��]-���= ����Q$�*&�M���h��Z�)>��hJ��GQ��C���J����3���S,�Gx�*QUt.Cr��RKfX��o?���:j�Y���\�w)lT+,���n� �`J��������Lm��V�>
Mif����-�
�9��L�+5����x� �-�l�� 3�nqwC���l���Z�{�_�)�?���x�qGt�L���� 9����������v]vWm$���0���!��za�T!���'���h �Y
RG��$86�5�������0����A7��I4����;�!]QU��i���U[����/^�������=9��>��8~��H@_u�n�\������Q���
T<cD�G���p�$c�T������K�T��b��V
?F2���!�c#}��(vF�ne���/����������)���&��*��M��.�-�����c�����z��� ~]5�Y�:e�g���_&���� ~���Ou��ljEv�9�}�V=jm���Q���� �������.1����g�.h���T}�i���z>�\�T�.�H��r�������wx�����S�\���S��>x,.j|��p;�S�.���WH$�5|��U��MAWN�t������� ��]*��M[��z��'G��P?�X�������t�=SH]��C�om �H�,��zg���z����#n�K�!���z2`��lmq���1��G$���n@@QW�o�+�P,�����7S���e T��F����d�Z�P8[�GF�3��s
.3�����{p������_��1i�|M���E������������fm��V��j���[��c����������0�1#{}���:���R�
����+�?�:�`��\q�cfD�F�)�/_�^���s5����������5�)?��r�;�����3X�D�4g�C�e4���*�Dv�����j��'���w���*��Pu9�n�������!���T�=����t���+`�;��M��\m���n@�sN$WK��rMT}Qs���V��#
����z�j~w�Z`t����� ��EZKSkW^�x0{Rj)��8�=��?��>�����
���|�������}$�t�����EM��L���:�������VoN�P�����M�%���k����;�x�?�������zS
��<l.���B������"�p�1E8�Q���7(6�NG�h
��8��co0&��]� ���j�]���S@,��p�0�������<�y��� �w���6ND�"T� ;��Ekd9��D���T�
��W �>���;�`o0���y{�����gN)�9��4ou�<48�E��>eZ����O����W�g4���=�!-a��xj}���-��A�Os� R#�;^J���L�B���4����"�@��2Z�{���:�] ����?P��_@��l�?2�_����U��r)����4b\�����fo?�>� ��s�x�+A�Ph�~:�FK;X���Y���M\���9KE�m�2(�=�f,"AR7��e��
s(��bS �o�����:�b�q���,����<(t�cl�P�E�/���p���T���5hBs���VLj.����vOC�$����f�& g���h��6� �f��AE���(o��R{���l�;#�du�4�s'%|&��
�c������m?�^��!5:�3�+���Z�]�t�+�l�%jw36}o������]��9�`�8�,vu��������.X�{�4��c�} ,W�j^y�T^B��,[}��8�3�����L
g��Xu~r�bD�1�8�����hL��0o����j��������v��P9��<}��n�����
Ur�%����Y�]���B0]��Y��Q%���-�M�/���Dn&�f��S�D}�76sfuH����g�=�mq���XS/S�IQ)��U�����Bn�%��\�e�Z��Z�J��\H� �~q����)H]-/��Om)����$J������
K�-r�d�Mcz�e�v����l�W`�fY�i>��tL���5�����G�k+�'aU*�C�+�Fb�����Rw����Ya����T\�j����n<�'���bSFc���sI�h7�*��>����T��(j����{'���� F��r���hP|�������vqvyr���� �����>b�8����8��`]q�P���f�n?����f��WbQ9g�L'�V~ow��e�D w�V[�V�B-�����tP�W�E���2Y�4�
..:���f�|�����j�_���������0;�������v���iC��4�~�E�-�(��4�$���\���e%d4Q]D���|V��� ���o��a��m���
��?�P5���?��kCW�.����_Q�iKJ� '�^
�`s�����N!���%�Z%�6�o?�c�[U p����.���f��s\����
yeeu�P�
��:���]{|��Lp<��y U��h������&),F�i��c����(���C�l�a���xna��f�����~�T'o���m��8N�~��q���{o�d�^%��j[d
4�&��$�D��_������N4<��������QI_"��FI���=��-4�#r�@��5�j�������O��>��������'�oz-�y�������M�F�w{����<�@L���Yz�E�����e�������J�����=�Sx���v�d���|2#�n��3aJ���t�A�w����V���8�!���k�IY�y�O��7��6��LY��s:l/�=�Cm�[�c�B�V/��k���(��&�����~����.�����*l�9��+�M����>kV�`�)��0
�L��v�q���lL��'&���W����v�*i��#&��~����S��G����#*V;�d���y��N�$��4�z�P��
�N�~M�F�N�2\�l�w�E��8�d��0&2Cgi�-�� L�i�Sq� I�Rvq�q2]�9��3uVN�Q6��Ag�`6D���L�g��I?���|�Y���sl��Hb"'����W���������}r��[�&f�=Z���s�����z��z���{�0��t�*�W������Tc��I�F��
�Kf{� ��p,9�o��S�P<��7��n������p�F�#{'����F=���R:��F�Qoj����������i��-/_w�i�6�+���������k�u�/8j���4e��nG0�y?��|�O��|Z3�%-O�G��FE'P�]@mG�����"�
��s*�4�!�Z: �\M���I8��`�@��G\����4cG��<L�i�s5\���^���L�)��"l��K}�o�gK�����U�q��B���j�4������l �y�����,��x���xW�7���HO��p��@r����8�x�9u�G
���b]�e�s �ULc���j4�Y�H�y��kEM�8�~v_�^G�����[�������g��d&�7�����)y��]������9�[��+���(��>�*�Q����������u5*�{�[����p��F��V,���W�i9��|���}�,}����8-�����i�0��zg?D#F�O{�^vlYgZ9g�e�:"