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
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");
--
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers
