On 7/6/26 3:48 PM, Heikki Linnakangas <[email protected]> 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.
