Make all Perl warnings fatal

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

On 2023-08-21 Mo 02:20, Peter Eisentraut wrote:

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&gt; 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 2023-08-22 Tu 00:05, Michael Paquier wrote:

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-08-22 Tu 09:20, Alvaro Herrera wrote:

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

On 2023-08-25 Fr 16:49, Dagfinn Ilmari Mannsåker wrote:

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&gt;
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]use strict; use warnings FATAL => 'all'; use PostgreSQL::Test::Cluster; use PostgreSQL::Test::Utils; use Test::More; 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;
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.

Hello,

On 18/01/2024 10:52, Peter Eisentraut wrote:

Committed, thanks.

since this patch two .pl files without FATAL in "use warnings" have been
committed to master:
src/test/recovery/t/043_wal_replay_wait.pl
src/test/modules/test_misc/t/006_signal_autovacuum.pl

They come from
commit 06c418e163e913966e17cb2d3fb1c5f8a8d58308
Author: Alexander Korotkov <akorotkov@postgresql.org>
Date: Tue Apr 2 22:48:03 2024

Implement pg_wal_replay_wait() stored procedure

and

commit d2b74882cab84b9f4fdce0f2f32e892ba9164f5c
Author: Michael Paquier <michael@paquier.xyz>
Date: Tue Jul 16 04:05:46 2024

Add tap test for pg_signal_autovacuum role

An obvious patch adding FATAL => 'all' is attached.

Cc Alexander Korotkov and Michael Paquier as committers of those commits.

--
Anton Voloshin
Postgres Professional, The Russian Postgres Company
https://postgrespro.ru

P.S. For what it's worth, this is an adapted snippet from a bash script
from our (postgrespro.com) internal CI used to detect those in
REL_17_STABLE+. It detects:
1. .pl .pm files without "use warnings"
2. .pl .pm files where "use warnings" does not have FATAL => 'all'

EXIT_CODE=0
LOG=ci_perl_files_without_warnings.log
grep --include='*.p[lm]' -R -L -w "^\s*use warnings" . | \
sed -e 's,^\./,,' | \
grep -v -F src/pl/plperl/plc_trusted.pl \

"$LOG"

if [ -s "$LOG" ]; then
N=$(wc -l < "$LOG");
echo "ERROR: \"use warnings\" is missing in the following $N Perl
files:";
cat "$LOG";
EXIT_CODE=1;
fi
# force "FATAL => 'all'" after any "use warnings" in Perl files
find . -name '*.p[lm]' -exec perl -i -p -e$'
s/^(\s*)
use \s+ warnings
(?! \s+ FATAL \s* => \s* \'all\' \s* );
/$1use warnings FATAL => \'all\';/x' {} \+;
PATCH_FILE=ci_perl_warnings.patch
git diff > "$PATCH_FILE"
if [ -s "$PATCH_FILE" ]; then
N=$(grep ^diff "$PATCH_FILE" | wc -l);
echo "ERROR: missing \"FATAL => 'all'\" in \"use warnings\" in the
following $N files:";
git status --porcelain | awk '/^ M/{print $2}';
PATCH_URL="$CI_JOB_URL/artifacts/file/$PATCH_FILE";
echo "NOTE: see $PATCH_URL for a suggested patch.";
EXIT_CODE=1;
fi
exit "$EXIT_CODE"

Please let me know if you think it's worth adapting into our general
cirrus pipeline. I'm not quite sure how to fit it yet.

I've added src/pl/plperl/plc_trusted.pl as an exception to the "contains
use warnings" check. It has require warnings, though. That's probably a
reasonable exception.

P.P.S. REL_17_STABLE is fine: all use warnings do have FATAL => 'all'.

Hi!

On Tue, Oct 22, 2024 at 12:26 PM Anton Voloshin
<a.voloshin@postgrespro.ru> wrote:

On 18/01/2024 10:52, Peter Eisentraut wrote:

Committed, thanks.

since this patch two .pl files without FATAL in "use warnings" have been
committed to master:
src/test/recovery/t/043_wal_replay_wait.pl
src/test/modules/test_misc/t/006_signal_autovacuum.pl

Thank you for spotting this.
I have just changed relevant line in 043_wal_replay_wait.pl.

------
Regards,
Alexander Korotkov
Supabase