pg_logging_init() can return ENOTTY with TAP tests

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

pg_logging_init() can return ENOTTY with TAP tests

Michael Paquier-2
Hi,

While hacking my way around a different patch (better option handling
for pg_test_fsync), I got surprised by the fact that if we run the TAP
tests, logging initialization could return with errno set to ENOTTY
because we call isatty() to check for output coloring.  I found that
this can be possible with Debian gid that uses a recent version of
IPC::Run.

Some system calls may not set errno even if they succeed, like
strtol() on Linux for example, so in this case it can cause option
handling to fail because of the error set by logging initialization.
Shouldn't we reset errno to 0 once logging initialization is done?  It
seems to me that this code path should finish with a clean sheet, and
attached is a proposal of patch to address this issue.

Thoughts?
--
Michael

logging-init-errno-v1.patch (437 bytes) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pg_logging_init() can return ENOTTY with TAP tests

Tom Lane-2
Michael Paquier <[hidden email]> writes:
> Some system calls may not set errno even if they succeed, like
> strtol() on Linux for example, so in this case it can cause option
> handling to fail because of the error set by logging initialization.
> Shouldn't we reset errno to 0 once logging initialization is done?  It
> seems to me that this code path should finish with a clean sheet, and
> attached is a proposal of patch to address this issue.

No, this is a bad idea.  The right place to fix this is whatever code
segment is relying on errno to be initially zero; that is NEVER a sane
assumption.  That is, a valid coding pattern is something like

             errno = 0;
             strtol(...);
             if (errno) ...

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: pg_logging_init() can return ENOTTY with TAP tests

Michael Paquier-2
On Fri, Sep 18, 2020 at 09:56:14AM -0400, Tom Lane wrote:
> No, this is a bad idea.  The right place to fix this is whatever code
> segment is relying on errno to be initially zero; that is NEVER a sane
> assumption.  That is, a valid coding pattern is something like

It seems to me that it could be a good idea to enforce a rule here
then.  There are problems in pg_resetwal and vacuumlo which use
strto[u]l() for option parsing without caring if errno is already set
or not, and we could have a ENOTTY while parsing options.  There is
more in the backend code, like assign_recovery_target_timeline() in
guc.c, bootstrap.c, just to notice a few ones.  We don't need to go
down to having our own wrapper for strtol() though.  There were lately
discussions about removing entirely our dependencies to strtol()
because we mostly rely on base 10 for the parsing and that's the part
impacting performance the most (it is possible to see a clear
difference for some workloads of COPY once you remove the base part).

So for now, what about looking at all those code paths and enforce
errno to 0?
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pg_logging_init() can return ENOTTY with TAP tests

Tom Lane-2
Michael Paquier <[hidden email]> writes:
> On Fri, Sep 18, 2020 at 09:56:14AM -0400, Tom Lane wrote:
>> No, this is a bad idea.  The right place to fix this is whatever code
>> segment is relying on errno to be initially zero; that is NEVER a sane
>> assumption.  That is, a valid coding pattern is something like

> So for now, what about looking at all those code paths and enforce
> errno to 0?

No, absolutely not.  That way leads to madness, because you will be trying
to enforce a system-wide property for the benefit of a few places.  There
is *no code anywhere* that promises to leave errno zero, but what you are
suggesting will soon lead to a situation where we have to require that of
everything.  It's not sane, it's not maintainable, and it's going to be
inefficient as heck, because it will require adding a whole lot more
"errno = 0" statements than the other way.

                        regards, tom lane



> --
> Michael


Reply | Threaded
Open this post in threaded view
|

Re: pg_logging_init() can return ENOTTY with TAP tests

Michael Paquier-2
On Fri, Sep 18, 2020 at 09:54:42PM -0400, Tom Lane wrote:
> No, absolutely not.  That way leads to madness, because you will be trying
> to enforce a system-wide property for the benefit of a few places.  There
> is *no code anywhere* that promises to leave errno zero, but what you are
> suggesting will soon lead to a situation where we have to require that of
> everything.  It's not sane, it's not maintainable, and it's going to be
> inefficient as heck, because it will require adding a whole lot more
> "errno = 0" statements than the other way.

Well, then what about cleaning up the state for the binaries where it
matters then?  FWIW, this example simply fails with an incorrect error
but it should succeed:
--- a/contrib/vacuumlo/t/001_basic.pl
+++ b/contrib/vacuumlo/t/001_basic.pl
@@ -2,8 +2,11 @@ use strict;
 use warnings;

 use TestLib;
-use Test::More tests => 8;
+use Test::More tests => 9;

 program_help_ok('vacuumlo');
 program_version_ok('vacuumlo');
 program_options_handling_ok('vacuumlo');
+
+command_ok([ 'vacuumlo', '-l', '1', 'postgres' ],
+       'vacuumlo: error: transaction limit must not be negative (0 disables)');

This makes me wonder if it could not be a real problem in some
environments.  vacuumlo is not that used, pg_resetwal though..
--
Michael

signature.asc (849 bytes) Download Attachment