On Sat, Jan 11, 2025 at 02:04:13PM -0500, Tom Lane wrote:
> After studying this more, I think what we should do in HEAD is
> even more aggressive: let's make the real name of port/pqsignal.c's
> function be either pqsignal_fe in frontend, or pqsignal_be in backend.
> This positively ensures that there's no collision with libpq's
> export, and it seems like a good idea for the additional reason that
> the frontend and backend versions are not really interchangeable.
That would be nice.
> However, we can't get away with that in released branches because
> it'd be an ABI break for any outside code that calls pqsignal.
> What I propose doing in the released branches is what's shown in
> the attached patch for v17: rename port/pqsignal.c's function to
> pqsignal_fe in frontend, but leave it as pqsignal in the backend.
> Leaving it as pqsignal in the backend preserves ABI for extension
> modules, at the cost that it's not entirely clear which pqsignal
> an extension that's also linked with libpq will get. But that
> hazard affects none of our code, and it existed already for any
> third-party extensions that call pqsignal. In principle using
> "pqsignal_fe" in frontend creates an ABI hazard for outside users of
> libpgport.a. But I think it's not really a problem, because we don't
> support that as a shared library. As long as people build with
> headers and .a files from the same minor release, they'll be OK.
I don't have any concrete reasons to think you are wrong about this, but it
does feel somewhat risky to me. It might be worth testing it with a couple
of third-party projects that use the frontend version of pqsignal().
codesearch.debian.net shows a couple that may be doing so [0] [1] [2].
> 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).
[0] https://github.com/gleu/pgstats
[1] https://github.com/hapostgres/pg_auto_failover
[2] https://github.com/credativ/pg_checksums
--
nathan
>From 93ea04b912a90e6a166941c575b4da128e3b7533 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nat...@postgresql.org>
Date: Mon, 13 Jan 2025 09:27:28 -0600
Subject: [PATCH v1 1/1] modify pqsignal
---
src/include/port.h | 2 +-
src/port/pqsignal.c | 32 ++++++--------------------------
2 files changed, 7 insertions(+), 27 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 f707b1c54e1..076e3dfe340 100644
--- a/src/port/pqsignal.c
+++ b/src/port/pqsignal.c
@@ -114,29 +114,15 @@ wrapper_handler(SIGNAL_ARGS)
*
* 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.
* 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 +141,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)
