Re: Re: In-core regression tests for replication, cascading, archiving, PITR, etc.

On Mon, Dec 7, 2015 at 12:06 PM, Noah Misch <noah@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:
Directorynot 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:
Directorynot 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
emptyat /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.pmline 308. 
>      28 Use of uninitialized value in concatenation (.) or string at
/data/nmisch/src/pg/postgresql/src/bin/pg_rewind/../../../src/test/perl/PostgresNode.pmline 309. 
>
> $pidfile is always defined.  Test the open() return value.

That's something I have addressed here:
http://www.postgresql.org/message-id/CAB7nPqTOP28Zxv_SXFo2axGJoesfvLLMO6syddAfV0DUvsFMDw@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.1449342569@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 <tgl@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