Redesign of parallel dump/restore's response to SIGINT

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)postgreSQL(dot)org
Subject: Redesign of parallel dump/restore's response to SIGINT
Date: 2016-05-31 01:14:34
Message-ID: 7005.1464657274@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

There are several things that are not very good about the handling of
cancel interrupts (control-C etc) in parallel pg_dump/pg_restore.
Currently, the signal handler just sets a "wantAbort" flag which is
tested in checkAborting(). That means:

1. There had better be a checkAborting() call in every potentially
long-running loop in pg_dump or pg_restore. I already fixed two such
oversights yesterday in commit 3c8aa6654, but I have little confidence
that there aren't more omissions of that ilk, and none whatsoever that
we won't introduce more of them in the future.

2. If we're not executing code on the client side, but just waiting for
a SQL query to finish, this infrastructure fails to move things along at
all. That's not a huge problem for pg_dump, nor for data transfer in
pg_restore. But operations such as a long-running CREATE INDEX in
pg_restore will not get cancelled, and IMV that *is* a problem.

3. On Unix, the key reason the current code manages to produce early exit
in parallel mode is that if you run pg_dump or pg_restore manually, and
type control-C at the console, the SIGINT will be delivered to all members
of the terminal process group; that is, all the worker processes along
with the master. It's not hard to envision use-cases in which someone
tries to SIGINT just the master. There's a kluge in the current code that
tries to deal with that case by checking wantAbort in select_loop(), but
that only helps if the master is waiting for workers at the instant the
SIGINT arrives. If it's someplace else, it will fail to notice wantAbort
till after the next worker finishes, which might be a long time.

4. On Windows, there's no signal handler and so no designed response to
control-C at all. The pg_dump or pg_restore process just exits, leaving
the connected backend(s) to clean up. This isn't too awful ... except in
the long-running-CREATE-INDEX case.

Point 2 is basically unfixable without a redesign of pg_dump/pg_restore's
interrupt handling ... so attached is one. The key idea here is to make
the signal handler itself send a PQcancel request, and then exit(1),
rather than trusting the mainline code to notice the signal anytime soon.
So we can get rid of checkAborting() and thus point 1 goes away too.
Point 3 is dealt with by teaching the master's signal handler to also send
SIGTERM to all the workers. (Each worker PQcancel's its own connection
and then exits.) Point 4 is dealt with by introducing a console interrupt
handler (code stolen from psql), which unlike the Unix case can directly
send all the required PQcancels, and then just let the process exit.

We need a bit of notational complexity to ensure that the signal handler
or console interrupt handler thread can't see an inconsistent state; but
we already had some critical-section code in place for that, so it doesn't
get much worse than before.

This patch fixes a few other minor bugs too, for instance that on Windows
we never bothered to set a ParallelSlot's args->AH, so that the intended
DisconnectDatabase call in a worker's archive_close_connection callback
never happened, and thus even the weak existing provision for sending a
PQcancel didn't work.

I've done a fair amount of testing of this behavior on Unix, including
strace'ing the run to make sure it did what I expected. One thing
I noticed while doing that is that in worker processes, DisconnectDatabase
tends to send a cancel request before exiting, even if nothing went wrong.
This is at least a waste of cycles, and could lead to unexpected log
messages, or maybe even data loss if it happened in pg_restore. I dug
into it and found that the reason is that after a COPY step, pg_dump was
leaving libpq in PGASYNC_BUSY state, causing PQtransactionStatus to report
PQTRANS_ACTIVE. That's normally harmless because the next PQexec() will
silently clear the PGASYNC_BUSY state; but in a parallel worker we might
exit without any additional commands after a COPY. The attached patch
adds an extra PQgetResult() call after a COPY to allow libpq to return to
PGASYNC_IDLE state.

I have not, however, tested the Windows side of this at all.

I think this is a bug fix and should be back-patched, but I'm not going
to risk committing it without review and testing of the Windows version.
Any volunteers?

regards, tom lane

Attachment Content-Type Size
dump-restore-sigint-1.patch text/x-diff 35.7 KB

Browse pgsql-hackers by date

  From Date Subject
Next Message Andreas Karlsson 2016-05-31 01:37:20 Re: Parallel safety tagging of extension functions
Previous Message Gavin Flower 2016-05-31 00:46:36 Re: Does people favor to have matrix data type?