Pending query cancel defeats SIGQUIT

From: Noah Misch <noah(at)leadboat(dot)com>
To: pgsql-hackers(at)postgresql(dot)org
Subject: Pending query cancel defeats SIGQUIT
Date: 2013-09-11 01:31:07
Message-ID: 20130911013107.GA225735@tornado.leadboat.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

I've been doing an excess of immediate shutdowns lately, and that has turned
up bugs old and new. This one goes back to 8.4 or earlier. If a query cancel
is pending when a backend receives SIGQUIT, the cancel takes precedence and
the backend survives:

[local] test=# select nmtest_spin(false);
Cancel request sent
WARNING: terminating connection because of crash of another server process
DETAIL: The postmaster has commanded this server process to roll back the current transaction and exit, because another server process exited abnormally and possibly corrupted shared memory.
HINT: In a moment you should be able to reconnect to the database and repeat your command.
ERROR: canceling statement due to user request
Time: 4932.257 ms
[local] test=# select 1;
?column?
----------
1
(1 row)

The errfinish() pertaining to that WARNING issues CHECK_FOR_INTERRUPTS(), and
the query cancel pending since before the SIGQUIT arrived then takes effect.
This is less bad on 9.4, because the postmaster will SIGKILL the backend after
5s. On older releases, the backend persists indefinitely.

Let's fix this by holding interrupts for the duration of quickdie(); see
attached patch. Surely we don't want any other kind of backend demise taking
precedence over quickdie(). Unfortunately, this patch does not fully prevent
that. If ImmediateInterruptOK==true, a SIGINT could arrive and longjmp()
between the start of quickdie() and its PG_SETMASK() call. The only
decently-portable way I know to close that race is to name SIGINT/SIGTERM in
the SIGQUIT handler's sa_mask. In any event, that's a *far* narrower race and
is a general problem shared by most of our signal use. I'm content to not fix
it in this patch, but I propose adding it as a TODO.

Here is the source code for the nmtest_spin() function used above:

Datum
nmtest_spin(PG_FUNCTION_ARGS)
{
bool no_sigquit = PG_GETARG_BOOL(0);

if (no_sigquit)
{
sigset_t mask;
sigemptyset(&mask);
sigaddset(&mask, SIGQUIT);
sigprocmask(SIG_BLOCK, &mask, NULL);
}

for (;;)
sleep(1);
}

Thanks,
nm

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com

Attachment Content-Type Size
quickdir-nointr-v1.patch text/plain 695 bytes

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Bruce Momjian 2013-09-11 01:34:09 Re: Broken link in contrib/fuzzystrmatch/dmetaphone.c
Previous Message Bruce Momjian 2013-09-11 00:18:59 Re: One-line comment to improve understanding of VARSIZE_ANY_EXHDR macro