Обсуждение: Make all Perl warnings fatal

We have a lot of Perl scripts in the tree, mostly code generation and 
TAP tests.  Occasionally, these scripts produce warnings.  These are 
AFAICT always mistakes on the developer side (true positives).  Typical 
examples are warnings from genbki.pl or related when you make a mess in 
the catalog files during development, or warnings from tests when they 
massage a config file that looks different on different hosts, or 
mistakes during merges (e.g., duplicate subroutine definitions), or just 
mistakes that weren't noticed, because, you know, there is a lot of 
output in a verbose build.

I wanted to figure put if we can catch these more reliably, in the style 
of -Werror.  AFAICT, there is no way to automatically turn all warnings 
into fatal errors.  But there is a way to do it per script, by replacing

     use warnings;

by

     use warnings FATAL => 'all';

See attached patch to try it out.

The documentation at <https://perldoc.perl.org/warnings#Fatal-Warnings> 
appears to sort of hand-wave against doing that.  Their argument appears 
to be something like, the modules you use might in the future produce 
additional warnings, thus breaking your scripts.  On balance, I'd take 
that risk, if it means I would notice the warnings in a more timely and 
robust way.  But that's just me at a moment in time.

Thoughts?


Now to some funny business.  If you apply this patch, the Cirrus CI 
Linux tasks will fail, because they get an undefined return from 
getprotobyname() in PostgreSQL/Test/Cluster.pm.  Huh?  I need this patch 
to get past that:

diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm 
b/src/test/perl/PostgreSQL/Test/Cluster.pm
index 3fa679ff97..dfe7bc7b1a 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -1570,7 +1570,7 @@ sub can_bind
     my ($host, $port) = @_;
     my $iaddr = inet_aton($host);
     my $paddr = sockaddr_in($port, $iaddr);
-   my $proto = getprotobyname("tcp");
+   my $proto = getprotobyname("tcp") || 6;

     socket(SOCK, PF_INET, SOCK_STREAM, $proto)
       or die "socket failed: $!";

What is going on there?  Does this host not have /etc/protocols, or 
something like that?


There are also a couple of issues in the MSVC legacy build system that 
would need to be tightened up in order to survive with fatal Perl 
warnings.  Obviously, there is a question whether it's worth spending 
any time on that anymore.

To avoid a complete bloodbath on cfbot, here is an updated patch set 
that includes a workaround for the getprotobyname() issue mentioned below.


On 10.08.23 07:58, Peter Eisentraut wrote:
> We have a lot of Perl scripts in the tree, mostly code generation and 
> TAP tests.  Occasionally, these scripts produce warnings.  These are 
> AFAICT always mistakes on the developer side (true positives).  Typical 
> examples are warnings from genbki.pl or related when you make a mess in 
> the catalog files during development, or warnings from tests when they 
> massage a config file that looks different on different hosts, or 
> mistakes during merges (e.g., duplicate subroutine definitions), or just 
> mistakes that weren't noticed, because, you know, there is a lot of 
> output in a verbose build.
> 
> I wanted to figure put if we can catch these more reliably, in the style 
> of -Werror.  AFAICT, there is no way to automatically turn all warnings 
> into fatal errors.  But there is a way to do it per script, by replacing
> 
>      use warnings;
> 
> by
> 
>      use warnings FATAL => 'all';
> 
> See attached patch to try it out.
> 
> The documentation at <https://perldoc.perl.org/warnings#Fatal-Warnings> 
> appears to sort of hand-wave against doing that.  Their argument appears 
> to be something like, the modules you use might in the future produce 
> additional warnings, thus breaking your scripts.  On balance, I'd take 
> that risk, if it means I would notice the warnings in a more timely and 
> robust way.  But that's just me at a moment in time.
> 
> Thoughts?
> 
> 
> Now to some funny business.  If you apply this patch, the Cirrus CI 
> Linux tasks will fail, because they get an undefined return from 
> getprotobyname() in PostgreSQL/Test/Cluster.pm.  Huh?  I need this patch 
> to get past that:
> 
> diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm 
> b/src/test/perl/PostgreSQL/Test/Cluster.pm
> index 3fa679ff97..dfe7bc7b1a 100644
> --- a/src/test/perl/PostgreSQL/Test/Cluster.pm
> +++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
> @@ -1570,7 +1570,7 @@ sub can_bind
>      my ($host, $port) = @_;
>      my $iaddr = inet_aton($host);
>      my $paddr = sockaddr_in($port, $iaddr);
> -   my $proto = getprotobyname("tcp");
> +   my $proto = getprotobyname("tcp") || 6;
> 
>      socket(SOCK, PF_INET, SOCK_STREAM, $proto)
>        or die "socket failed: $!";
> 
> What is going on there?  Does this host not have /etc/protocols, or 
> something like that?
> 
> 
> There are also a couple of issues in the MSVC legacy build system that 
> would need to be tightened up in order to survive with fatal Perl 
> warnings.  Obviously, there is a question whether it's worth spending 
> any time on that anymore.

To avoid a complete bloodbath on cfbot, here is an updated patch set that includes a workaround for the getprotobyname() issue mentioned below.


On 10.08.23 07:58, Peter Eisentraut wrote:

We have a lot of Perl scripts in the tree, mostly code generation and TAP tests.  Occasionally, these scripts produce warnings.  These are AFAICT always mistakes on the developer side (true positives).  Typical examples are warnings from genbki.pl or related when you make a mess in the catalog files during development, or warnings from tests when they massage a config file that looks different on different hosts, or mistakes during merges (e.g., duplicate subroutine definitions), or just mistakes that weren't noticed, because, you know, there is a lot of output in a verbose build.

I wanted to figure put if we can catch these more reliably, in the style of -Werror.  AFAICT, there is no way to automatically turn all warnings into fatal errors.  But there is a way to do it per script, by replacing

     use warnings;

by

     use warnings FATAL => 'all';

See attached patch to try it out.

The documentation at <https://perldoc.perl.org/warnings#Fatal-Warnings> appears to sort of hand-wave against doing that.  Their argument appears to be something like, the modules you use might in the future produce additional warnings, thus breaking your scripts.  On balance, I'd take that risk, if it means I would notice the warnings in a more timely and robust way.  But that's just me at a moment in time.

Thoughts?

It's not really the same as -Werror, because many warnings can be generated at runtime rather than compile-time.

Still, I guess that might not matter too much since apart from plperl we only use perl for building / testing.

Regarding the dangers mentioned, I guess we can undo it if it proves a nuisance.

+1 to getting rid if the unnecessary call to getprotobyname().

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Mon, Aug 21, 2023 at 11:51:24AM -0400, Andrew Dunstan wrote:
> It's not really the same as -Werror, because many warnings can be generated
> at runtime rather than compile-time.
>
> Still, I guess that might not matter too much since apart from plperl we
> only use perl for building / testing.

However, is it possible to trust the out-of-core perl modules posted
on CPAN, assuming that these will never produce warnings?  I've never
seen any issues with IPC::Run in these last years, so perhaps that's
OK in the long-run.

> Regarding the dangers mentioned, I guess we can undo it if it proves a
> nuisance.

Yeah.  I am wondering what the buildfarm would say with this change.

> +1 to getting rid if the unnecessary call to getprotobyname().

Looking around here..
https://perldoc.perl.org/perlipc#Sockets%3A-Client%2FServer-Communication

Hmm.  Are you sure that this is OK even in the case where the TAP
tests run on Windows without unix-domain socket support?  The CI runs
on Windows, but always with unix domain sockets around as far as I
know.
--
Michael

On Mon, Aug 21, 2023 at 11:51:24AM -0400, Andrew Dunstan wrote:

It's not really the same as -Werror, because many warnings can be generated
at runtime rather than compile-time.

