Subject: Can we get rid of TerminateThread() in pg_dump?

From: Thomas Munro <[email-masked]>
Date: Fri, 3 Jul 2026 15:32:54 +1200

Hi,

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).

(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

---

From: Bryan Green <[email-masked]>
Date: Fri, 3 Jul 2026 08:55:40 -0500

> [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

---

From: Heikki Linnakangas <[email-masked]>
Date: Fri, 3 Jul 2026 18:05:14 +0300

> (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

---

From: Bryan Green <[email-masked]>
Date: Fri, 3 Jul 2026 10:35:35 -0500

On 7/3/2026 10:05 AM, Heikki Linnakangas wrote:
>> https://www.postgresql.org/message-id/
>> flat/11515.1464961470%40sss.pgh.pa.us#3e78a2af445dd7e566cf499023e8cb97
>>
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).

---

From: Thomas Munro <[email-masked]>
Date: Sat, 4 Jul 2026 12:38:53 +1200

> call still in place, and the output is identical-- "terminated by user"

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?

> 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.

> 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.

> TerminateThread never did. So your change composes with that work.

Interesting.

---

From: Thomas Munro <[email-masked]>
Date: Sat, 4 Jul 2026 12:51:08 +1200

> 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?

---

From: "Jelte Fennema-Nio" <[email-masked]>
Date: Sat, 04 Jul 2026 13:15:09 +0200

On Sat, 4 Jul 2026 at 02:51, Thomas Munro <[email-masked]> wrote:
> We don't actually care about the threads
> 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#5642e337a4e4d04b21c66e089484f80d
From a283180ad3b3bffe8ae04d59f61212ff95ccbee6 Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio <[email-masked]>
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

From 87c6cbd43685b2f158fd68cc5fd8e15a0cd1b44a Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio <[email-masked]>
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

---

From: Bryan Green <[email-masked]>
Date: Sat, 4 Jul 2026 09:48:05 -0500

> 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.

---

From: Bryan Green <[email-masked]>
Date: Sat, 4 Jul 2026 10:24:24 -0500

> stderr either anymore (which is why you removed the "terminated by user"
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.
> [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.

---

From: Thomas Munro <[email-masked]>
Date: Sun, 5 Jul 2026 17:03:08 +1200

> 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.

> 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.

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Mon, 6 Jul 2026 14:18:20 +0200

On Sun, 5 Jul 2026 at 07:03, Thomas Munro <[email-masked]> wrote:
> 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.

> 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-masked]

---

From: Thomas Munro <[email-masked]>
Date: Tue, 7 Jul 2026 00:43:46 +1200

On Tue, Jul 7, 2026 at 12:18 AM Jelte Fennema-Nio <[email-masked]> wrote:
> > to use it in the backend)
>

W00t.

> We'd still need a MacOS version of stdatomic.h though before we could

Macs have <stdatomic.h>, just not <threads.h>.

---

From: Heikki Linnakangas <[email-masked]>
Date: Tue, 7 Jul 2026 00:11:54 +0300

> caused by cancellation happens in a context that can check the flag.

+1, much nicer!

> 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

---

From: Heikki Linnakangas <[email-masked]>
Date: Tue, 7 Jul 2026 00:23:42 +0300

> 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

---

From: Thomas Munro <[email-masked]>
Date: Wed, 8 Jul 2026 11:40:23 +1200

> the other threads that might receive the cancellation error from the

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.)

---

From: "Jelte Fennema-Nio" <[email-masked]>
Date: Wed, 08 Jul 2026 10:47:52 +0200

> 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.
From cfa1764ccc16552988c0deb7190c0b8ffb5b2215 Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio <[email-masked]>
Date: Sat, 4 Jul 2026 11:11:09 +0200
Subject: [PATCH v3] pg_dump: Remove TerminateThread() call

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(-)

index b77d2650df0..1bbcf91cd1f 100644
 
 #ifndef WIN32
 
 		/*
 
 				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 &&
@@ -445,8 +454,13 @@ EndDBCopyMode(Archive *AHX, const char *tocEntryTag)
 		PGresult   *res;

index 0368f7623a7..a291b9953e6 100644
@@ -21,6 +21,42 @@
 
+#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
index 9e98ed11619..31e66b61198 100644
 
+/*
+ * threads, so it's marked volatile; see the comment in pg_backup_utils.c.

base-commit: 07211f64ace0150c92a00769a1cfe8b9305b9e78
-- 
2.54.0

---

From: Heikki Linnakangas <[email-masked]>
Date: Wed, 8 Jul 2026 18:49:47 +0300

On 08/07/2026 11:47, Jelte Fennema-Nio wrote:
> 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.
> 

Ok, committed, thank you!

> 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

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Wed, 8 Jul 2026 20:13:56 +0200

> 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!

> one patch at a time and rebasing all remaining ones, we'll be done

Yeah, agreed. Thanks for closing this thread out at least.