Re: Cleaning up threading code

Hi,

On 2023-06-10 14:23:52 +1200, Thomas Munro wrote:
> 1. We should completely remove --disable-thread-safety and the
> various !defined(ENABLE_THREAD_SAFETY) code paths.

Yes, please!

> 2. I don't like the way we have to deal with POSIX vs Windows at
> every site where we use threads, and each place has a different style
> of wrappers. I considered a few different approaches to cleaning this
> up:
>
> * provide centralised and thorough pthread emulation for Windows; I
> don't like this, I don't even like all of pthreads and there are many
> details to get lost in
> * adopt C11 <threads.h>; unfortunately it is too early, so you'd need
> to write/borrow replacements for at least 3 of our 11 target systems
> * invent our own mini-abstraction for a carefully controlled subset of stuff
>
> Here is an attempt at that last thing. Since I don't really like
> making up new names for things, I just used all the C11 names but with
> pg_ in front, and declared it all in "port/pg_threads.h" (actually I
> tried to write C11 replacements and then noped out and added the pg_
> prefixes). I suppose the idea is that it and the prefixes might
> eventually go away. Note: here I am talking only about very basic
> operations like create, exit, join, explicit thread locals -- the
> stuff that we actually use today in frontend code.

Unsurprisingly, I like this.

> I'm not talking
> about other stuff like C11 atomics, memory models, and the
> thread_local storage class, which are all very good and interesting
> topics for another day.

Hm. I agree on C11 atomics and memory models, but I don't see a good reason to
not add support for thread_local?

In fact, I'd rather add support for thread_local and not add support for
"thread keys" than the other way round. Afaict most, if not all, the places
using keys would look simpler with thread_local than with keys.

> One mystery still eludes me on Windows: while trying to fix the
> ancient bug that ECPG leaks memory on Windows, because it doesn't call
> thread-local destructors, I discovered that if you use FlsAlloc
> instead of TlsAlloc you can pass in a destructor (that's
> "fibre-local", but we're not using fibres so it's works out the same
> as thread local AFAICT). It seems to crash in strange ways if you
> uncomment the line FlsAlloc(destructor).

Do you have a backtrace available?

> Subject: [PATCH 1/5] Remove obsolete comments and code from fe-auth.c.
> Subject: [PATCH 2/5] Rename port/thread.c to port/user.c.

LGTM

> -###############################################################
> -# Threading
> -###############################################################
> -
> -# XXX: About to rely on thread safety in the autoconf build, so not worth
> -# implementing a fallback.
> -cdata.set('ENABLE_THREAD_SAFETY', 1)

I wonder if we should just unconditionally set that in c.h or such? It'd not
be crazy for external projects to rely on that being set.

> From ca74df4ff11ce0fd1e51786eccaeca810921fc6d Mon Sep 17 00:00:00 2001
> From: Thomas Munro <thomas(dot)munro(at)gmail(dot)com>
> Date: Sat, 10 Jun 2023 09:14:07 +1200
> Subject: [PATCH 4/5] Add port/pg_threads.h for a common threading API.
>
> Loosely based on C11's <threads.h>, but with pg_ prefixes, this will
> allow us to clean up many places that have to cope with POSIX and
> Windows threads.
> ---
> src/include/port/pg_threads.h | 252 +++++++++++++++++++++++++++++++
> src/port/Makefile | 1 +
> src/port/meson.build | 1 +
> src/port/pg_threads.c | 117 ++++++++++++++
> src/tools/pgindent/typedefs.list | 7 +
> 5 files changed, 378 insertions(+)
> create mode 100644 src/include/port/pg_threads.h
> create mode 100644 src/port/pg_threads.c
>
> diff --git a/src/include/port/pg_threads.h b/src/include/port/pg_threads.h
> new file mode 100644
> index 0000000000..1706709994
> --- /dev/null
> +++ b/src/include/port/pg_threads.h
> @@ -0,0 +1,252 @@
> +/*
> + * A multi-threading API abstraction loosely based on the C11 standard's
> + * <threads.h> header. The identifiers have a pg_ prefix. Perhaps one day
> + * we'll use standard C threads directly, and we'll drop the prefixes.
> + *
> + * Exceptions:
> + * - pg_thrd_barrier_t is not based on C11
> + */
> +
> +#ifndef PG_THREADS_H
> +#define PG_THREADS_H
> +
> +#ifdef WIN32

I guess we could try to use C11 thread.h style threading on msvc 2022:
https://developercommunity.visualstudio.com/t/finalize-c11-support-for-threading/1629487

> +#define WIN32_LEAN_AND_MEAN

Why do we need this again here - shouldn't the define in
src/include/port/win32_port.h already take care of this?

> +#include <windows.h>
> +#include <processthreadsapi.h>
> +#include <fibersapi.h>
> +#include <synchapi.h>
> +#else
> +#include <errno.h>
> +#include "port/pg_pthread.h"
> +#endif

Seems somewhat odd to have port pg_threads.h and pg_pthread.h - why not merge
these?

> +#include <stdint.h>

I think we widely rely stdint.h, errno.h to be included via c.h.

> +#ifdef WIN32
> +typedef HANDLE pg_thrd_t;
> +typedef CRITICAL_SECTION pg_mtx_t;
> +typedef CONDITION_VARIABLE pg_cnd_t;
> +typedef SYNCHRONIZATION_BARRIER pg_thrd_barrier_t;
> +typedef DWORD pg_tss_t;
> +typedef INIT_ONCE pg_once_flag;
> +#define PG_ONCE_FLAG_INIT INIT_ONCE_STATIC_INIT
> +#else
> +typedef pthread_t pg_thrd_t;
> +typedef pthread_mutex_t pg_mtx_t;
> +typedef pthread_cond_t pg_cnd_t;
> +typedef pthread_barrier_t pg_thrd_barrier_t;
> +typedef pthread_key_t pg_tss_t;
> +typedef pthread_once_t pg_once_flag;
> +#define PG_ONCE_FLAG_INIT PTHREAD_ONCE_INIT
> +#endif
> +
> +typedef int (*pg_thrd_start_t) (void *);
> +typedef void (*pg_tss_dtor_t) (void *);
> +typedef void (*pg_call_once_function_t) (void);
> +
> +enum
> +{
> + pg_thrd_success = 0,
> + pg_thrd_nomem = 1,
> + pg_thrd_timedout = 2,
> + pg_thrd_busy = 3,
> + pg_thrd_error = 4
> +};
> +
> +enum
> +{
> + pg_mtx_plain = 0
> +};

I think we add typedefs for enums as well.

> +#ifndef WIN32
> +static inline int
> +pg_thrd_maperror(int error)

Seems like that should have pthread in the name?

> +{
> + if (error == 0)
> + return pg_thrd_success;
> + if (error == ENOMEM)
> + return pg_thrd_nomem;
> + return pg_thrd_error;
> +}
> +#endif
> +
> +#ifdef WIN32
> +BOOL pg_call_once_trampoline(pg_once_flag *flag, void *parameter, void **context);
> +#endif
> +
> +static inline void
> +pg_call_once(pg_once_flag *flag, pg_call_once_function_t function)
> +{
> +#ifdef WIN32
> + InitOnceExecuteOnce(flag, pg_call_once_trampoline, (void *) function, NULL);
> +#else
> + pthread_once(flag, function);
> +#endif
> +}
> +
> +static inline int
> +pg_thrd_equal(pg_thrd_t lhs, pg_thrd_t rhs)
> +{
> +#ifdef WIN32
> + return lhs == rhs;
> +#else
> + return pthread_equal(lhs, rhs);
> +#endif
> +}
> +
> +static inline int
> +pg_tss_create(pg_tss_t *key, pg_tss_dtor_t destructor)
> +{
> +#ifdef WIN32
> + //*key = FlsAlloc(destructor);
> + *key = FlsAlloc(NULL);
> + return pg_thrd_success;

Afaict FlsAlloc() can return errors:
> If the function fails, the return value is FLS_OUT_OF_INDEXES. To get
> extended error information, call GetLastError.

> +#else
> + return pg_thrd_maperror(pthread_key_create(key, destructor));
> +#endif
> +}

> +static inline int
> +pg_tss_set(pg_tss_t key, void *value)
> +{
> +#ifdef WIN32
> + return FlsSetValue(key, value) ? pg_thrd_success : pg_thrd_error;
> +#else
> + return pg_thrd_maperror(pthread_setspecific(key, value));
> +#endif
> +}

It's somewhat annoying that this can return errors.

> +static inline int
> +pg_mtx_init(pg_mtx_t *mutex, int type)

I am somewhat confused by the need for these three letter
abbreviations... Blaming C11, not you, to be clear.

>
> -# port needs to be in include path due to pthread-win32.h
> +# XXX why do we need this?
> libpq_inc = include_directories('.', '../../port')
> libpq_c_args = ['-DSO_MAJOR_VERSION=5']

Historically we need it because random places outside of libpq include
pthread-win32.h - but pthread-win32.h is in src/port, not in
src/include/port. Why on earth it was done this way, I do not know.

Greetings,

Andres Freund