public inbox for [email protected]
help / color / mirror / Atom feedFrom: Thomas Munro <[email protected]>
To: Bryan Green <[email protected]>
Cc: pgsql-hackers <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Kyotaro Horiguchi <[email protected]>
Subject: Re: Can we get rid of TerminateThread() in pg_dump?
Date: Sat, 4 Jul 2026 12:38:53 +1200
Message-ID: <CA+hUKGLPsBkJ9JOdu8Zo3pndccj8faoNithBJ6qgBKbSqvC8vg@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CA+hUKGJgO=o-vLFahGdR2WesuX3h1-0j=a8z72fChc-MG1Hveg@mail.gmail.com>
<[email protected]>
On Sat, Jul 4, 2026 at 1:56 AM Bryan Green <[email protected]> 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.
view thread (18+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: Can we get rid of TerminateThread() in pg_dump?
In-Reply-To: <CA+hUKGLPsBkJ9JOdu8Zo3pndccj8faoNithBJ6qgBKbSqvC8vg@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox