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

Hello, I'm studying this.

Two hunks in 0003 needed a fix but the other part applied cleanly
on master.

At Fri, 22 Jan 2016 15:17:51 +0900, Michael Paquier <michael.paquier@gmail.com> wrote in
<CAB7nPqTTAtVCEXAoyMtF4Xu9g=mXY4cjnP=+hy7jgYfnFzM=JA@mail.gmail.com>
> On Mon, Dec 21, 2015 at 4:45 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
> > As this thread is stalling a bit, please find attached a series of
> > patch gathering all the pending issues for this thread:
> > - 0001, fix config_default.pl for MSVC builds to take into account TAP tests
> > - 0002, append a node name in get_new_node (per Noah's request)
> > - 0003, the actual recovery test suite
> > Hopefully this facilitates future reviews.
> 
> Patch 2 has been pushed as c8642d9 (thanks Alvaro). The remaining two
> patches still apply and pass cleanly.

The TAP test framework doesn't remove existing temporary
directories when a test script suite (or a prove?) starts, and it
in turn removes all temprorary directories it has made even if
ended with fairures. It would be sometimes inconvenient to find
the cause of the failures and inconsistent with the behavior of
the ordinary(?)  make check, as far as my understanding goes.

tmp_check is left remained but it would be ok to preserve logs,
which is located in tmp_check differently than the ordinary
regressions.

One annoyance is the name of data directories is totally
meaningless. We cannot investigate them even if it is left
behind.

Addition to them, maybe it is useful that a test script can get
stderr content from PostgresNode->psql(). Setting
client_min_messages lower can give a plenty of useful information
about how server is working.


So, I'd like to propose four (or five) changes to this harness.
- prove_check to remove all in tmp_check
- TestLib to preserve temporary directories/files if the current  test fails.
- PostgresNode::get_new_node to create data directory with  meaningful basenames.
- PostgresNode::psql to return a list of ($stdout, $stderr) if  requested. (The previous behavior is not changed)
- (recovery/t/00x_* gives test number to node name)

As a POC, the attached diff will appliy on the 0001 and (fixed)
0003 patches.

It might be good to give test number to the name of temp dirs by
any automated way, but it is not included in it.


Opinions? Thoughts?

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 4602e3e..75ddcaf 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -51,7 +51,7 @@ sub new    my $self   = {        _port     => $pgport,        _host     => $pghost,
-        _basedir  => TestLib::tempdir,
+        _basedir  => TestLib::tempdir($name),        _name     => $name,        _logfile  =>
"$TestLib::log_path/${testname}_${name}.log"};
 
@@ -513,10 +513,16 @@ sub psql        print "#### Begin standard error\n";        print $stderr;        print "#### End
standarderror\n";
 
+        if (wantarray)
+        {
+            chomp $stderr;
+            $stderr =~ s/\r//g if $Config{osname} eq 'msys';
+        }    }    chomp $stdout;    $stdout =~ s/\r//g if $Config{osname} eq 'msys';
-    return $stdout;
+
+    return wantarray ? ($stdout, $stderr) : $stdout;}# Run a query once a second, until it returns 't' (i.e. SQL
booleantrue).
 
diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index 3d11cbb..841dc06 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -107,13 +107,24 @@ INIT    autoflush TESTLOG 1;}
+END
+{
+    # Preserve temporary directory for this test if failure
+    if (!Test::More->builder->is_passing)
+    {
+        $File::Temp::KEEP_ALL = 1;
+    }
+}
+## Helper functions#sub tempdir{
+    my ($prefix) = @_;
+    $prefix = "tmp_test" if (!$prefix);    return File::Temp::tempdir(
-        'tmp_testXXXX',
+        $prefix.'_XXXX',        DIR => $tmp_check,        CLEANUP => 1);}
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl
index 3ed9be3..8a00d00 100644
--- a/src/test/recovery/t/001_stream_rep.pl
+++ b/src/test/recovery/t/001_stream_rep.pl
@@ -6,16 +6,18 @@ use TestLib;use Test::More tests => 4;# Initialize master node
-my $node_master = get_new_node('master');
+my $node_master = get_new_node('001_master');$node_master->init(allows_streaming => 1);$node_master->start;my
$backup_name= 'my_backup';
 
+$File::Temp::KEEP_ALL = 0;
+# Take backup$node_master->backup($backup_name);# Create streaming standby linking to master
-my $node_standby_1 = get_new_node('standby_1');
+my $node_standby_1 = get_new_node('001_standby_1');$node_standby_1->init_from_backup($node_master, $backup_name,
                          has_streaming => 1);$node_standby_1->start;
 
@@ -25,7 +27,7 @@ $node_standby_1->start;$node_standby_1->backup($backup_name);# Create second standby node linking to
standby1
 
-my $node_standby_2 = get_new_node('standby_2');
+my $node_standby_2 = get_new_node('001_standby_2');$node_standby_2->init_from_backup($node_standby_1, $backup_name,
                             has_streaming => 1);$node_standby_2->start;
 
@@ -43,11 +45,11 @@ $caughtup_query = "SELECT pg_last_xlog_replay_location() = write_location FROM
p$node_standby_1->poll_query_until('postgres',$caughtup_query)    or die "Timed out while waiting for standby 2 to
catchup";
 
-my $result = $node_standby_1->psql('postgres', "SELECT count(*) FROM tab_int");
+my $result = $node_standby_1->psql('postgres', "set client_min_messages to debug5;SELECT count(*) FROM tab_int");print
"standby1: $result\n";
 
-is($result, qq(1002), 'check streamed content on standby 1');
+isnt($result[0], qq(1002), 'check streamed content on standby 1');
-$result =  $node_standby_2->psql('postgres', "SELECT count(*) FROM tab_int");
+my $result =  $node_standby_2->psql('postgres', "SELECT count(*) FROM tab_int");print "standby 2:
$result\n";is($result,qq(1002), 'check streamed content on standby 2');
 
diff --git a/src/test/recovery/t/002_archiving.pl b/src/test/recovery/t/002_archiving.pl
index 930125c..bb620c6 100644
--- a/src/test/recovery/t/002_archiving.pl
+++ b/src/test/recovery/t/002_archiving.pl
@@ -7,7 +7,7 @@ use Test::More tests => 1;use File::Copy;# Initialize master node, doing archives
-my $node_master = get_new_node('master');
+my $node_master = get_new_node('002_master');$node_master->init(has_archiving => 1,                   allows_streaming
=>1);my $backup_name = 'my_backup';
 
@@ -19,7 +19,7 @@ $node_master->start;$node_master->backup($backup_name);# Initialize standby node from backup,
fetchingWAL from archives
 
-my $node_standby = get_new_node('standby');
+my $node_standby = get_new_node('002_standby');$node_standby->init_from_backup($node_master, $backup_name,
                  has_restoring => 1);$node_standby->append_conf('postgresql.conf', qq(
 
diff --git a/src/test/recovery/t/003_recovery_targets.pl b/src/test/recovery/t/003_recovery_targets.pl
index 293603a..4d59277 100644
--- a/src/test/recovery/t/003_recovery_targets.pl
+++ b/src/test/recovery/t/003_recovery_targets.pl
@@ -16,7 +16,7 @@ sub test_recovery_standby    my $num_rows = shift;    my $until_lsn = shift;
-    my $node_standby = get_new_node($node_name);
+    my $node_standby = get_new_node("003_".$node_name);    $node_standby->init_from_backup($node_master, 'my_backup',
                                 has_restoring => 1);
 
@@ -43,7 +43,7 @@ sub test_recovery_standby}# Initialize master node
-my $node_master = get_new_node('master');
+my $node_master = get_new_node('003_master');$node_master->init(has_archiving => 1, allows_streaming => 1);# Start it
diff --git a/src/test/recovery/t/004_timeline_switch.pl b/src/test/recovery/t/004_timeline_switch.pl
index c58c602..0a2362b 100644
--- a/src/test/recovery/t/004_timeline_switch.pl
+++ b/src/test/recovery/t/004_timeline_switch.pl
@@ -11,7 +11,7 @@ use Test::More tests => 1;$ENV{PGDATABASE} = 'postgres';# Initialize master node
-my $node_master = get_new_node('master');
+my $node_master = get_new_node('004_master');$node_master->init(allows_streaming => 1);$node_master->start;
@@ -20,11 +20,11 @@ my $backup_name = 'my_backup';$node_master->backup($backup_name);# Create two standbys linking to
it
-my $node_standby_1 = get_new_node('standby_1');
+my $node_standby_1 = get_new_node('004_standby_1');$node_standby_1->init_from_backup($node_master, $backup_name,
                          has_streaming => 1);$node_standby_1->start;
 
-my $node_standby_2 = get_new_node('standby_2');
+my $node_standby_2 = get_new_node('004_standby_2');$node_standby_2->init_from_backup($node_master, $backup_name,
                          has_streaming => 1);$node_standby_2->start;
 
diff --git a/src/test/recovery/t/005_replay_delay.pl b/src/test/recovery/t/005_replay_delay.pl
index 14d9b29..ae209e4 100644
--- a/src/test/recovery/t/005_replay_delay.pl
+++ b/src/test/recovery/t/005_replay_delay.pl
@@ -6,7 +6,7 @@ use TestLib;use Test::More tests => 2;# Initialize master node
-my $node_master = get_new_node();
+my $node_master = get_new_node('005_master');$node_master->init(allows_streaming => 1);$node_master->start;
@@ -18,7 +18,7 @@ my $backup_name = 'my_backup';$node_master->backup($backup_name);# Create streaming standby from
backup
-my $node_standby = get_new_node();
+my $node_standby = get_new_node('005_standby');$node_standby->init_from_backup($node_master, $backup_name,
                  has_streaming => 1);$node_standby->append_conf('recovery.conf', qq(