That looks pretty solid to me, and I can confirm that it passes
on wrasse's host.  The only nit I can find to pick is that this:

+       usleep(10_000) until -s "$tempdir/psql.pid" or ($count++ > 180 * 100 
and die 'pid file did not appear');

basically assumes that psql.pid will be written atomically.

It'd be marginally safer to wait till psql.pid can be seen to
contain a newline.  I don't think that would be too hard to do,
if you put the slurp_file call inside the wait loop and inspect
its result.

I finally came around to have a look at the patch.

For the issue raised above, I can see that the file could be created but not yet written, but as the patch waits for the file to be non zero, ISTM that the probability of a torn write of a single integer by echo is so remote that we could let it as that?

Attached a version with the slurping in the loop anyway.

--
Fabien.
diff --git a/src/bin/psql/t/020_cancel.pl b/src/bin/psql/t/020_cancel.pl
index 7713fff8e4..545ce57ead 100644
--- a/src/bin/psql/t/020_cancel.pl
+++ b/src/bin/psql/t/020_cancel.pl
@@ -7,6 +7,7 @@ use warnings;
 use PostgresNode;
 use TestLib;
 use Test::More tests => 2;
+use Time::HiRes qw(usleep);
 
 my $tempdir = TestLib::tempdir;
 
@@ -28,19 +29,35 @@ SKIP: {
 	my ($stdin, $stdout, $stderr);
 
 	# Test whether shell supports $PPID.  It's part of POSIX, but some
-	# pre-/non-POSIX shells don't support it (e.g., NetBSD, Solaris).
+	# pre-/non-POSIX shells don't support it (e.g., NetBSD).
 	$stdin = "\\! echo \$PPID";
 	IPC::Run::run(['psql', '-X', '-v', 'ON_ERROR_STOP=1'], '<', \$stdin, '>', \$stdout, '2>', \$stderr);
 	$stdout =~ /^\d+$/ or skip "shell apparently does not support \$PPID", 2;
 
-	local $SIG{ALRM} = sub {
-		my $psql_pid = TestLib::slurp_file("$tempdir/psql.pid");
-		kill 'INT', $psql_pid;
-	};
-	alarm 1;
-
-	$stdin = "\\! echo \$PPID >$tempdir/psql.pid\nselect pg_sleep(3);";
-	my $result = IPC::Run::run(['psql', '-X', '-v', 'ON_ERROR_STOP=1'], '<', \$stdin, '>', \$stdout, '2>', \$stderr);
+	# Now start the real test
+	my $h = IPC::Run::start(['psql', '-X', '-v', 'ON_ERROR_STOP=1'], \$stdin, \$stdout, \$stderr);
+
+	# Get the PID
+	$stdout = '';
+	$stderr = '';
+	$stdin = "\\! echo \$PPID >$tempdir/psql.pid\n";
+	pump $h while length $stdin;
+	my $count;
+	my $psql_pid;
+	usleep(10_000)
+	  until (-s "$tempdir/psql.pid" and ($psql_pid = TestLib::slurp_file("$tempdir/psql.pid")) =~ /^\d+\n/s)
+		 or ($count++ > 180 * 100 and die 'pid file did not appear');
+
+	# Send sleep command and wait until the server has registered it
+	$stdin = "select pg_sleep(180);\n";
+	pump $h while length $stdin;
+	$node->poll_query_until('postgres', q{SELECT (SELECT count(*) FROM pg_stat_activity WHERE query ~ 'pg_sleep') > 0;})
+	  or die "timed out";
+
+	# Send cancel request
+	kill 'INT', $psql_pid;
+
+	my $result = finish $h;
 
 	ok(!$result, 'query failed as expected');
 	like($stderr, qr/canceling statement due to user request/, 'query was canceled');

Reply via email to