Обсуждение: PostgresNode::_update_pid using undefined variables in tap tests

Hi all,

While running the test suite this morning I have noticed the following error:
server stopped
readline() on closed filehandle $pidfile at
/Users/ioltas/git/postgres/src/bin/pg_rewind/../../../src/test/perl/PostgresNode.pm
line 308.
Use of uninitialized value in concatenation (.) or string at
/Users/ioltas/git/postgres/src/bin/pg_rewind/../../../src/test/perl/PostgresNode.pm
line 309.
# Postmaster PID is

This does not impact the run, but it creates unwelcome warnings in the
logs. This is actually caused by the following code in PostgresNode
that uses an incorrect check to see if the file has been correctly
opened or not:
    open my $pidfile, $self->data_dir . "/postmaster.pid";
    if (not defined $pidfile)

One way to fix this is to use if(open(...)), a second way I know of is
to check if the opened file handle matches tell($pidfile) == -1. The
patch attached uses the first method to fix the issue.
Regards,
--
Michael

On Thu, Dec 3, 2015 at 11:28 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> This does not impact the run, but it creates unwelcome warnings in the
> logs. This is actually caused by the following code in PostgresNode
> that uses an incorrect check to see if the file has been correctly
> opened or not:
>     open my $pidfile, $self->data_dir . "/postmaster.pid";
>     if (not defined $pidfile)
>
> One way to fix this is to use if(open(...)), a second way I know of is
> to check if the opened file handle matches tell($pidfile) == -1. The
> patch attached uses the first method to fix the issue.

My Perl-fu must be getting weak.  What's wrong with the existing code?

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

On Wed, Dec 9, 2015 at 4:47 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Thu, Dec 3, 2015 at 11:28 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
> > This does not impact the run, but it creates unwelcome warnings in the
> > logs. This is actually caused by the following code in PostgresNode
> > that uses an incorrect check to see if the file has been correctly
> > opened or not:
> >     open my $pidfile, $self->data_dir . "/postmaster.pid";
> >     if (not defined $pidfile)
> >
> > One way to fix this is to use if(open(...)), a second way I know of is
> > to check if the opened file handle matches tell($pidfile) == -1. The
> > patch attached uses the first method to fix the issue.
>
> My Perl-fu must be getting weak.  What's wrong with the existing code?

This code should have checked for the return result of open instead of
looking at $pidfile. This has been noticed by Noah as well afterwards
and already addressed as 9821492.
-- 
Michael

Michael Paquier wrote:

> This code should have checked for the return result of open instead of
> looking at $pidfile. This has been noticed by Noah as well afterwards
> and already addressed as 9821492.

Oops, sorry I didn't credit you in the commit message.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Dec 9, 2015 at 8:57 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> Michael Paquier wrote:
>
>> This code should have checked for the return result of open instead of
>> looking at $pidfile. This has been noticed by Noah as well afterwards
>> and already addressed as 9821492.
>
> Oops, sorry I didn't credit you in the commit message.

That's fine. Don't worry.
-- 
Michael

On Tue, Dec 8, 2015 at 6:09 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Wed, Dec 9, 2015 at 4:47 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Thu, Dec 3, 2015 at 11:28 PM, Michael Paquier
>> <michael.paquier@gmail.com> wrote:
>> > This does not impact the run, but it creates unwelcome warnings in the
>> > logs. This is actually caused by the following code in PostgresNode
>> > that uses an incorrect check to see if the file has been correctly
>> > opened or not:
>> >     open my $pidfile, $self->data_dir . "/postmaster.pid";
>> >     if (not defined $pidfile)
>> >
>> > One way to fix this is to use if(open(...)), a second way I know of is
>> > to check if the opened file handle matches tell($pidfile) == -1. The
>> > patch attached uses the first method to fix the issue.
>>
>> My Perl-fu must be getting weak.  What's wrong with the existing code?
>
> This code should have checked for the return result of open instead of
> looking at $pidfile. This has been noticed by Noah as well afterwards
> and already addressed as 9821492.

I see that open() returns a value, but I figured $pidfile would end up
as undef if open failed.  I see that's not the case:

[rhaas pgsql]$ perl -MData::Dumper -e 'open(my $pidfile, "<",
"/fscsfasf") || warn $!; print Dumper($pidfile);'
No such file or directory at -e line 1.
$VAR1 = \*{'::$pidfile'};

Boy, that's awful.  Whoever designed that bit of wonderfulness should
have their language design license revoked.

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

On Wed, Dec 9, 2015 at 11:23 AM, Robert Haas wrote:
> Boy, that's awful.  Whoever designed that bit of wonderfulness should
> have their language design license revoked.

If one would want to work on a check with pidfile, he/she could check
for tell($pidfile) == -1 which corresponds to an undetermined
position, that's ugly but we need to live with that.
-- 
Michael