Date: Mon, 13 Jan 2025 10:04:31 -0600
From: Nathan Bossart <nathandbossart@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Fujii Masao <masao.fujii@oss.nttdata.com>,
	Andy Fan <zhihuifan1213@163.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: pgbench error: (setshell) of script 0; execution of meta-command
 failed
Message-ID: <Z4U5j_FWpNLjtdU8@nathan>
References: <87msfz5qv2.fsf@163.com>
 <87ikqn5em7.fsf@163.com>
 <e03fc711-8c48-4fb8-ba94-0862c920c30f@oss.nttdata.com>
 <b0afe844-84b7-4154-ad2c-6763ff84d876@oss.nttdata.com>
 <2422778.1736521065@sss.pgh.pa.us>
 <2425050.1736522451@sss.pgh.pa.us>
 <127979.1736622253@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="1NgM7R4OkTY29DBX"
Content-Disposition: inline
In-Reply-To: <127979.1736622253@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/Z4U5j_FWpNLjtdU8%40nathan>
Precedence: bulk


--1NgM7R4OkTY29DBX
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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

--1NgM7R4OkTY29DBX
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename="v1-0001-modify-pqsignal.patch"

From 93ea04b912a90e6a166941c575b4da128e3b7533 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@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)


--1NgM7R4OkTY29DBX--