Re: Cleaning up threading code
On Mon, Jul 3, 2023 at 8:43 PM Heikki Linnakangas wrote: > On 10/06/2023 05:23, Thomas Munro wrote: > > 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 ; 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 > > Google search on "c11 threads on Windows" found some emulation wrappers: > https://github.com/jtsiomb/c11threads and > https://github.com/tinycthread/tinycthread, for example. Would either of > those work for us? > > Even if we use an existing emulation wrapper, I wouldn't mind having our > own pg_* abstration on top of it, to document which subset of the POSIX > or C11 functions we actually use. Yeah. I am still interested in getting our thread API tidied up, and I intend to do that for PG 18. The patch I posted on that front, which can be summarised as a very lightweight subset of standard except with pg_ prefixes everywhere mapping to Windows or POSIX threads, still needs one tiny bit more work: figuring out how to get the TLS destructors to run on Windows FLS or similar, or implementing destructors myself (= little destructor callback list that a thread exit hook would run, work I was hoping to avoid by using something from the OS libs... I will try again soon). Andres also opined upthread that we should think about offering a thread_local storage class and getting away from TLS with keys. One thing to note is that the ECPG code is using TLS with destructors (in fact they are b0rked in master, git grep "FIXME: destructor" so ECPG leaks memory on Windows, so the thing that I'm trying to fix in pg_threads.h is actually fixing a long-standing bug), and although thread_local has destructors in C++ it doesn't in C, so if we decided to add the storage class but not bother with the tss_create feature, then ECPG would need to do cleanup another way. I will look into that option. One new development since last time I wrote the above stuff is that the Microsoft toolchain finally implemented the library components of C11 : https://devblogs.microsoft.com/cppblog/c11-threads-in-visual-studio-2022-version-17-8-preview-2/ It seems like it would be a long time before we could contemplate using that stuff though, especially given our duty of keeping libpq and ECPG requirements low and reasonable. However, it seems fairly straightforward to say that we require C99 + some way to access a C11-like thread local storage class. In practice that means a pg_thread_local macro that points to MSVC __declspec(thread) (which works since MSVC 2014?) or GCC/Clang __thread_local (which works since GCC4.8 in 2013?) or whatever. Of course every serious toolchain supports this stuff somehow or other, having been required to for C11. I can't immediately see any build farm animals that would be affected. Grison's compiler info is out of date, it's really running 8.something. The old OpenBSD GCC 4.2 animal was upgraded, and antique AIX got the boot: that's not even a coincidence, those retirements came about because those systems didn't support arbitrary alignment, another C11 feature that we now effectively require. (We could have worked around it it we had to but on but they just weren't reasonable targets.) So I'll go ahead and add the storage class to the next version, and contemplate a couple of different options for the tss stuff, including perhaps leaving it out if that seems doable. https://stackoverflow.com/questions/29399494/what-is-the-current-state-of-support-for-thread-local-across-platforms
Re: Cleaning up threading code
On Wed, Jul 12, 2023 at 3:34 PM Thomas Munro wrote: > On Wed, Jul 12, 2023 at 3:21 PM Thomas Munro wrote: > > Apparently "drongo" didn't like something about this commit, and an > > ecpg test failed, but I can't immediately see why. Just "server > > closed the connection unexpectedly". drongo is running the new Meson > > buildfarm variant on Windows+MSVC, so, being still in development, I > > wonder if some diagnostic clue/log/backtrace etc is not being uploaded > > yet. FWIW Meson+Windows+MSVC passes on CI. > > Oh, that's probably unrelated to this commit. It's failed 6 times > like that in the past ~3 months. Ah, right, that is a symptom of the old Windows TCP linger vs process exit thing. Something on my list to try to investigate again, but not today. https://www.postgresql.org/message-id/flat/16678-253e48d34dc0c376%40postgresql.org
Re: Cleaning up threading code
On Wed, Jul 12, 2023 at 3:21 PM Thomas Munro wrote: > Apparently "drongo" didn't like something about this commit, and an > ecpg test failed, but I can't immediately see why. Just "server > closed the connection unexpectedly". drongo is running the new Meson > buildfarm variant on Windows+MSVC, so, being still in development, I > wonder if some diagnostic clue/log/backtrace etc is not being uploaded > yet. FWIW Meson+Windows+MSVC passes on CI. Oh, that's probably unrelated to this commit. It's failed 6 times like that in the past ~3 months.
Re: Cleaning up threading code
Apparently "drongo" didn't like something about this commit, and an ecpg test failed, but I can't immediately see why. Just "server closed the connection unexpectedly". drongo is running the new Meson buildfarm variant on Windows+MSVC, so, being still in development, I wonder if some diagnostic clue/log/backtrace etc is not being uploaded yet. FWIW Meson+Windows+MSVC passes on CI.
Re: Cleaning up threading code
On 2023-07-12 08:58:29 +1200, Thomas Munro wrote: > On Mon, Jul 10, 2023 at 10:45 AM Thomas Munro wrote: > > * defined ENABLE_THREAD_SAFETY 1 in c.h, for the benefit of extensions > > I may lack imagination but I couldn't think of any use for that > vestigial macro in backend/extension code, and client code doesn't see > c.h and might not get the right answer anyway if it's dynamically > linked which is the usual case. I took it out for now. Open to > discussing further if someone can show what kinds of realistic > external code would be affected. WFM.
Re: Cleaning up threading code
On Mon, Jul 10, 2023 at 10:45 AM Thomas Munro wrote: > * defined ENABLE_THREAD_SAFETY 1 in c.h, for the benefit of extensions I may lack imagination but I couldn't think of any use for that vestigial macro in backend/extension code, and client code doesn't see c.h and might not get the right answer anyway if it's dynamically linked which is the usual case. I took it out for now. Open to discussing further if someone can show what kinds of realistic external code would be affected. > * defined ENABLE_THREAD_SAFETY 1 ecpg_config.h, for the benefit of ECPG > clients I left this one in. I'm not sure if it could really be needed. Perhaps at a stretch, perhaps ECPG code that is statically linked might test that instead of calling PQisthreadsafe(). Pushed.
Re: Cleaning up threading code
Thanks all for the reviews. I pushed the first two small patches.
Here is a new version of the main --disable-thread-safety removal
part, with these changes:
* fixed LDAP_LIBS_FE snafu reported by Peter E
* defined ENABLE_THREAD_SAFETY 1 in c.h, for the benefit of extensions
* defined ENABLE_THREAD_SAFETY 1 ecpg_config.h, for the benefit of ECPG clients
* added Heikki's doc change as a separate patch (with minor xml snafu fixed)
I'll follow up with a new version of the later pg_threads.h proposal
and responses to feedback on that after getting this basic clean-up
work in. Any other thoughts on this part?
From 18975f437e6b8e7c36da99238a90e5efa68434b9 Mon Sep 17 00:00:00 2001
From: Thomas Munro
Date: Fri, 9 Jun 2023 14:07:26 +1200
Subject: [PATCH v2 1/2] Remove --disable-thread-safety and related code.
All supported computers have either POSIX or Windows threads, and we no
longer have any automated testing of --disable-thread-safety. We define
a vestigial ENABLE_THREAD_SAFETY macro to 1 in c.h and ecpg_config.h for
the benefit of external code that might be testing for that, but we no
longer test it anywhere in PostgreSQL code, and associated dead code
paths are removed.
The Meson and perl-based Windows build scripts never had an equivalent
build option.
Reviewed-by: Andres Freund
Reviewed-by: Peter Eisentraut
Reviewed-by: Heikki Linnakangas
Discussion: https://postgr.es/m/CA%2BhUKGLtmexrpMtxBRLCVePqV_dtWG-ZsEbyPrYc%2BNBB2TkNsw%40mail.gmail.com
---
configure | 54 ++
configure.ac | 26 ++-
doc/src/sgml/installation.sgml| 13
meson.build | 11 ---
src/Makefile.global.in| 1 -
src/bin/pgbench/pgbench.c | 22 +-
src/include/c.h | 7 ++
src/include/pg_config.h.in| 4 --
src/interfaces/ecpg/ecpglib/connect.c | 40 ---
src/interfaces/ecpg/ecpglib/descriptor.c | 10 ---
src/interfaces/ecpg/ecpglib/ecpglib_extern.h | 2 -
src/interfaces/ecpg/ecpglib/execute.c | 2 -
src/interfaces/ecpg/ecpglib/memory.c | 7 --
src/interfaces/ecpg/ecpglib/misc.c| 47
.../ecpg/include/ecpg-pthread-win32.h | 3 -
src/interfaces/ecpg/include/ecpg_config.h.in | 5 +-
src/interfaces/ecpg/include/ecpglib.h | 2 -
src/interfaces/ecpg/include/meson.build | 3 +-
.../ecpg/test/expected/thread-alloc.c | 43 +--
.../ecpg/test/expected/thread-descriptor.c| 22 +++---
.../ecpg/test/expected/thread-prep.c | 71 ---
.../ecpg/test/expected/thread-thread.c| 63 +++-
.../test/expected/thread-thread_implicit.c| 63 +++-
src/interfaces/ecpg/test/thread/alloc.pgc | 9 ---
.../ecpg/test/thread/descriptor.pgc | 8 +--
src/interfaces/ecpg/test/thread/prep.pgc | 9 ---
src/interfaces/ecpg/test/thread/thread.pgc| 9 ---
.../ecpg/test/thread/thread_implicit.pgc | 9 ---
src/interfaces/libpq/Makefile | 5 +-
src/interfaces/libpq/fe-connect.c | 4 --
src/interfaces/libpq/fe-exec.c| 4 --
src/interfaces/libpq/fe-print.c | 13 +---
src/interfaces/libpq/fe-secure-openssl.c | 17 +
src/interfaces/libpq/fe-secure.c | 26 +--
src/interfaces/libpq/legacy-pqsignal.c| 4 +-
src/interfaces/libpq/libpq-int.h | 9 +--
src/makefiles/meson.build | 1 -
src/tools/msvc/Solution.pm| 3 +-
src/tools/msvc/ecpg_regression.proj | 2 +-
39 files changed, 144 insertions(+), 509 deletions(-)
diff --git a/configure b/configure
index 4a8f652129..2e518c8007 100755
--- a/configure
+++ b/configure
@@ -722,7 +722,6 @@ with_tcl
ICU_LIBS
ICU_CFLAGS
with_icu
-enable_thread_safety
INCLUDES
autodepend
PKG_CONFIG_LIBDIR
@@ -848,7 +847,6 @@ with_CC
with_llvm
enable_depend
enable_cassert
-enable_thread_safety
with_icu
with_tcl
with_tclconfig
@@ -1536,7 +1534,6 @@ Optional Features:
--enable-tap-tests enable TAP tests (requires Perl and IPC::Run)
--enable-depend turn on automatic dependency tracking
--enable-cassertenable assertion checks (for debugging)
- --disable-thread-safety disable thread-safety in client libraries
--disable-largefile omit support for large files
Optional Packages:
@@ -8338,43 +8335,6 @@ $as_echo "$as_me: WARNING: *** Library directory $dir does not exist." >&2;}
done
IFS=$ac_save_IFS
-#
-# Enable thread-safe client libraries
-#
-{ $as_echo "$as_me:${as_lineno-$LINENO}: checking allow thread-safe client libraries" >&5
-$as_echo_n "checking allow thread-safe client libraries... " >&6; }
-
-
-# Check whether --enable-thread-safety was given.
-if test "${enable_thread_safety+set}" = set; then :
-
Re: Cleaning up threading code
On 03/07/2023 10:29, Peter Eisentraut wrote:
On 10.06.23 07:26, Andres Freund wrote:
-###
-# 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.
We definitely should keep the mention in ecpg_config.h.in, since that is
explicitly put there for client code to use. We keep HAVE_LONG_LONG_INT
etc. there for similar reasons.
+1. Patches 1-3 look good to me, with the things Andres & Peter already
pointed out.
The docs at https://www.postgresql.org/docs/current/libpq-threading.html
needs updating. It's technically still accurate, but it ought to at
least mention that libpq on v17 and above is always thread-safe. I
propose the attached. It moves the note on "you can only use one PGConn
from one thread at a time" to the top, before the description of the
PQisthreadsafe() function.
On 10/06/2023 05:23, Thomas Munro wrote:
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 ; 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
Google search on "c11 threads on Windows" found some emulation wrappers:
https://github.com/jtsiomb/c11threads and
https://github.com/tinycthread/tinycthread, for example. Would either of
those work for us?
Even if we use an existing emulation wrapper, I wouldn't mind having our
own pg_* abstration on top of it, to document which subset of the POSIX
or C11 functions we actually use.
--
Heikki Linnakangas
Neon (https://neon.tech)
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 2225e4e0ef..bef03cfa16 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -9129,14 +9129,28 @@ void PQinitSSL(int do_ssl);
- libpq is reentrant and thread-safe by default.
- You might need to use special compiler command-line
- options when you compile your application code. Refer to your
- system's documentation for information about how to build
- thread-enabled applications, or look in
- src/Makefile.global for PTHREAD_CFLAGS
- and PTHREAD_LIBS. This function allows the querying of
- libpq's thread-safe status:
+ As of version 17, libpq is always reentrant and thread-safe.
+ However, one restriction is that no two threads attempt to manipulate
+ the same PGconn object at the same time. In particular,
+ you cannot issue concurrent commands from different threads through
+ the same connection object. (If you need to run concurrent commands,
+ use multiple connections.)
+
+
+
+ PGresult objects are normally read-only after creation,
+ and so can be passed around freely between threads. However, if you use
+ any of the PGresult-modifying functions described in
+or , it's up
+ to you to avoid concurrent operations on the same PGresult,
+ too.
+
+
+
+ In earlier versions, libpq could be compiled
+ with or without thread support, depending on compiler options. This
+ function allows the querying of libpq's
+ thread-safe status:
@@ -9154,29 +9168,12 @@ int PQisthreadsafe();
Returns 1 if the libpq is thread-safe
- and 0 if it is not.
+ and 0 if it is not. Always returns 1 on version 17 and above.
-
- One thread restriction is that no two threads attempt to manipulate
- the same PGconn object at the same time. In particular,
- you cannot issue concurrent commands from different threads through
- the same connection object. (If you need to run concurrent commands,
- use multiple connections.)
-
-
-
- PGresult objects are normally read-only after creation,
- and so can be passed around freely between threads. However, if you use
- any of the PGresult-modifying functions described in
-or , it's up
- to you to avoid concurrent operations on the same PGresult,
- too.
-
-
The deprecated functions and
are not thread-safe and should not be
Re: Cleaning up threading code
On 10.06.23 07:26, Andres Freund wrote:
-###
-# 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.
We definitely should keep the mention in ecpg_config.h.in, since that is
explicitly put there for client code to use. We keep HAVE_LONG_LONG_INT
etc. there for similar reasons.
Another comment on patch 0003: I think this removal in configure.ac is
too much:
-else
- LDAP_LIBS_FE="-lldap $EXTRA_LDAP_LIBS"
This could would still be reachable for $enable_thread_safety=yes and
$thread_safe_libldap=yes, which I suppose is the normal case?
Re: Cleaning up threading code
On 6/10/23 01:26, Andres Freund wrote: On 2023-06-10 14:23:52 +1200, Thomas Munro wrote: 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? Perhaps obvious to most/all on this thread, but something that bit me recently to keep in mind as we contemplate threads. When a shared object library (e.g. extensions) is loaded, the init() function, and any functions marked with the constructor attribute, are executed. However they are not run again during thread_start(). So if there is thread local state that needs to be initialized, the initialization needs to be manually redone in each thread. Unless there is some mechanism similar to a constructor that I missed? -- Joe Conway PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
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 ; 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
> 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 , 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 00..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
> + * 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
> +#include
> +#include
> +#include
> +#else
> +#include
> +#include "port/pg_pthread.h"
> +#endif
Seems somewhat odd to have port pg_threads.h and pg_pthread.h - why not merge
these?
> +#include
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;
>
