On Mon, Dec 7, 2015 at 12:06 PM, Noah Misch <n...@leadboat.com> wrote: > On Wed, Dec 02, 2015 at 06:59:09PM -0300, Alvaro Herrera wrote: >> --- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl >> +++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl > >> -like($recovery_conf, qr/^standby_mode = 'on[']$/m, 'recovery.conf sets >> standby_mode'); >> -like($recovery_conf, qr/^primary_conninfo = '.*port=$ENV{PGPORT}.*'$/m, >> 'recovery.conf sets primary_conninfo'); >> - >> -command_ok([ 'pg_basebackup', '-D', "$tempdir/backupxf", '-X', 'fetch' ], >> +like( >> + $recovery_conf, >> + qr/^standby_mode = 'on[']$/m, >> + 'recovery.conf sets standby_mode'); >> +like( >> + $recovery_conf, >> + qr/^primary_conninfo = '.*port=$ENV{PGPORT}.*'$/m, >> + 'recovery.conf sets primary_conninfo'); > > This now elicits a diagnostic: > > Use of uninitialized value $ENV{"PGPORT"} in regexp compilation at > t/010_pg_basebackup.pl line 175, <> line 1.
Fixed. >> --- a/src/bin/pg_controldata/t/001_pg_controldata.pl >> +++ b/src/bin/pg_controldata/t/001_pg_controldata.pl > >> -standard_initdb "$tempdir/data"; >> -command_like([ 'pg_controldata', "$tempdir/data" ], >> + >> +my $node = get_new_node(); >> +$node->init; >> +$node->start; > > Before the commit, this test did not start a server. Fixed. >> --- /dev/null >> +++ b/src/test/perl/PostgresNode.pm > >> +sub _update_pid >> +{ >> + my $self = shift; >> + >> + # If we can open the PID file, read its first line and that's the PID >> we >> + # want. If the file cannot be opened, presumably the server is not >> + # running; don't be noisy in that case. >> + open my $pidfile, $self->data_dir . "/postmaster.pid"; >> + if (not defined $pidfile) > > $ grep -h 'at /.*line' src/bin/*/tmp_check/log/* |sort|uniq -c > 1 cannot remove directory for > /data/nmisch/src/pg/postgresql/src/bin/scripts/tmp_testNNCZ: Directory not > empty at /home/nmisch/sw/cpan/lib/perl5/File/Temp.pm line 784 > 1 cannot remove directory for > /data/nmisch/src/pg/postgresql/src/bin/scripts/tmp_testNNCZ/pgdata/base/13264: > Directory not empty at /home/nmisch/sw/cpan/lib/perl5/File/Temp.pm line 784 > 1 cannot remove directory for > /data/nmisch/src/pg/postgresql/src/bin/scripts/tmp_testNNCZ/pgdata/base: > Directory not empty at /home/nmisch/sw/cpan/lib/perl5/File/Temp.pm line 784 > 1 cannot remove directory for > /data/nmisch/src/pg/postgresql/src/bin/scripts/tmp_testNNCZ/pgdata: Directory > not empty at /home/nmisch/sw/cpan/lib/perl5/File/Temp.pm line 784 > 28 readline() on closed filehandle $pidfile at > /data/nmisch/src/pg/postgresql/src/bin/pg_rewind/../../../src/test/perl/PostgresNode.pm > line 308. > 28 Use of uninitialized value in concatenation (.) or string at > /data/nmisch/src/pg/postgresql/src/bin/pg_rewind/../../../src/test/perl/PostgresNode.pm > line 309. > > $pidfile is always defined. Test the open() return value. That's something I have addressed here: http://www.postgresql.org/message-id/cab7npqtop28zxv_sxfo2axgjoesfvllmo6syddafv0duvsf...@mail.gmail.com I am including the fix as well here to do a group shot. >> + { >> + $self->{_pid} = undef; >> + print "# No postmaster PID\n"; >> + return; >> + } >> + >> + $self->{_pid} = <$pidfile>; > > chomp() that value to remove its trailing newline. Right. >> + print "# Postmaster PID is $self->{_pid}\n"; >> + close $pidfile; >> +} > >> + my $devnull = $TestLib::windows_os ? "nul" : "/dev/null"; > > Unused variable. Removed. >> +sub DESTROY >> +{ >> + my $self = shift; >> + return if not defined $self->{_pid}; >> + print "# signalling QUIT to $self->{_pid}\n"; >> + kill 'QUIT', $self->{_pid}; > > On Windows, that kill() is the wrong thing. I suspect "pg_ctl kill" will be > the right thing, but that warrants specific testing. I don't directly see any limitation with the use of kill on Windows.. http://perldoc.perl.org/functions/kill.html But indeed using directly pg_ctl kill seems like a better fit for the PG infrastructure. > Postmaster log file names became less informative. Before the commit: > Should nodes get a name, so we instead see master_57834.log? I am not sure that this is necessary. There is definitely a limitation regarding the fact that log files of nodes get overwritten after each test, hence I would tend with just appending the test name in front of the node_* files similarly to the other files. > See also the things Tom Lane identified in <27550.1449342...@sss.pgh.pa.us>. Yep, I marked this email as something to address when it was sent. On Sun, Dec 6, 2015 at 4:09 AM, Tom Lane <t...@sss.pgh.pa.us> wrote: > BTW, if we consider that gospel, why has PostgresNode.pm got this in its > BEGIN block? > > # PGHOST is set once and for all through a single series of tests when > # this module is loaded. > $test_pghost = > $TestLib::windows_os ? "127.0.0.1" : TestLib::tempdir_short; > $ENV{PGHOST} = $test_pghost; > > On non-Windows machines, the call of tempdir_short will result in > filesystem activity, ie creation of a directory. Offhand it looks like > all of the activity in this BEGIN block could go to an INIT block instead. OK, the whole block is switched to INIT instead. > I'm also bemused by why there was any question about this being wrong: > > # XXX: Should this use PG_VERSION_NUM? > $last_port_assigned = 90600 % 16384 + 49152; > If that's not a hard-coded PG version number then I don't know > what it is. Maybe it would be better to use random() instead, > but surely this isn't good as-is. We would definitely want something within the ephemeral port range, so we are up to that: rand() * 16384 + 49152; All those issues are addressed as per the patch attached. Regards, -- Michael
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl index 3e491a8..5991cf7 100644 --- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl +++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl @@ -168,13 +168,14 @@ my $recovery_conf = slurp_file "$tempdir/backupR/recovery.conf"; # using a character class for the final "'" here works around an apparent # bug in several version of the Msys DTK perl +my $port = $node->port; like( $recovery_conf, qr/^standby_mode = 'on[']$/m, 'recovery.conf sets standby_mode'); like( $recovery_conf, - qr/^primary_conninfo = '.*port=$ENV{PGPORT}.*'$/m, + qr/^primary_conninfo = '.*port=$port.*'$/m, 'recovery.conf sets primary_conninfo'); $node->command_ok( diff --git a/src/bin/pg_controldata/t/001_pg_controldata.pl b/src/bin/pg_controldata/t/001_pg_controldata.pl index ae45f41..073815a 100644 --- a/src/bin/pg_controldata/t/001_pg_controldata.pl +++ b/src/bin/pg_controldata/t/001_pg_controldata.pl @@ -13,7 +13,6 @@ command_fails([ 'pg_controldata', 'nonexistent' ], my $node = get_new_node(); $node->init; -$node->start; command_like([ 'pg_controldata', $node->data_dir ], qr/checkpoint/, 'pg_controldata produces output'); diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm index aa7a00c..db7459f 100644 --- a/src/test/perl/PostgresNode.pm +++ b/src/test/perl/PostgresNode.pm @@ -27,7 +27,7 @@ our @EXPORT = qw( our ($test_pghost, $last_port_assigned, @all_nodes); -BEGIN +INIT { # PGHOST is set once and for all through a single series of tests when @@ -38,8 +38,7 @@ BEGIN $ENV{PGDATABASE} = 'postgres'; # Tracking of last port value assigned to accelerate free port lookup. - # XXX: Should this use PG_VERSION_NUM? - $last_port_assigned = 90600 % 16384 + 49152; + $last_port_assigned = int(rand() * 16384) + 49152; # Node tracking @all_nodes = (); @@ -50,12 +49,14 @@ sub new my $class = shift; my $pghost = shift; my $pgport = shift; + my $testname = basename($0); + $testname =~ s/\.[^.]+$//; my $self = { _port => $pgport, _host => $pghost, _basedir => TestLib::tempdir, _applname => "node_$pgport", - _logfile => "$TestLib::log_path/node_$pgport.log" }; + _logfile => $TestLib::log_path . '/' . $testname . '_node_' . $pgport . '.log' }; bless $self, $class; $self->dump_info; @@ -297,17 +298,17 @@ sub _update_pid # If we can open the PID file, read its first line and that's the PID we # want. If the file cannot be opened, presumably the server is not # running; don't be noisy in that case. - open my $pidfile, $self->data_dir . "/postmaster.pid"; - if (not defined $pidfile) + if (open my $pidfile, $self->data_dir . "/postmaster.pid") { - $self->{_pid} = undef; - print "# No postmaster PID\n"; + my $pid = <$pidfile>; + $self->{_pid} = chomp($pid); + print "# Postmaster PID is $self->{_pid}\n"; + close $pidfile; return; } - $self->{_pid} = <$pidfile>; - print "# Postmaster PID is $self->{_pid}\n"; - close $pidfile; + $self->{_pid} = undef; + print "# No postmaster PID\n"; } # @@ -327,7 +328,6 @@ sub get_new_node { $port++; print "# Checking for port $port\n"; - my $devnull = $TestLib::windows_os ? "nul" : "/dev/null"; if (!TestLib::run_log([ 'pg_isready', '-p', $port ])) { $found = 1; @@ -360,7 +360,7 @@ sub DESTROY my $self = shift; return if not defined $self->{_pid}; print "# signalling QUIT to $self->{_pid}\n"; - kill 'QUIT', $self->{_pid}; + TestLib::system_log('pg_ctl', 'kill', 'QUIT', $self->{_pid}); } sub teardown_node
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers