Re: Can we get rid of TerminateThread() in pg_dump?
Heikki Linnakangas <hlinnaka@iki.fi>
From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Jelte Fennema-Nio <postgres@jeltef.nl>,
Thomas Munro <thomas.munro@gmail.com>
Cc: Bryan Green <dbryan.green@gmail.com>,
pgsql-hackers <pgsql-hackers@postgresql.org>, Tom Lane <tgl@sss.pgh.pa.us>,
Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Date: 2026-07-08T15:49:47Z
Lists: pgsql-hackers
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
Commits
-
Redesign handling of SIGTERM/control-C in parallel pg_dump/pg_restore.
- e652273e0735 9.6.0 cited