Re: What about Perl autodie?
On Wed, Feb 7, 2024 at 9:05 AM Peter Eisentraut wrote: > I came across the Perl autodie pragma > (https://perldoc.perl.org/autodie). This seems pretty useful; is this > something we can use? Any drawbacks? Any minimum Perl version? Big +1 No drawbacks. I've been using it heavily for many, many years. Came out in 5.10.1, which should be available everywhere at this point (2009 was the year of release) Cheers, Greg
Re: What about Perl autodie?
On Wed, Feb 7, 2024 at 11:52 PM Greg Sabino Mullane wrote: > > No drawbacks. I've been using it heavily for many, many years. Came out in > 5.10.1, > which should be available everywhere at this point (2009 was the year of > release) We moved our minimum to 5.14 fairly recently, so we're good on that point.
Re: What about Perl autodie?
John Naylor writes: > On Wed, Feb 7, 2024 at 11:52 PM Greg Sabino Mullane > wrote: >> No drawbacks. I've been using it heavily for many, many years. Came out in >> 5.10.1, >> which should be available everywhere at this point (2009 was the year of >> release) > We moved our minimum to 5.14 fairly recently, so we're good on that point. Yeah, but only recently. I'm a little worried about the value of this change relative to the amount of code churn involved, and more to the point I worry about the risk of future back-patches injecting bad code into back branches that don't use autodie. (Back-patching the use of autodie doesn't seem feasible, since before v16 we supported perl 5.8.something.) regards, tom lane
Re: What about Perl autodie?
On 08.02.24 07:03, Tom Lane wrote: John Naylor writes: On Wed, Feb 7, 2024 at 11:52 PM Greg Sabino Mullane wrote: No drawbacks. I've been using it heavily for many, many years. Came out in 5.10.1, which should be available everywhere at this point (2009 was the year of release) We moved our minimum to 5.14 fairly recently, so we're good on that point. Yeah, but only recently. I'm a little worried about the value of this change relative to the amount of code churn involved, and more to the point I worry about the risk of future back-patches injecting bad code into back branches that don't use autodie. (Back-patching the use of autodie doesn't seem feasible, since before v16 we supported perl 5.8.something.) Yeah, good points. I suppose we could start using it for completely new scripts.
Re: What about Perl autodie?
> On 8 Feb 2024, at 08:01, Peter Eisentraut wrote: > I suppose we could start using it for completely new scripts. +1, it would be nice to eventually be able to move to it everywhere so starting now with new scripts may make the eventual transition smoother. -- Daniel Gustafsson
Re: What about Perl autodie?
Daniel Gustafsson writes: >> On 8 Feb 2024, at 08:01, Peter Eisentraut wrote: >> I suppose we could start using it for completely new scripts. > +1, it would be nice to eventually be able to move to it everywhere so > starting > now with new scripts may make the eventual transition smoother. I'm still concerned about people carelessly using autodie-reliant code in places where they shouldn't. I offer two safer ways forward: 1. Wait till v16 is the oldest supported branch, and then migrate both HEAD and back branches to using autodie. 2. Don't wait, migrate them all now. This would mean requiring Perl 5.10.1 or later to run the TAP tests, even in back branches. I think #2 might not be all that radical. We have nothing older than 5.14.0 in the buildfarm, so we don't really have much grounds for claiming that 5.8.3 will work today. And 5.10.1 came out in 2009, so how likely is it that anyone cares anymore? regards, tom lane
Re: What about Perl autodie?
> On 8 Feb 2024, at 16:53, Tom Lane wrote: > 2. Don't wait, migrate them all now. This would mean requiring > Perl 5.10.1 or later to run the TAP tests, even in back branches. > > I think #2 might not be all that radical. We have nothing older > than 5.14.0 in the buildfarm, so we don't really have much grounds > for claiming that 5.8.3 will work today. And 5.10.1 came out in > 2009, so how likely is it that anyone cares anymore? I would vote for this option, if we don't run the trailing edge anywhere where breakage is visible to developers then it is like you say, far from guaranteed to work. -- Daniel Gustafsson
Re: What about Perl autodie?
> > 2. Don't wait, migrate them all now. This would mean requiring > Perl 5.10.1 or later to run the TAP tests, even in back branches. > #2 please. For context, meson did not even exist in 2009. Cheers, Greg
Re: What about Perl autodie?
Daniel Gustafsson writes: >> On 8 Feb 2024, at 16:53, Tom Lane wrote: > >> 2. Don't wait, migrate them all now. This would mean requiring >> Perl 5.10.1 or later to run the TAP tests, even in back branches. >> >> I think #2 might not be all that radical. We have nothing older >> than 5.14.0 in the buildfarm, so we don't really have much grounds >> for claiming that 5.8.3 will work today. And 5.10.1 came out in >> 2009, so how likely is it that anyone cares anymore? > > I would vote for this option, if we don't run the trailing edge anywhere where > breakage is visible to developers then it is like you say, far from guaranteed > to work. The oldest Perl I'm aware of on a still-supported (fsvo) OS is RHEL 6, which shipped 5.10.1 and has Extended Life-cycle Support until 2024-06-30. For comparison, last year the at the Perl Toolchain Summit in Lyon we decided that toolchain modules (the modules needed to build, test and install CPAN distributions) are only required support versions of Perl up to 10 years old, i.e. currently 5.18 (but there's a one-time excemption to keep it to 5.16 until RHEL 7 goes out of maintenance support on 2024-06-30). - ilmari
Re: What about Perl autodie?
On 2024-02-08 Th 11:08, Daniel Gustafsson wrote: On 8 Feb 2024, at 16:53, Tom Lane wrote: 2. Don't wait, migrate them all now. This would mean requiring Perl 5.10.1 or later to run the TAP tests, even in back branches. I think #2 might not be all that radical. We have nothing older than 5.14.0 in the buildfarm, so we don't really have much grounds for claiming that 5.8.3 will work today. And 5.10.1 came out in 2009, so how likely is it that anyone cares anymore? I would vote for this option, if we don't run the trailing edge anywhere where breakage is visible to developers then it is like you say, far from guaranteed to work. +1 from me too. We kept 5.8 going for a while because it was what the Msys (v1) DTK perl was, but that doesn't matter any more I think. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: What about Perl autodie?
Andrew Dunstan writes: > On 2024-02-08 Th 11:08, Daniel Gustafsson wrote: >> On 8 Feb 2024, at 16:53, Tom Lane wrote: >>> 2. Don't wait, migrate them all now. This would mean requiring >>> Perl 5.10.1 or later to run the TAP tests, even in back branches. >>> I think #2 might not be all that radical. We have nothing older >>> than 5.14.0 in the buildfarm, so we don't really have much grounds >>> for claiming that 5.8.3 will work today. And 5.10.1 came out in >>> 2009, so how likely is it that anyone cares anymore? >> I would vote for this option, if we don't run the trailing edge anywhere >> where >> breakage is visible to developers then it is like you say, far from >> guaranteed >> to work. > +1 from me too. We kept 5.8 going for a while because it was what the > Msys (v1) DTK perl was, but that doesn't matter any more I think. I've reconfigured longfin, which was using perl 5.14.0 on all branches, to use 5.10.1 on the pre-v16 branches (and it did pass). This seems like a good change even if we don't pull the trigger on the above proposal --- although if we don't, maybe I should see if I can get 5.8.3 to build on that machine. regards, tom lane
Re: What about Perl autodie?
On 08.02.24 16:53, Tom Lane wrote: Daniel Gustafsson writes: On 8 Feb 2024, at 08:01, Peter Eisentraut wrote: I suppose we could start using it for completely new scripts. +1, it would be nice to eventually be able to move to it everywhere so starting now with new scripts may make the eventual transition smoother. I'm still concerned about people carelessly using autodie-reliant code in places where they shouldn't. I offer two safer ways forward: 1. Wait till v16 is the oldest supported branch, and then migrate both HEAD and back branches to using autodie. 2. Don't wait, migrate them all now. This would mean requiring Perl 5.10.1 or later to run the TAP tests, even in back branches. I think #2 might not be all that radical. We have nothing older than 5.14.0 in the buildfarm, so we don't really have much grounds for claiming that 5.8.3 will work today. And 5.10.1 came out in 2009, so how likely is it that anyone cares anymore? A gentler way might be to start using some perlcritic policies like InputOutput::RequireCheckedOpen or the more general InputOutput::RequireCheckedSyscalls and add explicit error checking at the sites it points out. And then if we start using autodie in the future, any inappropriate backpatching of calls lacking error checks would be caught.
Re: What about Perl autodie?
On 2024-02-14 We 11:52, Peter Eisentraut wrote: On 08.02.24 16:53, Tom Lane wrote: Daniel Gustafsson writes: On 8 Feb 2024, at 08:01, Peter Eisentraut wrote: I suppose we could start using it for completely new scripts. +1, it would be nice to eventually be able to move to it everywhere so starting now with new scripts may make the eventual transition smoother. I'm still concerned about people carelessly using autodie-reliant code in places where they shouldn't. I offer two safer ways forward: 1. Wait till v16 is the oldest supported branch, and then migrate both HEAD and back branches to using autodie. 2. Don't wait, migrate them all now. This would mean requiring Perl 5.10.1 or later to run the TAP tests, even in back branches. I think #2 might not be all that radical. We have nothing older than 5.14.0 in the buildfarm, so we don't really have much grounds for claiming that 5.8.3 will work today. And 5.10.1 came out in 2009, so how likely is it that anyone cares anymore? A gentler way might be to start using some perlcritic policies like InputOutput::RequireCheckedOpen or the more general InputOutput::RequireCheckedSyscalls and add explicit error checking at the sites it points out. And then if we start using autodie in the future, any inappropriate backpatching of calls lacking error checks would be caught. Yeah, that should work. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: What about Perl autodie?
> On 19 Feb 2024, at 01:54, Andrew Dunstan wrote: > On 2024-02-14 We 11:52, Peter Eisentraut wrote: >> A gentler way might be to start using some perlcritic policies like >> InputOutput::RequireCheckedOpen or the more general >> InputOutput::RequireCheckedSyscalls and add explicit error checking at the >> sites it points out. And then if we start using autodie in the future, any >> inappropriate backpatching of calls lacking error checks would be caught. > > Yeah, that should work. I didn't study the referenced rules but the concept seems sane, so definitely a +1 on that. -- Daniel Gustafsson
Re: What about Perl autodie?
On 14.02.24 17:52, Peter Eisentraut wrote:
A gentler way might be to start using some perlcritic policies like
InputOutput::RequireCheckedOpen or the more general
InputOutput::RequireCheckedSyscalls and add explicit error checking at
the sites it points out.
Here is a start for that. I added the required stanza to perlcriticrc
and started with an explicit list of functions to check:
functions = chmod flock open read rename seek symlink system
and fixed all the issues it pointed out.
I picked those functions because most existing code already checked
those, so the omissions are probably unintended, or in some cases also
because I thought it would be important for test correctness (e.g., some
tests using chmod).
I didn't design any beautiful error messages, mostly just used "or die
$!", which mostly matches existing code, and also this is
developer-level code, so having the system error plus source code
reference should be ok.
In the second patch, I changed the perlcriticrc stanza to use an
exclusion list instead of an explicit inclusion list. That way, you can
see what we are currently *not* checking. I'm undecided which way
around is better, and exactly what functions we should be checking. (Of
course, in principle, all of them, but since this is test and build
support code, not production code, there are probably some reasonable
compromises to be made.)
From c32941ce95281ab21691c4181962d20a820b1f20 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Tue, 20 Feb 2024 10:12:12 +0100
Subject: [PATCH v1 1/2] perlcritic InputOutput::RequireCheckedSyscalls
---
.../t/010_pg_archivecleanup.pl | 2 +-
src/bin/pg_basebackup/t/010_pg_basebackup.pl| 8
src/bin/pg_ctl/t/001_start_stop.pl | 2 +-
src/bin/pg_resetwal/t/002_corrupted.pl | 2 +-
src/bin/pg_rewind/t/009_growing_files.pl| 2 +-
src/bin/pg_rewind/t/RewindTest.pm | 4 ++--
src/pl/plperl/text2macro.pl | 4 ++--
src/test/kerberos/t/001_auth.pl | 2 +-
.../ssl_passphrase_callback/t/001_testfunc.pl | 2 +-
src/test/perl/PostgreSQL/Test/Cluster.pm| 12 ++--
src/test/perl/PostgreSQL/Test/Utils.pm | 16
src/test/ssl/t/SSL/Server.pm| 10 +-
src/tools/msvc_gendef.pl| 4 ++--
src/tools/perlcheck/perlcriticrc| 4
src/tools/pgindent/pgindent | 17 +
15 files changed, 48 insertions(+), 43 deletions(-)
diff --git a/src/bin/pg_archivecleanup/t/010_pg_archivecleanup.pl
b/src/bin/pg_archivecleanup/t/010_pg_archivecleanup.pl
index 792f5677c87..91a98c71e99 100644
--- a/src/bin/pg_archivecleanup/t/010_pg_archivecleanup.pl
+++ b/src/bin/pg_archivecleanup/t/010_pg_archivecleanup.pl
@@ -36,7 +36,7 @@ sub create_files
{
foreach my $fn (map { $_->{name} } @_)
{
- open my $file, '>', "$tempdir/$fn";
+ open my $file, '>', "$tempdir/$fn" or die $!;
print $file 'CONTENT';
close $file;
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index 86cc01a640b..159da3029af 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
@@ -77,7 +77,7 @@
ok(-d "$tempdir/backup", 'backup directory was created and left behind');
rmtree("$tempdir/backup");
-open my $conf, '>>', "$pgdata/postgresql.conf";
+open my $conf, '>>', "$pgdata/postgresql.conf" or die $!;
print $conf "max_replication_slots = 10\n";
print $conf "max_wal_senders = 10\n";
print $conf "wal_level = replica\n";
@@ -175,7 +175,7 @@
qw(backup_label tablespace_map postgresql.auto.conf.tmp
current_logfiles.tmp global/pg_internal.init.123))
{
- open my $file, '>>', "$pgdata/$filename";
+ open my $file, '>>', "$pgdata/$filename" or die $!;
print $file "DONOTCOPY";
close $file;
}
@@ -185,7 +185,7 @@
# unintended side effects.
if ($Config{osname} ne 'darwin')
{
- open my $file, '>>', "$pgdata/.DS_Store";
+ open my $file, '>>', "$pgdata/.DS_Store" or die $!;
print $file "DONOTCOPY";
close $file;
}
@@ -424,7 +424,7 @@
my $tblspcoid = $1;
my $escapedRepTsDir = $realRepTsDir;
$escapedRepTsDir =~ s/\\//g;
- open my $mapfile, '>', $node2->data_dir . '/tablespace_map';
+ open my $mapfile, '>', $node2->data_dir . '/tablespace_map' or die $!;
print $mapfile "$tblspcoid $escapedRepTsDir\n";
close $mapfile;
diff --git a/src/bin/pg_ctl/t/001_start_stop.pl
b/src/bin/pg_ctl/t/001_start_stop.pl
index fd56bf7706a..cbdaee57fb1 100644
--- a/src/bin/pg_ctl/t/001_start_stop.pl
+++ b/src/bin/pg_ctl/t/001_start_stop.pl
@@ -23,7 +23,7 @@
command_ok([ $ENV{PG_REGRESS}, '--config-auth', "$tempdir/data" ],
'configure authentication')
Re: What about Perl autodie?
On 21.02.24 08:26, Peter Eisentraut wrote:
On 14.02.24 17:52, Peter Eisentraut wrote:
A gentler way might be to start using some perlcritic policies like
InputOutput::RequireCheckedOpen or the more general
InputOutput::RequireCheckedSyscalls and add explicit error checking at
the sites it points out.
Here is a start for that. I added the required stanza to perlcriticrc
and started with an explicit list of functions to check:
functions = chmod flock open read rename seek symlink system
and fixed all the issues it pointed out.
I picked those functions because most existing code already checked
those, so the omissions are probably unintended, or in some cases also
because I thought it would be important for test correctness (e.g., some
tests using chmod).
I didn't design any beautiful error messages, mostly just used "or die
$!", which mostly matches existing code, and also this is
developer-level code, so having the system error plus source code
reference should be ok.
In the second patch, I changed the perlcriticrc stanza to use an
exclusion list instead of an explicit inclusion list. That way, you can
see what we are currently *not* checking. I'm undecided which way
around is better, and exactly what functions we should be checking. (Of
course, in principle, all of them, but since this is test and build
support code, not production code, there are probably some reasonable
compromises to be made.)
After some pondering, I figured the exclude list is better. So here is
a squashed patch, also with a complete commit message.
Btw., do we check perlcritic in an automated way, like on the buildfarm?From c70c6af0e369496bec04d20dbe42d09a233f046f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Sun, 17 Mar 2024 16:10:50 +0100
Subject: [PATCH v2] Activate perlcritic InputOutput::RequireCheckedSyscalls
and fix resulting warnings
This checks that certain I/O-related Perl functions properly check
their return value. Some parts of the PostgreSQL code had been a bit
sloppy about that. The new perlcritic warnings are fixed here. I
didn't design any beautiful error messages, mostly just used "or die
$!", which mostly matches existing code, and also this is
developer-level code, so having the system error plus source code
reference should be ok.
Initially, we only activate this check for a subset of what the
perlcritic check would warn about. The effective list is
chmod flock open read rename seek symlink system
The initial set of functions is picked because most existing code
already checked the return value of those, so any omissions are
probably unintended, or because it seems important for test
correctness.
The actual perlcritic configuration is written as an exclude list.
That seems better so that we are clear on what we are currently not
checking. Maybe future patches want to investigate checking some of
the other functions. (In principle, we might eventually want to check
all of them, but since this is test and build support code, not
production code, there are probably some reasonable compromises to be
made.)
Discussion:
https://www.postgresql.org/message-id/flat/88b7d4f2-46d9-4cc7-b1f7-613c90f9a76a%40eisentraut.org
---
.../t/010_pg_archivecleanup.pl | 2 +-
src/bin/pg_basebackup/t/010_pg_basebackup.pl| 8
src/bin/pg_ctl/t/001_start_stop.pl | 2 +-
src/bin/pg_resetwal/t/002_corrupted.pl | 2 +-
src/bin/pg_rewind/t/009_growing_files.pl| 2 +-
src/bin/pg_rewind/t/RewindTest.pm | 4 ++--
src/pl/plperl/text2macro.pl | 4 ++--
src/test/kerberos/t/001_auth.pl | 2 +-
.../ssl_passphrase_callback/t/001_testfunc.pl | 2 +-
src/test/perl/PostgreSQL/Test/Cluster.pm| 12 ++--
src/test/perl/PostgreSQL/Test/Utils.pm | 16
src/test/ssl/t/SSL/Server.pm| 10 +-
src/tools/msvc_gendef.pl| 4 ++--
src/tools/perlcheck/perlcriticrc| 8
src/tools/pgindent/pgindent | 17 +
15 files changed, 52 insertions(+), 43 deletions(-)
diff --git a/src/bin/pg_archivecleanup/t/010_pg_archivecleanup.pl
b/src/bin/pg_archivecleanup/t/010_pg_archivecleanup.pl
index 792f5677c87..91a98c71e99 100644
--- a/src/bin/pg_archivecleanup/t/010_pg_archivecleanup.pl
+++ b/src/bin/pg_archivecleanup/t/010_pg_archivecleanup.pl
@@ -36,7 +36,7 @@ sub create_files
{
foreach my $fn (map { $_->{name} } @_)
{
- open my $file, '>', "$tempdir/$fn";
+ open my $file, '>', "$tempdir/$fn" or die $!;
print $file 'CONTENT';
close $file;
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index d3c83f26e4b..490a9822f09 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_baseback
Re: What about Perl autodie?
On Mon, Mar 18, 2024 at 2:28 AM Peter Eisentraut wrote: > On 21.02.24 08:26, Peter Eisentraut wrote: > > On 14.02.24 17:52, Peter Eisentraut wrote: > >> A gentler way might be to start using some perlcritic policies like > >> InputOutput::RequireCheckedOpen or the more general > >> InputOutput::RequireCheckedSyscalls and add explicit error checking at > >> the sites it points out. > > > > Here is a start for that. I added the required stanza to perlcriticrc > > and started with an explicit list of functions to check: > > > > functions = chmod flock open read rename seek symlink system > > > > and fixed all the issues it pointed out. > > > > I picked those functions because most existing code already checked > > those, so the omissions are probably unintended, or in some cases also > > because I thought it would be important for test correctness (e.g., some > > tests using chmod). > > > > I didn't design any beautiful error messages, mostly just used "or die > > $!", which mostly matches existing code, and also this is > > developer-level code, so having the system error plus source code > > reference should be ok. > > > > In the second patch, I changed the perlcriticrc stanza to use an > > exclusion list instead of an explicit inclusion list. That way, you can > > see what we are currently *not* checking. I'm undecided which way > > around is better, and exactly what functions we should be checking. (Of > > course, in principle, all of them, but since this is test and build > > support code, not production code, there are probably some reasonable > > compromises to be made.) > > After some pondering, I figured the exclude list is better. So here is > a squashed patch, also with a complete commit message. > > Btw., do we check perlcritic in an automated way, like on the buildfarm? Yes. crake and koel do. cheers andrew
Re: What about Perl autodie?
> On 18 Mar 2024, at 07:27, Peter Eisentraut wrote: > After some pondering, I figured the exclude list is better. Agreed. > So here is a squashed patch, also with a complete commit message. Looks good from a read-through. It would have been nice to standardize on using one of "|| die" and "or die" consistently but that's clearly not for this body of work. -- Daniel Gustafsson
Re: What about Perl autodie?
Daniel Gustafsson writes: >> On 18 Mar 2024, at 07:27, Peter Eisentraut wrote: > >> After some pondering, I figured the exclude list is better. > > Agreed. > >> So here is a squashed patch, also with a complete commit message. > > Looks good from a read-through. It would have been nice to standardize on > using one of "|| die" and "or die" consistently but that's clearly not for > this > body of work. "or die" is generally the preferred form, since || has higher precedence than comma, so it's easy to make mistakes if you don't parenthesise the function args, like: open my $fh, '>', $filname || die "can't open $filename: $!"; which will only fail if $filename is falsy (i.e. undef, "", or "0"). - ilmari
Re: What about Perl autodie?
> On 18 Mar 2024, at 14:18, Dagfinn Ilmari Mannsåker wrote: > Daniel Gustafsson writes: >> It would have been nice to standardize on >> using one of "|| die" and "or die" consistently but that's clearly not for >> this >> body of work. > > "or die" is generally the preferred form, since || has higher precedence > than comma, so it's easy to make mistakes if you don't parenthesise the > function args, like: > > open my $fh, '>', $filname || die "can't open $filename: $!"; > > which will only fail if $filename is falsy (i.e. undef, "", or "0"). Thanks for the clarification! Looking over the || die() codepaths we have, and we'll add as part of this patchset, none are vulnerable to the above issue AFAICT. -- Daniel Gustafsson
Re: What about Perl autodie?
On 18.03.24 09:17, Daniel Gustafsson wrote: On 18 Mar 2024, at 07:27, Peter Eisentraut wrote: After some pondering, I figured the exclude list is better. Agreed. So here is a squashed patch, also with a complete commit message. Looks good from a read-through. It would have been nice to standardize on using one of "|| die" and "or die" consistently but that's clearly not for this body of work. Committed. I was aware of the semantic difference between "||" and "or", and I had tried to keep it similar to surrounding code.
