Re: pg_threads.h take II - Mailing list pgsql-hackers

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