Re: warning handling in Perl scripts

Every time I make a change to the structure of the catalog files,
genbki.pl produces a bunch of warnings (like "Use of uninitialized value
in string eq at genbki.pl line ..."), and produces corrupted output
files, that are then (possibly) detected later by the compiler. Also,
getting out of that is difficult because due to the complicated
dependency relationship between the involved files, you need to remove a
bunch of files manually, or clean everything. So error handling could
be better.

It seems that adding

diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
index ebc4825..7d66da9 100644
--- a/src/backend/catalog/genbki.pl
+++ b/src/backend/catalog/genbki.pl
@@ -19,6 +19,8 @@
use strict;
use warnings;

+local $SIG{__WARN__} = sub { die $_[0] };
+
my @input_files;
our @include_path;
my $output_path = '';

would address that.

Could that cause any other problems? Should it be added to all Perl
scripts?

On Sun, Jun 24, 2012 at 2:40 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> Every time I make a change to the structure of the catalog files,
> genbki.pl produces a bunch of warnings (like "Use of uninitialized value
> in string eq at genbki.pl line ..."), and produces corrupted output
> files, that are then (possibly) detected later by the compiler.  Also,
> getting out of that is difficult because due to the complicated
> dependency relationship between the involved files, you need to remove a
> bunch of files manually, or clean everything.  So error handling could
> be better.
>
> It seems that adding
>
> diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
> index ebc4825..7d66da9 100644
> --- a/src/backend/catalog/genbki.pl
> +++ b/src/backend/catalog/genbki.pl
> @@ -19,6 +19,8 @@
>  use strict;
>  use warnings;
>
> +local $SIG{__WARN__} = sub { die $_[0] };
> +
>  my @input_files;
>  our @include_path;
>  my $output_path = '';
>
> would address that.
>
> Could that cause any other problems?  Should it be added to all Perl
> scripts?

This seems like a band-aid. How about if we instead add whatever
error-handling the script is missing, so that it produces an
appropriate, human-readable error message?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On sön, 2012-06-24 at 16:05 -0400, Robert Haas wrote:
> > diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
> > index ebc4825..7d66da9 100644
> > --- a/src/backend/catalog/genbki.pl
> > +++ b/src/backend/catalog/genbki.pl
> > @@ -19,6 +19,8 @@
> > use strict;
> > use warnings;
> >
> > +local $SIG{__WARN__} = sub { die $_[0] };
> > +
> > my @input_files;
> > our @include_path;
> > my $output_path = '';
> >
> > would address that.
> >
> > Could that cause any other problems? Should it be added to all Perl
> > scripts?
>
> This seems like a band-aid.

I'd think of it as a safety net.

> How about if we instead add whatever error-handling the script is
> missing, so that it produces an appropriate, human-readable error
> message?

That could also be worthwhile, but I think given the audience of that
script and the complexity of the possible failures, it could be a very
large and aimless project. But there should still be a catch-all for
uncaught failures.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On sn, 2012-06-24 at 16:05 -0400, Robert Haas wrote:
>>> +local $SIG{__WARN__} = sub { die $_[0] };

>> This seems like a band-aid.

> I'd think of it as a safety net.

+1 for the concept of turning warnings into errors, but is that really
the cleanest, most idiomatic way to do so in Perl? Sheesh.

regards, tom lane

On Jun 25, 2012, at 3:35 PM, Tom Lane wrote:

> +1 for the concept of turning warnings into errors, but is that really
> the cleanest, most idiomatic way to do so in Perl? Sheesh.

It’s the most backward-compatible, but the most idiomatic way to do it lexically is:

use warnings 'FATAL';

However, that works only for the current lexical scope. If there are warnings in the code you are calling from the current scope, the use of `local $SIG{__WARN__}` is required.

HTH,

David

Excerpts from David E. Wheeler's message of lun jun 25 11:23:34 -0400 2012:
> On Jun 25, 2012, at 3:35 PM, Tom Lane wrote:
>
> > +1 for the concept of turning warnings into errors, but is that really
> > the cleanest, most idiomatic way to do so in Perl? Sheesh.
>
> It’s the most backward-compatible, but the most idiomatic way to do it lexically is:
>
> use warnings 'FATAL';
>
> However, that works only for the current lexical scope. If there are warnings in the code you are calling from the current scope, the use of `local $SIG{__WARN__}` is required.

So lets add 'FATAL' to the already existing "use warnings" lines in
Catalog.pm and genbki.pl.

I think the other files we should add this to are generate-errcodes.pl,
generate-plerrorcodes.pl, generate-spiexceptions.pl, Gen_fmgrtab.pl.
Maybe psql/create_help.pl too.

We have a bunch of files in ECPG and MSVC areas and others in src/tools;
not sure about those.

We also have gen_qsort_tuple.pl which amusingly does not even
use warnings.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Jun 25, 2012, at 5:51 PM, Alvaro Herrera wrote:

>> However, that works only for the current lexical scope. If there are warnings in the code you are calling from the current scope, the use of `local $SIG{__WARN__}` is required.
>
> So lets add 'FATAL' to the already existing "use warnings" lines in
> Catalog.pm and genbki.pl.
>
> I think the other files we should add this to are generate-errcodes.pl,
> generate-plerrorcodes.pl, generate-spiexceptions.pl, Gen_fmgrtab.pl.
> Maybe psql/create_help.pl too.
>
> We have a bunch of files in ECPG and MSVC areas and others in src/tools;
> not sure about those.
>
> We also have gen_qsort_tuple.pl which amusingly does not even
> use warnings.

Hrm, I think that `use warnings 'FATAL';` might only work for core warnings. Which is annoying. I missed what was warning up-thread, but the most foolproof way to make all warnings fatal is the originally suggested

local $SIG{__WARN__} = sub { die shift };

A *bit* cleaner is to use Carp::croak:

use Carp;
local $SIG{__WARN__} = \&croak;

Or if you wanted to get a stack trace out of it, use Carp::confess:

use Carp;
local $SIG{__WARN__} = \&confess;

Exception-handling in Perl is one of the few places that annoy me regularly.

Best,

David

"David E. Wheeler" <david(at)justatheory(dot)com> writes:
> Hrm, I think that `use warnings 'FATAL';` might only work for core warnings. Which is annoying. I missed what was warning up-thread, but the most foolproof way to make all warnings fatal is the originally suggested

> local $SIG{__WARN__} = sub { die shift };

Sigh, let's do it that way then.

> A *bit* cleaner is to use Carp::croak:

> use Carp;
> local $SIG{__WARN__} = \&croak;

Just as soon not add a new module dependency if we don't have to.
In this case, since we're not really expecting the warnings to get
thrown, it seems like there'd be little value added by doing so.

regards, tom lane

On Mon, Jun 25, 2012 at 12:07:55PM -0400, Tom Lane wrote:
> "David E. Wheeler" <david(at)justatheory(dot)com> writes:
> > Hrm, I think that `use warnings 'FATAL';` might only work for core warnings. Which is annoying. I missed what was warning up-thread, but the most foolproof way to make all warnings fatal is the originally suggested
>
> > local $SIG{__WARN__} = sub { die shift };
>
> Sigh, let's do it that way then.
>
> > A *bit* cleaner is to use Carp::croak:
>
> > use Carp;
> > local $SIG{__WARN__} = \&croak;
>
> Just as soon not add a new module dependency if we don't have to.
Carp is core since Perl 5 [1994-10-17].

> In this case, since we're not really expecting the warnings to get
> thrown, it seems like there'd be little value added by doing so.
>
> regards, tom lane
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

-Ryan Kelly

On 06/24/2012 04:05 PM, Robert Haas wrote:
> On Sun, Jun 24, 2012 at 2:40 PM, Peter Eisentraut<peter_e(at)gmx(dot)net> wrote:
>> Every time I make a change to the structure of the catalog files,
>> genbki.pl produces a bunch of warnings (like "Use of uninitialized value
>> in string eq at genbki.pl line ..."), and produces corrupted output
>> files, that are then (possibly) detected later by the compiler. Also,
>> getting out of that is difficult because due to the complicated
>> dependency relationship between the involved files, you need to remove a
>> bunch of files manually, or clean everything. So error handling could
>> be better.
>>
>> It seems that adding
>>
>> diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
>> index ebc4825..7d66da9 100644
>> --- a/src/backend/catalog/genbki.pl
>> +++ b/src/backend/catalog/genbki.pl
>> @@ -19,6 +19,8 @@
>> use strict;
>> use warnings;
>>
>> +local $SIG{__WARN__} = sub { die $_[0] };
>> +
>> my @input_files;
>> our @include_path;
>> my $output_path = '';
>>
>> would address that.
>>
>> Could that cause any other problems? Should it be added to all Perl
>> scripts?
> This seems like a band-aid. How about if we instead add whatever
> error-handling the script is missing, so that it produces an
> appropriate, human-readable error message?

I realise I'm late to this party, but I'm with Robert. The root cause of
the errors should be fixed.

That's not to say that making warnings fatal might not also be a good
idea as a general defense mechanism.

cheers

andrew

On Jun 27, 2012, at 4:07 PM, Andrew Dunstan wrote:

> I realise I'm late to this party, but I'm with Robert. The root cause of the errors should be fixed.
>
> That's not to say that making warnings fatal might not also be a good idea as a general defense mechanism.

ISTM that if they are fatal, one will be more motivated to fix them. I think that was the point.

David