From 9ee6687674085271b1b123f0a3c0891607a7e30d Mon Sep 17 00:00:00 2001 From: Thomas Munro Date: Thu, 2 Jul 2026 21:30:44 +1200 Subject: [PATCH v1 09/14] 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. (UCRT's write() certainly does that.) Remove the TerminateThread() call and adopt a new approach that would also work on Unix (in later patches). This also prepares for using pg_threads.h, which doesn't have a terminate operation on principle. The new approach is to replace STDERR_FILENO with the null device atomically with dup2(), to swallow any future write() or fwrite() output after printing our goodbye message. Also change write(fileno(stderr), ...) to write(STDERR_FILENO, ...), as commit c290e79c allowed for, just in case stderr has been closed and fileno() returns a junk value. Discussion: https://postgr.es/m/CA%2BhUKGJgO%3Do-vLFahGdR2WesuX3h1-0j%3Da8z72fChc-MG1Hveg%40mail.gmail.com Reviewed-by: Bryan Green --- src/bin/pg_dump/parallel.c | 63 +++++++++++++++++++------------------- 1 file changed, 31 insertions(+), 32 deletions(-) diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c index 3e84b881ca3..2381fa405ba 100644 --- a/src/bin/pg_dump/parallel.c +++ b/src/bin/pg_dump/parallel.c @@ -56,10 +56,11 @@ #include #include #include -#include -#include #endif +#include +#include + #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,38 @@ 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. This + * prevents worker threads from reporting cancellation-related errors, + * which might clutter the user's screen if they manage arrive before + * the whole process exits and terminates them. Ignore errors. + */ + if ((null_device = open(DEVNULL, 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. 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 +681,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 +696,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