On Mon, Dec 7, 2015 at 12:06 PM, Noah Misch <[email protected]> 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 <[email protected]>.
Yep, I marked this email as something to address when it was sent.
On Sun, Dec 6, 2015 at 4:09 AM, Tom Lane <[email protected]> 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 ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
