Re: Can we get rid of TerminateThread() in pg_dump?
Re: Can we get rid of TerminateThread() in pg_dump?
От:
Bryan Green <dbryan.green@gmail.com>
Дата:
On 7/4/2026 6:15 AM, Jelte Fennema-Nio wrote: > On Sat, 4 Jul 2026 at 02:51, Thomas Munro 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
Re: Can we get rid of TerminateThread() in pg_dump?
От:
Bryan Green <dbryan.green@gmail.com>
Дата:
On 7/3/2026 7:38 PM, Thomas Munro wrote:
> On Sat, Jul 4, 2026 at 1:56 AM Bryan Green 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
Re: Can we get rid of TerminateThread() in pg_dump?
От:
Heikki Linnakangas <hlinnaka@iki.fi>
Дата:
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 >> wrote: >>> On Sat, 4 Jul 2026 at 02:51, Thomas Munro >>> 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 >> 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
Re: Can we get rid of TerminateThread() in pg_dump?
От:
Heikki Linnakangas <hlinnaka@iki.fi>
Дата:
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
Re: Can we get rid of TerminateThread() in pg_dump?
От:
Heikki Linnakangas <hlinnaka@iki.fi>
Дата:
On 05/07/2026 08:03, Thomas Munro wrote: > On Sat, Jul 4, 2026 at 11:15 PM Jelte Fennema-Nio wrote: >> On Sat, 4 Jul 2026 at 02:51, Thomas Munro 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 > 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
Re: Can we get rid of TerminateThread() in pg_dump?
От:
Heikki Linnakangas <hlinnaka@iki.fi>
Дата:
On 08/07/2026 11:47, Jelte Fennema-Nio wrote: > On Wed, Jul 8, 2026, 01:41 Thomas Munro wrote: > >> On Tue, Jul 7, 2026 at 9:23 AM Heikki Linnakangas >> 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
Re: Can we get rid of TerminateThread() in pg_dump?
От:
Thomas Munro <thomas.munro@gmail.com>
Дата:
On Tue, Jul 7, 2026 at 9:23 AM Heikki Linnakangas 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.)
Re: Can we get rid of TerminateThread() in pg_dump?
От:
Thomas Munro <thomas.munro@gmail.com>
Дата:
On Sat, Jul 4, 2026 at 11:15 PM Jelte Fennema-Nio wrote: > On Sat, 4 Jul 2026 at 02:51, Thomas Munro 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 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.
Can we get rid of TerminateThread() in pg_dump?
От:
Thomas Munro <thomas.munro@gmail.com>
Дата:
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
Re: Can we get rid of TerminateThread() in pg_dump?
От:
Thomas Munro <thomas.munro@gmail.com>
Дата:
On Tue, Jul 7, 2026 at 12:18 AM Jelte Fennema-Nio wrote: > On Sun, 5 Jul 2026 at 07:03, Thomas Munro wrote: > > 1. If we're ready to drop VS < 2022 and GCC < 4.9, we could just use > > 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 , just not .
Re: Can we get rid of TerminateThread() in pg_dump?
От:
Thomas Munro <thomas.munro@gmail.com>
Дата:
On Sat, Jul 4, 2026 at 3:36 AM Bryan Green 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?
Re: Can we get rid of TerminateThread() in pg_dump?
От:
Thomas Munro <thomas.munro@gmail.com>
Дата:
On Sat, Jul 4, 2026 at 1:56 AM Bryan Green 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.
Re: Can we get rid of TerminateThread() in pg_dump?
От:
Jelte Fennema-Nio <postgres@jeltef.nl>
Дата:
On Sun, 5 Jul 2026 at 07:03, Thomas Munro 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 > 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/2a965ac6-fa42-4054-bee0-b1618e7729d6@eisentraut.org
Re: Can we get rid of TerminateThread() in pg_dump?
От:
Jelte Fennema-Nio <postgres@jeltef.nl>
Дата:
On Wed, 8 Jul 2026 at 17:49, Heikki Linnakangas 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.
Re: Can we get rid of TerminateThread() in pg_dump?
От:
"Jelte Fennema-Nio" <postgres@jeltef.nl>
Дата:
On Sat, 4 Jul 2026 at 02:51, Thomas Munro 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#5642e337a4e4d04b21c66e089484f80d
Re: Can we get rid of TerminateThread() in pg_dump?
От:
"Jelte Fennema-Nio" <postgres@jeltef.nl>
Дата:
On Wed, Jul 8, 2026, 01:41 Thomas Munro wrote: > On Tue, Jul 7, 2026 at 9:23 AM Heikki Linnakangas 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.
Re: Can we get rid of TerminateThread() in pg_dump?
От:
Bryan Green <dbryan.green@gmail.com>
Дата:
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
Re: Can we get rid of TerminateThread() in pg_dump?
От:
Bryan Green <dbryan.green@gmail.com>
Дата:
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#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
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
