Remove pthread_is_threaded_np() checks in postmaster

Hi,
On 2024-01-23 13:20:15 -0600, Tristan Partin wrote:

These checks are not effective for what they are trying to prevent. A recent
commit[0] in libcurl when used on macOS has been tripping the
pthread_is_threaded_np() check in postmaster.c for shared_preload_libraries
entries which use libcurl (like Neon). Under the hood, libcurl calls
SCDynamicStoreCopyProxies[1], which apparently causes the check to fail.

Maybe I'm missing something, but isn't that indicating the exact opposite,
namely that the check is precisely doing what it's intended?

Greetings,

Andres Freund

On Tue Jan 23, 2024 at 1:47 PM CST, Andres Freund wrote:

Hi,
On 2024-01-23 13:20:15 -0600, Tristan Partin wrote:

These checks are not effective for what they are trying to prevent. A recent
commit[0] in libcurl when used on macOS has been tripping the
pthread_is_threaded_np() check in postmaster.c for shared_preload_libraries
entries which use libcurl (like Neon). Under the hood, libcurl calls
SCDynamicStoreCopyProxies[1], which apparently causes the check to fail.

Maybe I'm missing something, but isn't that indicating the exact opposite,
namely that the check is precisely doing what it's intended?

The logic in the original commit seems sound:

On Darwin, detect and report a multithreaded postmaster.

Darwin --enable-nls builds use a substitute setlocale() that may start a
thread. Buildfarm member orangutan experienced BackendList corruption
on account of different postmaster threads executing signal handlers
simultaneously. Furthermore, a multithreaded postmaster risks undefined
behavior from sigprocmask() and fork(). Emit LOG messages about the
problem and its workaround. Back-patch to 9.0 (all supported versions).

Some reading from signal(7):

A process-directed signal may be delivered to any one of the threads
that does not currently have the signal blocked. If more than one of
the threads has the signal unblocked, then the kernel chooses an
arbitrary thread to which to deliver the signal.

So it sounds like the commit is trying to protect from the last
sentence.

And then forks only copy from their parent thread...

Please ignore this thread. I need to think of a better solution to this
problem.

--
Tristan Partin
Neon (https://neon.tech)

On Tue Jan 23, 2024 at 2:10 PM CST, Tristan Partin wrote:

On Tue Jan 23, 2024 at 1:47 PM CST, Andres Freund wrote:

Hi,
On 2024-01-23 13:20:15 -0600, Tristan Partin wrote:

These checks are not effective for what they are trying to prevent. A recent
commit[0] in libcurl when used on macOS has been tripping the
pthread_is_threaded_np() check in postmaster.c for shared_preload_libraries
entries which use libcurl (like Neon). Under the hood, libcurl calls
SCDynamicStoreCopyProxies[1], which apparently causes the check to fail.

Maybe I'm missing something, but isn't that indicating the exact opposite,
namely that the check is precisely doing what it's intended?

The logic in the original commit seems sound:

On Darwin, detect and report a multithreaded postmaster.

Darwin --enable-nls builds use a substitute setlocale() that may start a
thread. Buildfarm member orangutan experienced BackendList corruption
on account of different postmaster threads executing signal handlers
simultaneously. Furthermore, a multithreaded postmaster risks undefined
behavior from sigprocmask() and fork(). Emit LOG messages about the
problem and its workaround. Back-patch to 9.0 (all supported versions).

Some reading from signal(7):

A process-directed signal may be delivered to any one of the threads
that does not currently have the signal blocked. If more than one of
the threads has the signal unblocked, then the kernel chooses an
arbitrary thread to which to deliver the signal.

So it sounds like the commit is trying to protect from the last
sentence.

And then forks only copy from their parent thread...

What is keeping us from using pthread_sigmask(3) instead of
sigprocmask(2)? If an extension can guarantee that threads that get
launched by it don't interact with anything Postgres-related, would that
be enough to protect from any fork(2) related issues? In the OP example,
is there any harm in the thread that libcurl inadvertantly launches from
a Postgres perspective?

--
Tristan Partin
Neon (https://neon.tech)

Hi,

On 2024-01-23 15:50:11 -0600, Tristan Partin wrote:

What is keeping us from using pthread_sigmask(3) instead of sigprocmask(2)?

We would need to make sure to compile with threading support everywhere. One
issue is that on some platforms things get slower once you actually start
using pthreads.

If an extension can guarantee that threads that get launched by it don't
interact with anything Postgres-related, would that be enough to protect
from any fork(2) related issues?

A fork() while threads are running is undefined behavior IIRC, and undefined
behavior isn't limited to a single thread. You'd definitely need to use
pthread_sigprocmask etc to address that aspect alone.

Greetings,

Andres Freund

On Tue Jan 23, 2024 at 4:23 PM CST, Andres Freund wrote:

Hi,

On 2024-01-23 15:50:11 -0600, Tristan Partin wrote:

What is keeping us from using pthread_sigmask(3) instead of sigprocmask(2)?

We would need to make sure to compile with threading support everywhere. One
issue is that on some platforms things get slower once you actually start
using pthreads.

What are examples of these reduced performance platforms?

From reading the meson.build files, it seems like building with
threading enabled is the future, so should we just rip the band-aid off
for 17?

If an extension can guarantee that threads that get launched by it don't
interact with anything Postgres-related, would that be enough to protect
from any fork(2) related issues?

A fork() while threads are running is undefined behavior IIRC, and undefined
behavior isn't limited to a single thread. You'd definitely need to use
pthread_sigprocmask etc to address that aspect alone.

If you can find a resource that explains the UB, I would be very
interested to read that. I found a SO[0]https://stackoverflow.com/a/42679479/7572728 answer that made it seem like
this actually wasn't the case.

[0]: https://stackoverflow.com/a/42679479/7572728

--
Tristan Partin
Neon (https://neon.tech)

Hi,

On 2024-01-23 17:26:19 -0600, Tristan Partin wrote:

On Tue Jan 23, 2024 at 4:23 PM CST, Andres Freund wrote:

Hi,

On 2024-01-23 15:50:11 -0600, Tristan Partin wrote:

What is keeping us from using pthread_sigmask(3) instead of sigprocmask(2)?

We would need to make sure to compile with threading support everywhere. One
issue is that on some platforms things get slower once you actually start
using pthreads.

What are examples of these reduced performance platforms?

With some libc, including IIRC glibc, FILE* style io starts to use locking,
for example. Which we likely shouldn't use as heavily as we do, but we
currently do use it for things like COPY.

From reading the meson.build files, it seems like building with threading
enabled is the future, so should we just rip the band-aid off for 17?

Building with -pthreads and using threads are separate things...

If an extension can guarantee that threads that get launched by it don't
interact with anything Postgres-related, would that be enough to protect
from any fork(2) related issues?

A fork() while threads are running is undefined behavior IIRC, and undefined
behavior isn't limited to a single thread. You'd definitely need to use
pthread_sigprocmask etc to address that aspect alone.

If you can find a resource that explains the UB, I would be very interested
to read that. I found a SO[0] answer that made it seem like this actually
wasn't the case.

I think there are safe ways to do it, but I don't think we currently reliably
do so. It certainly wouldn't be well defined to have a thread created in
postmaster, before backends are forked off ("the child process may only
execute async-signal-safe operations until such time as one of the exec
functions is called").

Greetings,

Andres Freund

On Wed, Jan 24, 2024 at 1:39 PM Andres Freund <andres@anarazel.de> wrote:

On 2024-01-23 17:26:19 -0600, Tristan Partin wrote:

On Tue Jan 23, 2024 at 4:23 PM CST, Andres Freund wrote:

A fork() while threads are running is undefined behavior IIRC, and undefined
behavior isn't limited to a single thread. You'd definitely need to use
pthread_sigprocmask etc to address that aspect alone.

If you can find a resource that explains the UB, I would be very interested
to read that. I found a SO[0] answer that made it seem like this actually
wasn't the case.

I think there are safe ways to do it, but I don't think we currently reliably
do so. It certainly wouldn't be well defined to have a thread created in
postmaster, before backends are forked off ("the child process may only
execute async-signal-safe operations until such time as one of the exec
functions is called").

Right, the classic example is that if you fork() while some other
thread is in malloc() or fwrite() or whatever libc or other unknown
code it might hold a mutex that will never be released in the child.

As for what exactly might be happening in this case, I tried calling
SCDynamicStoreCopyProxies() and saw a new thread sitting in
__workq_kernreturn, which smells like libdispatch AKA GCD, Apple's
thread pool job dispatch thing. I tried to step my way through and
follow along on Apple's github and saw plenty of uses of libdispatch
in CoreFoundation code, but not the culprit, and then I hit libxpc,
which is closed source so I lost interest. Boo. Then I found this
article that says some interesting stuff about all that:

https://www.evanjones.ca/fork-is-dangerous.html

That code has changed a bit since then but still tries to detect unsafe forks.

https://github.com/apple-oss-distributions/libdispatch/blob/main/src/init.c

These days, I don't think the original corruption complaint that led
to that am-I-multihreaded check being added to PostgreSQL could happen
anyway, because the postmaster would now process its state machine
serially in the main thread's work loop even if a random unexpected
thread happened to run the handler. But obviously that doesn't help
us with these other complications so that observation isn't very
interesting.

As for sigprocmask() vs pthread_sigmask(), the sources of
unspecifiedness I am aware of are: (1) Unix vendors disagreeing on
whether the former affected only the calling thread or the whole
process before threads were standardised, and we can see that they
still differ today (eg Darwin/XNU loops over all threads setting them,
while many other systems do exactly the same as pthread_sigmask()),
and (2) libcs sometimes use or defer some signals for their own
private purposes, so sometimes pthread_sigmask() has a wrapper doing
some footwork in userspace rather than just invoking the system call,
but I dunno. In one of the threads about avoiding bad behaviour
around system(), I think there might have been some ideas about
getting rid of the need to block signals at all, which I think must be
theoretically possible if the handlers are smart enough to avoid
misbehaving in child processes, and maybe we use moar latches.