Re: pg_threads.h take II

On 7/6/26 3:48 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> 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
>
>
GetCurrentThread() not GetCurrentThreadId()...  GetCurrentThread()
returns a special sentinel handle (-2) that when passed to another
Win32 function causes the kernel to resolve to whatever thread made
the call....

So, no-- not broken.