Re: pg_threads.h take II

On 06/07/2026 15:09, Thomas Munro wrote:
> Hi,
>
> Here's another go at port/pg_threads.h, a C11 <threads.h>-like
> interface, with some patches to use it (some just sketch-quality).
> Previous work:
>
> https://www.postgresql.org/message-id/flat/CA%2BhUKGLtmexrpMtxBRLCVePqV_dtWG-ZsEbyPrYc%2BNBB2TkNsw%40mail.gmail.com
>
> We still can't require C11 <threads.h> yet, though the interface has a
> clear future upgrade/remap path to do that eventually. Since I last
> wrote about this topic, we gained the ability to count on C11
> thread_local being available (it's a language/compiler feature and
> doesn't need a <threads.h> header or library), and partly because of
> that, but also because I don't think anyone really likes it, I ripped
> out that tss_t stuff. I came up with a narrower API to get
> thread-exit cleanup callbacks if you need them for your thread_local
> resources, same pthread_key_t/FlsAlloc stuff underneath it but without
> having to get lost in the weeds of <threads.h> conformance.

Nice!

I've now reviewed patches 0001-0007. I think they're ready to be
committed, with some small comments below:

> v1-0002-port-Provide-minimal-pg_threads.h-API.patch
>
> The main thing I'm wondering about is how to make the error reporting
> a bit nicer. Neither pthreads nor C11 threads set errno, but it's
> nice if you can use %m. Hmm.

Perhaps provide a function like:

void
set_errno_from_pg_thrd_error(int error)
{
switch((pg_thrd_error_t) error)
{
case pg_thrd_nomem:
errno = ENOMEM;
break;
case pg_thrd_busy:
errno = EBUSY;
break;
...
}
}

> +/* Convert native error to pg_thrd_error_t. */
> +static inline int
> +pg_thrd_maperror(int error)
> +{
> +#ifdef WIN32
> + return error ? pg_thrd_success : pg_thrd_error;
> +#else
> + return error == 0 ? pg_thrd_success : pg_thrd_error;
> +#endif
> +}

The WIN32 version of this is confusing. There is only one caller of this
with WIN32:

> +#elif defined(WIN32)
> + return pg_thrd_maperror(InitializeSynchronizationBarrier(barrier, count, 0));
> +#else

So that's not wrong. But I'd suggest only defining pg_thrd_maperror()
with pthreads, and handling that one WIN32 caller directly without the
helper function. Maybe rename pg_thrd_maperror() to something like
map_pthread_retval_to_pg_thrd_error() or something.

Do the pthread functions return an errno on error? Currently, all errors
get squashed to pg_thrd_error, which is a little unfortunate.

> +/* Like C11 mtx_type_t. */
> +typedef enum pg_mtx_type_t
> +{
> + pg_mtx_plain = 0
> +} pg_mtx_type_t;
> +
> +/* Like C11 mtx_init(). */
> +static inline int
> +pg_mtx_init(pg_mtx_t *mutex, int type)
> +{
> +#ifdef WIN32
> + return pg_rwlock_init(mutex);
> +#else
> + return pg_thrd_maperror(pthread_mutex_init(mutex, NULL));
> +#endif
> +}

Since we only support plain mutexes, how about "Assert(type ==
pg_mtx_plan)" here?

> +/*-------------------------------------------------------------------------
> + *
> + * Barriers. Not in C11.
> + *
> + *-------------------------------------------------------------------------
> + */

I wonder if we really need barriers. They're not that useful IMHO. I'd
tend to just open-code this directly with a mutex and the condition
variable in most cases. (Not a strong objection, there's little harm in
having it either)

> +/*
> + * Wait for all expected threads to arrive at the barrier, and elect one
> + * arbitrary thread to perform a phase of computation serially. Sets
> + * *elected_thread to true in the elected thread, and false in all others.
> + */
> +static inline int
> +pg_barrier_wait_and_elect(pg_barrier_t *barrier, bool *elected_thread)
> +{

No callers use the *elected_thread return value. What was the idea here?

> +#ifdef WIN32
> +
> + /*
> + * Retrieve handle passed here by pg_thrd_create() before allowing this
> + * thread to run. (pg_thrd_current() can't use CurrentThread(), because
> + * that returns a pseudo-handle with the same value in all threads.)
> + */
> + Assert(start_info->self);
> + my_thrd_handle = start_info->self;
> +#endif

Does that refer to the GetCurrentThreadId() function? We use that in a
few places currently.

How can it return the same value in all threads, isn't that completely
useless? And does that mean all our current uses of it are broken?

- Heikki