Thread
Commits
-
Add missing FATAL => 'all' to a use warnings in Perl
- 014720c6d9fb 18.0 landed
-
Make all Perl warnings fatal in 043_wal_replay_wait.pl
- e1555645d729 18.0 landed
-
Add tap test for pg_signal_autovacuum role
- d2b74882cab8 18.0 cited
-
Implement pg_wal_replay_wait() stored procedure
- 06c418e163e9 17.0 cited
-
Fix an issue in PostgreSQL::Test::Cluster:psql()
- 686db12de3a0 17.0 landed
-
Make all Perl warnings fatal
- c5385929593d 17.0 landed
-
Fix a warning in Perl test code
- a7ebd82b9e50 17.0 landed
-
Avoid use of Perl getprotobyname
- 01226c682ce6 17.0 landed
-
Make all Perl warnings fatal
Peter Eisentraut <peter@eisentraut.org> — 2023-08-10T05:58:27Z
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. -
Re: Make all Perl warnings fatal
Peter Eisentraut <peter@eisentraut.org> — 2023-08-21T06:20:08Z
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. -
Re: Make all Perl warnings fatal
Andrew Dunstan <andrew@dunslane.net> — 2023-08-21T15:51:24Z
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> 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
-
Re: Make all Perl warnings fatal
Michael Paquier <michael@paquier.xyz> — 2023-08-22T04:05:22Z
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
-
Re: Make all Perl warnings fatal
Andrew Dunstan <andrew@dunslane.net> — 2023-08-22T13:11:25Z
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
-
Re: Make all Perl warnings fatal
Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-08-22T13:20:28Z
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."
-
Re: Make all Perl warnings fatal
Andrew Dunstan <andrew@dunslane.net> — 2023-08-22T18:46:46Z
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
-
Re: Make all Perl warnings fatal
Peter Eisentraut <peter@eisentraut.org> — 2023-08-23T19:05:43Z
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.
-
Re: Make all Perl warnings fatal
Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> — 2023-08-25T20:49:34Z
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 -
Re: Make all Perl warnings fatal
Andrew Dunstan <andrew@dunslane.net> — 2023-08-27T11:23:11Z
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> 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 -
Re: Make all Perl warnings fatal
Peter Eisentraut <peter@eisentraut.org> — 2023-09-12T05:42:45Z
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.
-
Re: Make all Perl warnings fatal
Peter Eisentraut <peter@eisentraut.org> — 2023-12-22T21:33:18Z
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.
-
Re: Make all Perl warnings fatal
Peter Eisentraut <peter@eisentraut.org> — 2023-12-29T19:27:31Z
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
-
Re: Make all Perl warnings fatal
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2024-01-11T11:29:24Z
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 -
Re: Make all Perl warnings fatal
Peter Eisentraut <peter@eisentraut.org> — 2024-01-12T15:33:44Z
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). -
Re: Make all Perl warnings fatal
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2024-01-12T15:51:24Z
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
-
Re: Make all Perl warnings fatal
Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2024-01-16T11:08:47Z
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
-
Re: Make all Perl warnings fatal
Peter Eisentraut <peter@eisentraut.org> — 2024-01-18T07:52:33Z
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.
-
Re: Make all Perl warnings fatal
Anton Voloshin <a.voloshin@postgrespro.ru> — 2024-10-22T09:25:07Z
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'. -
Re: Make all Perl warnings fatal
Alexander Korotkov <aekorotkov@gmail.com> — 2024-10-22T10:26:31Z
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
-
Re: Make all Perl warnings fatal
Peter Eisentraut <peter@eisentraut.org> — 2024-10-29T09:34:09Z
On 22.10.24 11:25, Anton Voloshin wrote: > 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. Thanks, I committed the rest of this.