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

On 7/3/2026 7:38 PM, Thomas Munro wrote:
> On Sat, Jul 4, 2026 at 1:56 AM Bryan Green <dbryan(dot)green(at)gmail(dot)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