convert libpgport's pqsignal() to a void function
(moving to a new thread)
On Mon, Jan 13, 2025 at 10:04:31AM -0600, Nathan Bossart wrote:
On Sat, Jan 11, 2025 at 02:04:13PM -0500, Tom Lane wrote:
BTW, this decouples legacy-pqsignal.c from pqsignal.c enough
that we could now do what's contemplated in the comments from
3b00fdba9: simplify that version by making it return void,
or perhaps better just a true/false success report.
I've not touched that point here, though.I think it should just return void since AFAICT nobody checks the return
value. But it would probably be a good idea to add some sort of error
checking within the function. I've attached a draft patch that adds some
new assertions. I originally tried to use elog() where it was available,
but besides making the code even more unreadable, I think it's unnecessary
(since AFAICT any problems are likely coding errors).
With commit 9a45a89 in place, this is now possible. I've attached a new
version of the patch with a more fleshed-out commit message and a small
comment fix.
For background, the protections added by commit 3b00fdb introduced race
conditions to pqsignal() that could lead to bogus return values. That's of
little consequence since nobody seems to look at the return values, but it
would have been nice to convert it to a void function to avoid any
possibility of a bogus return value. Unfortunately, doing so would have
required also modifying legacy-pqsignal.c's version of the function, which
would've required an SONAME bump, which didn't seem worth it. Or so I
thought...
Thanks to commit 9a45a89, legacy-pqsignal.c now has its own dedicated
extern for pqsignal(), which decouples it enough that we can follow through
with changing libpqport's pqsignal() to a void function.
Thoughts?
--
nathan
Attachments:
v2-0001-Convert-libpgport-s-pqsignal-to-a-void-function.patchtext/plain; charset=us-asciiDownload
From 7b6e061be774d05d888bc5eac20e9aa09fc75403 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Tue, 14 Jan 2025 20:34:10 -0600
Subject: [PATCH v2 1/1] Convert libpgport's pqsignal() to a void function.
The protections added by commit 3b00fdba9f introduced race
conditions to this function that can lead to bogus return values.
Since nobody seems to inspect the return value, this is of little
consequence, but it would have been nice to convert it to a void
function to avoid any possibility of a bogus return value.
Unfortunately, doing so would have required also modifying
legacy-pqsignal.c's version of the function, which would've
required an SONAME bump. Or so I thought...
Thanks to commit 9a45a89c38, legacy-pqsignal.c now has its own
dedicated extern for pqsignal(), which decouples it enough that we
can follow through with changing libpgport's pqsignal() to a void
function. This commit also adds a bit of error checking in the
form of assertions for the return value of sigaction()/signal().
Since a failure most likely indicates a coding error, and nobody
has ever bothered to check pqsignal()'s return value, it's probably
not worth doing anything fancier.
---
src/include/port.h | 2 +-
src/port/pqsignal.c | 34 ++++++----------------------------
2 files changed, 7 insertions(+), 29 deletions(-)
diff --git a/src/include/port.h b/src/include/port.h
index f0e28ce5c53..4e9e5657872 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -520,7 +520,7 @@ extern int pg_mkdir_p(char *path, int omode);
#define pqsignal pqsignal_be
#endif
typedef void (*pqsigfunc) (SIGNAL_ARGS);
-extern pqsigfunc pqsignal(int signo, pqsigfunc func);
+extern void pqsignal(int signo, pqsigfunc func);
/* port/quotes.c */
extern char *escape_single_quotes_ascii(const char *src);
diff --git a/src/port/pqsignal.c b/src/port/pqsignal.c
index 1169de6b81e..5dd8b76bae8 100644
--- a/src/port/pqsignal.c
+++ b/src/port/pqsignal.c
@@ -112,31 +112,15 @@ wrapper_handler(SIGNAL_ARGS)
/*
* Set up a signal handler, with SA_RESTART, for signal "signo"
*
- * Returns the previous handler.
- *
- * NB: If called within a signal handler, race conditions may lead to bogus
- * return values. You should either avoid calling this within signal handlers
- * or ignore the return value.
- *
- * XXX: Since no in-tree callers use the return value, and there is little
- * reason to do so, it would be nice if we could convert this to a void
- * function instead of providing potentially-bogus return values.
- * Unfortunately, that requires modifying the pqsignal() in legacy-pqsignal.c,
- * which in turn requires an SONAME bump, which is probably not worth it.
- *
* Note: the actual name of this function is either pqsignal_fe when
* compiled with -DFRONTEND, or pqsignal_be when compiled without that.
* This is to avoid a name collision with libpq's legacy-pqsignal.c.
*/
-pqsigfunc
+void
pqsignal(int signo, pqsigfunc func)
{
- pqsigfunc orig_func = pqsignal_handlers[signo]; /* assumed atomic */
#if !(defined(WIN32) && defined(FRONTEND))
- struct sigaction act,
- oact;
-#else
- pqsigfunc ret;
+ struct sigaction act;
#endif
Assert(signo < PG_NSIG);
@@ -155,17 +139,11 @@ pqsignal(int signo, pqsigfunc func)
if (signo == SIGCHLD)
act.sa_flags |= SA_NOCLDSTOP;
#endif
- if (sigaction(signo, &act, &oact) < 0)
- return SIG_ERR;
- else if (oact.sa_handler == wrapper_handler)
- return orig_func;
- else
- return oact.sa_handler;
+ if (sigaction(signo, &act, NULL) < 0)
+ Assert(false); /* probably indicates coding error */
#else
/* Forward to Windows native signal system. */
- if ((ret = signal(signo, func)) == wrapper_handler)
- return orig_func;
- else
- return ret;
+ if (signal(signo, func) == SIG_ERR)
+ Assert(false); /* probably indicates coding error */
#endif
}
--
2.39.5 (Apple Git-154)
Nathan Bossart <nathandbossart@gmail.com> writes:
Thanks to commit 9a45a89, legacy-pqsignal.c now has its own dedicated
extern for pqsignal(), which decouples it enough that we can follow through
with changing libpqport's pqsignal() to a void function.
Thoughts?
LGTM, although I don't know enough about Windows to know if the
"== SIG_ERR" test in that path is correct.
regards, tom lane
On Tue, Jan 14, 2025 at 10:02:46PM -0500, Tom Lane wrote:
LGTM, although I don't know enough about Windows to know if the
"== SIG_ERR" test in that path is correct.
It's apparently not [0]https://cirrus-ci.com/task/6237809813487616. :(
My guess is that this has something to do with redefining SIG_ERR in
win32_port.h. We might be able to use push_macro/pop_macro to keep the old
value around, but at the moment I'm leaning towards just removing the
assertion in that path.
[0]: https://cirrus-ci.com/task/6237809813487616
--
nathan
Nathan Bossart <nathandbossart@gmail.com> writes:
On Tue, Jan 14, 2025 at 10:02:46PM -0500, Tom Lane wrote:
LGTM, although I don't know enough about Windows to know if the
"== SIG_ERR" test in that path is correct.
It's apparently not [0]. :(
Bleah.
My guess is that this has something to do with redefining SIG_ERR in
win32_port.h. We might be able to use push_macro/pop_macro to keep the old
value around, but at the moment I'm leaning towards just removing the
assertion in that path.
I wonder why we redefine those values? But I tend to agree that just
removing the test is sufficient for now. Given the lack of failure
checks in the existing code, and the lack of trouble reports
suggesting any problem, it's hard to muster enthusiasm for spending
a lot of effort on this.
regards, tom lane
On Tue, Jan 14, 2025 at 11:08:05PM -0500, Tom Lane wrote:
Nathan Bossart <nathandbossart@gmail.com> writes:
My guess is that this has something to do with redefining SIG_ERR in
win32_port.h. We might be able to use push_macro/pop_macro to keep the old
value around, but at the moment I'm leaning towards just removing the
assertion in that path.I wonder why we redefine those values?
I wondered the same. Those redefines have been there since commit 5049196,
but I haven't been able to find any real discussion in the archives about
it. Maybe I will bug Magnus about it sometime, in case he happens to
remember the reason.
But I tend to agree that just
removing the test is sufficient for now. Given the lack of failure
checks in the existing code, and the lack of trouble reports
suggesting any problem, it's hard to muster enthusiasm for spending
a lot of effort on this.
Assuming cfbot likes this new version of the patch, I'll commit it shortly.
Thanks for reviewing.
--
nathan
Attachments:
v3-0001-Convert-libpgport-s-pqsignal-to-a-void-function.patchtext/plain; charset=us-asciiDownload
From c7d182c41aa518f3b6479ee4bd6fdf6e8f2484c7 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Wed, 15 Jan 2025 13:04:58 -0600
Subject: [PATCH v3 1/1] Convert libpgport's pqsignal() to a void function.
The protections added by commit 3b00fdba9f introduced race
conditions to this function that can lead to bogus return values.
Since nobody seems to inspect the return value, this is of little
consequence, but it would have been nice to convert it to a void
function to avoid any possibility of a bogus return value.
Unfortunately, doing so would have required also modifying
legacy-pqsignal.c's version of the function, which would've
required an SONAME bump. Or so I thought...
Thanks to commit 9a45a89c38, legacy-pqsignal.c now has its own
dedicated extern for pqsignal(), which decouples it enough that we
can follow through with changing libpgport's pqsignal() to a void
function. This commit also adds a bit of error checking in the
form of an assertion for the return value of sigaction(). Since a
failure most likely indicates a coding error, and nobody has ever
bothered to check pqsignal()'s return value, it's probably not
worth doing anything fancier.
---
src/include/port.h | 2 +-
src/port/pqsignal.c | 39 ++++++++++-----------------------------
2 files changed, 11 insertions(+), 30 deletions(-)
diff --git a/src/include/port.h b/src/include/port.h
index f0e28ce5c5..4e9e565787 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -520,7 +520,7 @@ extern int pg_mkdir_p(char *path, int omode);
#define pqsignal pqsignal_be
#endif
typedef void (*pqsigfunc) (SIGNAL_ARGS);
-extern pqsigfunc pqsignal(int signo, pqsigfunc func);
+extern void pqsignal(int signo, pqsigfunc func);
/* port/quotes.c */
extern char *escape_single_quotes_ascii(const char *src);
diff --git a/src/port/pqsignal.c b/src/port/pqsignal.c
index 1169de6b81..9777b3a930 100644
--- a/src/port/pqsignal.c
+++ b/src/port/pqsignal.c
@@ -112,31 +112,15 @@ wrapper_handler(SIGNAL_ARGS)
/*
* Set up a signal handler, with SA_RESTART, for signal "signo"
*
- * Returns the previous handler.
- *
- * NB: If called within a signal handler, race conditions may lead to bogus
- * return values. You should either avoid calling this within signal handlers
- * or ignore the return value.
- *
- * XXX: Since no in-tree callers use the return value, and there is little
- * reason to do so, it would be nice if we could convert this to a void
- * function instead of providing potentially-bogus return values.
- * Unfortunately, that requires modifying the pqsignal() in legacy-pqsignal.c,
- * which in turn requires an SONAME bump, which is probably not worth it.
- *
* Note: the actual name of this function is either pqsignal_fe when
* compiled with -DFRONTEND, or pqsignal_be when compiled without that.
* This is to avoid a name collision with libpq's legacy-pqsignal.c.
*/
-pqsigfunc
+void
pqsignal(int signo, pqsigfunc func)
{
- pqsigfunc orig_func = pqsignal_handlers[signo]; /* assumed atomic */
#if !(defined(WIN32) && defined(FRONTEND))
- struct sigaction act,
- oact;
-#else
- pqsigfunc ret;
+ struct sigaction act;
#endif
Assert(signo < PG_NSIG);
@@ -155,17 +139,14 @@ pqsignal(int signo, pqsigfunc func)
if (signo == SIGCHLD)
act.sa_flags |= SA_NOCLDSTOP;
#endif
- if (sigaction(signo, &act, &oact) < 0)
- return SIG_ERR;
- else if (oact.sa_handler == wrapper_handler)
- return orig_func;
- else
- return oact.sa_handler;
+ if (sigaction(signo, &act, NULL) < 0)
+ Assert(false); /* probably indicates coding error */
#else
- /* Forward to Windows native signal system. */
- if ((ret = signal(signo, func)) == wrapper_handler)
- return orig_func;
- else
- return ret;
+
+ /*
+ * Forward to Windows native signal system. Ideally, we'd check for
+ * SIG_ERR here, but win32_port.h redefines it for some reason.
+ */
+ (void) signal(signo, func);
#endif
}
--
2.39.5 (Apple Git-154)
On Thu, Jan 16, 2025 at 8:15 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
On Tue, Jan 14, 2025 at 11:08:05PM -0500, Tom Lane wrote:
Nathan Bossart <nathandbossart@gmail.com> writes:
My guess is that this has something to do with redefining SIG_ERR in
win32_port.h. We might be able to use push_macro/pop_macro to keep the old
value around, but at the moment I'm leaning towards just removing the
assertion in that path.I wonder why we redefine those values?
I wondered the same. Those redefines have been there since commit 5049196,
but I haven't been able to find any real discussion in the archives about
it. Maybe I will bug Magnus about it sometime, in case he happens to
remember the reason.
My guess would be: perhaps some ancient version of MinGW didn't define
them? They're defined by MinGW and native signal.h now and they have
the same values, so we should remove them I think.
Assertion failed: 0, file ../src/port/pqsignal.c, line 147
Could be due to calling native signal() with a signal number other
than the 6 values required to work by the C standard?
On Thu, Jan 16, 2025 at 08:21:08AM +1300, Thomas Munro wrote:
On Thu, Jan 16, 2025 at 8:15 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
On Tue, Jan 14, 2025 at 11:08:05PM -0500, Tom Lane wrote:
I wonder why we redefine those values?
I wondered the same. Those redefines have been there since commit 5049196,
but I haven't been able to find any real discussion in the archives about
it. Maybe I will bug Magnus about it sometime, in case he happens to
remember the reason.My guess would be: perhaps some ancient version of MinGW didn't define
them? They're defined by MinGW and native signal.h now and they have
the same values, so we should remove them I think.
Okay. If nothing else, it'd be interesting to see what the buildfarm
thinks.
Assertion failed: 0, file ../src/port/pqsignal.c, line 147
Could be due to calling native signal() with a signal number other
than the 6 values required to work by the C standard?
Looking closer, that probably makes more sense than my SIG_ERR redefinition
theory. If that assertion is getting hit, that means signal() _is_
returning SIG_ERR (either the system one or our redefined version), and it
looks like it's pretty common to use -1 for SIG_ERR. That'd only affect
Windows frontend programs, but it still sounds scary. I'll try getting
more details about the error with some custom cfbot runs.
--
nathan
On Wed, Jan 15, 2025 at 01:47:18PM -0600, Nathan Bossart wrote:
On Thu, Jan 16, 2025 at 08:21:08AM +1300, Thomas Munro wrote:
Could be due to calling native signal() with a signal number other
than the 6 values required to work by the C standard?Looking closer, that probably makes more sense than my SIG_ERR redefinition
theory. If that assertion is getting hit, that means signal() _is_
returning SIG_ERR (either the system one or our redefined version), and it
looks like it's pretty common to use -1 for SIG_ERR. That'd only affect
Windows frontend programs, but it still sounds scary. I'll try getting
more details about the error with some custom cfbot runs.
I think this is what's happening. cfbot's test failures are caused by
initdb's setup_signals(). The call to pqsignal(SIGHUP, trapsig) seems to
fail because SIGHUP isn't a real signal number, just something that's made
up in win32_port.h. This SIGHUP definition was added by commit 12c9423 in
May 2003, then the pqsignal(SIGHUP, ...) call was added in initdb by commit
279598b in November 2003, but it might not have been broken at that time
because it doesn't look like initdb.c included the SIGHUP #define. In any
case, I think this has been broken since at least commit ed9b360 (November
2017), if not earlier.
Perhaps we should surround all those extra signal #defines in win32_port.h
with an #ifndef FRONTEND. initdb seems to be good about avoiding
pqsignal() calls if the signal doesn't exist, but I wouldn't be surprised
if there are other frontend programs that are not so cautious. I'll give
it a try on cfbot and see what breaks...
--
nathan
On Thu, Jan 16, 2025 at 8:47 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
On Thu, Jan 16, 2025 at 08:21:08AM +1300, Thomas Munro wrote:
Could be due to calling native signal() with a signal number other
than the 6 values required to work by the C standard?Looking closer, that probably makes more sense than my SIG_ERR redefinition
theory. If that assertion is getting hit, that means signal() _is_
returning SIG_ERR (either the system one or our redefined version), and it
looks like it's pretty common to use -1 for SIG_ERR. That'd only affect
Windows frontend programs, but it still sounds scary. I'll try getting
more details about the error with some custom cfbot runs.
Windows barely has signals, so I don't think it's too scary... SIGINT
works but surprisingly runs the handler in another thread when you
press ^C, SIGABRT, SIGFPE, SIGSEGV presumably have the obvious
synchronous/exception-trapping implementations but we aren't even
trying to catch those*, and SIGTERM, SIGILL are pro forma, never
generated by the system. We've basically just invented more pro forma
ones that will also never be generated except in the backend's
entirely separate fake signal system that I hope to remove. +1 for
your idea of not defining them at all outside the backend, it's just
confusing noise.
The second surprising thing, unless you're an armchair Unix
archeologist, is that all but SIGFPE revert to SIG_DFL when they fire,
like POSIX's SA_RESETHAND mode. I don't know if it also has
SA_NODEFER behaviour (a combination of behaviours seen in some old
Unixen of yestermillennium: your handler had to keep reinstalling
itself, cf initdb.c:trapsig, but for a brief window the default
process-terminating-core-dumping-nasal-daemon behaviour was installed
and there was nothing you could do about that race; the BSD crew fixed
that mistake a long time ago, everyone does it that way now, and POSIX
sigaction() made those policies explicit and is the recommended
replacement, but to this day the C standard has just signal() with
undefined semantics and no requirement that any of it even work).
*Even in the backend we don't catch Windows' native SIGFPE AFAICS, so
I guess those must exit instead of being converted to an ERROR by
FloatExceptionHandler. I wonder if there is a way to reach that
condition from a SQL query.
On Thu, Jan 16, 2025 at 11:07:41AM +1300, Thomas Munro wrote:
+1 for your idea of not defining them at all outside the backend, it's
just confusing noise.
I tried that, but these extra signals are needed even in the frontend for
pgkill(), etc. My next thought was to simply ignore signal() errors for
the extra signals, but I don't think that's very nice because it just masks
broken code. Finally, I settled on modifying initdb's setup_signals() to
use WIN32 checks instead of checking for the signals themselves. This is
how it's done elsewhere, and this is apparently all that's needed to make
cfbot's Windows run happy.
I've also attached a 0002 that removes the redefinitions of SIG_ERR and
friends. That looks good on cfbot, but we'll see what the buildfarm has to
say...
--
nathan
Attachments:
v4-0001-Convert-libpgport-s-pqsignal-to-a-void-function.patchtext/plain; charset=us-asciiDownload
From 15613d15ea907fa1aceee84d3a2e8b3708f95b4f Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Wed, 15 Jan 2025 17:30:23 -0600
Subject: [PATCH v4 1/2] Convert libpgport's pqsignal() to a void function.
The protections added by commit 3b00fdba9f introduced race
conditions to this function that can lead to bogus return values.
Since nobody seems to inspect the return value, this is of little
consequence, but it would have been nice to convert it to a void
function to avoid any possibility of a bogus return value.
Unfortunately, doing so would have required also modifying
legacy-pqsignal.c's version of the function, which would've
required an SONAME bump. Or so I thought...
Thanks to commit 9a45a89c38, legacy-pqsignal.c now has its own
dedicated extern for pqsignal(), which decouples it enough that we
can follow through with changing libpgport's pqsignal() to a void
function. This commit also adds a bit of error checking in the
form of assertions for the return value of sigaction()/signal().
Since a failure most likely indicates a coding error, and nobody
has ever bothered to check pqsignal()'s return value, it's probably
not worth doing anything fancier.
While at it, modify initdb's setup_signals() to check for WIN32
instead of the signals themselves. win32_port.h defines many extra
signals, so the previous checks were insufficient to avoid calling
pqsignal() with invalid signals on Windows. It'd be nice to avoid
defining these extra signals in the frontend altogether, but they
are used for pgkill(), etc., and we already use WIN32 checks for
invalid signals in various places.
---
src/bin/initdb/initdb.c | 19 +++++--------------
src/include/port.h | 2 +-
src/port/pqsignal.c | 34 ++++++----------------------------
3 files changed, 12 insertions(+), 43 deletions(-)
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 101c780012..ea4b66b3bf 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -2874,27 +2874,18 @@ setup_text_search(void)
void
setup_signals(void)
{
- /* some of these are not valid on Windows */
-#ifdef SIGHUP
- pqsignal(SIGHUP, trapsig);
-#endif
-#ifdef SIGINT
pqsignal(SIGINT, trapsig);
-#endif
-#ifdef SIGQUIT
- pqsignal(SIGQUIT, trapsig);
-#endif
-#ifdef SIGTERM
pqsignal(SIGTERM, trapsig);
-#endif
+
+ /* the following are not valid on Windows */
+#ifndef WIN32
+ pqsignal(SIGHUP, trapsig);
+ pqsignal(SIGQUIT, trapsig);
/* Ignore SIGPIPE when writing to backend, so we can clean up */
-#ifdef SIGPIPE
pqsignal(SIGPIPE, SIG_IGN);
-#endif
/* Prevent SIGSYS so we can probe for kernel calls that might not work */
-#ifdef SIGSYS
pqsignal(SIGSYS, SIG_IGN);
#endif
}
diff --git a/src/include/port.h b/src/include/port.h
index f0e28ce5c5..4e9e565787 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -520,7 +520,7 @@ extern int pg_mkdir_p(char *path, int omode);
#define pqsignal pqsignal_be
#endif
typedef void (*pqsigfunc) (SIGNAL_ARGS);
-extern pqsigfunc pqsignal(int signo, pqsigfunc func);
+extern void pqsignal(int signo, pqsigfunc func);
/* port/quotes.c */
extern char *escape_single_quotes_ascii(const char *src);
diff --git a/src/port/pqsignal.c b/src/port/pqsignal.c
index 1169de6b81..5dd8b76bae 100644
--- a/src/port/pqsignal.c
+++ b/src/port/pqsignal.c
@@ -112,31 +112,15 @@ wrapper_handler(SIGNAL_ARGS)
/*
* Set up a signal handler, with SA_RESTART, for signal "signo"
*
- * Returns the previous handler.
- *
- * NB: If called within a signal handler, race conditions may lead to bogus
- * return values. You should either avoid calling this within signal handlers
- * or ignore the return value.
- *
- * XXX: Since no in-tree callers use the return value, and there is little
- * reason to do so, it would be nice if we could convert this to a void
- * function instead of providing potentially-bogus return values.
- * Unfortunately, that requires modifying the pqsignal() in legacy-pqsignal.c,
- * which in turn requires an SONAME bump, which is probably not worth it.
- *
* Note: the actual name of this function is either pqsignal_fe when
* compiled with -DFRONTEND, or pqsignal_be when compiled without that.
* This is to avoid a name collision with libpq's legacy-pqsignal.c.
*/
-pqsigfunc
+void
pqsignal(int signo, pqsigfunc func)
{
- pqsigfunc orig_func = pqsignal_handlers[signo]; /* assumed atomic */
#if !(defined(WIN32) && defined(FRONTEND))
- struct sigaction act,
- oact;
-#else
- pqsigfunc ret;
+ struct sigaction act;
#endif
Assert(signo < PG_NSIG);
@@ -155,17 +139,11 @@ pqsignal(int signo, pqsigfunc func)
if (signo == SIGCHLD)
act.sa_flags |= SA_NOCLDSTOP;
#endif
- if (sigaction(signo, &act, &oact) < 0)
- return SIG_ERR;
- else if (oact.sa_handler == wrapper_handler)
- return orig_func;
- else
- return oact.sa_handler;
+ if (sigaction(signo, &act, NULL) < 0)
+ Assert(false); /* probably indicates coding error */
#else
/* Forward to Windows native signal system. */
- if ((ret = signal(signo, func)) == wrapper_handler)
- return orig_func;
- else
- return ret;
+ if (signal(signo, func) == SIG_ERR)
+ Assert(false); /* probably indicates coding error */
#endif
}
--
2.39.5 (Apple Git-154)
v4-0002-Remove-redeclarations-of-signal-functions-on-Wind.patchtext/plain; charset=us-asciiDownload
From e99d35bb7ee3370a5a8688be320edd1df3dd0cbc Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Wed, 15 Jan 2025 17:36:09 -0600
Subject: [PATCH v4 2/2] Remove redeclarations of signal functions on Windows.
---
src/include/port/win32_port.h | 8 --------
1 file changed, 8 deletions(-)
diff --git a/src/include/port/win32_port.h b/src/include/port/win32_port.h
index 7900272e8d..ff7028bdc8 100644
--- a/src/include/port/win32_port.h
+++ b/src/include/port/win32_port.h
@@ -154,14 +154,6 @@
#define sigmask(sig) ( 1 << ((sig)-1) )
-/* Signal function return values */
-#undef SIG_DFL
-#undef SIG_ERR
-#undef SIG_IGN
-#define SIG_DFL ((pqsigfunc)0)
-#define SIG_ERR ((pqsigfunc)-1)
-#define SIG_IGN ((pqsigfunc)1)
-
/* Some extra signals */
#define SIGHUP 1
#define SIGQUIT 3
--
2.39.5 (Apple Git-154)
On Thu, Jan 16, 2025 at 12:47 PM Nathan Bossart
<nathandbossart@gmail.com> wrote:
On Thu, Jan 16, 2025 at 11:07:41AM +1300, Thomas Munro wrote:
+1 for your idea of not defining them at all outside the backend, it's
just confusing noise.I tried that, but these extra signals are needed even in the frontend for
pgkill(), etc.
Right, of course.
(In an unconf in Vancouver, a group of us were plotting that there
should be a more explicit control socket to talk to the postmaster, on
all OSes, to get current status, recovery progress, etc, and also
support shutdown commands more explicitly. If you separately turn all
the backend IPC signals into interrupts (CF 5118 and further work like
that), and these cluster-level ones you just mentioned into commands
in the hypothetical new cluster control protocol, then you don't need
fake signals for Windows anymore, but apparently I got ahead of myself
with that comment :-))
My next thought was to simply ignore signal() errors for
the extra signals, but I don't think that's very nice because it just masks
broken code. Finally, I settled on modifying initdb's setup_signals() to
use WIN32 checks instead of checking for the signals themselves. This is
how it's done elsewhere, and this is apparently all that's needed to make
cfbot's Windows run happy.
Cool.
I've also attached a 0002 that removes the redefinitions of SIG_ERR and
friends. That looks good on cfbot, but we'll see what the buildfarm has to
say...
Ditto.
I've now committed all of this. I ended up finding a couple other frontend
programs that called pqsignal() with an invalid signal number on Windows,
so I fixed those as well. AFAICT the reason I didn't catch them in my
earlier testing is because they aren't tested! I'll keep an eye on the
buildfarm for any problems with the SIG_* redefinition removal, too.
Thanks for reviewing and for sharing lots of context about this stuff.
--
nathan