TAP / recovery-test fs-level backups, psql enhancements etc

From bd913623092d92cc65d5b94b1b0c3fd5e66b8856 Mon Sep 17 00:00:00 2001
From: Craig Ringer <craig@2ndquadrant.com>
Date: Wed, 2 Mar 2016 11:17:46 +0800
Subject: [PATCH] TAP: Fix Perl 5.8.8 support

Commit 7132810c (Retain tempdirs for failed tests) used the is_passing
method present only after Perl 5.10 in Test::More 0.89_01. Work around
its absence.
---
 src/test/perl/TestLib.pm | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index 63a0e77..c069d43 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -110,7 +110,23 @@ INIT
 END
 {
 	# Preserve temporary directory for this test on failure
-	$File::Temp::KEEP_ALL = 1 unless Test::More->builder->is_passing;
+	#
+	# We should be able to use Test::More->builder->is_passing here, but
+	# we require Perl 5.8.8 support so we have to work around it.
+	#
+	$File::Temp::KEEP_ALL = 1 unless all_tests_passing;
+}
+
+# Workaround for is_passing being missing prior to Test::More 0.89_01
+# Credit http://stackoverflow.com/a/1506700/398670
+sub all_tests_passing
+{
+	my $FAILcount = 0;
+	foreach my $detail (Test::Builder->details())
+	{
+		if (${%$detail}{ok} == 0) { $FAILcount++; }
+	}
+	return !$FAILcount;
 }
 
 #
-- 
2.1.0

From a6aa0139cc8fea2fb93b7c3087d8878d8923c49a Mon Sep 17 00:00:00 2001
From: Craig Ringer <craig@2ndquadrant.com>
Date: Wed, 2 Mar 2016 11:17:46 +0800
Subject: [PATCH] TAP: Fix Perl 5.8.8 support

Commit 7132810c (Retain tempdirs for failed tests) used the is_passing
method present only after Perl 5.10 in Test::More 0.89_01. Work around
its absence.
---
 src/test/perl/TestLib.pm | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index 63a0e77..1900922 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -110,7 +110,23 @@ INIT
 END
 {
 	# Preserve temporary directory for this test on failure
-	$File::Temp::KEEP_ALL = 1 unless Test::More->builder->is_passing;
+	#
+	# We should be able to use Test::More->builder->is_passing here, but
+	# we require Perl 5.8.8 support so we have to work around it.
+	#
+	$File::Temp::KEEP_ALL = 1 unless all_tests_passing();
+}
+
+# Workaround for is_passing being missing prior to Test::More 0.89_01
+# Credit http://stackoverflow.com/a/1506700/398670
+sub all_tests_passing
+{
+	my $FAILcount = 0;
+	foreach my $detail (Test::Builder->details())
+	{
+		if (${%$detail}{ok} == 0) { $FAILcount++; }
+	}
+	return !$FAILcount;
 }
 
 #
-- 
2.1.0

diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index 4fb43fe..564936e 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -110,7 +110,17 @@ INIT
 END
 {
 	# Preserve temporary directory for this test on failure
-	$File::Temp::KEEP_ALL = 1 unless Test::More->builder->is_passing;
+	$File::Temp::KEEP_ALL = 1 unless all_tests_passing();
+}
+
+sub all_tests_passing
+{
+	my $fail_count = 0;
+	foreach my $status (Test::More->builder->summary)
+	{
+		return 0 unless $status;
+	}
+	return 1;
 }
 
 #

From a703cd78cf834df794253bb1cfd897daa96d244a Mon Sep 17 00:00:00 2001
From: Craig Ringer <craig@2ndquadrant.com>
Date: Thu, 3 Mar 2016 13:18:23 +0800
Subject: [PATCH] TAP: File::Path::remove_tree is new in 2.x, use 1.x rmtree
 instead

Preserve Perl 5.8.8 support by using the legacy rmtree call
instead of remove_tree from File::Path 2.x.
---
 src/test/recovery/t/004_timeline_switch.pl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/test/recovery/t/004_timeline_switch.pl b/src/test/recovery/t/004_timeline_switch.pl
index 58bf580..a180b94 100644
--- a/src/test/recovery/t/004_timeline_switch.pl
+++ b/src/test/recovery/t/004_timeline_switch.pl
@@ -3,7 +3,7 @@
 # on a new timeline.
 use strict;
 use warnings;
-use File::Path qw(remove_tree);
+use File::Path;
 use PostgresNode;
 use TestLib;
 use Test::More tests => 1;
@@ -46,7 +46,7 @@ $node_master->teardown_node;
 $node_standby_1->promote;
 
 # Switch standby 2 to replay from standby 1
-remove_tree($node_standby_2->data_dir . '/recovery.conf');
+File::Path::rmtree($node_standby_2->data_dir . '/recovery.conf');
 my $connstr_1 = $node_standby_1->connstr;
 $node_standby_2->append_conf(
 	'recovery.conf', qq(
-- 
2.1.0

From 7f5cf0ac7368790e4597642b59560fa1c16597d8 Mon Sep 17 00:00:00 2001
From: Craig Ringer <craig@2ndquadrant.com>
Date: Thu, 3 Mar 2016 13:18:23 +0800
Subject: [PATCH 1/7] TAP: File::Path::remove_tree is new in 2.x, use 1.x
 rmtree instead

Preserve Perl 5.8.8 support by using the legacy rmtree call
instead of remove_tree from File::Path 2.x.
---
 src/test/recovery/t/004_timeline_switch.pl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/test/recovery/t/004_timeline_switch.pl b/src/test/recovery/t/004_timeline_switch.pl
index 58bf580..a180b94 100644
--- a/src/test/recovery/t/004_timeline_switch.pl
+++ b/src/test/recovery/t/004_timeline_switch.pl
@@ -3,7 +3,7 @@
 # on a new timeline.
 use strict;
 use warnings;
-use File::Path qw(remove_tree);
+use File::Path;
 use PostgresNode;
 use TestLib;
 use Test::More tests => 1;
@@ -46,7 +46,7 @@ $node_master->teardown_node;
 $node_standby_1->promote;
 
 # Switch standby 2 to replay from standby 1
-remove_tree($node_standby_2->data_dir . '/recovery.conf');
+File::Path::rmtree($node_standby_2->data_dir . '/recovery.conf');
 my $connstr_1 = $node_standby_1->connstr;
 $node_standby_2->append_conf(
 	'recovery.conf', qq(
-- 
2.1.0

From c346e15f613931d0b8d65025076a998234dbe26e Mon Sep 17 00:00:00 2001
From: Craig Ringer <craig@2ndquadrant.com>
Date: Thu, 3 Mar 2016 14:07:32 +0800
Subject: [PATCH 2/7] TAP: Fix syntax error in recovery tests

pg_create_restore_point was never running successfully in the
TAP tests for recovery because of a typo.
---
 src/test/recovery/t/003_recovery_targets.pl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/test/recovery/t/003_recovery_targets.pl b/src/test/recovery/t/003_recovery_targets.pl
index 8b581cc..a0d8b45 100644
--- a/src/test/recovery/t/003_recovery_targets.pl
+++ b/src/test/recovery/t/003_recovery_targets.pl
@@ -83,8 +83,8 @@ $node_master->psql('postgres',
 my $recovery_name = "my_target";
 my $lsn4 =
   $node_master->psql('postgres', "SELECT pg_current_xlog_location();");
-$node_master->psql('postgres',
-	"SELECT pg_create_restore_point('$recovery_name'");
+$node_master->psql_check('postgres',
+	"SELECT pg_create_restore_point('$recovery_name');");
 
 # Force archiving of WAL file
 $node_master->psql('postgres', "SELECT pg_switch_xlog()");
-- 
2.1.0

From ca0c28be54329473ef0bed07358b519835c5b447 Mon Sep 17 00:00:00 2001
From: Craig Ringer <craig@2ndquadrant.com>
Date: Tue, 1 Mar 2016 21:44:11 +0800
Subject: [PATCH 3/7] TAP: Perltidy PostgresNode.pm

Recent PostgresNode changes were committed without using the project's
perltidyrc. Tidy it now.
---
 src/test/perl/PostgresNode.pm | 58 ++++++++++++++++++++++---------------------
 src/test/perl/README          |  3 +++
 src/test/perl/SimpleTee.pm    | 13 ++++++----
 3 files changed, 41 insertions(+), 33 deletions(-)

diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 4ca7f77..7c8e66e 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -1,3 +1,4 @@
+
 =pod
 
 =head1 NAME
@@ -106,18 +107,15 @@ of finding port numbers, registering instances for cleanup, etc.
 
 sub new
 {
-	my $class  = shift;
-	my $name   = shift;
-	my $pghost = shift;
-	my $pgport = shift;
+	my ($class, $name, $pghost, $pgport) = @_;
 	my $testname = basename($0);
 	$testname =~ s/\.[^.]+$//;
-	my $self   = {
-		_port     => $pgport,
-		_host     => $pghost,
-		_basedir  => TestLib::tempdir("data_" . $name),
-		_name     => $name,
-		_logfile  => "$TestLib::log_path/${testname}_${name}.log" };
+	my $self = {
+		_port    => $pgport,
+		_host    => $pghost,
+		_basedir => TestLib::tempdir("data_" . $name),
+		_name    => $name,
+		_logfile => "$TestLib::log_path/${testname}_${name}.log" };
 
 	bless $self, $class;
 	$self->dump_info;
@@ -367,7 +365,7 @@ sub init
 	$params{hba_permit_replication} = 1
 	  unless defined $params{hba_permit_replication};
 	$params{allows_streaming} = 0 unless defined $params{allows_streaming};
-	$params{has_archiving} = 0 unless defined $params{has_archiving};
+	$params{has_archiving}    = 0 unless defined $params{has_archiving};
 
 	mkdir $self->backup_dir;
 	mkdir $self->archive_dir;
@@ -405,7 +403,7 @@ sub init
 	close $conf;
 
 	$self->set_replication_conf if $params{hba_permit_replication};
-	$self->enable_archiving if $params{has_archiving};
+	$self->enable_archiving     if $params{has_archiving};
 }
 
 =pod
@@ -492,7 +490,7 @@ sub init_from_backup
 
 	$params{has_streaming} = 0 unless defined $params{has_streaming};
 	$params{hba_permit_replication} = 1
-	   unless defined $params{hba_permit_replication};
+	  unless defined $params{hba_permit_replication};
 	$params{has_restoring} = 0 unless defined $params{has_restoring};
 
 	print
@@ -514,7 +512,7 @@ sub init_from_backup
 		qq(
 port = $port
 ));
-	$self->set_replication_conf if $params{hba_permit_replication};
+	$self->set_replication_conf         if $params{hba_permit_replication};
 	$self->enable_streaming($root_node) if $params{has_streaming};
 	$self->enable_restoring($root_node) if $params{has_restoring};
 }
@@ -607,19 +605,19 @@ sub promote
 	my $logfile = $self->logfile;
 	my $name    = $self->name;
 	print "### Promoting node \"$name\"\n";
-	TestLib::system_log('pg_ctl', '-D', $pgdata, '-l', $logfile,
-						'promote');
+	TestLib::system_log('pg_ctl', '-D', $pgdata, '-l', $logfile, 'promote');
 }
 
 # Internal routine to enable streaming replication on a standby node.
 sub enable_streaming
 {
-	my ($self, $root_node)  = @_;
+	my ($self, $root_node) = @_;
 	my $root_connstr = $root_node->connstr;
-	my $name    = $self->name;
+	my $name         = $self->name;
 
 	print "### Enabling streaming replication for node \"$name\"\n";
-	$self->append_conf('recovery.conf', qq(
+	$self->append_conf(
+		'recovery.conf', qq(
 primary_conninfo='$root_connstr application_name=$name'
 standby_mode=on
 ));
@@ -628,7 +626,7 @@ standby_mode=on
 # Internal routine to enable archive recovery command on a standby node
 sub enable_restoring
 {
-	my ($self, $root_node)  = @_;
+	my ($self, $root_node) = @_;
 	my $path = $root_node->archive_dir;
 	my $name = $self->name;
 
@@ -641,11 +639,13 @@ sub enable_restoring
 	# first. Paths also need to be double-quoted to prevent failures where
 	# the path contains spaces.
 	$path =~ s{\\}{\\\\}g if ($TestLib::windows_os);
-	my $copy_command = $TestLib::windows_os ?
-		qq{copy "$path\\\\%f" "%p"} :
-		qq{cp $path/%f %p};
+	my $copy_command =
+	  $TestLib::windows_os
+	  ? qq{copy "$path\\\\%f" "%p"}
+	  : qq{cp $path/%f %p};
 
-	$self->append_conf('recovery.conf', qq(
+	$self->append_conf(
+		'recovery.conf', qq(
 restore_command = '$copy_command'
 standby_mode = on
 ));
@@ -667,12 +667,14 @@ sub enable_archiving
 	# first. Paths also need to be double-quoted to prevent failures where
 	# the path contains spaces.
 	$path =~ s{\\}{\\\\}g if ($TestLib::windows_os);
-	my $copy_command = $TestLib::windows_os ?
-		qq{copy "%p" "$path\\\\%f"} :
-		qq{cp %p $path/%f};
+	my $copy_command =
+	  $TestLib::windows_os
+	  ? qq{copy "%p" "$path\\\\%f"}
+	  : qq{cp %p $path/%f};
 
 	# Enable archive_mode and archive_command on node
-	$self->append_conf('postgresql.conf', qq(
+	$self->append_conf(
+		'postgresql.conf', qq(
 archive_mode = on
 archive_command = '$copy_command'
 ));
diff --git a/src/test/perl/README b/src/test/perl/README
index 7b6de5f..9eae159 100644
--- a/src/test/perl/README
+++ b/src/test/perl/README
@@ -11,6 +11,9 @@ isolation tester specs in src/test/isolation, if possible. If not, check to
 see if your new tests make sense under an existing tree in src/test, like
 src/test/ssl, or should be added to one of the suites for an existing utility.
 
+Note that all tests and test tools should have perltidy run on them before
+patches are submitted, using perltidy --profile=src/tools/pgindent/perltidyrc
+
 Writing tests
 -------------
 
diff --git a/src/test/perl/SimpleTee.pm b/src/test/perl/SimpleTee.pm
index 5da82d0..ea2f2ee 100644
--- a/src/test/perl/SimpleTee.pm
+++ b/src/test/perl/SimpleTee.pm
@@ -10,17 +10,20 @@
 package SimpleTee;
 use strict;
 
-sub TIEHANDLE {
+sub TIEHANDLE
+{
 	my $self = shift;
 	bless \@_, $self;
 }
 
-sub PRINT {
+sub PRINT
+{
 	my $self = shift;
-	my $ok = 1;
-	for my $fh (@$self) {
+	my $ok   = 1;
+	for my $fh (@$self)
+	{
 		print $fh @_ or $ok = 0;
-		$fh->flush or $ok = 0;
+		$fh->flush   or $ok = 0;
 	}
 	return $ok;
 }
-- 
2.1.0

From 548469e609cc14d69c49908035950bdc37a74d3c Mon Sep 17 00:00:00 2001
From: Craig Ringer <craig@2ndquadrant.com>
Date: Tue, 1 Mar 2016 21:06:47 +0800
Subject: [PATCH 4/7] TAP: Add filtering to RecursiveCopy

Allow RecursiveCopy to accept a filter function so callers can
exclude unwanted files. Also POD-ify it.
---
 src/test/perl/RecursiveCopy.pm | 76 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 70 insertions(+), 6 deletions(-)

diff --git a/src/test/perl/RecursiveCopy.pm b/src/test/perl/RecursiveCopy.pm
index 9362aa8..97f84e5 100644
--- a/src/test/perl/RecursiveCopy.pm
+++ b/src/test/perl/RecursiveCopy.pm
@@ -1,4 +1,18 @@
-# RecursiveCopy, a simple recursive copy implementation
+
+=pod
+
+=head1 NAME
+
+RecursiveCopy - simple recursive copy implementation
+
+=head1 SYNOPSIS
+
+use RecursiveCopy;
+
+RecursiveCopy::copypath($from, $to);
+
+=cut
+
 package RecursiveCopy;
 
 use strict;
@@ -7,10 +21,56 @@ use warnings;
 use File::Basename;
 use File::Copy;
 
+=pod
+
+=head2 copypath($from, $to)
+
+Copy all files and directories from $from to $to. Raises an exception
+if a file would be overwritten, the source dir can't be read, or any
+I/O operation fails.  Always returns true. On failure the copy may be
+in some incomplete state; no cleanup is attempted.
+
+If the keyword param 'filterfn' is defined it's invoked as a sub that
+returns true if the file/directory should be copied, false otherwise.
+The passed path is the full path to the file relative to the source
+directory.
+
+e.g.
+
+RecursiveCopy::copypath('/some/path', '/empty/dir',
+	filterfn => sub {^
+		# omit children of pg_log
+		my $src = shift;
+		return ! $src ~= /\/pg_log\//
+	}
+);
+
+=cut
+
 sub copypath
 {
-	my $srcpath  = shift;
-	my $destpath = shift;
+	my ($srcpath, $destpath, %params) = @_;
+
+	die("if specified, 'filterfn' must be a sub ref")
+	  if defined $params{filterfn} && !ref $params{filterfn};
+
+	my $filterfn;
+	if (defined $params{filterfn})
+	{
+		$filterfn = $params{filterfn};
+	}
+	else
+	{
+		$filterfn = sub { return 1; };
+	}
+
+	return _copypath_recurse($srcpath, $destpath, $filterfn);
+}
+
+# Recursive private guts of copypath
+sub _copypath_recurse
+{
+	my ($srcpath, $destpath, $filterfn) = @_;
 
 	die "Cannot operate on symlinks" if -l $srcpath or -l $destpath;
 
@@ -19,8 +79,11 @@ sub copypath
 	die "Destination path $destpath exists as file" if -f $destpath;
 	if (-f $srcpath)
 	{
-		copy($srcpath, $destpath)
-		  or die "copy $srcpath -> $destpath failed: $!";
+		if ($filterfn->($srcpath))
+		{
+			copy($srcpath, $destpath)
+			  or die "copy $srcpath -> $destpath failed: $!";
+		}
 		return 1;
 	}
 
@@ -32,7 +95,8 @@ sub copypath
 	while (my $entry = readdir($directory))
 	{
 		next if ($entry eq '.' || $entry eq '..');
-		RecursiveCopy::copypath("$srcpath/$entry", "$destpath/$entry")
+		RecursiveCopy::_copypath_recurse("$srcpath/$entry",
+			"$destpath/$entry", $filterfn)
 		  or die "copypath $srcpath/$entry -> $destpath/$entry failed";
 	}
 	closedir($directory);
-- 
2.1.0

From bf7e72c64f5f1b727ae5f3463ffb544959e57259 Mon Sep 17 00:00:00 2001
From: Craig Ringer <craig@2ndquadrant.com>
Date: Tue, 1 Mar 2016 21:08:21 +0800
Subject: [PATCH 5/7] TAP: Add easier, more flexible ways to invoke psql

The PostgresNode::psql method is limited - it offers no access
to the return code from psql, ignores SQL errors, and offers
no access to psql's stderr.

Provide a new psql_expert that addresses those limitations and
can be used more flexibly - see the embedded PerlDoc for details.

Also add a new psql_check method that invokes psql and dies if
the SQL fails with any error. Test scripts should use this so
they automatically die if SQL that should succeed fails instead;
with the current psql method such failures would go undetected.
---
 src/test/perl/PostgresNode.pm | 289 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 277 insertions(+), 12 deletions(-)

diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 7c8e66e..115b9e3 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -22,8 +22,20 @@ PostgresNode - class representing PostgreSQL server instance
   $node->restart('fast');
 
   # run a query with psql
-  # like: psql -qAXt postgres -c 'SELECT 1;'
-  $psql_stdout = $node->psql('postgres', 'SELECT 1');
+  # like:
+  #   echo 'SELECT 1' | psql -qAXt postgres -v ON_ERROR_STOP=1
+  $psql_stdout = $node->psql_check('postgres', 'SELECT 1');
+
+  # Run psql with a timeout, capturing stdout and stderr
+  # as well as the psql exit code. Pass some extra psql
+  # options. If there's an error from psql raise an exception.
+  my ($stdout, $stderr, $timed_out);
+  my $cmdret = $psql_expert('postgres', 'SELECT pg_sleep(60)',
+	  stdout => \$stdout, stderr => \$stderr,
+	  timeout => 30, timed_out => \$timed_out,
+	  extra_params => ['--single-transaction'],
+	  on_error_die => 1)
+  print "Sleep timed out" if $timed_out;
 
   # run query every second until it returns 't'
   # or times out
@@ -70,6 +82,7 @@ use IPC::Run;
 use RecursiveCopy;
 use Test::More;
 use TestLib ();
+use Scalar::Util qw(blessed);
 
 our @EXPORT = qw(
   get_new_node
@@ -782,11 +795,16 @@ sub teardown_node
 
 =item $node->psql(dbname, sql)
 
-Run a query with psql and return stdout, or on error print stderr.
+Run a query with psql and return stdout if psql returns with no error.
 
-Executes a query/script with psql and returns psql's standard output.  psql is
-run in unaligned tuples-only quiet mode with psqlrc disabled so simple queries
-will just return the result row(s) with fields separated by commas.
+psql is run in unaligned tuples-only quiet mode with psqlrc disabled so simple
+queries will just return the result row(s) with fields separated by commas.
+
+If any stderr output is generated it is printed and discarded.
+
+Nonzero return codes from psql are ignored and discarded.
+
+Use psql_expert for more control.
 
 =cut
 
@@ -795,24 +813,271 @@ sub psql
 	my ($self, $dbname, $sql) = @_;
 
 	my ($stdout, $stderr);
+
 	my $name = $self->name;
 	print("### Running SQL command on node \"$name\": $sql\n");
 
-	IPC::Run::run [ 'psql', '-XAtq', '-d', $self->connstr($dbname), '-f',
-		'-' ], '<', \$sql, '>', \$stdout, '2>', \$stderr
-	  or die;
+	# Run the command, ignoring errors
+	$self->psql_expert(
+		$dbname, $sql,
+		stdout        => \$stdout,
+		stderr        => \$stderr,
+		on_error_die  => 0,
+		on_error_stop => 0);
 
 	if ($stderr ne "")
 	{
 		print "#### Begin standard error\n";
 		print $stderr;
-		print "#### End standard error\n";
+		print "\n#### End standard error\n";
 	}
-	chomp $stdout;
-	$stdout =~ s/\r//g if $Config{osname} eq 'msys';
 	return $stdout;
 }
 
+=pod $node->psql_check($dbname, $sql) => stdout
+
+Invoke 'psql' to run 'sql' on 'dbname' and return its stdout on success.
+Die if the SQL produces an error. Runs with ON_ERROR_STOP set.
+
+Takes optional extra params like timeout and timed_out parameters with the same
+options as psql_expert.
+
+=cut
+
+sub psql_check
+{
+	my ($self, $dbname, $sql, %params) = @_;
+
+	my ($stdout, $stderr);
+
+	my $ret = $self->psql_expert(
+		$dbname, $sql,
+		%params,
+		stdout        => \$stdout,
+		stderr        => \$stderr,
+		on_error_die  => 1,
+		on_error_stop => 1);
+
+	# psql can emit stderr from NOTICEs etc
+	if ($stderr ne "")
+	{
+		print "#### Begin standard error\n";
+		print $stderr;
+		print "\n#### End standard error\n";
+	}
+
+	return $stdout;
+}
+
+=pod $node->psql_expert($dbname, $sql, %params) => psql_retval
+
+Invoke 'psql' to run 'sql' on 'dbname' and return the return value from
+psql, which is run with on_error_stop by default so that it will stop running
+sql and return 3 if the passed SQL results in an error.
+
+psql is invoked in tuples-only unaligned mode with reading of psqlrc disabled. That
+may be overridden by passing extra psql parameters.
+
+stdout and stderr are transformed to unix line endings if on Windows and any
+trailing newline is removed.
+
+Dies on failure to invoke psql but not if psql exits with a nonzero return code
+(unless on_error_die specified). Dies if psql exits with a signal.
+
+=over
+
+=item stdout => \$stdout
+
+If a scalar to write stdout to is passed as the keyword parameter 'stdout' it
+will be set to psql's stdout. If unset stdout is not redirected and will be
+printed to Perl's stdout unless psql is called in array context, in which case
+it's captured and returned.
+
+=item stderr => \$stderr
+
+Same as 'stdout' but gets stderr. If the same scalar is passed for both stdout
+and stderr the results may be interleaved unpredictably.
+
+=item on_error_stop => 1
+
+By default psql_expert invokes psql with ON_ERROR_STOP=1 set so it will
+stop executing SQL at the first error and return exit code 2. If you want
+to ignore errors, pass 0 to on_error_stop.
+
+=item on_error_die => 0
+
+By default psql_expert returns psql's result code. Pass on_error_die to instead
+die with an informative message.
+
+=item timeout => 'interval'
+
+Set a timeout for the psql call as an interval accepted by IPC::Run::timer.
+Integer seconds is fine. psql_expert dies with an exception on timeout unless
+the timed_out parameter is passed.
+
+=item timed_out => \$timed_out
+
+Keyword parameter. Pass a scalar reference and it'll be set to true if the psql
+call timed out instead of dying. Has no effect unless 'timeout' is set.
+
+=item extra_params => ['--single-transaction']
+
+Pass additional parameters to psql. Must be an arrayref.
+
+=back
+
+e.g.
+
+	my ($stdout, $stderr, $timed_out);
+	my $cmdret = $psql_expert('postgres', 'SELECT pg_sleep(60)',
+		stdout => \$stdout, stderr => \$stderr,
+		timeout => 30, timed_out => \$timed_out,
+		extra_params => ['--single-transaction'])
+
+will set $cmdret to undef and $timed_out to a true value.
+
+	$psql_expert('postgres', $sql, on_error_die => 1);
+
+dies with an informative message if $sql fails.
+
+=cut
+
+sub psql_expert
+{
+	my ($self, $dbname, $sql, %params) = @_;
+
+	my $stdout            = $params{stdout};
+	my $stderr            = $params{stderr};
+	my $timeout           = undef;
+	my $timeout_exception = 'psql timed out';
+	my @psql_params =
+	  ('psql', '-XAtq', '-d', $self->connstr($dbname), '-f', '-');
+
+	# If the caller wants an array and hasn't passed stdout/stderr
+	# references, allocate temporary ones to capture them so we
+	# can return them. Otherwise we won't redirect them at all.
+	if (wantarray)
+	{
+		if (!defined($stdout))
+		{
+			my $temp_stdout = "";
+			$stdout = \$temp_stdout;
+		}
+		if (!defined($stderr))
+		{
+			my $temp_stderr = "";
+			$stderr = \$temp_stderr;
+		}
+	}
+
+	$params{on_error_stop} = 1 unless defined $params{on_error_stop};
+	$params{on_error_die}  = 0 unless defined $params{on_error_die};
+
+	push @psql_params, '-v', 'ON_ERROR_STOP=1' if $params{on_error_stop};
+	push @psql_params, @{ $params{extra_params} }
+	  if defined $params{extra_params};
+
+	$timeout =
+	  IPC::Run::timeout($params{timeout}, exception => $timeout_exception)
+	  if (defined($params{timeout}));
+
+	# IPC::Run would otherwise append to existing contents:
+	$$stdout = "" if ref($stdout);
+	$$stderr = "" if ref($stderr);
+
+	my $ret;
+
+   # Perl's exception handling is ... interesting. Especially since we have to
+   # support 5.8.8. So we hide that from the caller, returning true/false for
+   # timeout instead.  See
+   # http://search.cpan.org/~ether/Try-Tiny-0.24/lib/Try/Tiny.pm for
+   # background.
+	my $error = do
+	{
+		local $@;
+		eval {
+			my @ipcrun_opts = (\@psql_params, '<', \$sql);
+			push @ipcrun_opts, '>', $stdout if defined $stdout;
+			push @ipcrun_opts, '2>', $stderr if defined $stderr;
+			push @ipcrun_opts, $timeout if defined $timeout;
+
+			IPC::Run::run @ipcrun_opts;
+			$ret = $?;
+		};
+		my $exc_save = $@;
+		if ($exc_save)
+		{
+			# IPC::Run::run threw an exception. re-throw unless it's a
+			# timeout, which we'll handle by testing is_expired
+			if (blessed($exc_save) || $exc_save ne $timeout_exception)
+			{
+				print
+"Exception from IPC::Run::run when invoking psql: '$exc_save'\n";
+				die $exc_save;
+			}
+			else
+			{
+				$ret = undef;
+
+				die
+				  "Got timeout exception '$exc_save' but timer not expired?!"
+				  unless $timeout->is_expired;
+
+				if (defined($params{timed_out}))
+				{
+					${ $params{timed_out} } = 1;
+				}
+				else
+				{
+					die
+"psql timed out while running '@psql_params', stderr '$$stderr'";
+				}
+			}
+		}
+	};
+
+	if (defined $$stdout)
+	{
+	  chomp $$stdout;
+	  $$stdout =~ s/\r//g if $Config{osname} eq 'msys';
+	}
+
+	if (defined $$stderr)
+	{
+	  chomp $$stderr;
+	  $$stderr =~ s/\r//g if $Config{osname} eq 'msys';
+	}
+
+	# See http://perldoc.perl.org/perlvar.html#%24CHILD_ERROR
+	# We don't use IPC::Run::Simple to limit dependencies.
+	#
+	# We always die on signal. If someone wants to capture signals
+	# to psql we can return it with another reference out parameter.
+	die "psql exited with signal "
+	  . ($ret & 127)
+	  . ": '$$stderr' while running '@psql_params'"
+	  if $ret & 127;
+	die "psql exited with core dump: '$$stderr' while running '@psql_params'"
+	  if $ret & 128;
+	$ret = $ret >> 8;
+
+	if ($ret && $params{on_error_die})
+	{
+		die
+"psql command line syntax error or internal error: '$$stderr' while running '@psql_params'"
+		  if $ret == 1;
+		die "psql connection error: '$$stderr' while running '@psql_params'"
+		  if $ret == 2;
+		die
+"error when running passed SQL: '$$stderr' while running '@psql_params'"
+		  if $ret == 3;
+		die
+"unexpected error code $ret from psql: '$$stderr' while running '@psql_params'";
+	}
+
+	return $ret;
+}
+
 =pod
 
 =item $node->poll_query_until(dbname, query)
-- 
2.1.0

From e61b696033698c60df4186d5cb939621124ea09a Mon Sep 17 00:00:00 2001
From: Craig Ringer <craig@2ndquadrant.com>
Date: Thu, 3 Mar 2016 13:40:53 +0800
Subject: [PATCH 6/7] TAP: Rename psql_expert to psql and use it in all tests

Make TAP tests check the exit code of psql by default, modifying
them in a few places where failures are actually expected.
---
 src/bin/pg_basebackup/t/010_pg_basebackup.pl  | 22 ++++-----
 src/bin/pgbench/t/001_pgbench.pl              |  2 +-
 src/bin/scripts/t/010_clusterdb.pl            |  2 +-
 src/bin/scripts/t/030_createlang.pl           |  2 +-
 src/bin/scripts/t/050_dropdb.pl               |  2 +-
 src/bin/scripts/t/070_dropuser.pl             |  2 +-
 src/bin/scripts/t/090_reindexdb.pl            |  2 +-
 src/test/modules/commit_ts/t/001_base.pl      |  8 +--
 src/test/modules/commit_ts/t/002_standby.pl   | 22 +++++----
 src/test/modules/commit_ts/t/003_standby_2.pl | 20 ++++----
 src/test/perl/PostgresNode.pm                 | 71 +++++++--------------------
 src/test/recovery/t/001_stream_rep.pl         | 20 ++------
 src/test/recovery/t/002_archiving.pl          | 10 ++--
 src/test/recovery/t/003_recovery_targets.pl   | 24 ++++-----
 src/test/recovery/t/004_timeline_switch.pl    | 10 ++--
 src/test/recovery/t/005_replay_delay.pl       | 10 ++--
 16 files changed, 95 insertions(+), 134 deletions(-)

diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index a275077..b68ffad 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
@@ -112,9 +112,9 @@ SKIP:
 	symlink "$tempdir", $shorter_tempdir;
 
 	mkdir "$tempdir/tblspc1";
-	$node->psql('postgres',
+	$node->psql_check('postgres',
 		"CREATE TABLESPACE tblspc1 LOCATION '$shorter_tempdir/tblspc1';");
-	$node->psql('postgres', "CREATE TABLE test1 (a int) TABLESPACE tblspc1;");
+	$node->psql_check('postgres', "CREATE TABLE test1 (a int) TABLESPACE tblspc1;");
 	$node->command_ok([ 'pg_basebackup', '-D', "$tempdir/tarbackup2", '-Ft' ],
 		'tar format with tablespaces');
 	ok(-f "$tempdir/tarbackup2/base.tar", 'backup tar was created');
@@ -140,9 +140,9 @@ SKIP:
 	closedir $dh;
 
 	mkdir "$tempdir/tbl=spc2";
-	$node->psql('postgres', "DROP TABLE test1;");
-	$node->psql('postgres', "DROP TABLESPACE tblspc1;");
-	$node->psql('postgres',
+	$node->psql_check('postgres', "DROP TABLE test1;");
+	$node->psql_check('postgres', "DROP TABLESPACE tblspc1;");
+	$node->psql_check('postgres',
 		"CREATE TABLESPACE tblspc2 LOCATION '$shorter_tempdir/tbl=spc2';");
 	$node->command_ok(
 		[   'pg_basebackup', '-D', "$tempdir/backup3", '-Fp',
@@ -150,15 +150,15 @@ SKIP:
 		'mapping tablespace with = sign in path');
 	ok(-d "$tempdir/tbackup/tbl=spc2",
 		'tablespace with = sign was relocated');
-	$node->psql('postgres', "DROP TABLESPACE tblspc2;");
+	$node->psql_check('postgres', "DROP TABLESPACE tblspc2;");
 
 	mkdir "$tempdir/$superlongname";
-	$node->psql('postgres',
+	$node->psql_check('postgres',
 		"CREATE TABLESPACE tblspc3 LOCATION '$tempdir/$superlongname';");
 	$node->command_ok(
 		[ 'pg_basebackup', '-D', "$tempdir/tarbackup_l3", '-Ft' ],
 		'pg_basebackup tar with long symlink target');
-	$node->psql('postgres', "DROP TABLESPACE tblspc3;");
+	$node->psql_check('postgres', "DROP TABLESPACE tblspc3;");
 }
 
 $node->command_ok([ 'pg_basebackup', '-D', "$tempdir/backupR", '-R' ],
@@ -199,9 +199,9 @@ $node->command_fails(
 		'slot1' ],
 	'pg_basebackup fails with nonexistent replication slot');
 
-$node->psql('postgres',
+$node->psql_check('postgres',
 	q{SELECT * FROM pg_create_physical_replication_slot('slot1')});
-my $lsn = $node->psql('postgres',
+my $lsn = $node->psql_check('postgres',
 	q{SELECT restart_lsn FROM pg_replication_slots WHERE slot_name = 'slot1'}
 );
 is($lsn, '', 'restart LSN of new slot is null');
@@ -209,7 +209,7 @@ $node->command_ok(
 	[   'pg_basebackup', '-D', "$tempdir/backupxs_sl", '-X',
 		'stream',        '-S', 'slot1' ],
 	'pg_basebackup -X stream with replication slot runs');
-$lsn = $node->psql('postgres',
+$lsn = $node->psql_check('postgres',
 	q{SELECT restart_lsn FROM pg_replication_slots WHERE slot_name = 'slot1'}
 );
 like($lsn, qr!^0/[0-9A-Z]{7,8}$!, 'restart LSN of slot has advanced');
diff --git a/src/bin/pgbench/t/001_pgbench.pl b/src/bin/pgbench/t/001_pgbench.pl
index 88e83ab..ba4d751 100644
--- a/src/bin/pgbench/t/001_pgbench.pl
+++ b/src/bin/pgbench/t/001_pgbench.pl
@@ -12,7 +12,7 @@ use Test::More tests => 3;
 my $node = get_new_node('main');
 $node->init;
 $node->start;
-$node->psql('postgres',
+$node->psql_check('postgres',
 	    'CREATE UNLOGGED TABLE oid_tbl () WITH OIDS; '
 	  . 'ALTER TABLE oid_tbl ADD UNIQUE (oid);');
 my $script = $node->basedir . '/pgbench_script';
diff --git a/src/bin/scripts/t/010_clusterdb.pl b/src/bin/scripts/t/010_clusterdb.pl
index 11d678a..0665262 100644
--- a/src/bin/scripts/t/010_clusterdb.pl
+++ b/src/bin/scripts/t/010_clusterdb.pl
@@ -21,7 +21,7 @@ $node->issues_sql_like(
 $node->command_fails([ 'clusterdb', '-t', 'nonexistent' ],
 	'fails with nonexistent table');
 
-$node->psql('postgres',
+$node->psql_check('postgres',
 'CREATE TABLE test1 (a int); CREATE INDEX test1x ON test1 (a); CLUSTER test1 USING test1x'
 );
 $node->issues_sql_like(
diff --git a/src/bin/scripts/t/030_createlang.pl b/src/bin/scripts/t/030_createlang.pl
index 38e3516..783db45 100644
--- a/src/bin/scripts/t/030_createlang.pl
+++ b/src/bin/scripts/t/030_createlang.pl
@@ -16,7 +16,7 @@ $node->start;
 $node->command_fails([ 'createlang', 'plpgsql' ],
 	'fails if language already exists');
 
-$node->psql('postgres', 'DROP EXTENSION plpgsql');
+$node->psql_check('postgres', 'DROP EXTENSION plpgsql');
 $node->issues_sql_like(
 	[ 'createlang', 'plpgsql' ],
 	qr/statement: CREATE EXTENSION "plpgsql"/,
diff --git a/src/bin/scripts/t/050_dropdb.pl b/src/bin/scripts/t/050_dropdb.pl
index fb4f656..31ab9de 100644
--- a/src/bin/scripts/t/050_dropdb.pl
+++ b/src/bin/scripts/t/050_dropdb.pl
@@ -13,7 +13,7 @@ my $node = get_new_node('main');
 $node->init;
 $node->start;
 
-$node->psql('postgres', 'CREATE DATABASE foobar1');
+$node->psql_check('postgres', 'CREATE DATABASE foobar1');
 $node->issues_sql_like(
 	[ 'dropdb', 'foobar1' ],
 	qr/statement: DROP DATABASE foobar1/,
diff --git a/src/bin/scripts/t/070_dropuser.pl b/src/bin/scripts/t/070_dropuser.pl
index 22079f6..8d25374 100644
--- a/src/bin/scripts/t/070_dropuser.pl
+++ b/src/bin/scripts/t/070_dropuser.pl
@@ -13,7 +13,7 @@ my $node = get_new_node('main');
 $node->init;
 $node->start;
 
-$node->psql('postgres', 'CREATE ROLE foobar1');
+$node->psql_check('postgres', 'CREATE ROLE foobar1');
 $node->issues_sql_like(
 	[ 'dropuser', 'foobar1' ],
 	qr/statement: DROP ROLE foobar1/,
diff --git a/src/bin/scripts/t/090_reindexdb.pl b/src/bin/scripts/t/090_reindexdb.pl
index 7f57af8..437fbae 100644
--- a/src/bin/scripts/t/090_reindexdb.pl
+++ b/src/bin/scripts/t/090_reindexdb.pl
@@ -20,7 +20,7 @@ $node->issues_sql_like(
 	qr/statement: REINDEX DATABASE postgres;/,
 	'SQL REINDEX run');
 
-$node->psql('postgres',
+$node->psql_check('postgres',
 	'CREATE TABLE test1 (a int); CREATE INDEX test1x ON test1 (a);');
 $node->issues_sql_like(
 	[ 'reindexdb', '-t', 'test1', 'postgres' ],
diff --git a/src/test/modules/commit_ts/t/001_base.pl b/src/test/modules/commit_ts/t/001_base.pl
index 122b515..f227186 100644
--- a/src/test/modules/commit_ts/t/001_base.pl
+++ b/src/test/modules/commit_ts/t/001_base.pl
@@ -13,17 +13,17 @@ $node->append_conf('postgresql.conf', 'track_commit_timestamp = on');
 $node->start;
 
 # Create a table, compare "now()" to the commit TS of its xmin
-$node->psql('postgres', 'create table t as select now from (select now(), pg_sleep(1)) f');
-my $true = $node->psql('postgres',
+$node->psql_check('postgres', 'create table t as select now from (select now(), pg_sleep(1)) f');
+my $true = $node->psql_check('postgres',
 	'select t.now - ts.* < \'1s\' from t, pg_class c, pg_xact_commit_timestamp(c.xmin) ts where relname = \'t\'');
 is($true, 't', 'commit TS is set');
-my $ts = $node->psql('postgres',
+my $ts = $node->psql_check('postgres',
 	'select ts.* from pg_class, pg_xact_commit_timestamp(xmin) ts where relname = \'t\'');
 
 # Verify that we read the same TS after crash recovery
 $node->stop('immediate');
 $node->start;
 
-my $recovered_ts = $node->psql('postgres',
+my $recovered_ts = $node->psql_check('postgres',
 	'select ts.* from pg_class, pg_xact_commit_timestamp(xmin) ts where relname = \'t\'');
 is($recovered_ts, $ts, 'commit TS remains after crash recovery');
diff --git a/src/test/modules/commit_ts/t/002_standby.pl b/src/test/modules/commit_ts/t/002_standby.pl
index 5cc2501..04fde00 100644
--- a/src/test/modules/commit_ts/t/002_standby.pl
+++ b/src/test/modules/commit_ts/t/002_standby.pl
@@ -4,7 +4,7 @@ use strict;
 use warnings;
 
 use TestLib;
-use Test::More tests => 2;
+use Test::More tests => 4;
 use PostgresNode;
 
 my $bkplabel = 'backup';
@@ -25,31 +25,33 @@ $standby->start;
 
 for my $i (1 .. 10)
 {
-	$master->psql('postgres', "create table t$i()");
+	$master->psql_check('postgres', "create table t$i()");
 }
-my $master_ts = $master->psql('postgres',
+my $master_ts = $master->psql_check('postgres',
 	qq{SELECT ts.* FROM pg_class, pg_xact_commit_timestamp(xmin) AS ts WHERE relname = 't10'});
-my $master_lsn = $master->psql('postgres',
+my $master_lsn = $master->psql_check('postgres',
 	'select pg_current_xlog_location()');
 $standby->poll_query_until('postgres',
 	qq{SELECT '$master_lsn'::pg_lsn <= pg_last_xlog_replay_location()})
 	or die "slave never caught up";
 
-my $standby_ts = $standby->psql('postgres',
+my $standby_ts = $standby->psql_check('postgres',
 	qq{select ts.* from pg_class, pg_xact_commit_timestamp(xmin) ts where relname = 't10'});
 is($master_ts, $standby_ts, "standby gives same value as master");
 
 $master->append_conf('postgresql.conf', 'track_commit_timestamp = off');
 $master->restart;
-$master->psql('postgres', 'checkpoint');
-$master_lsn = $master->psql('postgres',
+$master->psql_check('postgres', 'checkpoint');
+$master_lsn = $master->psql_check('postgres',
 	'select pg_current_xlog_location()');
 $standby->poll_query_until('postgres',
 	qq{SELECT '$master_lsn'::pg_lsn <= pg_last_xlog_replay_location()})
 	or die "slave never caught up";
-$standby->psql('postgres', 'checkpoint');
+$standby->psql_check('postgres', 'checkpoint');
 
 # This one should raise an error now
-$standby_ts = $standby->psql('postgres',
+my ($ret, $standby_ts_stdout, $standby_ts_stderr) = $standby->psql('postgres',
 	'select ts.* from pg_class, pg_xact_commit_timestamp(xmin) ts where relname = \'t10\'');
-is($standby_ts, '', "standby gives no value when master turned feature off");
+is($ret, 3, 'standby errors when master turned feature off');
+is($standby_ts_stdout, '', "standby gives no value when master turned feature off");
+like($standby_ts_stderr, qr/could not get commit timestamp data/, 'expected error when master turned feature off');
diff --git a/src/test/modules/commit_ts/t/003_standby_2.pl b/src/test/modules/commit_ts/t/003_standby_2.pl
index fadb6a2..ba94848 100644
--- a/src/test/modules/commit_ts/t/003_standby_2.pl
+++ b/src/test/modules/commit_ts/t/003_standby_2.pl
@@ -4,7 +4,7 @@ use strict;
 use warnings;
 
 use TestLib;
-use Test::More tests => 2;
+use Test::More tests => 4;
 use PostgresNode;
 
 my $bkplabel = 'backup';
@@ -24,23 +24,25 @@ $standby->start;
 
 for my $i (1 .. 10)
 {
-	$master->psql('postgres', "create table t$i()");
+	$master->psql_check('postgres', "create table t$i()");
 }
 $master->append_conf('postgresql.conf', 'track_commit_timestamp = off');
 $master->restart;
-$master->psql('postgres', 'checkpoint');
-my $master_lsn = $master->psql('postgres',
+$master->psql_check('postgres', 'checkpoint');
+my $master_lsn = $master->psql_check('postgres',
 	'select pg_current_xlog_location()');
 $standby->poll_query_until('postgres',
 	qq{SELECT '$master_lsn'::pg_lsn <= pg_last_xlog_replay_location()})
 	or die "slave never caught up";
 
-$standby->psql('postgres', 'checkpoint');
+$standby->psql_check('postgres', 'checkpoint');
 $standby->restart;
 
-my $standby_ts = $standby->psql('postgres',
+my ($psql_ret, $standby_ts_stdout, $standby_ts_stderr) = $standby->psql('postgres',
 	qq{SELECT ts.* FROM pg_class, pg_xact_commit_timestamp(xmin) AS ts WHERE relname = 't10'});
-is($standby_ts, '', "standby does not return a value after restart");
+is($psql_ret, 3, 'expect error when getting commit timestamp after restart');
+is($standby_ts_stdout, '', "standby does not return a value after restart");
+like($standby_ts_stderr, qr/could not get commit timestamp data/, 'expected err msg after restart');
 
 $master->append_conf('postgresql.conf', 'track_commit_timestamp = on');
 $master->restart;
@@ -50,7 +52,7 @@ $master->restart;
 system_or_bail('pg_ctl', '-w', '-D', $standby->data_dir, 'promote');
 $standby->poll_query_until('postgres', "SELECT pg_is_in_recovery() <> true");
 
-$standby->psql('postgres', "create table t11()");
-$standby_ts = $standby->psql('postgres',
+$standby->psql_check('postgres', "create table t11()");
+my $standby_ts = $standby->psql_check('postgres',
 	qq{SELECT ts.* FROM pg_class, pg_xact_commit_timestamp(xmin) AS ts WHERE relname = 't11'});
 isnt($standby_ts, '', "standby gives valid value ($standby_ts) after promotion");
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 115b9e3..c637a9b 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -30,7 +30,7 @@ PostgresNode - class representing PostgreSQL server instance
   # as well as the psql exit code. Pass some extra psql
   # options. If there's an error from psql raise an exception.
   my ($stdout, $stderr, $timed_out);
-  my $cmdret = $psql_expert('postgres', 'SELECT pg_sleep(60)',
+  my $cmdret = $psql('postgres', 'SELECT pg_sleep(60)',
 	  stdout => \$stdout, stderr => \$stderr,
 	  timeout => 30, timed_out => \$timed_out,
 	  extra_params => ['--single-transaction'],
@@ -791,56 +791,13 @@ sub teardown_node
 	$self->stop('immediate');
 }
 
-=pod
-
-=item $node->psql(dbname, sql)
-
-Run a query with psql and return stdout if psql returns with no error.
-
-psql is run in unaligned tuples-only quiet mode with psqlrc disabled so simple
-queries will just return the result row(s) with fields separated by commas.
-
-If any stderr output is generated it is printed and discarded.
-
-Nonzero return codes from psql are ignored and discarded.
-
-Use psql_expert for more control.
-
-=cut
-
-sub psql
-{
-	my ($self, $dbname, $sql) = @_;
-
-	my ($stdout, $stderr);
-
-	my $name = $self->name;
-	print("### Running SQL command on node \"$name\": $sql\n");
-
-	# Run the command, ignoring errors
-	$self->psql_expert(
-		$dbname, $sql,
-		stdout        => \$stdout,
-		stderr        => \$stderr,
-		on_error_die  => 0,
-		on_error_stop => 0);
-
-	if ($stderr ne "")
-	{
-		print "#### Begin standard error\n";
-		print $stderr;
-		print "\n#### End standard error\n";
-	}
-	return $stdout;
-}
-
 =pod $node->psql_check($dbname, $sql) => stdout
 
 Invoke 'psql' to run 'sql' on 'dbname' and return its stdout on success.
 Die if the SQL produces an error. Runs with ON_ERROR_STOP set.
 
 Takes optional extra params like timeout and timed_out parameters with the same
-options as psql_expert.
+options as psql.
 
 =cut
 
@@ -850,7 +807,7 @@ sub psql_check
 
 	my ($stdout, $stderr);
 
-	my $ret = $self->psql_expert(
+	my $ret = $self->psql(
 		$dbname, $sql,
 		%params,
 		stdout        => \$stdout,
@@ -869,12 +826,15 @@ sub psql_check
 	return $stdout;
 }
 
-=pod $node->psql_expert($dbname, $sql, %params) => psql_retval
+=pod $node->psql($dbname, $sql, %params) => psql_retval
 
 Invoke 'psql' to run 'sql' on 'dbname' and return the return value from
 psql, which is run with on_error_stop by default so that it will stop running
 sql and return 3 if the passed SQL results in an error.
 
+As a convenience, if psql is called in array context it returns a ($retval,
+$stdout, $stderr) tuple.
+
 psql is invoked in tuples-only unaligned mode with reading of psqlrc disabled. That
 may be overridden by passing extra psql parameters.
 
@@ -900,19 +860,19 @@ and stderr the results may be interleaved unpredictably.
 
 =item on_error_stop => 1
 
-By default psql_expert invokes psql with ON_ERROR_STOP=1 set so it will
+By default psql invokes psql with ON_ERROR_STOP=1 set so it will
 stop executing SQL at the first error and return exit code 2. If you want
 to ignore errors, pass 0 to on_error_stop.
 
 =item on_error_die => 0
 
-By default psql_expert returns psql's result code. Pass on_error_die to instead
+By default psql returns psql's result code. Pass on_error_die to instead
 die with an informative message.
 
 =item timeout => 'interval'
 
 Set a timeout for the psql call as an interval accepted by IPC::Run::timer.
-Integer seconds is fine. psql_expert dies with an exception on timeout unless
+Integer seconds is fine. psql dies with an exception on timeout unless
 the timed_out parameter is passed.
 
 =item timed_out => \$timed_out
@@ -942,7 +902,7 @@ dies with an informative message if $sql fails.
 
 =cut
 
-sub psql_expert
+sub psql
 {
 	my ($self, $dbname, $sql, %params) = @_;
 
@@ -1075,7 +1035,14 @@ sub psql_expert
 "unexpected error code $ret from psql: '$$stderr' while running '@psql_params'";
 	}
 
-	return $ret;
+	if (wantarray)
+	{
+		return ($ret, $$stdout, $$stderr);
+	}
+	else
+	{
+		return $ret;
+	}
 }
 
 =pod
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl
index 7dcca65..54de159 100644
--- a/src/test/recovery/t/001_stream_rep.pl
+++ b/src/test/recovery/t/001_stream_rep.pl
@@ -31,7 +31,7 @@ $node_standby_2->init_from_backup($node_standby_1, $backup_name,
 $node_standby_2->start;
 
 # Create some content on master and check its presence in standby 1
-$node_master->psql('postgres',
+$node_master->psql_check('postgres',
 	"CREATE TABLE tab_int AS SELECT generate_series(1,1002) AS a");
 
 # Wait for standbys to catch up
@@ -47,24 +47,14 @@ $node_standby_1->poll_query_until('postgres', $caughtup_query)
   or die "Timed out while waiting for standby 2 to catch up";
 
 my $result =
-  $node_standby_1->psql('postgres', "SELECT count(*) FROM tab_int");
+  $node_standby_1->psql_check('postgres', "SELECT count(*) FROM tab_int");
 print "standby 1: $result\n";
 is($result, qq(1002), 'check streamed content on standby 1');
 
-$result = $node_standby_2->psql('postgres', "SELECT count(*) FROM tab_int");
+$result = $node_standby_2->psql_check('postgres', "SELECT count(*) FROM tab_int");
 print "standby 2: $result\n";
 is($result, qq(1002), 'check streamed content on standby 2');
 
 # Check that only READ-only queries can run on standbys
-$node_standby_1->command_fails(
-	[   'psql', '-A',
-		'-t',   '--no-psqlrc',
-		'-d',   $node_standby_1->connstr,
-		'-c',   "INSERT INTO tab_int VALUES (1)" ],
-	'Read-only queries on standby 1');
-$node_standby_2->command_fails(
-	[   'psql', '-A',
-		'-t',   '--no-psqlrc',
-		'-d',   $node_standby_2->connstr,
-		'-c',   "INSERT INTO tab_int VALUES (1)" ],
-	'Read-only queries on standby 2');
+is($node_standby_1->psql('postgres', 'INSERT INTO tab_int VALUES (1)'), 3, 'Read-only queries on standby 1');
+is($node_standby_2->psql('postgres', 'INSERT INTO tab_int VALUES (1)'), 3, 'Read-only queries on standby 2');
diff --git a/src/test/recovery/t/002_archiving.pl b/src/test/recovery/t/002_archiving.pl
index 67bb7df..d2eb01b 100644
--- a/src/test/recovery/t/002_archiving.pl
+++ b/src/test/recovery/t/002_archiving.pl
@@ -30,16 +30,16 @@ wal_retrieve_retry_interval = '100ms'
 $node_standby->start;
 
 # Create some content on master
-$node_master->psql('postgres',
+$node_master->psql_check('postgres',
 	"CREATE TABLE tab_int AS SELECT generate_series(1,1000) AS a");
 my $current_lsn =
-  $node_master->psql('postgres', "SELECT pg_current_xlog_location();");
+  $node_master->psql_check('postgres', "SELECT pg_current_xlog_location();");
 
 # Force archiving of WAL file to make it present on master
-$node_master->psql('postgres', "SELECT pg_switch_xlog()");
+$node_master->psql_check('postgres', "SELECT pg_switch_xlog()");
 
 # Add some more content, it should not be present on standby
-$node_master->psql('postgres',
+$node_master->psql_check('postgres',
 	"INSERT INTO tab_int VALUES (generate_series(1001,2000))");
 
 # Wait until necessary replay has been done on standby
@@ -48,5 +48,5 @@ my $caughtup_query =
 $node_standby->poll_query_until('postgres', $caughtup_query)
   or die "Timed out while waiting for standby to catch up";
 
-my $result = $node_standby->psql('postgres', "SELECT count(*) FROM tab_int");
+my $result = $node_standby->psql_check('postgres', "SELECT count(*) FROM tab_int");
 is($result, qq(1000), 'check content from archives');
diff --git a/src/test/recovery/t/003_recovery_targets.pl b/src/test/recovery/t/003_recovery_targets.pl
index a0d8b45..7dcb734 100644
--- a/src/test/recovery/t/003_recovery_targets.pl
+++ b/src/test/recovery/t/003_recovery_targets.pl
@@ -38,7 +38,7 @@ sub test_recovery_standby
 
 	# Create some content on master and check its presence in standby
 	my $result =
-	  $node_standby->psql('postgres', "SELECT count(*) FROM tab_int");
+	  $node_standby->psql_check('postgres', "SELECT count(*) FROM tab_int");
 	is($result, qq($num_rows), "check standby content for $test_name");
 
 	# Stop standby node
@@ -54,40 +54,40 @@ $node_master->start;
 
 # Create data before taking the backup, aimed at testing
 # recovery_target = 'immediate'
-$node_master->psql('postgres',
+$node_master->psql_check('postgres',
 	"CREATE TABLE tab_int AS SELECT generate_series(1,1000) AS a");
 my $lsn1 =
-  $node_master->psql('postgres', "SELECT pg_current_xlog_location();");
+  $node_master->psql_check('postgres', "SELECT pg_current_xlog_location();");
 
 # Take backup from which all operations will be run
 $node_master->backup('my_backup');
 
 # Insert some data with used as a replay reference, with a recovery
 # target TXID.
-$node_master->psql('postgres',
+$node_master->psql_check('postgres',
 	"INSERT INTO tab_int VALUES (generate_series(1001,2000))");
-my $recovery_txid = $node_master->psql('postgres', "SELECT txid_current()");
+my $recovery_txid = $node_master->psql_check('postgres', "SELECT txid_current()");
 my $lsn2 =
-  $node_master->psql('postgres', "SELECT pg_current_xlog_location();");
+  $node_master->psql_check('postgres', "SELECT pg_current_xlog_location();");
 
 # More data, with recovery target timestamp
-$node_master->psql('postgres',
+$node_master->psql_check('postgres',
 	"INSERT INTO tab_int VALUES (generate_series(2001,3000))");
-my $recovery_time = $node_master->psql('postgres', "SELECT now()");
+my $recovery_time = $node_master->psql_check('postgres', "SELECT now()");
 my $lsn3 =
-  $node_master->psql('postgres', "SELECT pg_current_xlog_location();");
+  $node_master->psql_check('postgres', "SELECT pg_current_xlog_location();");
 
 # Even more data, this time with a recovery target name
-$node_master->psql('postgres',
+$node_master->psql_check('postgres',
 	"INSERT INTO tab_int VALUES (generate_series(3001,4000))");
 my $recovery_name = "my_target";
 my $lsn4 =
-  $node_master->psql('postgres', "SELECT pg_current_xlog_location();");
+  $node_master->psql_check('postgres', "SELECT pg_current_xlog_location();");
 $node_master->psql_check('postgres',
 	"SELECT pg_create_restore_point('$recovery_name');");
 
 # Force archiving of WAL file
-$node_master->psql('postgres', "SELECT pg_switch_xlog()");
+$node_master->psql_check('postgres', "SELECT pg_switch_xlog()");
 
 # Test recovery targets
 my @recovery_params = ("recovery_target = 'immediate'");
diff --git a/src/test/recovery/t/004_timeline_switch.pl b/src/test/recovery/t/004_timeline_switch.pl
index a180b94..e549680 100644
--- a/src/test/recovery/t/004_timeline_switch.pl
+++ b/src/test/recovery/t/004_timeline_switch.pl
@@ -30,10 +30,10 @@ $node_standby_2->init_from_backup($node_master, $backup_name,
 $node_standby_2->start;
 
 # Create some content on master
-$node_master->psql('postgres',
+$node_master->psql_check('postgres',
 	"CREATE TABLE tab_int AS SELECT generate_series(1,1000) AS a");
 my $until_lsn =
-  $node_master->psql('postgres', "SELECT pg_current_xlog_location();");
+  $node_master->psql_check('postgres', "SELECT pg_current_xlog_location();");
 
 # Wait until standby has replayed enough data on standby 1
 my $caughtup_query =
@@ -61,15 +61,15 @@ $node_standby_2->restart;
 # to exit recovery first before moving on with the test.
 $node_standby_1->poll_query_until('postgres',
 	"SELECT pg_is_in_recovery() <> true");
-$node_standby_1->psql('postgres',
+$node_standby_1->psql_check('postgres',
 	"INSERT INTO tab_int VALUES (generate_series(1001,2000))");
 $until_lsn =
-  $node_standby_1->psql('postgres', "SELECT pg_current_xlog_location();");
+  $node_standby_1->psql_check('postgres', "SELECT pg_current_xlog_location();");
 $caughtup_query =
   "SELECT '$until_lsn'::pg_lsn <= pg_last_xlog_replay_location()";
 $node_standby_2->poll_query_until('postgres', $caughtup_query)
   or die "Timed out while waiting for standby to catch up";
 
 my $result =
-  $node_standby_2->psql('postgres', "SELECT count(*) FROM tab_int");
+  $node_standby_2->psql_check('postgres', "SELECT count(*) FROM tab_int");
 is($result, qq(2000), 'check content of standby 2');
diff --git a/src/test/recovery/t/005_replay_delay.pl b/src/test/recovery/t/005_replay_delay.pl
index 401d17b..d8e20c0 100644
--- a/src/test/recovery/t/005_replay_delay.pl
+++ b/src/test/recovery/t/005_replay_delay.pl
@@ -11,7 +11,7 @@ $node_master->init(allows_streaming => 1);
 $node_master->start;
 
 # And some content
-$node_master->psql('postgres',
+$node_master->psql_check('postgres',
 	"CREATE TABLE tab_int AS SELECT generate_series(1,10) AS a");
 
 # Take backup
@@ -30,20 +30,20 @@ $node_standby->start;
 
 # Make new content on master and check its presence in standby
 # depending on the delay of 2s applied above.
-$node_master->psql('postgres',
+$node_master->psql_check('postgres',
 	"INSERT INTO tab_int VALUES (generate_series(11,20))");
 sleep 1;
 
 # Here we should have only 10 rows
-my $result = $node_standby->psql('postgres', "SELECT count(*) FROM tab_int");
+my $result = $node_standby->psql_check('postgres', "SELECT count(*) FROM tab_int");
 is($result, qq(10), 'check content with delay of 1s');
 
 # Now wait for replay to complete on standby
 my $until_lsn =
-  $node_master->psql('postgres', "SELECT pg_current_xlog_location();");
+  $node_master->psql_check('postgres', "SELECT pg_current_xlog_location();");
 my $caughtup_query =
   "SELECT '$until_lsn'::pg_lsn <= pg_last_xlog_replay_location()";
 $node_standby->poll_query_until('postgres', $caughtup_query)
   or die "Timed out while waiting for standby to catch up";
-$result = $node_standby->psql('postgres', "SELECT count(*) FROM tab_int");
+$result = $node_standby->psql_check('postgres', "SELECT count(*) FROM tab_int");
 is($result, qq(20), 'check content with delay of 2s');
-- 
2.1.0

From 91d6718c9a31c9f7e50090867a8784072a963a89 Mon Sep 17 00:00:00 2001
From: Craig Ringer <craig@2ndquadrant.com>
Date: Tue, 1 Mar 2016 21:21:25 +0800
Subject: [PATCH 7/7] TAP: Add support for taking filesystem level backups

---
 src/test/perl/PostgresNode.pm | 88 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 76 insertions(+), 12 deletions(-)

diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index c637a9b..ba74aa5 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -461,11 +461,86 @@ sub backup
 	my $port        = $self->port;
 	my $name        = $self->name;
 
-	print "# Taking backup $backup_name from node \"$name\"\n";
+	print "# Taking pg_basebackup $backup_name from node \"$name\"\n";
 	TestLib::system_or_bail("pg_basebackup -D $backup_path -p $port -x");
 	print "# Backup finished\n";
 }
 
+=item $node->backup_fs_hot(backup_name)
+
+Create a backup with a filesystem level copy in $node->backup_dir,
+including transaction logs. Archiving must be enabled as pg_start_backup
+and pg_stop_backup are used. This is not checked or enforced.
+
+The backup name is passed as the backup label to pg_start_backup.
+
+=cut
+
+sub backup_fs_hot
+{
+	my ($self, $backup_name) = @_;
+	$self->_backup_fs($backup_name, 1);
+}
+
+=item $node->backup_fs_cold(backup_name)
+
+Create a backup with a filesystem level copy in $node->backup dir,
+including transaction logs. The server must be stopped as no
+attempt to handle concurrent writes is made.
+
+Use backup or backup_fs_hot if you want to back up a running
+server.
+
+=cut
+
+sub backup_fs_cold
+{
+	my ($self, $backup_name) = @_;
+	$self->_backup_fs($backup_name, 0);
+}
+
+
+# Common sub of backup_fs_hot and backup_fs_cold
+sub _backup_fs
+{
+	my ($self, $backup_name, $hot) = @_;
+	my $backup_path = $self->backup_dir . '/' . $backup_name;
+	my $port        = $self->port;
+	my $name        = $self->name;
+
+	print
+	  "# Taking filesystem level backup $backup_name from node \"$name\"\n";
+
+	if ($hot)
+	{
+		my $stdout = $self->psql_check('postgres',
+			"SELECT * FROM pg_start_backup('$backup_name');");
+		print "# pg_start_backup: $stdout\n";
+	}
+
+	RecursiveCopy::copypath(
+		$self->data_dir,
+		$backup_path,
+		filterfn => sub {
+			my $src = shift;
+			return $src !~ /\/pg_log\// && $src !~ /\/postmaster.pid$/;
+		});
+
+	if ($hot)
+	{
+		# We ignore pg_stop_backup's return value. We also assume archiving
+		# is enabled; otherwise the caller will have to copy the remaining
+		# segments.
+		my $stdout =
+		  $self->psql_check('postgres', 'SELECT * FROM pg_stop_backup();');
+		print "# pg_stop_backup: $stdout\n";
+	}
+
+	print "# Backup finished\n";
+}
+
+
+
 =pod
 
 =item $node->init_from_backup(root_node, backup_name)
@@ -888,17 +963,6 @@ Pass additional parameters to psql. Must be an arrayref.
 
 e.g.
 
-	my ($stdout, $stderr, $timed_out);
-	my $cmdret = $psql_expert('postgres', 'SELECT pg_sleep(60)',
-		stdout => \$stdout, stderr => \$stderr,
-		timeout => 30, timed_out => \$timed_out,
-		extra_params => ['--single-transaction'])
-
-will set $cmdret to undef and $timed_out to a true value.
-
-	$psql_expert('postgres', $sql, on_error_die => 1);
-
-dies with an informative message if $sql fails.
 
 =cut
 
-- 
2.1.0

From b7158678086f137eaa38782aadb51ec16c44871b Mon Sep 17 00:00:00 2001
From: Craig Ringer <craig@2ndquadrant.com>
Date: Fri, 4 Mar 2016 20:40:43 +0800
Subject: [PATCH] TAP: Correctly initialized $timed_out if passed

---
 src/test/perl/PostgresNode.pm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index f0f70ea..ea4fb63 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -1025,6 +1025,8 @@ sub psql
 	  IPC::Run::timeout($params{timeout}, exception => $timeout_exception)
 	  if (defined($params{timeout}));
 
+	${$params{timed_out}} = 0 if defined $params{timed_out};
+
 	# IPC::Run would otherwise append to existing contents:
 	$$stdout = "" if ref($stdout);
 	$$stderr = "" if ref($stderr);
-- 
2.1.0

From c1ee29e350d9532ae9d536252a0ebefc136c4a39 Mon Sep 17 00:00:00 2001
From: Craig Ringer <craig@2ndquadrant.com>
Date: Fri, 4 Mar 2016 20:40:43 +0800
Subject: [PATCH] TAP: Correctly initialize $timed_out if passed

Corrects an oversight of mine in 2c83f435a3 (rework PostgresNode's psql
method) where the $timed_out reference var isn't correctly initialized.
This doesn't affect any existing test code in the tree.
---
 src/test/perl/PostgresNode.pm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 090d48c..3a740ec 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -1032,6 +1032,8 @@ sub psql
 	  IPC::Run::timeout($params{timeout}, exception => $timeout_exception)
 	  if (defined($params{timeout}));
 
+	${$params{timed_out}} = 0 if defined $params{timed_out};
+
 	# IPC::Run would otherwise append to existing contents:
 	$$stdout = "" if ref($stdout);
 	$$stderr = "" if ref($stderr);
-- 
2.1.0