Can we get rid of TerminateThread() in pg_dump?

On 7/2/2026 10:32 PM, Thomas Munro wrote:

Hi,

Following up on this ancient discussion and resulting commit e652273e...

/messages/by-id/11515.1464961470@sss.pgh.pa.us

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]/messages/by-id/8c712d76-ecf7-4749-a6d8-dddc01f298ec@gmail.com, 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]: /messages/by-id/8c712d76-ecf7-4749-a6d8-dddc01f298ec@gmail.com
/messages/by-id/8c712d76-ecf7-4749-a6d8-dddc01f298ec@gmail.com

--
Bryan Green
EDB: https://www.enterprisedb.com

On 03/07/2026 06:32, Thomas Munro wrote:

Following up on this ancient discussion and resulting commit e652273e...

/messages/by-id/11515.1464961470@sss.pgh.pa.us

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

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

On Sat, Jul 4, 2026 at 1:56 AM Bryan Green <dbryan.green@gmail.com> 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.

On Sat, Jul 4, 2026 at 3:36 AM Bryan Green <dbryan.green@gmail.com> 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?

On Sat, 4 Jul 2026 at 02:51, Thomas Munro <thomas.munro@gmail.com> 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]/messages/by-id/DJPAH0WPJV3K.1PYZ8P0QXZVMX@jeltef.nl 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]: /messages/by-id/DJPAH0WPJV3K.1PYZ8P0QXZVMX@jeltef.nl

On 7/3/2026 7:38 PM, Thomas Munro wrote:

On Sat, Jul 4, 2026 at 1:56 AM Bryan Green <dbryan.green@gmail.com> 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

On 7/4/2026 6:15 AM, Jelte Fennema-Nio wrote:

On Sat, 4 Jul 2026 at 02:51, Thomas Munro <thomas.munro@gmail.com> 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]: /messages/by-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

On Sat, Jul 4, 2026 at 11:15 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:

On Sat, 4 Jul 2026 at 02:51, Thomas Munro <thomas.munro@gmail.com> 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.

On Sun, 5 Jul 2026 at 07:03, Thomas Munro <thomas.munro@gmail.com> 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]/messages/by-id/2a965ac6-fa42-4054-bee0-b1618e7729d6@eisentraut.org.

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

[1]: /messages/by-id/2a965ac6-fa42-4054-bee0-b1618e7729d6@eisentraut.org

On Tue, Jul 7, 2026 at 12:18 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:

On Sun, 5 Jul 2026 at 07:03, Thomas Munro <thomas.munro@gmail.com> 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>.

On 05/07/2026 08:03, Thomas Munro wrote:

On Sat, Jul 4, 2026 at 11:15 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:

On Sat, 4 Jul 2026 at 02:51, Thomas Munro <thomas.munro@gmail.com> 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

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 <postgres@jeltef.nl>
wrote:

On Sat, 4 Jul 2026 at 02:51, Thomas Munro <thomas.munro@gmail.com>
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

On Tue, Jul 7, 2026 at 9:23 AM Heikki Linnakangas <hlinnaka@iki.fi> 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.)

On Wed, Jul 8, 2026, 01:41 Thomas Munro <thomas.munro@gmail.com> wrote:

On Tue, Jul 7, 2026 at 9:23 AM Heikki Linnakangas <hlinnaka@iki.fi> 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.

On 08/07/2026 11:47, Jelte Fennema-Nio wrote:

On Wed, Jul 8, 2026, 01:41 Thomas Munro <thomas.munro@gmail.com> wrote:

On Tue, Jul 7, 2026 at 9:23 AM Heikki Linnakangas <hlinnaka@iki.fi>
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

On Wed, 8 Jul 2026 at 17:49, Heikki Linnakangas <hlinnaka@iki.fi> 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.