From: Nathan Bossart <nathandbossart@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: Andy Fan <zhihuifan1213@163.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: pgbench error: (setshell) of script 0; execution of meta-command failed
Date: Mon, 13 Jan 2025 10:04:31 -0600
Message-ID: <Z4U5j_FWpNLjtdU8@nathan> (raw)
In-Reply-To: <127979.1736622253@sss.pgh.pa.us>
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>
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
Attachments:v1-0001-modify-pqsignal.patch (text/plain, 2.7K) download
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.hindex 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.cindex 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)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, tgl@sss.pgh.pa.us, masao.fujii@oss.nttdata.com, zhihuifan1213@163.com, pgsql-hackers@lists.postgresql.org
Subject: Re: pgbench error: (setshell) of script 0; execution of meta-command failed
In-Reply-To: <Z4U5j_FWpNLjtdU8@nathan>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox