Re: pg_threads.h take II

On Fri, Jul 31, 2026 at 4:19 AM Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
>
> [..v3..]

Hi Thomas,

I've wanted to somehow help the threads initiative for long time so I've
started playing with patchset, mainly with using multi-threaded pgbench
and pgbasebackup to get a basic feeling, and yay it works! I haven't catched
any problems so far with it, but I had some ideas when dealing with this:
so all of this is mostly about v3 0002+0007+0009:

1. Couldn't we have another (optional?) arg for pg_thrd_create() to issue
pthread_setname_np() from day 1? It would be nice to have something to see
which thread does what (similiar to setproctiltle()) or should we directly
embed something like pg_thrd_setname() that uses pthread_setname_np()?

E.g. @@ -7493,6 +7493,9 @@ threadRun(void *arg)

+ char thrid[20];
+ snprintf(thrid, sizeof(thrid), "pgbench thr%d", thread->tid);
+ pthread_setname_np(pg_thrd_current(), thrid);

Then we could use something like "ps -aeL -o tid,comm,args | grep bench"
or in GDB to see those threads. So problem seems to be that comm is not
often displayed and sometimes thread 0 (tid=pid) is the process itself
because e.g. in pgbench.c case main calls threadRun() directly too to
make it thread#0. Alternative we could make it conditional there in
threadRun() to bypass that if tid == pid...

BTW: I have found portable way of doing this in MySQL code, see [1]

2. In src/include/port/pg_threads.h shouldn't the enum be like below?
enum
{
pg_mtx_plain = pg_mtx_plain_impl,
- pg_mtx_recursive = pg_mtx_plain_impl,
+ pg_mtx_recursive = pg_mtx_recursive_impl,
?

3. Could we have PTHREAD_MUTEX_ERRORCHECK enabled by default in at least
assert builds? I'm not sure, but if we have stuff like that

pthread_mutexattr_t attr;
pthread_mutexattr_init(&attr);
pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_ERRORCHECK);
pthread_mutex_init(&mutex, &attr);
pthread_mutex_unlock(&mutex); // returns EPERM (unlocking unlocked)
pthread_mutex_lock(&mutex);
pthread_mutex_lock(&mutex); // returns EDEADLK (instead of deadlock)

Please see attached sample nanopatch, but this bring me to the next
problem:

3b.The problem with above is that e.g. double pg_mtx_lock() on same mutex in
with the patchset didn't abort with above add-on for #2 (ERRORCHECK),
because the remapping pg_threads layer does not trigger any Assert() or
we are not checking for any non-zero retcode at all, so which way it should
be ? (should check errors on every pg_mtx_* on every call site?
or should be that part of API to have code reuse?)

4. BTW: the v03-0009/pg_basebackup didn't want to apply due to the
introduction of g_parse_lsn() there in f31d6fbc31d3. Attached is simple
fixup patch. I was kind of interested in that to see how threads there
could be used in far future to unlock even more performance (but that would
have to occur probably after [2] with some paralell-backup redesign, but
that got me thinking on how we are going to be compatible with all this
stuff: liburing, pthreads one day) - frankley I couldn't think of any
issues. Anyway, thinking of basebackups, I've reminded myself that zstd
can already uses pthreads both on client (relevant to this $thread) and
server too, e.g:
pg_basebackup -c fast -v -Ft -D /tmp/full.tar -Z client-zstd:workers=2
but when thinking through all of this of my only worry would be, that
in such case with this patch applied and in far future I would be having
>100% CPU PID with multiple threads and without fix from #1 it would be
impossible to tell what is being bottlenecked? (saturated ZSTD threads or
now the the thread fetching the)

5. This is something that sent me to the land of doubt: when reviewing that
0009 for basebackup there's this change:
-static volatile sig_atomic_t bgchild_exited = false;
+static volatile bool wal_streamer_thread_exited = false;
It works here (x86_64), but is it safe/platform compatible? I've read a lot
about _Atomic / atomic_int / sig_atomic_t / pg_atomic_flag / stdatomic.h
patch of Your's in [3] and Greg even mention pg_atomic_bool by Heikki [4]
there, but the more I read the more confused I am, so any gudiance and
help please? :) (and could we maybe put some README to nearby API
implementaion to mention that for such usecase what should be used going
forward as solid point of reference? I would almost by defintion use
"sig_atomic_t" there for such case.
(it's similiar to size_t vs pgoff_t vs Size vs ...). The only thing I
believe right now that atomic_int store really disassembly down to xchgl'
instruction (sounds like it is safer?)

6. I was wondering shouldn't we have some stub (for now) to initialize the
whole thing just before first use, something like: pg_thrd_init().
Over time we
could place pthread_attr_setstacksize() there if necessary or some (frontend
for now?) stuff like even fprintf() to show some debug info.

-J.

[1] - https://github.com/MariaDB/server/blob/bfabe0e53042d6a954c84b58a3c9eade794b9e90/mysys/my_thread_name.cc#L73
[2] - https://www.postgresql.org/message-id/flat/CAKZiRmwwW-hDc3B6ERJB+paX7RNSBcQLheq1KdsTf42cGuRvuA(at)mail(dot)gmail(dot)com
[3] - https://www.postgresql.org/message-id/CA%2BhUKGKfNuXYVKT7WPpKTNYTgPduzu0%3DG5yFEMju_4kbW0ybOQ%40mail.gmail.com
[4] - https://www.postgresql.org/message-id/bb0ba423-816c-4e21-a40f-b1be13b54c5f%40iki.fi