Still, I guess that might not matter too much since apart from plperl we
only use perl for building / testing.

However, is it possible to trust the out-of-core perl modules posted
on CPAN, assuming that these will never produce warnings?  I've never
seen any issues with IPC::Run in these last years, so perhaps that's
OK in the long-run.

If we do find any such issues then warnings can be turned off locally. We already do that in several places.

Regarding the dangers mentioned, I guess we can undo it if it proves a
nuisance.

Yeah.  I am wondering what the buildfarm would say with this change.

+1 to getting rid if the unnecessary call to getprotobyname().

Looking around here..
https://perldoc.perl.org/perlipc#Sockets%3A-Client%2FServer-Communication

Hmm.  Are you sure that this is OK even in the case where the TAP
tests run on Windows without unix-domain socket support?  The CI runs
on Windows, but always with unix domain sockets around as far as I
know.

The socket call in question is for a PF_INET socket, so this has nothing at all to do with unix domain sockets. See the man page for socket() (2) for an explanation of why 0 is ok in this case. (There's only one protocol that matches the rest of the parameters).

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 2023-Aug-10, Peter Eisentraut wrote:

> I wanted to figure put if we can catch these more reliably, in the style of
> -Werror.  AFAICT, there is no way to automatically turn all warnings into
> fatal errors.  But there is a way to do it per script, by replacing
> 
>     use warnings;
> 
> by
> 
>     use warnings FATAL => 'all';
> 
> See attached patch to try it out.

BTW in case we do find that there's some unforeseen problem and we want
to roll back, it would be great to have a way to disable this without
having to edit every single Perl file again later.  However, I didn't
find a way to do it -- I thought about creating a separate PgWarnings.pm
file that would do the "use warnings FATAL => 'all'" dance and which
every other Perl file would use or include; but couldn't make it work.
Maybe some Perl expert knows a good answer to this.

Maybe the BEGIN block of each file can `eval` a new PgWarnings.pm that
emits the "use warnings" line?

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"No renuncies a nada. No te aferres a nada."

On 2023-Aug-10, Peter Eisentraut wrote:

I wanted to figure put if we can catch these more reliably, in the style of
-Werror.  AFAICT, there is no way to automatically turn all warnings into
fatal errors.  But there is a way to do it per script, by replacing
    use warnings;

by
    use warnings FATAL => 'all';

See attached patch to try it out.

BTW in case we do find that there's some unforeseen problem and we want
to roll back, it would be great to have a way to disable this without
having to edit every single Perl file again later.  However, I didn't
find a way to do it -- I thought about creating a separate PgWarnings.pm
file that would do the "use warnings FATAL => 'all'" dance and which
every other Perl file would use or include; but couldn't make it work.
Maybe some Perl expert knows a good answer to this.

Maybe the BEGIN block of each file can `eval` a new PgWarnings.pm that
emits the "use warnings" line?

Once we try it, I doubt we would want to revoke it globally, and if we did I'd rather not be left with a wart like this. As I mentioned upthread, it is possible to override the setting locally. The manual page for the warnings pragma contains details.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 21.08.23 17:51, Andrew Dunstan wrote:
> Still, I guess that might not matter too much since apart from plperl we 
> only use perl for building / testing.
> 
> Regarding the dangers mentioned, I guess we can undo it if it proves a 
> nuisance.
> 
> +1 to getting rid if the unnecessary call to getprotobyname().

I have committed the latter part.

The rest would still depend on some fixes for the MSVC build system, so 
I'll hold that until we decide what to do with that.

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

> On 2023-Aug-10, Peter Eisentraut wrote:
>
>> I wanted to figure put if we can catch these more reliably, in the style of
>> -Werror.  AFAICT, there is no way to automatically turn all warnings into
>> fatal errors.  But there is a way to do it per script, by replacing
>> 
>>     use warnings;
>> 
>> by
>> 
>>     use warnings FATAL => 'all';
>> 
>> See attached patch to try it out.
>
> BTW in case we do find that there's some unforeseen problem and we want
> to roll back, it would be great to have a way to disable this without
> having to edit every single Perl file again later.  However, I didn't
> find a way to do it -- I thought about creating a separate PgWarnings.pm
> file that would do the "use warnings FATAL => 'all'" dance and which
> every other Perl file would use or include; but couldn't make it work.
> Maybe some Perl expert knows a good answer to this.

Like most pragmas (all-lower-case module names), `warnings` affects the
currently-compiling lexical scope, so to have a module like PgWarnings
inject it into the module that uses it, you'd call warnings->import in
its import method (which gets called when the `use PgWarnings;``
statement is compiled, e.g.:

package PgWarnings;

sub import {
    warnings->import(FATAL => 'all');
}

I wouldn't bother with a whole module just for that, but if we have a
group of pragmas or modules we always want to enable/import and have the
ability to change this set without having to edit all the files, it's
quite common to have a ProjectName::Policy module that does that.  For
example, to exclude warnings that are unsafe, pointless, or impossible
to fatalise (c.f. https://metacpan.org/pod/strictures#CATEGORY-SELECTIONS):

package PostgreSQL::Policy;

sub import {
    strict->import;
    warnings->import(
        FATAL => 'all',
        NONFATAL => qw(exec internal malloc recursion),
    );
    warnings->uniport(qw(once));
}

Now that we require Perl 5.14, we might want to consider enabling its
feature bundle as well, with:

    feature->import(':5.14')

Although the only features of note that adds are:

   - say: the `say` function, like `print` but appends a newline

   - state: `state` variables, like `my` but only initialised the first
     time the function they're in is called, and the value persists
     between calls (like function-scoped `static` variables in C)

   - unicode_strings: use unicode semantics for characters in the
     128-255 range, regardless of internal representation

> Maybe the BEGIN block of each file can `eval` a new PgWarnings.pm that
> emits the "use warnings" line?

That's ugly as sin, and thankfully not necessary.

-ilmari

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

On 2023-Aug-10, Peter Eisentraut wrote:

I wanted to figure put if we can catch these more reliably, in the style of
-Werror.  AFAICT, there is no way to automatically turn all warnings into
fatal errors.  But there is a way to do it per script, by replacing
    use warnings;

by
    use warnings FATAL => 'all';

See attached patch to try it out.

BTW in case we do find that there's some unforeseen problem and we want
to roll back, it would be great to have a way to disable this without
having to edit every single Perl file again later.  However, I didn't
find a way to do it -- I thought about creating a separate PgWarnings.pm
file that would do the "use warnings FATAL => 'all'" dance and which
every other Perl file would use or include; but couldn't make it work.
Maybe some Perl expert knows a good answer to this.

Like most pragmas (all-lower-case module names), `warnings` affects the
currently-compiling lexical scope, so to have a module like PgWarnings
inject it into the module that uses it, you'd call warnings->import in
its import method (which gets called when the `use PgWarnings;``
statement is compiled, e.g.:

package PgWarnings;

sub import {	warnings->import(FATAL => 'all');
}

I wouldn't bother with a whole module just for that, but if we have a
group of pragmas or modules we always want to enable/import and have the
ability to change this set without having to edit all the files, it's
quite common to have a ProjectName::Policy module that does that.  For
example, to exclude warnings that are unsafe, pointless, or impossible
to fatalise (c.f. https://metacpan.org/pod/strictures#CATEGORY-SELECTIONS):

package PostgreSQL::Policy;

sub import {	strict->import;	warnings->import(		FATAL => 'all',		NONFATAL => qw(exec internal malloc recursion),	);	warnings->uniport(qw(once));
}

Now that we require Perl 5.14, we might want to consider enabling its
feature bundle as well, with:
	feature->import(':5.14')

Although the only features of note that adds are:
   - say: the `say` function, like `print` but appends a newline
   - state: `state` variables, like `my` but only initialised the first     time the function they're in is called, and the value persists     between calls (like function-scoped `static` variables in C)
   - unicode_strings: use unicode semantics for characters in the     128-255 range, regardless of internal representation

We'd probably have to modify the perlcritic rules to account for it. See <https://metacpan.org/pod/Perl::Critic::Policy::TestingAndDebugging::RequireUseStrict> and similarly for RequireUseWarnings. In any case, it seems a bit like overkill.

Maybe the BEGIN block of each file can `eval` a new PgWarnings.pm that
emits the "use warnings" line?

That's ugly as sin, and thankfully not necessary.

Agreed.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 10.08.23 07:58, Peter Eisentraut wrote:
> There are also a couple of issues in the MSVC legacy build system that 
> would need to be tightened up in order to survive with fatal Perl 
> warnings.  Obviously, there is a question whether it's worth spending 
> any time on that anymore.

It looks like there are no principled objections to the overall idea 
here, but given this dependency on the MSVC build system removal, I'm 
going to set this patch to Returned with feedback for now.

On 12.09.23 07:42, Peter Eisentraut wrote:
> On 10.08.23 07:58, Peter Eisentraut wrote:
>> There are also a couple of issues in the MSVC legacy build system that 
>> would need to be tightened up in order to survive with fatal Perl 
>> warnings.  Obviously, there is a question whether it's worth spending 
>> any time on that anymore.
> 
> It looks like there are no principled objections to the overall idea 
> here, but given this dependency on the MSVC build system removal, I'm 
> going to set this patch to Returned with feedback for now.

Now that that is done, here is an updated patch for this.

I found one more bug in the Perl code because of this, a fix for which 
is included here.

With this fix, this passes all the CI tests on all platforms.

On 22.12.23 22:33, Peter Eisentraut wrote:
> On 12.09.23 07:42, Peter Eisentraut wrote:
>> On 10.08.23 07:58, Peter Eisentraut wrote:
>>> There are also a couple of issues in the MSVC legacy build system 
>>> that would need to be tightened up in order to survive with fatal 
>>> Perl warnings.  Obviously, there is a question whether it's worth 
>>> spending any time on that anymore.
>>
>> It looks like there are no principled objections to the overall idea 
>> here, but given this dependency on the MSVC build system removal, I'm 
>> going to set this patch to Returned with feedback for now.
> 
> Now that that is done, here is an updated patch for this.
> 
> I found one more bug in the Perl code because of this, a fix for which 
> is included here.
> 
> With this fix, this passes all the CI tests on all platforms.

committed

On Sat, Dec 30, 2023 at 12:57 AM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> committed

With the commit c5385929 converting perl warnings to FATAL, use of
psql/safe_psql with timeout parameters [1] fail with the following
error:

Use of uninitialized value $ret in bitwise and (&) at
/home/ubuntu/postgres/src/test/recovery/../../../src/test/perl/PostgreSQL/Test/Cluster.pm
line 2015.

Perhaps assigning a default error code to $ret instead of undef in
PostgreSQL::Test::Cluster - psql() function is the solution.

[1]
use strict;
use warnings FATAL => 'all';
use PostgreSQL::Test::Cluster;
use PostgreSQL::Test::Utils;
use Test::More;

my $node = PostgreSQL::Test::Cluster->new('test');
$node->init;
$node->start;

my ($stdout, $stderr, $timed_out);
my $cmdret = $node->psql('postgres', q[SELECT pg_sleep(600)],
          stdout => \$stdout, stderr => \$stderr,
          timeout => 5,
          timed_out => \$timed_out,
          extra_params => ['--single-transaction'],
          on_error_die => 1);
print "pg_sleep timed out" if $timed_out;

done_testing();

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On 11.01.24 12:29, Bharath Rupireddy wrote:
> On Sat, Dec 30, 2023 at 12:57 AM Peter Eisentraut <peter@eisentraut.org> wrote:
>>
>> committed
> 
> With the commit c5385929 converting perl warnings to FATAL, use of
> psql/safe_psql with timeout parameters [1] fail with the following
> error:
> 
> Use of uninitialized value $ret in bitwise and (&) at
> /home/ubuntu/postgres/src/test/recovery/../../../src/test/perl/PostgreSQL/Test/Cluster.pm
> line 2015.

I think what is actually relevant is the timed_out parameter, otherwise 
the psql/safe_psql function ends up calling "die" and you don't get any 
further.

> Perhaps assigning a default error code to $ret instead of undef in
> PostgreSQL::Test::Cluster - psql() function is the solution.

I would put this code

     my $core = $ret & 128 ? " (core dumped)" : "";
     die "psql exited with signal "
       . ($ret & 127)
       . "$core: '$$stderr' while running '@psql_params'"
       if $ret & 127;
     $ret = $ret >> 8;

inside a if (defined $ret) block.

Then the behavior would be that the whole function returns undef on 
timeout, which is usefully different from returning 0 (and matches 
previous behavior).

On Fri, Jan 12, 2024 at 9:03 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 11.01.24 12:29, Bharath Rupireddy wrote:
> > On Sat, Dec 30, 2023 at 12:57 AM Peter Eisentraut <peter@eisentraut.org> wrote:
> >>
> >> committed
> >
> > With the commit c5385929 converting perl warnings to FATAL, use of
> > psql/safe_psql with timeout parameters [1] fail with the following
> > error:
> >
> > Use of uninitialized value $ret in bitwise and (&) at
> > /home/ubuntu/postgres/src/test/recovery/../../../src/test/perl/PostgreSQL/Test/Cluster.pm
> > line 2015.
>
> I think what is actually relevant is the timed_out parameter, otherwise
> the psql/safe_psql function ends up calling "die" and you don't get any
> further.
>
> > Perhaps assigning a default error code to $ret instead of undef in
> > PostgreSQL::Test::Cluster - psql() function is the solution.
>
> I would put this code
>
>      my $core = $ret & 128 ? " (core dumped)" : "";
>      die "psql exited with signal "
>        . ($ret & 127)
>        . "$core: '$$stderr' while running '@psql_params'"
>        if $ret & 127;
>      $ret = $ret >> 8;
>
> inside a if (defined $ret) block.
>
> Then the behavior would be that the whole function returns undef on
> timeout, which is usefully different from returning 0 (and matches
> previous behavior).

WFM.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Fri, Jan 12, 2024 at 9:21 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Fri, Jan 12, 2024 at 9:03 PM Peter Eisentraut <peter@eisentraut.org> wrote:
> >
> > I would put this code
> >
> >      my $core = $ret & 128 ? " (core dumped)" : "";
> >      die "psql exited with signal "
> >        . ($ret & 127)
> >        . "$core: '$$stderr' while running '@psql_params'"
> >        if $ret & 127;
> >      $ret = $ret >> 8;
> >
> > inside a if (defined $ret) block.
> >
> > Then the behavior would be that the whole function returns undef on
> > timeout, which is usefully different from returning 0 (and matches
> > previous behavior).
>
> WFM.

I've attached a patch for the above change.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On 16.01.24 12:08, Bharath Rupireddy wrote:
> On Fri, Jan 12, 2024 at 9:21 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
>>
>> On Fri, Jan 12, 2024 at 9:03 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>>>
>>> I would put this code
>>>
>>>       my $core = $ret & 128 ? " (core dumped)" : "";
>>>       die "psql exited with signal "
>>>         . ($ret & 127)
>>>         . "$core: '$$stderr' while running '@psql_params'"
>>>         if $ret & 127;
>>>       $ret = $ret >> 8;
>>>
>>> inside a if (defined $ret) block.
>>>
>>> Then the behavior would be that the whole function returns undef on
>>> timeout, which is usefully different from returning 0 (and matches
>>> previous behavior).
>>
>> WFM.
> 
> I've attached a patch for the above change.

Committed, thanks.