0001-pg_dump-Remove-TerminateThread-call.patch
application/x-patch
Filename: 0001-pg_dump-Remove-TerminateThread-call.patch
Type: application/x-patch
Part: 0
Patch
Format: format-patch
Series: patch 0001
Subject: pg_dump: Remove TerminateThread() call.
| File | + | − |
|---|---|---|
| src/bin/pg_dump/parallel.c | 33 | 31 |
From be2654c76896a9d916778d8663a5da043135c8e0 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
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