Rework PostgresNode's psql method

This makes the psql() method much more capable: it captures both stdout
and stderr; it now returns the psql exit code rather than stdout; a
timeout can now be specified, as can ON_ERROR_STOP behavior; it gained a
new "on_error_die" (defaulting to off) parameter to raise an exception
if there's any problem.  Finally, additional parameters to psql can be
passed if there's need for further tweaking.

For convenience, a new safe_psql() method retains much of the old
behavior of psql(), except that it uses on_error_die on, so that
problems like syntax errors in SQL commands can be detected more easily.

Many existing TAP test files now use safe_psql, which is what is really
wanted.  A couple of ->psql() calls are now added in the commit_ts
tests, which verify that the right thing is happening on certain errors.
Some ->command_fails() calls in recovery tests that were verifying that
psql failed also became ->psql() calls now.

Author: Craig Ringer. Some tweaks by Álvaro Herrera
Reviewed-By: Michaël Paquier

Author: alvherre

src/bin/pg_basebackup/t/010_pg_basebackup.pl

@@ -112,9 +112,9 @@
 	symlink "$tempdir", $shorter_tempdir;
 
 	mkdir "$tempdir/tblspc1";
-	$node->psql('postgres',
+	$node->safe_psql('postgres',
 		"CREATE TABLESPACE tblspc1 LOCATION '$shorter_tempdir/tblspc1';");
-	$node->psql('postgres', "CREATE TABLE test1 (a int) TABLESPACE tblspc1;");
+	$node->safe_psql('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,25 +140,25 @@
 	closedir $dh;
 
 	mkdir "$tempdir/tbl=spc2";
-	$node->psql('postgres', "DROP TABLE test1;");
-	$node->psql('postgres', "DROP TABLESPACE tblspc1;");
-	$node->psql('postgres',
+	$node->safe_psql('postgres', "DROP TABLE test1;");
+	$node->safe_psql('postgres', "DROP TABLESPACE tblspc1;");
+	$node->safe_psql('postgres',
 		"CREATE TABLESPACE tblspc2 LOCATION '$shorter_tempdir/tbl=spc2';");
 	$node->command_ok(
 		[   'pg_basebackup', '-D', "$tempdir/backup3", '-Fp',
 			"-T$shorter_tempdir/tbl\\=spc2=$tempdir/tbackup/tbl\\=spc2" ],
 		'mapping tablespace with = sign in path');
 	ok(-d "$tempdir/tbackup/tbl=spc2",
 		'tablespace with = sign was relocated');
-	$node->psql('postgres', "DROP TABLESPACE tblspc2;");
+	$node->safe_psql('postgres', "DROP TABLESPACE tblspc2;");
 
 	mkdir "$tempdir/$superlongname";
-	$node->psql('postgres',
+	$node->safe_psql('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->safe_psql('postgres', "DROP TABLESPACE tblspc3;");
 }
 
 $node->command_ok([ 'pg_basebackup', '-D', "$tempdir/backupR", '-R' ],
@@ -199,17 +199,17 @@
 		'slot1' ],
 	'pg_basebackup fails with nonexistent replication slot');
 
-$node->psql('postgres',
+$node->safe_psql('postgres',
 	q{SELECT * FROM pg_create_physical_replication_slot('slot1')});
-my $lsn = $node->psql('postgres',
+my $lsn = $node->safe_psql('postgres',
 	q{SELECT restart_lsn FROM pg_replication_slots WHERE slot_name = 'slot1'}
 );
 is($lsn, '', 'restart LSN of new slot is null');
 $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->safe_psql('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');

src/bin/pgbench/t/001_pgbench.pl

@@ -12,7 +12,7 @@
 my $node = get_new_node('main');
 $node->init;
 $node->start;
-$node->psql('postgres',
+$node->safe_psql('postgres',
 	    'CREATE UNLOGGED TABLE oid_tbl () WITH OIDS; '
 	  . 'ALTER TABLE oid_tbl ADD UNIQUE (oid);');
 my $script = $node->basedir . '/pgbench_script';

src/bin/scripts/t/010_clusterdb.pl

@@ -21,7 +21,7 @@
 $node->command_fails([ 'clusterdb', '-t', 'nonexistent' ],
 	'fails with nonexistent table');
 
-$node->psql('postgres',
+$node->safe_psql('postgres',
 'CREATE TABLE test1 (a int); CREATE INDEX test1x ON test1 (a); CLUSTER test1 USING test1x'
 );
 $node->issues_sql_like(

src/bin/scripts/t/030_createlang.pl

@@ -16,7 +16,7 @@
 $node->command_fails([ 'createlang', 'plpgsql' ],
 	'fails if language already exists');
 
-$node->psql('postgres', 'DROP EXTENSION plpgsql');
+$node->safe_psql('postgres', 'DROP EXTENSION plpgsql');
 $node->issues_sql_like(
 	[ 'createlang', 'plpgsql' ],
 	qr/statement: CREATE EXTENSION "plpgsql"/,

src/bin/scripts/t/050_dropdb.pl

@@ -13,7 +13,7 @@
 $node->init;
 $node->start;
 
-$node->psql('postgres', 'CREATE DATABASE foobar1');
+$node->safe_psql('postgres', 'CREATE DATABASE foobar1');
 $node->issues_sql_like(
 	[ 'dropdb', 'foobar1' ],
 	qr/statement: DROP DATABASE foobar1/,

src/bin/scripts/t/070_dropuser.pl

@@ -13,7 +13,7 @@
 $node->init;
 $node->start;
 
-$node->psql('postgres', 'CREATE ROLE foobar1');
+$node->safe_psql('postgres', 'CREATE ROLE foobar1');
 $node->issues_sql_like(
 	[ 'dropuser', 'foobar1' ],
 	qr/statement: DROP ROLE foobar1/,

src/bin/scripts/t/090_reindexdb.pl

@@ -20,7 +20,7 @@
 	qr/statement: REINDEX DATABASE postgres;/,
 	'SQL REINDEX run');
 
-$node->psql('postgres',
+$node->safe_psql('postgres',
 	'CREATE TABLE test1 (a int); CREATE INDEX test1x ON test1 (a);');
 $node->issues_sql_like(
 	[ 'reindexdb', '-t', 'test1', 'postgres' ],

src/test/modules/commit_ts/t/001_base.pl

@@ -13,17 +13,17 @@
 $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->safe_psql('postgres', 'create table t as select now from (select now(), pg_sleep(1)) f');
+my $true = $node->safe_psql('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->safe_psql('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->safe_psql('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');

src/test/modules/commit_ts/t/002_standby.pl

@@ -4,7 +4,7 @@
 use warnings;
 
 use TestLib;
-use Test::More tests => 2;
+use Test::More tests => 4;
 use PostgresNode;
 
 my $bkplabel = 'backup';
@@ -25,31 +25,33 @@
 
 for my $i (1 .. 10)
 {
-	$master->psql('postgres', "create table t$i()");
+	$master->safe_psql('postgres', "create table t$i()");
 }
-my $master_ts = $master->psql('postgres',
+my $master_ts = $master->safe_psql('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->safe_psql('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->safe_psql('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->safe_psql('postgres', 'checkpoint');
+$master_lsn = $master->safe_psql('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->safe_psql('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');

src/test/modules/commit_ts/t/003_standby_2.pl

@@ -4,7 +4,7 @@
 use warnings;
 
 use TestLib;
-use Test::More tests => 2;
+use Test::More tests => 4;
 use PostgresNode;
 
 my $bkplabel = 'backup';
@@ -24,23 +24,25 @@
 
 for my $i (1 .. 10)
 {
-	$master->psql('postgres', "create table t$i()");
+	$master->safe_psql('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->safe_psql('postgres', 'checkpoint');
+my $master_lsn = $master->safe_psql('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->safe_psql('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 @@
 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->safe_psql('postgres', "create table t11()");
+my $standby_ts = $standby->safe_psql('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");