MIME-Version: 1.0
References: 
 <CA+hUKGJgO=o-vLFahGdR2WesuX3h1-0j=a8z72fChc-MG1Hveg@mail.gmail.com>
 <fc4d8e83-0057-450a-917c-d8c2c79b336b@gmail.com>
In-Reply-To: <fc4d8e83-0057-450a-917c-d8c2c79b336b@gmail.com>
From: Thomas Munro <thomas.munro@gmail.com>
Date: Sat, 4 Jul 2026 12:38:53 +1200
Message-ID: 
 <CA+hUKGLPsBkJ9JOdu8Zo3pndccj8faoNithBJ6qgBKbSqvC8vg@mail.gmail.com>
Subject: Re: Can we get rid of TerminateThread() in pg_dump?
To: Bryan Green <dbryan.green@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>,
 Tom Lane <tgl@sss.pgh.pa.us>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CA%2BhUKGLPsBkJ9JOdu8Zo3pndccj8faoNithBJ6qgBKbSqvC8vg%40mail.gmail.com>
Precedence: bulk

On Sat, Jul 4, 2026 at 1:56=E2=80=AFAM 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 advan=
ce.

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.