Re: pg_threads.h take II

On Wed, Nov 11, 2026 at 11:14 AM Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
> I still need to think about Jakub's recent feedback, but once I've
> done that, and once we have a decision on v3-0003, I think something
> like 0001-0008 should be about ready to commit?

Hey Thomas,

I hope you don't mind that I've tried to update your patches a bit. I'm
very interested in seeing this work eventually merged so I thought I'd
pitch in a bit.

v3 no longer applies (cfbot has shown it red since October), so here
is v4: a rebase, Jakub's feedback, a proposal for v3-0003, and fixes
for some bugs that turned up in testing. Your twelve patches are all
here under your name, unchanged apart from the rebase and one fix to
0002, described below. Everything I added is a separate patch placed
right after the one it touches, so you can squash or drop each one.

The one change to your 0002: it didn't compile on its own, because
pg_mtx_init_impl() calls pg_mtx_validate_type(), which only arrived
with the Windows mapping. I moved that function and its rwlock twin
into 0002, so that every patch in the series builds.

The new patches:

0003 fixes bugs in 0002. pg_mtx_recursive mapped to the plain type
(Jakub's point 2). On POSIX, pg_thrd_barrier_wait() reported an
error to one waiter in every round, because pthread_barrier_wait()
returns PTHREAD_BARRIER_SERIAL_THREAD to that waiter; pgbench ignores
the result, which is why nobody noticed. The error detail was
formatted with strerror(), but that is pg_strerror(), with a static
buffer, so two threads failing at once raced on it; it now uses
strerror_r(). Also a missing va_end(), and two macros that expanded
to names no implementation defines (PG_TSS_DTOR_ITERATIONS and
PG_RWLOCK_PLAIN_INIT).

0004 is Jakub's error-checking mutexes for assertion-enabled builds,
plus an answer to his question 3b: pg_mtx_lock() and pg_mtx_unlock()
now assert success themselves. Only 4 of the 36 lock and unlock
calls in the tree check the result, so otherwise an EDEADLK would
just be ignored. Statically initialized mutexes are only checked on
glibc, since PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP is a glibc
extension.

0005 adds src/test/modules/test_pg_threads, a standalone program run
as a TAP test. It checks behavior rather than implementation details
(create and join, detach, the three mutex types, condition variables,
call_once, tss destructors, barriers, rwlocks, error strings), so the
same test covers all three mappings. Against v3-0002 it fails the
recursive-mutex and barrier checks. Built with
PG_THREADS_USE_WINDOWS_H, it also found a bug in the native Windows
mapping.

0007 fixes that bug and one other in the native Windows mapping.
SleepConditionVariable*() fail with ERROR_TIMEOUT, but
pg_thrd_map_windows_error() only knew WAIT_TIMEOUT, so
pg_cnd_timedwait() reported a timeout as an error. And
pg_tss_ensure_fls_registered() read pg_tss_fls without the lock and
returned an uninitialized result if FlsAlloc() failed; it now uses
pg_call_once().

0008 is about v3-0003, which you asked about. I'd keep 0003 and go
one step further: use the native mapping for MSVC too, so that
<threads.h> is only ever chosen by PG_THREADS_USE_THREADS_H, for
conformance testing. With v3's selection only MinGW used 0003, and
MinGW binaries load libwinpthread-1.dll anyway through the gcc
runtime, so there it bought nothing. Using it for MSVC means MSVC
builds don't depend on vcruntime140_threads.dll, which would
otherwise have to be redistributed with PostgreSQL, and leaves one
Windows implementation instead of two to keep in agreement. The cost
is that we own that code, which is what test_pg_threads is for. We
also lose pg_mtx_timed on MSVC, which Visual Studio's mtx_t has, but
nothing uses it.

0013 makes pgbench report thread errors with
pg_thrd_error_string_with_detail(). It assigned the pg_thrd_*
result to errno and printed %m, so a generic failure came out as
"Interrupted system call".

0016 is Jakub's point 5. The pg_basebackup flag that the WAL
streamer sets and the main thread polls was a volatile bool, which is
a data race between threads, and sig_atomic_t wouldn't help, since
it only promises atomicity against a signal handler interrupting the
same thread. Until frontend code can use atomics, a mutex seemed the
smallest correct fix; the same file already does that for
xlogendptr.

0017 is Jakub's point 1, pg_thrd_setname(const char *name) in
pg_threads_ext.h. It names only the calling thread, because macOS
can't name another one, and it is best-effort and reports nothing.
On Windows it uses SetThreadDescription(), and test_pg_threads reads
the name back there and on Linux. I kept it out of pg_thrd_create()
so that pg_threads.h stays a faithful copy of <threads.h>. pgbench
names its worker threads and pg_basebackup its WAL streamer; main
threads are left alone, since on Linux naming the main thread renames
the process in ps.

I didn't add Jakub's point 6, pg_thrd_init(): pg_call_once() covers
lazy setup, and libpq and ecpg can't count on their callers to call
an init function.

0021 fixes the pg_dump/011_restore failure at 0020. On Unix,
_ReopenArchive() closed the original FILE before reopening it, and
only skipped that on Windows, where the workers were already threads
sharing the leader's FILE through the cloned ArchiveHandle. With
threads everywhere, the first worker closed the file under the
others, and "pg_restore -j2" of a custom-format archive failed or
hung. Bryan, your v2 series has the same problem: I rebased it onto
this branch, and with this change all of the pg_dump TAP tests pass.

About 0018-0020: as I read it, 0020 removes fork() but keeps the
pipes and the string protocol, while Bryan's v2-0004 and v2-0005
replace those with an in-memory channel and struct commands, so on
transport his series supersedes 0020. 0018 (pg_signal_processor) and
0019 (cancel_set) are about something else, namely where ^C cleanup
runs, and Bryan's series doesn't change that: it keeps the
async-signal-safe handler on Unix and the console handler on Windows.
The two seem to fit together, with cancel_set as the set of
connections that his handle_async_cancellation() walks today. I've
kept 0018-0020 as they were, plus the fix, so that the series stays
green.

Testing, at the tip unless noted:

* every patch builds with -Werror and assertions on Linux x86_64, and
test_pg_threads passes at each patch that has it
* full meson test on Debian 13, x86_64 and aarch64, with assertions:
377 ok, 53 skipped, no failures
* Linux with PG_THREADS_USE_THREADS_H defined: the suites of all the
thread users, and regress, pass
* FreeBSD 15.1, clang 19: builds with -Werror, and the same suites
pass; the thread names show up in procstat -t
* riscv64 Linux: builds with -Werror; ecpg and test_pg_threads pass
* Windows 11 arm64, MSVC 19.51: builds, and test_pg_threads, ecpg,
pg_dump, and regress pass. The libpq, pg_basebackup, and pgbench
TAP tests fail on that machine with plain master too, so I haven't
counted them. The binaries import nothing from
vcruntime140_threads.dll, and pgbench's and pg_basebackup's thread
names show up through GetThreadDescription().
* test_pg_threads built for both the <windows.h> and <threads.h>
mappings, on arm64 and on native x64 Windows, and the mingw-w64
UCRT cross-build (as in the CompilerWarnings CI task), run on native
x64
* GitHub Actions with the in-tree workflow: all jobs pass (Linux
autoconf and meson 32- and 64-bit, macOS, Windows with Visual Studio
and with MinGW, CompilerWarnings, SanityCheck)

best.

-greg

[0] https://github.com/gburd/postgres/tree/thomas/thr