Date: Sat, 30 Sep 2017 15:44:24 -0700
From: Andres Freund <andres@anarazel.de>
To: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>,
	pgsql-committers <pgsql-committers@postgresql.org>
Subject: Re: pgsql: Add test for postmaster crash restarts.
Message-ID: <20170930224424.ud5ilchmclbl5y5n@alap3.anarazel.de>
References: <17084.1505837634@sss.pgh.pa.us>
 <20170919164718.66hcedq2rtlkntvf@alap3.anarazel.de>
 <18695.1505840008@sss.pgh.pa.us>
 <D2FBABBF-F0E3-43FE-8EA4-E2431DD6EEBA@anarazel.de>
 <20170919174232.soc2skrqxxdtnh22@alap3.anarazel.de>
 <21486.1506601297@sss.pgh.pa.us>
 <5ce9b1f4-dff8-6fe9-74ab-d8e1b993b1d5@2ndQuadrant.com>
 <20170930221043.iay4s7rmwm5l7wiw@alap3.anarazel.de>
 <56b65da3-f7cc-494c-7952-ba5f6c804d7e@2ndQuadrant.com>
 <20170930222712.xgdgxnt4sjvig5mn@alap3.anarazel.de>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="6c5vpau3pwll7d4k"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20170930222712.xgdgxnt4sjvig5mn@alap3.anarazel.de>
Precedence: bulk
Sender: pgsql-committers-owner@postgresql.org


--6c5vpau3pwll7d4k
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On 2017-09-30 15:27:12 -0700, Andres Freund wrote:
> On 2017-09-30 18:21:33 -0400, Andrew Dunstan wrote:
> > 
> > [re-adding commiters which I inadvertently left off]
> > 
> > 
> > On 09/30/2017 06:10 PM, Andres Freund wrote:
> > >
> > >
> > >> I was just looking at this. Why aren't we using "pg_ctl kill" to
> > >> terminate the backend? That's supposed to be portable.
> > > Because pg_ctl can't do that for any process but postmaster, no? The
> > > test is supposed to find issues with backend death (and has
> > > defficiencies in error reporting already, and would have caught a bug
> > > I'd introduced previously).
> 
> > No, I don't think so. That's not what the docs say. That's why you give
> > it a pid argument" "pg_ctl kill signal_name process_id"
> 
> Oh, cool. Didn't know that one. So the answer is:
> "Because Andres doesn't know squat.".
> 
> But even after fixing that, there unfortunately is:
> 
> static void
> set_sig(char *signame)
> {
> …
> #if 0
> 	/* probably should NOT provide SIGKILL */
> 	else if (strcmp(signame, "KILL") == 0)
> 		sig = SIGKILL;
> #endif
> 
> I'm unclear on what that provision is achieving? If you can kill with
> pg_ctl you can do other nasty stuff too (like just use kill instead of
> pg_ctl)?

Could you perhaps test whether windows likes things after the following
patch?  I don't think the kill_kill guarantees are really needed here,
so we might even be able to allow this on msvc.

Greetings,

Andres Freund

--6c5vpau3pwll7d4k
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="windowsify.patch"

diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 4e02c4cea1..0990647297 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -1961,11 +1961,8 @@ set_sig(char *signame)
 		sig = SIGQUIT;
 	else if (strcmp(signame, "ABRT") == 0)
 		sig = SIGABRT;
-#if 0
-	/* probably should NOT provide SIGKILL */
 	else if (strcmp(signame, "KILL") == 0)
 		sig = SIGKILL;
-#endif
 	else if (strcmp(signame, "TERM") == 0)
 		sig = SIGTERM;
 	else if (strcmp(signame, "USR1") == 0)
diff --git a/src/test/recovery/t/013_crash_restart.pl b/src/test/recovery/t/013_crash_restart.pl
index ca02054ff0..91a8ef90c1 100644
--- a/src/test/recovery/t/013_crash_restart.pl
+++ b/src/test/recovery/t/013_crash_restart.pl
@@ -16,16 +16,8 @@ use Test::More;
 use Config;
 use Time::HiRes qw(usleep);
 
-if ($Config{osname} eq 'MSWin32')
-{
-	# some Windows Perls at least don't like IPC::Run's
-	# start/kill_kill regime.
-	plan skip_all => "Test fails on Windows perl";
-}
-else
-{
-	plan tests => 18;
-}
+plan tests => 18;
+
 
 # To avoid hanging while expecting some specific input from a psql
 # instance being driven by us, add a timeout high enough that it
@@ -106,10 +98,10 @@ $monitor_stdout = '';
 $monitor_stderr = '';
 
 # kill once with QUIT - we expect psql to exit, while emitting error message first
-my $cnt = kill 'QUIT', $pid;
+my $ret = TestLib::system_log('pg_ctl', 'kill', 'QUIT', $pid);
 
 # Exactly process should have been alive to be killed
-is($cnt, 1, "exactly one process killed with SIGQUIT");
+is($ret, 0, "killed process with SIGQUIT");
 
 # Check that psql sees the killed backend as having been terminated
 $killme_stdin .= q[
@@ -119,14 +111,14 @@ ok(pump_until($killme, \$killme_stderr, qr/WARNING:  terminating connection beca
    "psql query died successfully after SIGQUIT");
 $killme_stderr = '';
 $killme_stdout = '';
-$killme->kill_kill;
+$killme->finish;
 
 # Wait till server restarts - we should get the WARNING here, but
 # sometimes the server is unable to send that, if interrupted while
 # sending.
 ok(pump_until($monitor, \$monitor_stderr, qr/WARNING:  terminating connection because of crash of another server process|server closed the connection unexpectedly/m),
    "psql monitor died successfully after SIGQUIT");
-$monitor->kill_kill;
+$monitor->finish;
 
 # Wait till server restarts
 is($node->poll_query_until('postgres', 'SELECT $$restarted after sigquit$$;', 'restarted after sigquit'),
@@ -179,8 +171,8 @@ $monitor_stderr = '';
 
 # kill with SIGKILL this time - we expect the backend to exit, without
 # being able to emit an error error message
-$cnt = kill 'KILL', $pid;
-is($cnt, 1, "exactly one process killed with KILL");
+$ret = TestLib::system_log('pg_ctl', 'kill', 'KILL', $pid);
+is($ret, 0, "killed process with KILL");
 
 # Check that psql sees the server as being terminated. No WARNING,
 # because signal handlers aren't being run on SIGKILL.
@@ -189,14 +181,14 @@ SELECT 1;
 ];
 ok(pump_until($killme, \$killme_stderr, qr/server closed the connection unexpectedly/m),
    "psql query died successfully after SIGKILL");
-$killme->kill_kill;
+$killme->finish;
 
 # Wait till server restarts - we should get the WARNING here, but
 # sometimes the server is unable to send that, if interrupted while
 # sending.
 ok(pump_until($monitor, \$monitor_stderr, qr/WARNING:  terminating connection because of crash of another server process|server closed the connection unexpectedly/m),
    "psql monitor died successfully after SIGKILL");
-$monitor->kill_kill;
+$monitor->finish;
 
 # Wait till server restarts
 is($node->poll_query_until('postgres', 'SELECT 1', '1'), "1", "reconnected after SIGKILL");

--6c5vpau3pwll7d4k
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

--6c5vpau3pwll7d4k--