Thread: pg_logging_init() can return ENOTTY with TAP tests
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
Attachment
Michael Paquier <michael@paquier.xyz> 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
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
Attachment
Michael Paquier <michael@paquier.xyz> 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
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