Обсуждение: What about Perl autodie?

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?

Attached is a sample patch of the kind of thing I'd be interested in. 
The existing error handling of file operations in Perl is pretty 
cumbersome, and this would simplify that.

Btw., here is a sample error message from autodie:

Can't open '../src/include/mb/pg_wchar.h' for reading: 'No such file or 
directory' at ../src/include/catalog/../../backend/catalog/genbki.pl 
line 391

which seems as good or better than the stuff we produce manually.

On Wed, Feb 7, 2024 at 11:52 PM Greg Sabino Mullane <htamfids@gmail.com> 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.

John Naylor <johncnaylorls@gmail.com> writes:
> On Wed, Feb 7, 2024 at 11:52 PM Greg Sabino Mullane <htamfids@gmail.com> 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

On 08.02.24 07:03, Tom Lane wrote:
> John Naylor <johncnaylorls@gmail.com> writes:
>> On Wed, Feb 7, 2024 at 11:52 PM Greg Sabino Mullane <htamfids@gmail.com> 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.

> On 8 Feb 2024, at 08:01, Peter Eisentraut <peter@eisentraut.org> 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

Daniel Gustafsson <daniel@yesql.se> writes:
>> On 8 Feb 2024, at 08:01, Peter Eisentraut <peter@eisentraut.org> 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

> On 8 Feb 2024, at 16:53, Tom Lane <tgl@sss.pgh.pa.us> 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

Daniel Gustafsson <daniel@yesql.se> writes:

>> On 8 Feb 2024, at 16:53, Tom Lane <tgl@sss.pgh.pa.us> 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

On 2024-02-08 Th 11:08, Daniel Gustafsson wrote:
>> On 8 Feb 2024, at 16:53, Tom Lane <tgl@sss.pgh.pa.us> 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

Andrew Dunstan <andrew@dunslane.net> writes:
> On 2024-02-08 Th 11:08, Daniel Gustafsson wrote:
>> On 8 Feb 2024, at 16:53, Tom Lane <tgl@sss.pgh.pa.us> 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

On 08.02.24 16:53, Tom Lane wrote:
> Daniel Gustafsson <daniel@yesql.se> writes:
>>> On 8 Feb 2024, at 08:01, Peter Eisentraut <peter@eisentraut.org> 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.

On 2024-02-14 We 11:52, Peter Eisentraut wrote:
> On 08.02.24 16:53, Tom Lane wrote:
>> Daniel Gustafsson <daniel@yesql.se> writes:
>>>> On 8 Feb 2024, at 08:01, Peter Eisentraut <peter@eisentraut.org> 
>>>> 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

> On 19 Feb 2024, at 01:54, Andrew Dunstan <andrew@dunslane.net> 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
generalInputOutput::RequireCheckedSyscalls and add explicit error checking at the sites it points out.  And then if we
startusing 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

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.)

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?

> On 18 Mar 2024, at 07:27, Peter Eisentraut <peter@eisentraut.org> 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

Daniel Gustafsson <daniel@yesql.se> writes:

>> On 18 Mar 2024, at 07:27, Peter Eisentraut <peter@eisentraut.org> 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

> On 18 Mar 2024, at 14:18, Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> wrote:
> Daniel Gustafsson <daniel@yesql.se> 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

On 18.03.24 09:17, Daniel Gustafsson wrote:
>> On 18 Mar 2024, at 07:27, Peter Eisentraut <peter@eisentraut.org> 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.