Thread: 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
Attachment
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