On Fri, Jul 15, 2022 at 3:27 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > Alvaro Herrera <alvhe...@alvh.no-ip.org> writes: > > ISTM it would be cleaner to patch PG_SETMASK to have a second argument > > and to return the original mask if that's not NULL. This is more > > invasive, but there aren't that many callsites of that macro. > > [ shoulda read your message before replying ] > > Given that this needs back-patched, I think changing PG_SETMASK > is a bad idea: there might be outside callers. However, we could > add another macro with the additional argument. PG_GET_AND_SET_MASK?
It's funny though, the reason we had PG_SETMASK in the first place is not for Windows. Ancient commit 47937403676 added that for long gone pre-POSIX systems like NeXTSTEP which only had single-argument sigsetmask(), not sigprocmask(). In general on Windows we're emulating POSIX signal interfaces with normal names like sigemptyset() etc, it just so happens that we chose to emulate that pre-standard sigsetmask() interface (as you complained about in the commit message for a65e0864). So why would I add another wrapper like PG_SETMASK and leave it unimplemented for now on Windows, when I could just use sigprocmask() directly and leave it unimplemented for now on Windows? The only reason I can think of for a wrapper is to provide a place to check the return code and ereport (panic?). That seems to be of limited value (how can it fail ... bad "how" value, or a sandbox denying some system calls, ...?). I did make sure to preserve the errno though; even though we're assuming these calls can't fail by long standing tradition, I didn't feel like additionally assuming that successful calls don't clobber errno. I guess, coded like this, it should also be safe to do it in the postmaster, but I think maybe we should leave it conditional, rather than relying on BlockSig being initialised and sane during early postmaster initialisation.
From 17eb588af303873dbd76d0f24b536f74747cb3bf Mon Sep 17 00:00:00 2001 From: Thomas Munro <thomas.mu...@gmail.com> Date: Fri, 15 Jul 2022 09:00:12 +1200 Subject: [PATCH] Make dsm_impl_resize more future-proof. Commit 4518c798 blocks signals for a short region of code, but it assumed that whatever calls it had the mask set to UnBlockSig on entry. That may be true today, but it would be more resilient to save and restore the caller's signal mask. Previously we couldn't do this because our macro for abstracting signal mask changes was based on the old pre-POSIX sigsetmask(), due to 1990s portability concerns. Since this is a POSIX-only code path, and since a65e0864 established that supported Unix systems must have sigprocmask(), we can just use sigprocmask() directly. It would also be implementable for Windows, but that's not needed for this. Back-patch to all releases, like 4518c798 and 80845b7c. Reviewed-by: Alvaro Herrera <alvhe...@alvh.no-ip.org> Reviewed-by: Tom Lane <t...@sss.pgh.pa.us> Discussion: https://postgr.es/m/CA%2BhUKGKx6Biq7_UuV0kn9DW%2B8QWcpJC1qwhizdtD9tN-fn0H0g%40mail.gmail.com --- src/backend/storage/ipc/dsm_impl.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/backend/storage/ipc/dsm_impl.c b/src/backend/storage/ipc/dsm_impl.c index 0247d13a91..69c6df75b4 100644 --- a/src/backend/storage/ipc/dsm_impl.c +++ b/src/backend/storage/ipc/dsm_impl.c @@ -49,6 +49,7 @@ #include "postgres.h" #include <fcntl.h> +#include <signal.h> #include <unistd.h> #ifndef WIN32 #include <sys/mman.h> @@ -62,7 +63,7 @@ #endif #include "common/file_perm.h" -#include "libpq/pqsignal.h" /* for PG_SETMASK macro */ +#include "libpq/pqsignal.h" #include "miscadmin.h" #include "pgstat.h" #include "portability/mem.h" @@ -355,6 +356,7 @@ dsm_impl_posix_resize(int fd, off_t size) { int rc; int save_errno; + sigset_t save_sigmask; /* * Block all blockable signals, except SIGQUIT. posix_fallocate() can run @@ -363,7 +365,7 @@ dsm_impl_posix_resize(int fd, off_t size) * conflicts), the retry loop might never succeed. */ if (IsUnderPostmaster) - PG_SETMASK(&BlockSig); + sigprocmask(SIG_SETMASK, &BlockSig, &save_sigmask); pgstat_report_wait_start(WAIT_EVENT_DSM_ALLOCATE); #if defined(HAVE_POSIX_FALLOCATE) && defined(__linux__) @@ -402,7 +404,7 @@ dsm_impl_posix_resize(int fd, off_t size) if (IsUnderPostmaster) { save_errno = errno; - PG_SETMASK(&UnBlockSig); + sigprocmask(SIG_SETMASK, &save_sigmask, NULL); errno = save_errno; } -- 2.30.2
