public inbox for [email protected]help / color / mirror / Atom feed
Can we get rid of TerminateThread() in pg_dump? 18+ messages / 4 participants [nested] [flat]
* Can we get rid of TerminateThread() in pg_dump? @ 2026-07-03 03:32 Thomas Munro <[email protected]> 0 siblings, 2 replies; 18+ messages in thread From: Thomas Munro @ 2026-07-03 03:32 UTC (permalink / raw) To: pgsql-hackers; +Cc: Tom Lane <[email protected]>; Kyotaro Horiguchi <[email protected]> Hi, Following up on this ancient discussion and resulting commit e652273e... https://www.postgresql.org/message-id/flat/11515.1464961470%40sss.pgh.pa.us#3e78a2af445dd7e566cf4990... write(fd, ...) locks fd's entry in a user space descriptor table on Windows, so if the terminated thread was also writing to stderr, I am pretty sure it would hang. Someone with Windows might be able to repro that by hacking the workers to write to stderr a lot? WriteFile(_get_osfhandle(STDERR_FILENO), ...) might avoid that specific issue, but for all I know that's just the tip of the iceberg considering the socket stuff. Apparently this hasn't been a problem in practice. Error reporting coinciding with ^C must be unlikely? I'd still like to find a non-evil way to suppress log output, though. What if we atomically pointed STDERR_FILENO to /dev/null, with dup2()? I wrote a patch to try that idea out, but I don't have Windows, so I'm sharing this as a curiosity in case anyone wants to try it and/or comment on all this. Or has a better idea. Preferably that would work on Windows and Unix (if it also used threads). (How I arrived at this obscure hypothetical problem: I've been teaching pg_dump (and everything else) to use threads on Unix too, ie harmonising Windows and Unix code paths. We certainly don't want thread termination/cancellation in pg_threads.h (modern APIs don't even have that, designers of older APIs regret it, including the Windows people who are quite emphatic about there being no safe way to use it[1]), so I wanted to find another way to silence error output. close() would kinda work but cause chaos if another open() squats the fd number, which leads to the idea of dup2(dummy, STDERR_FILENO). Then I wondered if that would also work on Windows' fake fd system, ie whether dup2(dummy, STDERR_FILENO) would play nicely with concurrent write(STDERR_FILENO) or fwrite(stderr) as the file is switched. Yes, as far as I can tell, but as soon as I read about the user space fd locks that make that work, the above concrete problem with write() vs TerminateThread() became clear.) [1] https://devblogs.microsoft.com/oldnewthing/20150814-00/?p=91811 Attachments: [application/x-patch] 0001-pg_dump-Remove-TerminateThread-call.patch (4.9K, ../../CA+hUKGJgO=o-vLFahGdR2WesuX3h1-0j=a8z72fChc-MG1Hveg@mail.gmail.com/2-0001-pg_dump-Remove-TerminateThread-call.patch) download | inline diff: From be2654c76896a9d916778d8663a5da043135c8e0 Mon Sep 17 00:00:00 2001 From: Thomas Munro <[email protected]> Date: Thu, 2 Jul 2026 21:30:44 +1200 Subject: [PATCH] pg_dump: Remove TerminateThread() call. When pg_dump or pg_restore --jobs N is interrupted with ^C on Windows, we cancel all queries, but we don't want the cancellations to be reported as errors to the user in the short time before the whole process exits. That was previously achieved by calling TerminateThread() on each worker thread before sending the cancel message, but that doesn't appear to be 100% safe: the implementations of write() and the socket calls inside PQcancel() might acquire user space locks that were held by the terminated threads. (write() certainly does that.) Instead of relying on theories about all that, remove the TerminateThread() call and adopt a new approach that should also work if we move to threads for parallel.c on Unix too: atomically replace STDERR_FILENO with a descriptor pointing to the null device, to swallow future writes. XXX Does this actually work on Windows? --- src/bin/pg_dump/parallel.c | 64 ++++++++++++++++++++------------------ 1 file changed, 33 insertions(+), 31 deletions(-) diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c index 3e84b881ca3..ec00b440311 100644 --- a/src/bin/pg_dump/parallel.c +++ b/src/bin/pg_dump/parallel.c @@ -56,10 +56,11 @@ #include <sys/select.h> #include <sys/wait.h> #include <signal.h> -#include <unistd.h> #include <fcntl.h> #endif +#include <unistd.h> + #include "fe_utils/string_utils.h" #include "parallel.h" #include "pg_backup_utils.h" @@ -187,7 +188,7 @@ static CRITICAL_SECTION signal_info_lock; do { \ const char *str_ = (str); \ int rc_; \ - rc_ = write(fileno(stderr), str_, strlen(str_)); \ + rc_ = write(STDERR_FILENO, str_, strlen(str_)); \ (void) rc_; \ } while (0) @@ -641,18 +642,41 @@ consoleHandler(DWORD dwCtrlType) if (dwCtrlType == CTRL_C_EVENT || dwCtrlType == CTRL_BREAK_EVENT) { + int null_device; + + /* + * Report we're quitting, using nothing more complicated than + * write(2). + */ + if (progname) + { + write_stderr(progname); + write_stderr(": "); + } + write_stderr("terminated by user\n"); + + /* + * Atomically replace STDERR_FILENO with the null device, to swallow + * future write() and fwrite() output to that descriptor number. This + * prevents worker threads from reporting query cancels as error, + * which might clutter the user's screen, if they manage arrive before + * the whole process exits and terminates them. + * + * XXX Open NUL in advance? + */ + if ((null_device = open("NUL", O_WRONLY | O_BINARY, 0)) >= 0) + dup2(null_device, STDERR_FILENO); + /* Critical section prevents changing data we look at here */ EnterCriticalSection(&signal_info_lock); /* * If in parallel mode, stop worker threads and send QueryCancel to - * their connected backends. The main point of stopping the worker - * threads is to keep them from reporting the query cancels as errors, - * which would clutter the user's screen. We needn't stop the leader - * thread since it won't be doing much anyway. Do this before - * canceling the main transaction, else we might get invalid-snapshot - * errors reported before we can stop the workers. Ignore errors, - * there's not much we can do about them anyway. + * their connected backends. We needn't stop the leader thread since + * it won't be doing much anyway. Do this before canceling the main + * transaction, else we might get invalid-snapshot errors reported + * before we can stop the workers. Ignore errors, there's not much we + * can do about them anyway. */ if (signal_info.pstate != NULL) { @@ -660,15 +684,6 @@ consoleHandler(DWORD dwCtrlType) { ParallelSlot *slot = &(signal_info.pstate->parallelSlot[i]); ArchiveHandle *AH = slot->AH; - HANDLE hThread = (HANDLE) slot->hThread; - - /* - * Using TerminateThread here may leave some resources leaked, - * but it doesn't matter since we're about to end the whole - * process. - */ - if (hThread != INVALID_HANDLE_VALUE) - TerminateThread(hThread, 0); if (AH != NULL && AH->connCancel != NULL) (void) PQcancel(AH->connCancel, errbuf, sizeof(errbuf)); @@ -684,19 +699,6 @@ consoleHandler(DWORD dwCtrlType) errbuf, sizeof(errbuf)); LeaveCriticalSection(&signal_info_lock); - - /* - * Report we're quitting, using nothing more complicated than - * write(2). (We might be able to get away with using pg_log_*() - * here, but since we terminated other threads uncleanly above, it - * seems better to assume as little as possible.) - */ - if (progname) - { - write_stderr(progname); - write_stderr(": "); - } - write_stderr("terminated by user\n"); } /* Always return FALSE to allow signal handling to continue */ -- 2.47.3 ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Can we get rid of TerminateThread() in pg_dump? @ 2026-07-03 13:55 Bryan Green <[email protected]> parent: Thomas Munro <[email protected]> 1 sibling, 1 reply; 18+ messages in thread From: Bryan Green @ 2026-07-03 13:55 UTC (permalink / raw) To: Thomas Munro <[email protected]>; pgsql-hackers; +Cc: Tom Lane <[email protected]>; Kyotaro Horiguchi <[email protected]> On 7/2/2026 10:32 PM, Thomas Munro wrote: > Hi, > > Following up on this ancient discussion and resulting commit e652273e... > > https://www.postgresql.org/message-id/flat/11515.1464961470%40sss.pgh.pa.us#3e78a2af445dd7e566cf4990... > > write(fd, ...) locks fd's entry in a user space descriptor table on > Windows, so if the terminated thread was also writing to stderr, I am > pretty sure it would hang. Someone with Windows might be able to > repro that by hacking the workers to write to stderr a lot? > WriteFile(_get_osfhandle(STDERR_FILENO), ...) might avoid that > specific issue, but for all I know that's just the tip of the iceberg > considering the socket stuff. > > Apparently this hasn't been a problem in practice. Error reporting > coinciding with ^C must be unlikely? I'd still like to find a > non-evil way to suppress log output, though. What if we atomically > pointed STDERR_FILENO to /dev/null, with dup2()? I wrote a patch to > try that idea out, but I don't have Windows, so I'm sharing this as a > curiosity in case anyone wants to try it and/or comment on all this. > Or has a better idea. Preferably that would work on Windows and Unix > (if it also used threads). > > (How I arrived at this obscure hypothetical problem: I've been > teaching pg_dump (and everything else) to use threads on Unix too, ie > harmonising Windows and Unix code paths. We certainly don't want > thread termination/cancellation in pg_threads.h (modern APIs don't > even have that, designers of older APIs regret it, including the > Windows people who are quite emphatic about there being no safe way to > use it[1]), so I wanted to find another way to silence error output. > close() would kinda work but cause chaos if another open() squats the > fd number, which leads to the idea of dup2(dummy, STDERR_FILENO). > Then I wondered if that would also work on Windows' fake fd system, ie > whether dup2(dummy, STDERR_FILENO) would play nicely with concurrent > write(STDERR_FILENO) or fwrite(stderr) as the file is switched. Yes, > as far as I can tell, but as soon as I read about the user space fd > locks that make that work, the above concrete problem with write() vs > TerminateThread() became clear.) > > [1] https://devblogs.microsoft.com/oldnewthing/20150814-00/?p=91811 I build pg_dump on Windows, so I tried this out (MSVC 19.51, VS 2026). It works. It compiles clean, and on a ^C during a parallel dump it suppresses the worker cancel messages the same way the TerminateThread call did-- I checked against unmodified master with the TerminateThread call still in place, and the output is identical-- "terminated by user" and nothing else. Moving write_stderr off fileno(stderr) to STDERR_FILENO is worth having on its own: that shim's comment notes _fileno returns -1 when the stream is closed, which is a state the shutdown path can be in. Opening NUL in the handler worked here; I didn't need to open it in advance. Since you mention moving parallel.c to threads on non-Windows, I've been replacing the socketpair/select() worker protocol on Windows with an in-process queue (a mutex and two condition variables)[1], heading the same direction to unify across platforms. There a worker signals its own death through the condition variable rather than the leader seeing pipe EOF, so ^C hits an extra pg_fatal("a worker process died unexpectedly") in the leader-- and your redirect swallows that one too. I put your patch on top of that series and hit ^C on a 2.3 GB dump repeatedly at -j 2, 4, 8, and 100: clean exit every time, "terminated by user", nothing else. pg_stat_activity right afterward showed no live COPYs, up through -j 100-- so the PQcancel()s land and the backends actually stop, which TerminateThread never did. So your change composes with that work. [1] https://www.postgresql.org/message-id/flat/8c712d76-ecf7-4749-a6d8-dddc01f298ec%40gmail.com -- Bryan Green EDB: https://www.enterprisedb.com ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Can we get rid of TerminateThread() in pg_dump? @ 2026-07-03 15:05 Heikki Linnakangas <[email protected]> parent: Thomas Munro <[email protected]> 1 sibling, 1 reply; 18+ messages in thread From: Heikki Linnakangas @ 2026-07-03 15:05 UTC (permalink / raw) To: Thomas Munro <[email protected]>; pgsql-hackers; +Cc: Tom Lane <[email protected]>; Kyotaro Horiguchi <[email protected]> On 03/07/2026 06:32, Thomas Munro wrote: > Following up on this ancient discussion and resulting commit e652273e... > > https://www.postgresql.org/message-id/flat/11515.1464961470%40sss.pgh.pa.us#3e78a2af445dd7e566cf4990... > > write(fd, ...) locks fd's entry in a user space descriptor table on > Windows, so if the terminated thread was also writing to stderr, I am > pretty sure it would hang. Someone with Windows might be able to > repro that by hacking the workers to write to stderr a lot? > WriteFile(_get_osfhandle(STDERR_FILENO), ...) might avoid that > specific issue, but for all I know that's just the tip of the iceberg > considering the socket stuff. > > Apparently this hasn't been a problem in practice. Error reporting > coinciding with ^C must be unlikely? I'd still like to find a > non-evil way to suppress log output, though. What if we atomically > pointed STDERR_FILENO to /dev/null, with dup2()? I wrote a patch to > try that idea out, but I don't have Windows, so I'm sharing this as a > curiosity in case anyone wants to try it and/or comment on all this. > Or has a better idea. Preferably that would work on Windows and Unix > (if it also used threads). Huh, that's pretty hacky. I think we should adopt our usual signal handling approach here: Instead of calling PQcancel() from the signal handler (or consoleHandler()), just set a global flag, wake up the main thread, and perform the cancellation from the main thread. In the worker threads, you can refrain from printing the cancellation error if the flag is set. Dunno how complicated that is to change.. - Heikki ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Can we get rid of TerminateThread() in pg_dump? @ 2026-07-03 15:35 Bryan Green <[email protected]> parent: Heikki Linnakangas <[email protected]> 0 siblings, 1 reply; 18+ messages in thread From: Bryan Green @ 2026-07-03 15:35 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; Thomas Munro <[email protected]>; pgsql-hackers; +Cc: Tom Lane <[email protected]>; Kyotaro Horiguchi <[email protected]> On 7/3/2026 10:05 AM, Heikki Linnakangas wrote: > On 03/07/2026 06:32, Thomas Munro wrote: >> Following up on this ancient discussion and resulting commit e652273e... >> >> https://www.postgresql.org/message-id/ >> flat/11515.1464961470%40sss.pgh.pa.us#3e78a2af445dd7e566cf499023e8cb97 >> >> write(fd, ...) locks fd's entry in a user space descriptor table on >> Windows, so if the terminated thread was also writing to stderr, I am >> pretty sure it would hang. Someone with Windows might be able to >> repro that by hacking the workers to write to stderr a lot? >> WriteFile(_get_osfhandle(STDERR_FILENO), ...) might avoid that >> specific issue, but for all I know that's just the tip of the iceberg >> considering the socket stuff. >> >> Apparently this hasn't been a problem in practice. Error reporting >> coinciding with ^C must be unlikely? I'd still like to find a >> non-evil way to suppress log output, though. What if we atomically >> pointed STDERR_FILENO to /dev/null, with dup2()? I wrote a patch to >> try that idea out, but I don't have Windows, so I'm sharing this as a >> curiosity in case anyone wants to try it and/or comment on all this. >> Or has a better idea. Preferably that would work on Windows and Unix >> (if it also used threads). > > Huh, that's pretty hacky. I think we should adopt our usual signal > handling approach here: Instead of calling PQcancel() from the signal > handler (or consoleHandler()), just set a global flag, wake up the main > thread, and perform the cancellation from the main thread. In the worker > threads, you can refrain from printing the cancellation error if the > flag is set. > > Dunno how complicated that is to change.. > > - Heikki > > > The main thread is waiting at the select()...waking that from a different thread is going to be doable but awkward in the current implementation on Windows (pipe and sockets). -- Bryan Green EDB: https://www.enterprisedb.com ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Can we get rid of TerminateThread() in pg_dump? @ 2026-07-04 00:38 Thomas Munro <[email protected]> parent: Bryan Green <[email protected]> 0 siblings, 1 reply; 18+ messages in thread From: Thomas Munro @ 2026-07-04 00:38 UTC (permalink / raw) To: Bryan Green <[email protected]>; +Cc: pgsql-hackers; Tom Lane <[email protected]>; Kyotaro Horiguchi <[email protected]> On Sat, Jul 4, 2026 at 1:56 AM Bryan Green <[email protected]> wrote: > It works. It compiles clean, and on a ^C during a parallel dump it > suppresses the worker cancel messages the same way the TerminateThread > call did-- I checked against unmodified master with the TerminateThread > call still in place, and the output is identical-- "terminated by user" > and nothing else. Thank you for confirming! So now I'm wondering, do we want to let sleeping dogs lie, or do we want to back-patch this? If we're happy to leave the back-branches, which have never received a user complaint, then I'll simply adopt this approach in my pg_dump-with-threads-on-Unix-too patch set (which I'll post shortly), ie the same dup2() code will be used on all systems. But if we think this problem is worth fixing in back-branches, we could back-patch this stand-alone patch first. Thoughts? > Moving write_stderr off fileno(stderr) to STDERR_FILENO is worth having > on its own: that shim's comment notes _fileno returns -1 when the stream > is closed, which is a state the shutdown path can be in. Cool. Yeah, I failed to mention why I'd done that in my previous email, and that was it. > Opening NUL in the handler worked here; I didn't need to open it in advance. I was worrying that open() might fail with EMFILE. I don't think it's really going to happen though, given the bounded numbers of files that pg_dump/pg_restore should be working with, so maybe it's OK as it is. > Since you mention moving parallel.c to threads on non-Windows, I've been > replacing the socketpair/select() worker protocol on Windows with an > in-process queue (a mutex and two condition variables)[1], heading the > same direction to unify across platforms. There a worker signals its own > death through the condition variable rather than the leader seeing pipe > EOF, so ^C hits an extra pg_fatal("a worker process died unexpectedly") > in the leader-- and your redirect swallows that one too. I put your Excellent, and 100% agreed. No need for pipes/sockets and clunky protocols if the fork() path is gone. I had already made a start on exactly same idea, because I hadn't seen your thread (sorry). I have a few comments/thoughts (short version: can we make a reusable generic thread pool component for that?). I will look more closely at your patches and comment on your thread. > patch on top of that series and hit ^C on a 2.3 GB dump repeatedly at -j > 2, 4, 8, and 100: clean exit every time, "terminated by user", nothing > else. pg_stat_activity right afterward showed no live COPYs, up through > -j 100-- so the PQcancel()s land and the backends actually stop, which > TerminateThread never did. So your change composes with that work. Interesting. ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Can we get rid of TerminateThread() in pg_dump? @ 2026-07-04 00:51 Thomas Munro <[email protected]> parent: Bryan Green <[email protected]> 0 siblings, 1 reply; 18+ messages in thread From: Thomas Munro @ 2026-07-04 00:51 UTC (permalink / raw) To: Bryan Green <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; pgsql-hackers; Tom Lane <[email protected]>; Kyotaro Horiguchi <[email protected]> On Sat, Jul 4, 2026 at 3:36 AM Bryan Green <[email protected]> wrote: > On 7/3/2026 10:05 AM, Heikki Linnakangas wrote: > >> Apparently this hasn't been a problem in practice. Error reporting > >> coinciding with ^C must be unlikely? I'd still like to find a > >> non-evil way to suppress log output, though. What if we atomically > >> pointed STDERR_FILENO to /dev/null, with dup2()? I wrote a patch to > >> try that idea out, but I don't have Windows, so I'm sharing this as a > >> curiosity in case anyone wants to try it and/or comment on all this. > >> Or has a better idea. Preferably that would work on Windows and Unix > >> (if it also used threads). > > > > Huh, that's pretty hacky. I think we should adopt our usual signal > > handling approach here: Instead of calling PQcancel() from the signal > > handler (or consoleHandler()), just set a global flag, wake up the main > > thread, and perform the cancellation from the main thread. In the worker > > threads, you can refrain from printing the cancellation error if the > > flag is set. > > > > Dunno how complicated that is to change.. > The main thread is waiting at the select()...waking that from a > different thread is going to be doable but awkward in the current > implementation on Windows (pipe and sockets). I agree that, in general, cooperative/flag based interruption is the right architecture and standard approach, and I did think a bit about how to make that work, but I couldn't figure out how to do it without introducing extra waits. You'd have to ask each thread to shut down and wait for it to acknowledge your request, but this is supposed to be a fast shutdown, and abnormal termination. We're subseconds away from _exit() nuking everything, and we really just want to tell the *server* to cancel whatever it's doing so we don't leave a long CREATE INDEX or whatever running. We don't actually care about the threads themselves, and it doesn't seem that great if we have to introduce an IPC ping-pong of some kind with each thread. With this approach we don't have to, we just disconnect them from stderr slightly before the kernel vaporises them. Doesn't seem that bad to me? ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Can we get rid of TerminateThread() in pg_dump? @ 2026-07-04 11:15 Jelte Fennema-Nio <[email protected]> parent: Thomas Munro <[email protected]> 0 siblings, 2 replies; 18+ messages in thread From: Jelte Fennema-Nio @ 2026-07-04 11:15 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: Bryan Green <[email protected]>; Heikki Linnakangas <[email protected]>; pgsql-hackers; Tom Lane <[email protected]>; Kyotaro Horiguchi <[email protected]> On Sat, 4 Jul 2026 at 02:51, Thomas Munro <[email protected]> wrote: > We don't actually care about the threads > themselves, and it doesn't seem that great if we have to introduce an > IPC ping-pong of some kind with each thread. Agreed. But I do agree with Heikki that swapping out stderr seems pretty hacky. At the very least because now the main thread cannot write to stderr either anymore (which is why you removed the "terminated by user" write I guess). How about instead we do something like the attached? To be clear, I do think we should stop using TerminateThread because I wanna replace PQcancel there with PQcancelBlocking[1] PQcancelBlocking does a whole TLS handshake, which is almost certainly taking some locks. Note that the way to achieve that I moved the Ctrl+C handler to a dedicated thread on Unix too, so it starts behaving the same as Windows in that respect. I think combined with you changing pg_dump to use worker *threads* on Unix too, we would then get pretty much identical behaviour across OSes for pg_dump. P.S. I now realize that anything involving the (already existing) CancelRequested flag is actually not actually safe/correct on Windows, because it's not using read nor written using atomic operations while the consoleHandler runs on its own thread. Would be good to fix that too I guess, but it seems that for now it has worked in practice at least. [1]: https://www.postgresql.org/message-id/flat/DJPAH0WPJV3K.1PYZ8P0QXZVMX%40jeltef.nl#5642e337a4e4d04b21... Attachments: [text/x-patch] v2-0001-Use-exit_nicely-in-die_on_query_failure.patch (1.1K, ../../[email protected]/2-v2-0001-Use-exit_nicely-in-die_on_query_failure.patch) download | inline diff: From a283180ad3b3bffe8ae04d59f61212ff95ccbee6 Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio <[email protected]> Date: Sat, 4 Jul 2026 11:11:09 +0200 Subject: [PATCH v2 1/2] Use exit_nicely in die_on_query_failure We want a query failure to kill the worker not the whole process. On Unix that's currently the same thing, but on Windows workers are threads. --- src/bin/pg_dump/pg_backup_db.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/bin/pg_dump/pg_backup_db.c b/src/bin/pg_dump/pg_backup_db.c index 5c349279beb..f6bca7dc508 100644 --- a/src/bin/pg_dump/pg_backup_db.c +++ b/src/bin/pg_dump/pg_backup_db.c @@ -210,7 +210,13 @@ die_on_query_failure(ArchiveHandle *AH, const char *query) pg_log_error("query failed: %s", PQerrorMessage(AH->connection)); pg_log_error_detail("Query was: %s", query); - exit(1); + + /* + * Use exit_nicely() rather than exit(): on Windows the workers are + * threads, and a failed worker's query must only end its own thread, not + * bring down the whole process. + */ + exit_nicely(1); } void base-commit: e42d4a1f3dc59420be796883404020cb41ddd05e -- 2.54.0 [text/x-patch] v2-0002-pg_dump-Remove-TerminateThread-call.patch (9.4K, ../../[email protected]/3-v2-0002-pg_dump-Remove-TerminateThread-call.patch) download | inline diff: From 87c6cbd43685b2f158fd68cc5fd8e15a0cd1b44a Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio <[email protected]> Date: Sat, 4 Jul 2026 11:12:15 +0200 Subject: [PATCH v2 2/2] pg_dump: Remove TerminateThread() call When pg_dump or pg_restore --jobs N is interrupted with ^C on Windows, we cancel all queries, but we don't want the cancellations to be reported as errors to the user in the short time before the whole process exits. That was previously achieved by calling TerminateThread() on each worker thread before sending the cancel message, but that doesn't appear to be 100% safe: the implementations of write() and the socket calls inside PQcancel() might acquire user space locks that were held by the terminated threads. (write() certainly does that.) Instead of doing it that sketchy way this now atomically sets a flag (before sending any cancel requests) that tells the threads to not log errors anymore. --- src/bin/pg_dump/parallel.c | 49 +++++++++++++--------------- src/bin/pg_dump/pg_backup_archiver.c | 14 ++++++-- src/bin/pg_dump/pg_backup_db.c | 18 +++++++--- src/bin/pg_dump/pg_backup_utils.c | 24 ++++++++++++++ src/bin/pg_dump/pg_backup_utils.h | 21 ++++++++++++ 5 files changed, 92 insertions(+), 34 deletions(-) diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c index 7e2e9f958ea..e11f7ca9363 100644 --- a/src/bin/pg_dump/parallel.c +++ b/src/bin/pg_dump/parallel.c @@ -531,11 +531,12 @@ WaitForTerminatingWorkers(ParallelState *pstate) * might be that only the leader gets signaled. * * On Windows, the cancel handler runs in a separate thread, because that's - * how SetConsoleCtrlHandler works. We make it stop worker threads, send - * cancels on all active connections, and then return FALSE, which will allow - * the process to die. For safety's sake, we use a critical section to - * protect the PGcancel structures against being changed while the signal - * thread runs. + * how SetConsoleCtrlHandler works. Because the workers are threads in this + * same process, we set a flag (is_cancel_in_progress()) so they stay quiet about + * the query cancellations instead of cluttering the screen, then send cancels + * on all active connections and return FALSE, which will allow the process to + * die. For safety's sake, we use a critical section to protect the PGcancel + * structures against being changed while the signal thread runs. */ #ifndef WIN32 @@ -641,34 +642,30 @@ consoleHandler(DWORD dwCtrlType) if (dwCtrlType == CTRL_C_EVENT || dwCtrlType == CTRL_BREAK_EVENT) { + /* + * Tell worker threads to stay quiet about the query cancellations + * we're about to send them; otherwise they'd report them as errors + * and clutter the user's screen. This must be set before we send any + * cancel, so that a worker is guaranteed to see it by the time its + * query fails as a result. + */ + set_cancel_in_progress(); + /* Critical section prevents changing data we look at here */ EnterCriticalSection(&signal_info_lock); /* - * If in parallel mode, stop worker threads and send QueryCancel to - * their connected backends. The main point of stopping the worker - * threads is to keep them from reporting the query cancels as errors, - * which would clutter the user's screen. We needn't stop the leader - * thread since it won't be doing much anyway. Do this before - * canceling the main transaction, else we might get invalid-snapshot - * errors reported before we can stop the workers. Ignore errors, - * there's not much we can do about them anyway. + * If in parallel mode, send QueryCancel to each worker's connected + * backend. Do this before canceling the main transaction, else we + * might get invalid-snapshot errors reported before we can stop the + * workers. Ignore errors, there's not much we can do about them + * anyway. */ if (signal_info.pstate != NULL) { for (i = 0; i < signal_info.pstate->numWorkers; i++) { - ParallelSlot *slot = &(signal_info.pstate->parallelSlot[i]); - ArchiveHandle *AH = slot->AH; - HANDLE hThread = (HANDLE) slot->hThread; - - /* - * Using TerminateThread here may leave some resources leaked, - * but it doesn't matter since we're about to end the whole - * process. - */ - if (hThread != INVALID_HANDLE_VALUE) - TerminateThread(hThread, 0); + ArchiveHandle *AH = signal_info.pstate->parallelSlot[i].AH; if (AH != NULL && AH->connCancel != NULL) (void) PQcancel(AH->connCancel, errbuf, sizeof(errbuf)); @@ -687,9 +684,7 @@ consoleHandler(DWORD dwCtrlType) /* * Report we're quitting, using nothing more complicated than - * write(2). (We might be able to get away with using pg_log_*() - * here, but since we terminated other threads uncleanly above, it - * seems better to assume as little as possible.) + * write(2). */ if (progname) { diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c index 46f4c518347..5c0983fd11a 100644 --- a/src/bin/pg_dump/pg_backup_archiver.c +++ b/src/bin/pg_dump/pg_backup_archiver.c @@ -1933,9 +1933,17 @@ warn_or_exit_horribly(ArchiveHandle *AH, const char *fmt, ...) AH->lastErrorStage = AH->stage; AH->lastErrorTE = AH->currentTE; - va_start(ap, fmt); - pg_log_generic_v(PG_LOG_ERROR, PG_LOG_PRIMARY, fmt, ap); - va_end(ap); + /* + * Don't report the failure if we're cancelling this query ourselves as + * part of tearing down the process; the error is expected. (Only + * possible on Windows, where the workers are threads.) + */ + if (!is_cancel_in_progress()) + { + va_start(ap, fmt); + pg_log_generic_v(PG_LOG_ERROR, PG_LOG_PRIMARY, fmt, ap); + va_end(ap); + } if (AH->public.exit_on_error) exit_nicely(1); diff --git a/src/bin/pg_dump/pg_backup_db.c b/src/bin/pg_dump/pg_backup_db.c index f6bca7dc508..c960ca57243 100644 --- a/src/bin/pg_dump/pg_backup_db.c +++ b/src/bin/pg_dump/pg_backup_db.c @@ -402,8 +402,13 @@ ExecuteSqlCommandBuf(Archive *AHX, const char *buf, size_t bufLen) */ if (AH->pgCopyIn && PQputCopyData(AH->connection, buf, bufLen) <= 0) - pg_fatal("error returned by PQputCopyData: %s", - PQerrorMessage(AH->connection)); + { + /* Stay quiet if this is a result of our own cancellation. */ + if (!is_cancel_in_progress()) + pg_log_error("error returned by PQputCopyData: %s", + PQerrorMessage(AH->connection)); + exit_nicely(1); + } } else if (AH->outputKind == OUTPUT_OTHERDATA) { @@ -451,8 +456,13 @@ EndDBCopyMode(Archive *AHX, const char *tocEntryTag) PGresult *res; if (PQputCopyEnd(AH->connection, NULL) <= 0) - pg_fatal("error returned by PQputCopyEnd: %s", - PQerrorMessage(AH->connection)); + { + /* Stay quiet if this is a result of our own cancellation. */ + if (!is_cancel_in_progress()) + pg_log_error("error returned by PQputCopyEnd: %s", + PQerrorMessage(AH->connection)); + exit_nicely(1); + } /* Check command status and return to normal libpq state */ res = PQgetResult(AH->connection); diff --git a/src/bin/pg_dump/pg_backup_utils.c b/src/bin/pg_dump/pg_backup_utils.c index 0368f7623a7..c529ec078fe 100644 --- a/src/bin/pg_dump/pg_backup_utils.c +++ b/src/bin/pg_dump/pg_backup_utils.c @@ -21,6 +21,30 @@ /* Globals exported by this file */ const char *progname = NULL; +#ifdef WIN32 + +/* + * Flag telling worker threads to stay quiet about query failures because we're + * cancelling their queries as part of tearing down the process. See the + * comment in pg_backup_utils.h. Accessed with interlocked primitives because + * the cancel thread writes it while worker threads read it. + */ +static volatile LONG cancelInProgress = 0; + +void +set_cancel_in_progress(void) +{ + InterlockedExchange(&cancelInProgress, 1); +} + +bool +is_cancel_in_progress(void) +{ + return InterlockedCompareExchange(&cancelInProgress, 0, 0) != 0; +} + +#endif /* WIN32 */ + #define MAX_ON_EXIT_NICELY 20 static struct diff --git a/src/bin/pg_dump/pg_backup_utils.h b/src/bin/pg_dump/pg_backup_utils.h index 9e98ed11619..779b10d8af5 100644 --- a/src/bin/pg_dump/pg_backup_utils.h +++ b/src/bin/pg_dump/pg_backup_utils.h @@ -31,6 +31,27 @@ extern void set_dump_section(const char *arg, int *dumpSections); extern void on_exit_nicely(on_exit_nicely_callback function, void *arg); pg_noreturn extern void exit_nicely(int code); +/* + * On Windows the parallel workers are threads inside the leader process. When + * a cancel is processed there, the leader sends cancels to the workers' + * in-flight queries; without this flag each worker would then report the + * resulting "canceling statement due to user request" error and clutter the + * screen in the brief window before the whole process exits. The cancel + * thread sets this flag before sending any cancel, and worker threads check it + * before reporting a query failure. It's written and read from different + * threads, so it's accessed with the Windows interlocked (atomic) primitives. + * + * On other platforms the workers are separate processes that just _exit() when + * cancelled, so they never reach the error-reporting code; there the check is + * compiled out to a constant false and the flag doesn't exist. + */ +#ifdef WIN32 +extern void set_cancel_in_progress(void); +extern bool is_cancel_in_progress(void); +#else +#define is_cancel_in_progress() false +#endif + /* In pg_dump, we modify pg_fatal to call exit_nicely instead of exit */ #undef pg_fatal #define pg_fatal(...) do { \ -- 2.54.0 ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Can we get rid of TerminateThread() in pg_dump? @ 2026-07-04 14:48 Bryan Green <[email protected]> parent: Thomas Munro <[email protected]> 0 siblings, 0 replies; 18+ messages in thread From: Bryan Green @ 2026-07-04 14:48 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: pgsql-hackers; Tom Lane <[email protected]>; Kyotaro Horiguchi <[email protected]> On 7/3/2026 7:38 PM, Thomas Munro wrote: > On Sat, Jul 4, 2026 at 1:56 AM Bryan Green <[email protected]> wrote: >> It works. It compiles clean, and on a ^C during a parallel dump it >> suppresses the worker cancel messages the same way the TerminateThread >> call did-- I checked against unmodified master with the TerminateThread >> call still in place, and the output is identical-- "terminated by user" >> and nothing else. > > Thank you for confirming! So now I'm wondering, do we want to let > sleeping dogs lie, or do we want to back-patch this? If we're happy > to leave the back-branches, which have never received a user > complaint, then I'll simply adopt this approach in my > pg_dump-with-threads-on-Unix-too patch set (which I'll post shortly), > ie the same dup2() code will be used on all systems. But if we think > this problem is worth fixing in back-branches, we could back-patch > this stand-alone patch first. Thoughts? > >> Moving write_stderr off fileno(stderr) to STDERR_FILENO is worth having >> on its own: that shim's comment notes _fileno returns -1 when the stream >> is closed, which is a state the shutdown path can be in. > > Cool. Yeah, I failed to mention why I'd done that in my previous > email, and that was it. > >> Opening NUL in the handler worked here; I didn't need to open it in advance. > > I was worrying that open() might fail with EMFILE. I don't think it's > really going to happen though, given the bounded numbers of files that > pg_dump/pg_restore should be working with, so maybe it's OK as it is. > >> Since you mention moving parallel.c to threads on non-Windows, I've been >> replacing the socketpair/select() worker protocol on Windows with an >> in-process queue (a mutex and two condition variables)[1], heading the >> same direction to unify across platforms. There a worker signals its own >> death through the condition variable rather than the leader seeing pipe >> EOF, so ^C hits an extra pg_fatal("a worker process died unexpectedly") >> in the leader-- and your redirect swallows that one too. I put your > > Excellent, and 100% agreed. No need for pipes/sockets and clunky > protocols if the fork() path is gone. I had already made a start on > exactly same idea, because I hadn't seen your thread (sorry). I have > a few comments/thoughts (short version: can we make a reusable generic > thread pool component for that?). I will look more closely at your > patches and comment on your thread. > Part of it -- the worker lifecycle and the work-item queue are generic and worth factoring out. The scheduling isn't, because on the restore side it's dependency and lock-conflict aware (pop_next_work_item and the ready heap). It's not workers pulling the next item off a queue -- what's runnable depends on the whole in-flight set. A pool would own the threads and the channel, with that policy on top. Dump is basically a plain pool already, so it'd drop straight in. Good to know we landed on the same shape independently, and thanks for offering to look at the patches. I'll take the pool/scheduler split up in more detail over there. >> patch on top of that series and hit ^C on a 2.3 GB dump repeatedly at -j >> 2, 4, 8, and 100: clean exit every time, "terminated by user", nothing >> else. pg_stat_activity right afterward showed no live COPYs, up through >> -j 100-- so the PQcancel()s land and the backends actually stop, which >> TerminateThread never did. So your change composes with that work. > > Interesting. -- Bryan Green EDB: https://www.enterprisedb.com ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Can we get rid of TerminateThread() in pg_dump? @ 2026-07-04 15:24 Bryan Green <[email protected]> parent: Jelte Fennema-Nio <[email protected]> 1 sibling, 0 replies; 18+ messages in thread From: Bryan Green @ 2026-07-04 15:24 UTC (permalink / raw) To: Jelte Fennema-Nio <[email protected]>; Thomas Munro <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; pgsql-hackers; Tom Lane <[email protected]>; Kyotaro Horiguchi <[email protected]> On 7/4/2026 6:15 AM, Jelte Fennema-Nio wrote: > On Sat, 4 Jul 2026 at 02:51, Thomas Munro <[email protected]> wrote: >> We don't actually care about the threads >> themselves, and it doesn't seem that great if we have to introduce an >> IPC ping-pong of some kind with each thread. > > Agreed. But I do agree with Heikki that swapping out stderr seems pretty > hacky. At the very least because now the main thread cannot write to > stderr either anymore (which is why you removed the "terminated by user" > write I guess). One correction from running it on Windows -- the "terminated by user" line still prints. Thomas moved that write ahead of the dup2(), so it goes out before stderr is redirected. > > How about instead we do something like the attached? > > To be clear, I do think we should stop using TerminateThread because I > wanna replace PQcancel there with PQcancelBlocking[1] PQcancelBlocking > does a whole TLS handshake, which is almost certainly taking some locks. > > Note that the way to achieve that I moved the Ctrl+C handler to a > dedicated thread on Unix too, so it starts behaving the same as Windows > in that respect. I think combined with you changing pg_dump to use > worker *threads* on Unix too, we would then get pretty much identical > behaviour across OSes for pg_dump. > > P.S. I now realize that anything involving the (already existing) > CancelRequested flag is actually not actually safe/correct on Windows, > because it's not using read nor written using atomic operations while > the consoleHandler runs on its own thread. Would be good to fix that too > I guess, but it seems that for now it has worked in practice at least. > > [1]: https://www.postgresql.org/message-id/flat/ > DJPAH0WPJV3K.1PYZ8P0QXZVMX%40jeltef.nl#5642e337a4e4d04b21c66e089484f80d > I ran v2 (both patches) on Windows: parallel restore, -j4, into a dump with PK + secondary indexes so there is a CREATE INDEX phase to interrupt. I then ^C in the middle of the index build. The workers canceled cleanly (no orphaned backends, process exits, no hang. No "query failed" spam. Your guard swallows them. I don't know whether this is worth restructuring the code for, but at -v you will get (as an example) pg_restore: while PROCESSING TOC: pg_restore: from TOC entry NNNN; ... INDEX t1_v_idx postgres Those context lines sit above is_cancel_in_progress() check in warn_or_exit_horribly... so the preamble does not get suppressed. The die_on_query_failure is unguarded, it might spam if a ^C happens to land for metadata/SET queries. I did not test that. I may roll your patches onto my branch to test with the multi-threaded queue. -- Bryan Green EDB: https://www.enterprisedb.com ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Can we get rid of TerminateThread() in pg_dump? @ 2026-07-05 05:03 Thomas Munro <[email protected]> parent: Jelte Fennema-Nio <[email protected]> 1 sibling, 2 replies; 18+ messages in thread From: Thomas Munro @ 2026-07-05 05:03 UTC (permalink / raw) To: Jelte Fennema-Nio <[email protected]>; +Cc: Bryan Green <[email protected]>; Heikki Linnakangas <[email protected]>; pgsql-hackers; Tom Lane <[email protected]>; Kyotaro Horiguchi <[email protected]> On Sat, Jul 4, 2026 at 11:15 PM Jelte Fennema-Nio <[email protected]> wrote: > On Sat, 4 Jul 2026 at 02:51, Thomas Munro <[email protected]> wrote: > > We don't actually care about the threads > > themselves, and it doesn't seem that great if we have to introduce an > > IPC ping-pong of some kind with each thread. > > Agreed. But I do agree with Heikki that swapping out stderr seems pretty > hacky. At the very least because now the main thread cannot write to > stderr either anymore (which is why you removed the "terminated by user" > write I guess). > > How about instead we do something like the attached? That's definitely nicer, if we know that all potential error logging caused by cancellation happens in a context that can check the flag. I didn't even look into that, because I was deliberately trying to avoid needing atomics from here, because I need this to work on Unix too, and I didn't want to open too many cans of worms at the same time. Hence the appeal of a simple async-signal-safe system call that has the right concurrency properties already and works also on Windows without a separate code path. But... reaching for the can opener... 1. If we're ready to drop VS < 2022 and GCC < 4.9, we could just use <stdatomic.h> directly in frontend code (independently of the project to use it in the backend). 2. If we're not ready yet we could make "port/atomics.h" or selected parts of it frontend-allowed. 3. Maybe all we really need for this case is memory barriers, and we could move those out to a frontend-allowed header. > To be clear, I do think we should stop using TerminateThread because I > wanna replace PQcancel there with PQcancelBlocking[1] PQcancelBlocking > does a whole TLS handshake, which is almost certainly taking some locks. > > Note that the way to achieve that I moved the Ctrl+C handler to a > dedicated thread on Unix too, so it starts behaving the same as Windows > in that respect. I think combined with you changing pg_dump to use > worker *threads* on Unix too, we would then get pretty much identical > behaviour across OSes for pg_dump. I wondered about something like this too. Nice Unix/Windows convergence, and it sounds like you have no other choice if you want to do fancy non-async-signal-safe footwork. ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Can we get rid of TerminateThread() in pg_dump? @ 2026-07-06 12:18 Jelte Fennema-Nio <[email protected]> parent: Thomas Munro <[email protected]> 1 sibling, 1 reply; 18+ messages in thread From: Jelte Fennema-Nio @ 2026-07-06 12:18 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: Bryan Green <[email protected]>; Heikki Linnakangas <[email protected]>; pgsql-hackers; Tom Lane <[email protected]>; Kyotaro Horiguchi <[email protected]> On Sun, 5 Jul 2026 at 07:03, Thomas Munro <[email protected]> wrote: > I didn't even look into that, because I was deliberately trying to > avoid needing atomics from here, because I need this to work on Unix > too, and I didn't want to open too many cans of worms at the same > time. We do have cross-platform locks on the frontend, so we could use those for now if needed. I don't think this needs atomic for performance reasons and once we have atomics on the frontend it should be easy to swap out the a lock for an atomic operation. > 1. If we're ready to drop VS < 2022 and GCC < 4.9, we could just use > <stdatomic.h> directly in frontend code (independently of the project > to use it in the backend). I think we are ready, see this thread[1]. We'd still need a MacOS version of stdatomic.h though before we could rely on it. [1]: https://www.postgresql.org/message-id/[email protected] ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Can we get rid of TerminateThread() in pg_dump? @ 2026-07-06 12:43 Thomas Munro <[email protected]> parent: Jelte Fennema-Nio <[email protected]> 0 siblings, 0 replies; 18+ messages in thread From: Thomas Munro @ 2026-07-06 12:43 UTC (permalink / raw) To: Jelte Fennema-Nio <[email protected]>; +Cc: Bryan Green <[email protected]>; Heikki Linnakangas <[email protected]>; pgsql-hackers; Tom Lane <[email protected]>; Kyotaro Horiguchi <[email protected]> On Tue, Jul 7, 2026 at 12:18 AM Jelte Fennema-Nio <[email protected]> wrote: > On Sun, 5 Jul 2026 at 07:03, Thomas Munro <[email protected]> wrote: > > 1. If we're ready to drop VS < 2022 and GCC < 4.9, we could just use > > <stdatomic.h> directly in frontend code (independently of the project > > to use it in the backend) > > I think we are ready, see this thread[1]. W00t. > We'd still need a MacOS version of stdatomic.h though before we could > rely on it. Macs have <stdatomic.h>, just not <threads.h>. ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Can we get rid of TerminateThread() in pg_dump? @ 2026-07-06 21:11 Heikki Linnakangas <[email protected]> parent: Thomas Munro <[email protected]> 1 sibling, 1 reply; 18+ messages in thread From: Heikki Linnakangas @ 2026-07-06 21:11 UTC (permalink / raw) To: Thomas Munro <[email protected]>; Jelte Fennema-Nio <[email protected]>; +Cc: Bryan Green <[email protected]>; pgsql-hackers; Tom Lane <[email protected]>; Kyotaro Horiguchi <[email protected]> On 05/07/2026 08:03, Thomas Munro wrote: > On Sat, Jul 4, 2026 at 11:15 PM Jelte Fennema-Nio <[email protected]> wrote: >> On Sat, 4 Jul 2026 at 02:51, Thomas Munro <[email protected]> wrote: >>> We don't actually care about the threads >>> themselves, and it doesn't seem that great if we have to introduce an >>> IPC ping-pong of some kind with each thread. >> >> Agreed. But I do agree with Heikki that swapping out stderr seems pretty >> hacky. At the very least because now the main thread cannot write to >> stderr either anymore (which is why you removed the "terminated by user" >> write I guess). >> >> How about instead we do something like the attached? > > That's definitely nicer, if we know that all potential error logging > caused by cancellation happens in a context that can check the flag. +1, much nicer! > I didn't even look into that, because I was deliberately trying to > avoid needing atomics from here, because I need this to work on Unix > too, and I didn't want to open too many cans of worms at the same > time. Hence the appeal of a simple async-signal-safe system call that > has the right concurrency properties already and works also on Windows > without a separate code path. But... reaching for the can opener... > > 1. If we're ready to drop VS < 2022 and GCC < 4.9, we could just use > <stdatomic.h> directly in frontend code (independently of the project > to use it in the backend). > 2. If we're not ready yet we could make "port/atomics.h" or selected > parts of it frontend-allowed. > 3. Maybe all we really need for this case is memory barriers, and we > could move those out to a frontend-allowed header. To be honest, I didn't realize we didn't allow "port/atomics.h" in frontend code. I think spinlock-simulated 64-bit atomics is the only thing that wouldn't just work. - Heikki ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Can we get rid of TerminateThread() in pg_dump? @ 2026-07-06 21:23 Heikki Linnakangas <[email protected]> parent: Heikki Linnakangas <[email protected]> 0 siblings, 1 reply; 18+ messages in thread From: Heikki Linnakangas @ 2026-07-06 21:23 UTC (permalink / raw) To: Thomas Munro <[email protected]>; Jelte Fennema-Nio <[email protected]>; +Cc: Bryan Green <[email protected]>; pgsql-hackers; Tom Lane <[email protected]>; Kyotaro Horiguchi <[email protected]> On 07/07/2026 00:11, Heikki Linnakangas wrote: > On 05/07/2026 08:03, Thomas Munro wrote: >> On Sat, Jul 4, 2026 at 11:15 PM Jelte Fennema-Nio <[email protected]> >> wrote: >>> On Sat, 4 Jul 2026 at 02:51, Thomas Munro <[email protected]> >>> wrote: >>>> We don't actually care about the threads >>>> themselves, and it doesn't seem that great if we have to introduce an >>>> IPC ping-pong of some kind with each thread. >>> >>> Agreed. But I do agree with Heikki that swapping out stderr seems pretty >>> hacky. At the very least because now the main thread cannot write to >>> stderr either anymore (which is why you removed the "terminated by user" >>> write I guess). >>> >>> How about instead we do something like the attached? >> >> That's definitely nicer, if we know that all potential error logging >> caused by cancellation happens in a context that can check the flag. > > +1, much nicer! > >> I didn't even look into that, because I was deliberately trying to >> avoid needing atomics from here, because I need this to work on Unix >> too, and I didn't want to open too many cans of worms at the same >> time. Hence the appeal of a simple async-signal-safe system call that >> has the right concurrency properties already and works also on Windows >> without a separate code path. But... reaching for the can opener... >> >> 1. If we're ready to drop VS < 2022 and GCC < 4.9, we could just use >> <stdatomic.h> directly in frontend code (independently of the project >> to use it in the backend). >> 2. If we're not ready yet we could make "port/atomics.h" or selected >> parts of it frontend-allowed. >> 3. Maybe all we really need for this case is memory barriers, and we >> could move those out to a frontend-allowed header. > > To be honest, I didn't realize we didn't allow "port/atomics.h" in > frontend code. I think spinlock-simulated 64-bit atomics is the only > thing that wouldn't just work. In this case, though, I think all we need is a "volatile sigatomic_t" flag. Sending the query cancellation over the network surely acts as a full compiler and memory barrier in the cancelling thread. And similarly receiving the error message from the network acts as a full barrier in the other threads that might receive the cancellation error from the backend. - Heikki ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Can we get rid of TerminateThread() in pg_dump? @ 2026-07-07 23:40 Thomas Munro <[email protected]> parent: Heikki Linnakangas <[email protected]> 0 siblings, 1 reply; 18+ messages in thread From: Thomas Munro @ 2026-07-07 23:40 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: Jelte Fennema-Nio <[email protected]>; Bryan Green <[email protected]>; pgsql-hackers; Tom Lane <[email protected]>; Kyotaro Horiguchi <[email protected]> On Tue, Jul 7, 2026 at 9:23 AM Heikki Linnakangas <[email protected]> wrote: > In this case, though, I think all we need is a "volatile sigatomic_t" > flag. Sending the query cancellation over the network surely acts as a > full compiler and memory barrier in the cancelling thread. And similarly > receiving the error message from the network acts as a full barrier in > the other threads that might receive the cancellation error from the > backend. You're right. So basically Jelte's patch, except it doesn't need the Win32 atomics stuff, just volatile, and a comment to explain that assumption. (Then some later version could use an explicit barrier instead of a comment, I guess, just to be clearer.) ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Can we get rid of TerminateThread() in pg_dump? @ 2026-07-08 08:47 Jelte Fennema-Nio <[email protected]> parent: Thomas Munro <[email protected]> 0 siblings, 1 reply; 18+ messages in thread From: Jelte Fennema-Nio @ 2026-07-08 08:47 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; Bryan Green <[email protected]>; pgsql-hackers; Tom Lane <[email protected]>; Kyotaro Horiguchi <[email protected]> On Wed, Jul 8, 2026, 01:41 Thomas Munro <[email protected]> wrote: > On Tue, Jul 7, 2026 at 9:23 AM Heikki Linnakangas <[email protected]> wrote: > > In this case, though, I think all we need is a "volatile sigatomic_t" > > flag. Sending the query cancellation over the network surely acts as a > > full compiler and memory barrier in the cancelling thread. And similarly > > receiving the error message from the network acts as a full barrier in > > the other threads that might receive the cancellation error from the > > backend. > > You're right. So basically Jelte's patch, except it doesn't need the > Win32 atomics stuff, just volatile, and a comment to explain that > assumption. (Then some later version could use an explicit barrier > instead of a comment, I guess, just to be clearer.) Without any synchronization primitives there's still room for a data-race, right? But I guess in practice that doesn't matter for the messages we actually care about. Attached is a patch with that change, altough I'm using "volatile bool" instead of "volatile sigatomic_t" since there are no signal handlers involved here. I also added !is_cancel_in_progress() checks in a few more places to silence the places that Bryan had found. To be clear: putting all of these threads together I'd like to remove this function again completely. Instead I'd like this to use CancelRequested for this, but that requires my PQblockingCancel patchset to be merged first. And then I'd like to make CancelRequested a C11 atomic_flag instead of a volatile. Attachments: [text/x-patch] v3-0001-pg_dump-Remove-TerminateThread-call.patch (12.8K, ../../[email protected]/2-v3-0001-pg_dump-Remove-TerminateThread-call.patch) download | inline diff: From cfa1764ccc16552988c0deb7190c0b8ffb5b2215 Mon Sep 17 00:00:00 2001 From: Jelte Fennema-Nio <[email protected]> Date: Sat, 4 Jul 2026 11:11:09 +0200 Subject: [PATCH v3] pg_dump: Remove TerminateThread() call When pg_dump or pg_restore --jobs N is interrupted with ^C on Windows, we cancel all queries, but we don't want the cancellations to be reported as errors to the user in the short time before the whole process exits. That was previously achieved by calling TerminateThread() on each worker thread before sending the cancel message, but that doesn't appear to be 100% safe: the implementations of write() and the socket calls inside PQcancel() might acquire user space locks that were held by the terminated threads. (write() certainly does that.) Instead of silencing the threads in such a sketchy way this now sets a volatile flag (before sending any cancel requests) that tells the threads to not log errors anymore. Instead of a volatile, it would be better to use an atomic operation here, that has to wait until we add support for atomics on the frontend though. Note that this also stops using pg_fatal and exit to exit from workers on failure and instead use pg_log_error combined with exit_nicely. If a query fails in a worker we want it to kill the worker not the whole process. On Unix that's currently the same thing, but on Windows workers are threads. --- src/bin/pg_dump/parallel.c | 50 ++++++++++---------- src/bin/pg_dump/pg_backup_archiver.c | 69 +++++++++++++++------------- src/bin/pg_dump/pg_backup_db.c | 30 ++++++++---- src/bin/pg_dump/pg_backup_utils.c | 36 +++++++++++++++ src/bin/pg_dump/pg_backup_utils.h | 21 +++++++++ 5 files changed, 139 insertions(+), 67 deletions(-) diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c index b77d2650df0..1bbcf91cd1f 100644 --- a/src/bin/pg_dump/parallel.c +++ b/src/bin/pg_dump/parallel.c @@ -531,11 +531,12 @@ WaitForTerminatingWorkers(ParallelState *pstate) * might be that only the leader gets signaled. * * On Windows, the cancel handler runs in a separate thread, because that's - * how SetConsoleCtrlHandler works. We make it stop worker threads, send - * cancels on all active connections, and then return FALSE, which will allow - * the process to die. For safety's sake, we use a critical section to - * protect the PGcancel structures against being changed while the signal - * thread runs. + * how SetConsoleCtrlHandler works. Because the workers are threads in this + * same process, we set a flag (is_cancel_in_progress()) so they stay quiet about + * the query cancellations instead of cluttering the screen, then send cancels + * on all active connections and return FALSE, which will allow the process to + * die. For safety's sake, we use a critical section to protect the PGcancel + * structures against being changed while the signal thread runs. */ #ifndef WIN32 @@ -641,34 +642,30 @@ consoleHandler(DWORD dwCtrlType) if (dwCtrlType == CTRL_C_EVENT || dwCtrlType == CTRL_BREAK_EVENT) { + /* + * Tell worker threads to stay quiet about the query cancellations + * we're about to send them; otherwise they'd report them as errors + * and clutter the user's screen. This must be set before we send any + * cancel, so that a worker is guaranteed to see it by the time its + * query fails as a result. + */ + set_cancel_in_progress(); + /* Critical section prevents changing data we look at here */ EnterCriticalSection(&signal_info_lock); /* - * If in parallel mode, stop worker threads and send QueryCancel to - * their connected backends. The main point of stopping the worker - * threads is to keep them from reporting the query cancels as errors, - * which would clutter the user's screen. We needn't stop the leader - * thread since it won't be doing much anyway. Do this before - * canceling the main transaction, else we might get invalid-snapshot - * errors reported before we can stop the workers. Ignore errors, - * there's not much we can do about them anyway. + * If in parallel mode, send QueryCancel to each worker's connected + * backend. Do this before canceling the main transaction, else we + * might get invalid-snapshot errors reported before we can stop the + * workers. Ignore errors, there's not much we can do about them + * anyway. */ if (signal_info.pstate != NULL) { for (i = 0; i < signal_info.pstate->numWorkers; i++) { - ParallelSlot *slot = &(signal_info.pstate->parallelSlot[i]); - ArchiveHandle *AH = slot->AH; - HANDLE hThread = (HANDLE) slot->hThread; - - /* - * Using TerminateThread here may leave some resources leaked, - * but it doesn't matter since we're about to end the whole - * process. - */ - if (hThread != INVALID_HANDLE_VALUE) - TerminateThread(hThread, 0); + ArchiveHandle *AH = signal_info.pstate->parallelSlot[i].AH; if (AH != NULL && AH->connCancel != NULL) (void) PQcancel(AH->connCancel, errbuf, sizeof(errbuf)); @@ -687,9 +684,8 @@ consoleHandler(DWORD dwCtrlType) /* * Report we're quitting, using nothing more complicated than - * write(2). (We might be able to get away with using pg_log_*() - * here, but since we terminated other threads uncleanly above, it - * seems better to assume as little as possible.) + * write(2). We should be able to use pg_log_*() here, but for now we + * stay aligned with the sigTermHandler behavior. */ if (progname) { diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c index 0557eb6d6ed..77cc50e0607 100644 --- a/src/bin/pg_dump/pg_backup_archiver.c +++ b/src/bin/pg_dump/pg_backup_archiver.c @@ -1896,47 +1896,52 @@ ahwrite(const void *ptr, size_t size, size_t nmemb, ArchiveHandle *AH) void warn_or_exit_horribly(ArchiveHandle *AH, const char *fmt, ...) { - va_list ap; - - switch (AH->stage) + /* Stay quiet if this is a result of our own cancellation. */ + if (!is_cancel_in_progress()) { + va_list ap; - case STAGE_NONE: - /* Do nothing special */ - break; + switch (AH->stage) + { - case STAGE_INITIALIZING: - if (AH->stage != AH->lastErrorStage) - pg_log_info("while INITIALIZING:"); - break; + case STAGE_NONE: + /* Do nothing special */ + break; - case STAGE_PROCESSING: - if (AH->stage != AH->lastErrorStage) - pg_log_info("while PROCESSING TOC:"); - break; + case STAGE_INITIALIZING: + if (AH->stage != AH->lastErrorStage) + pg_log_info("while INITIALIZING:"); + break; - case STAGE_FINALIZING: - if (AH->stage != AH->lastErrorStage) - pg_log_info("while FINALIZING:"); - break; - } - if (AH->currentTE != NULL && AH->currentTE != AH->lastErrorTE) - { - pg_log_info("from TOC entry %d; %u %u %s %s %s", - AH->currentTE->dumpId, - AH->currentTE->catalogId.tableoid, - AH->currentTE->catalogId.oid, - AH->currentTE->desc ? AH->currentTE->desc : "(no desc)", - AH->currentTE->tag ? AH->currentTE->tag : "(no tag)", - AH->currentTE->owner ? AH->currentTE->owner : "(no owner)"); + case STAGE_PROCESSING: + if (AH->stage != AH->lastErrorStage) + pg_log_info("while PROCESSING TOC:"); + break; + + case STAGE_FINALIZING: + if (AH->stage != AH->lastErrorStage) + pg_log_info("while FINALIZING:"); + break; + } + if (AH->currentTE != NULL && AH->currentTE != AH->lastErrorTE) + { + pg_log_info("from TOC entry %d; %u %u %s %s %s", + AH->currentTE->dumpId, + AH->currentTE->catalogId.tableoid, + AH->currentTE->catalogId.oid, + AH->currentTE->desc ? AH->currentTE->desc : "(no desc)", + AH->currentTE->tag ? AH->currentTE->tag : "(no tag)", + AH->currentTE->owner ? AH->currentTE->owner : "(no owner)"); + } + + va_start(ap, fmt); + pg_log_generic_v(PG_LOG_ERROR, PG_LOG_PRIMARY, fmt, ap); + va_end(ap); } + AH->lastErrorStage = AH->stage; AH->lastErrorTE = AH->currentTE; - va_start(ap, fmt); - pg_log_generic_v(PG_LOG_ERROR, PG_LOG_PRIMARY, fmt, ap); - va_end(ap); - if (AH->public.exit_on_error) exit_nicely(1); else diff --git a/src/bin/pg_dump/pg_backup_db.c b/src/bin/pg_dump/pg_backup_db.c index ec0ddf1d718..17c0b7cbdf2 100644 --- a/src/bin/pg_dump/pg_backup_db.c +++ b/src/bin/pg_dump/pg_backup_db.c @@ -207,10 +207,14 @@ notice_processor(void *arg, const char *message) static void die_on_query_failure(ArchiveHandle *AH, const char *query) { - pg_log_error("query failed: %s", - PQerrorMessage(AH->connection)); - pg_log_error_detail("Query was: %s", query); - exit(1); + if (!is_cancel_in_progress()) + { + pg_log_error("query failed: %s", + PQerrorMessage(AH->connection)); + pg_log_error_detail("Query was: %s", query); + } + + exit_nicely(1); } void @@ -396,8 +400,13 @@ ExecuteSqlCommandBuf(Archive *AHX, const char *buf, size_t bufLen) */ if (AH->pgCopyIn && PQputCopyData(AH->connection, buf, bufLen) <= 0) - pg_fatal("error returned by PQputCopyData: %s", - PQerrorMessage(AH->connection)); + { + /* Stay quiet if this is a result of our own cancellation. */ + if (!is_cancel_in_progress()) + pg_log_error("error returned by PQputCopyData: %s", + PQerrorMessage(AH->connection)); + exit_nicely(1); + } } else if (AH->outputKind == OUTPUT_OTHERDATA) { @@ -445,8 +454,13 @@ EndDBCopyMode(Archive *AHX, const char *tocEntryTag) PGresult *res; if (PQputCopyEnd(AH->connection, NULL) <= 0) - pg_fatal("error returned by PQputCopyEnd: %s", - PQerrorMessage(AH->connection)); + { + /* Stay quiet if this is a result of our own cancellation. */ + if (!is_cancel_in_progress()) + pg_log_error("error returned by PQputCopyEnd: %s", + PQerrorMessage(AH->connection)); + exit_nicely(1); + } /* Check command status and return to normal libpq state */ res = PQgetResult(AH->connection); diff --git a/src/bin/pg_dump/pg_backup_utils.c b/src/bin/pg_dump/pg_backup_utils.c index 0368f7623a7..a291b9953e6 100644 --- a/src/bin/pg_dump/pg_backup_utils.c +++ b/src/bin/pg_dump/pg_backup_utils.c @@ -21,6 +21,42 @@ /* Globals exported by this file */ const char *progname = NULL; +#ifdef WIN32 + +/* + * Flag telling worker threads to stay quiet about query failures because we're + * cancelling their queries as part of tearing down the process. See the + * comment in pg_backup_utils.h. + * + * The cancel thread writes it while worker threads read it, so it's marked + * volatile to keep the compiler from caching the value. A plain volatile bool + * isn't a real memory barrier, but it's good enough here: the flag is only ever + * flipped one way (false to true) and a worker briefly observing the stale + * false just means it prints one error before the process dies. The only goal + * of this flag is to make sure workers don't log "query cancelled" errors + * during the shutdown process. When such an error is received that acts as a + * full barrier so for the messages that we care about a stale value won't be + * observed. + * + * XXX: This should be swapped out for a proper atomic when we have those in + * the frontend code. + */ +static volatile bool cancelInProgress = false; + +void +set_cancel_in_progress(void) +{ + cancelInProgress = true; +} + +bool +is_cancel_in_progress(void) +{ + return cancelInProgress; +} + +#endif /* WIN32 */ + #define MAX_ON_EXIT_NICELY 20 static struct diff --git a/src/bin/pg_dump/pg_backup_utils.h b/src/bin/pg_dump/pg_backup_utils.h index 9e98ed11619..31e66b61198 100644 --- a/src/bin/pg_dump/pg_backup_utils.h +++ b/src/bin/pg_dump/pg_backup_utils.h @@ -31,6 +31,27 @@ extern void set_dump_section(const char *arg, int *dumpSections); extern void on_exit_nicely(on_exit_nicely_callback function, void *arg); pg_noreturn extern void exit_nicely(int code); +/* + * On Windows the parallel workers are threads inside the leader process. When + * a cancel is processed there, the leader sends cancels to the workers' + * in-flight queries; without this flag each worker would then report the + * resulting "canceling statement due to user request" error and clutter the + * screen in the brief window before the whole process exits. The cancel + * thread sets this flag before sending any cancel, and worker threads check it + * before reporting a query failure. It's written and read from different + * threads, so it's marked volatile; see the comment in pg_backup_utils.c. + * + * On other platforms the workers are separate processes that just _exit() when + * cancelled, so they never reach the error-reporting code; there the check is + * compiled out to a constant false and the flag doesn't exist. + */ +#ifdef WIN32 +extern void set_cancel_in_progress(void); +extern bool is_cancel_in_progress(void); +#else +#define is_cancel_in_progress() false +#endif + /* In pg_dump, we modify pg_fatal to call exit_nicely instead of exit */ #undef pg_fatal #define pg_fatal(...) do { \ base-commit: 07211f64ace0150c92a00769a1cfe8b9305b9e78 -- 2.54.0 ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Can we get rid of TerminateThread() in pg_dump? @ 2026-07-08 15:49 Heikki Linnakangas <[email protected]> parent: Jelte Fennema-Nio <[email protected]> 0 siblings, 1 reply; 18+ messages in thread From: Heikki Linnakangas @ 2026-07-08 15:49 UTC (permalink / raw) To: Jelte Fennema-Nio <[email protected]>; Thomas Munro <[email protected]>; +Cc: Bryan Green <[email protected]>; pgsql-hackers; Tom Lane <[email protected]>; Kyotaro Horiguchi <[email protected]> On 08/07/2026 11:47, Jelte Fennema-Nio wrote: > On Wed, Jul 8, 2026, 01:41 Thomas Munro <[email protected]> wrote: > >> On Tue, Jul 7, 2026 at 9:23 AM Heikki Linnakangas <[email protected]> >> wrote: >> > In this case, though, I think all we need is a "volatile sigatomic_t" >> > flag. Sending the query cancellation over the network surely acts as a >> > full compiler and memory barrier in the cancelling thread. And >> similarly >> > receiving the error message from the network acts as a full barrier in >> > the other threads that might receive the cancellation error from the >> > backend. >> >> You're right. So basically Jelte's patch, except it doesn't need the >> Win32 atomics stuff, just volatile, and a comment to explain that >> assumption. (Then some later version could use an explicit barrier >> instead of a comment, I guess, just to be clearer.) > > Without any synchronization primitives there's still room for a > data-race, right? The network operations on the cancel thread (to send cancellation) and on the other thread (to receive the error messages from the server) should act as reliable memory barriers. > But I guess in practice that doesn't matter for the messages we > actually care about. Also true. > Attached is a patch with that change, altough I'm using "volatile > bool" instead of "volatile sigatomic_t" since there are no signal > handlers involved here. > > I also added !is_cancel_in_progress() checks in a few more places to > silence the places that Bryan had found. Ok, committed, thank you! > To be clear: putting all of these threads together I'd like to remove > this function again completely. Instead I'd like this to use > CancelRequested for this, but that requires my PQblockingCancel patchset > to be merged first. And then I'd like to make CancelRequested a C11 > atomic_flag instead of a volatile. Ack. I'm getting a little confused by all the interdependent patches flying around. But as long as we keep grinding, committing what we can one patch at a time and rebasing all remaining ones, we'll be done eventually :-). - Heikki ^ permalink raw reply [nested|flat] 18+ messages in thread
* Re: Can we get rid of TerminateThread() in pg_dump? @ 2026-07-08 18:13 Jelte Fennema-Nio <[email protected]> parent: Heikki Linnakangas <[email protected]> 0 siblings, 0 replies; 18+ messages in thread From: Jelte Fennema-Nio @ 2026-07-08 18:13 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: Thomas Munro <[email protected]>; Bryan Green <[email protected]>; pgsql-hackers; Tom Lane <[email protected]>; Kyotaro Horiguchi <[email protected]> On Wed, 8 Jul 2026 at 17:49, Heikki Linnakangas <[email protected]> wrote: > > Without any synchronization primitives there's still room for a > > data-race, right? > > The network operations on the cancel thread (to send cancellation) and > on the other thread (to receive the error messages from the server) > should act as reliable memory barriers. This is true for errors originating from the cancellation, but not for an unluckily timed random other error. If an error occurs in a worker query at the same time that the cancel flag is flipped, then the worker thread could read the cancel_in_progress flag. In practice the read would return either true or false and both values are acceptable, so it doesn't matter. But strictly speaking, such a read pattern is a data race that violates the C abstract machine and thus results in undefined behaviour. But like I said in practice it doesn't matter, especially if we swap this out for an atomic operation soon(tm). > Ok, committed, thank you! Awesome! > Ack. I'm getting a little confused by all the interdependent patches > flying around. But as long as we keep grinding, committing what we can > one patch at a time and rebasing all remaining ones, we'll be done > eventually :-). Yeah, agreed. Thanks for closing this thread out at least. ^ permalink raw reply [nested|flat] 18+ messages in thread
end of thread, other threads:[~2026-07-08 18:13 UTC | newest] Thread overview: 18+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-07-03 03:32 Can we get rid of TerminateThread() in pg_dump? Thomas Munro <[email protected]> 2026-07-03 13:55 ` Bryan Green <[email protected]> 2026-07-04 00:38 ` Thomas Munro <[email protected]> 2026-07-04 14:48 ` Bryan Green <[email protected]> 2026-07-03 15:05 ` Heikki Linnakangas <[email protected]> 2026-07-03 15:35 ` Bryan Green <[email protected]> 2026-07-04 00:51 ` Thomas Munro <[email protected]> 2026-07-04 11:15 ` Jelte Fennema-Nio <[email protected]> 2026-07-04 15:24 ` Bryan Green <[email protected]> 2026-07-05 05:03 ` Thomas Munro <[email protected]> 2026-07-06 12:18 ` Jelte Fennema-Nio <[email protected]> 2026-07-06 12:43 ` Thomas Munro <[email protected]> 2026-07-06 21:11 ` Heikki Linnakangas <[email protected]> 2026-07-06 21:23 ` Heikki Linnakangas <[email protected]> 2026-07-07 23:40 ` Thomas Munro <[email protected]> 2026-07-08 08:47 ` Jelte Fennema-Nio <[email protected]> 2026-07-08 15:49 ` Heikki Linnakangas <[email protected]> 2026-07-08 18:13 ` Jelte Fennema-Nio <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox