public inbox for [email protected]
help / color / mirror / Atom feedFrom: warda Bibi <[email protected]>
To: Jim Jones <[email protected]>
Cc: Roman Khapov <[email protected]>
Cc: Andrey Borodin <[email protected]>
Cc: Kirill Reshke <[email protected]>
Cc: Daniel Gustafsson <[email protected]>
Cc: [email protected]
Subject: Re: Additional message in pg_terminate_backend
Date: Mon, 6 Apr 2026 00:27:33 +0500
Message-ID: <CAJqHjGogwJd9+ESibKU1WYqhMK+7UaoTDF1oh99z9Q3wiEn0jw@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<[email protected]>
<CALdSSPhU526xXqjsb=BPO689+qFJQDeimWrhOv=ehzveQsZJgw@mail.gmail.com>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
Hi all,
Thank you for the continued work on this patch.
Prafulla Ranadive and I reviewed this patch through the patch review
workshop. Overall, we find that the proposed feature is useful. We
have reviewed all five versions and have the following observations.
We agree with Jim's point of view that the v3 approach is better. v5
introduced separate _msg overloads (OID 8223 for
pg_cancel_backend_msg, OID 8222 for pg_terminate_backend_msg), which
adds catalog bloat and forces callers to use a different function
name:
> +{ oid => '8223', descr => 'cancel a server process\' current query',
> proname => 'pg_cancel_backend', provolatile => 'v', prorettype => 'bool',
> proargtypes => 'int4 text', prosrc => 'pg_cancel_backend_msg' },
> +{ oid => '8222', descr => 'terminate a server process',
> proname => 'pg_terminate_backend', provolatile => 'v', prorettype => 'bool',
> proargtypes => 'int4 int8 text', proargnames => '{pid,timeout,message}',
> prosrc => 'pg_terminate_backend_msg' },
V3 keeps one OID per function and stays backward-compatible. v6
restores the v3 approach.
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -6727,19 +6727,14 @@
{ oid => '2171', descr => 'cancel a server process\' current query',
proname => 'pg_cancel_backend', provolatile => 'v', prorettype => 'bool',
- proargtypes => 'int4', prosrc => 'pg_cancel_backend' },
-{ oid => '8223', descr => 'cancel a server process\' current query',
- proname => 'pg_cancel_backend', provolatile => 'v', prorettype => 'bool',
- proargtypes => 'int4 text', prosrc => 'pg_cancel_backend_msg' },
+ proargtypes => 'int4 text', proargnames => '{pid,message}',
+ proargdefaults => '{""}'.
+ prosrc => 'pg_cancel_backend' },
{ oid => '2096', descr => 'terminate a server process',
- proname => 'pg_terminate_backend', provolatile => 'v', prorettype => 'bool',
- proargtypes => 'int4 int8', proargnames => '{pid,timeout}',
- proargdefaults => '{0}',
- prosrc => 'pg_terminate_backend' },
-{ oid => '8222', descr => 'terminate a server process',
proname => 'pg_terminate_backend', provolatile => 'v', prorettype => 'bool',
proargtypes => 'int4 int8 text', proargnames => '{pid,timeout,message}',
- prosrc => 'pg_terminate_backend_msg' },
+ proargdefaults => '{0,""}',
+ prosrc => 'pg_terminate_backend' },
--- a/src/backend/catalog/system_functions.sql
+++ b/src/backend/catalog/system_functions.sql
@@ -378,6 +378,16 @@ BEGIN ATOMIC
END;
+CREATE OR REPLACE FUNCTION
+ pg_cancel_backend(pid integer, message text DEFAULT '')
+ RETURNS boolean STRICT VOLATILE LANGUAGE INTERNAL AS 'pg_cancel_backend'
+ PARALLEL SAFE;
+
+CREATE OR REPLACE FUNCTION
+ pg_terminate_backend(pid integer, timeout int8 DEFAULT 0, message
text DEFAULT '')
+ RETURNS boolean STRICT VOLATILE LANGUAGE INTERNAL AS 'pg_terminate_backend'
+ PARALLEL SAFE;
+
The current approach scans all MaxBackends slots by PID twice. Since
pg_signal_backend() already calls BackendPidGetProc(pid) and has the
PGPROC* in hand, the PGPROC index into allProcs[ ] is the ProcNumber,
which is also the direct index into BackendMsgSlots[]. So v6 changes
the signature to BackendMsgSet(ProcNumber procno, ...) and the call in
pg_signal_backend() passes GetNumberFromPGProc(proc), making the
lookup O(1).
-int BackendMsgSet(pid_t pid, const char *msg)
+int BackendMsgSet(ProcNumber procno, const char *msg)
{
BackendMsgSlot *slot;
int len;
@@ -94,35 +94,26 @@ int BackendMsgSet(pid_t pid, const char *msg)
if (msg == NULL || msg[0] == '\0')
return 0;
- for (int i = 0; i < MaxBackends; ++i)
- {
- slot = &BackendMsgSlots[i];
-
- if (slot->pid == 0 || slot->pid != pid)
- continue;
-
- SpinLockAcquire(&slot->lock);
-
- if (slot->pid != pid)
- {
- SpinLockRelease(&slot->lock);
- break;
- }
+ slot = &BackendMsgSlots[procno];
Also, fixed a stale file-header path in backend_msg.c:
--- a/src/backend/utils/misc/backend_msg.c
+++ b/src/backend/utils/misc/backend_msg.c
@@ -7,7 +7,7 @@
* Portions Copyright (c) 1996-2026, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
*
- * src/include/utils/misc/backend_msg.c
+ * src/backend/utils/misc/backend_msg.c
Removed dead NULL checks on msg in pg_cancel_backend_msg() and
pg_terminate_backend_msg() because text_to_cstring() never returns
NULL.
pid = PG_GETARG_INT32(0);
msg = text_to_cstring(PG_GETARG_TEXT_PP(1));
- if (msg == NULL) {
- PG_RETURN_BOOL(false);
- }
Furthermore, the else before the fallback ereport(FATAL) in the
terminate path is missing in v5. Without the else, the fallback
ereport is unreachable when BackendMsgIsSet() is true because
ereport(FATAL) never returns, but it misleads readers into thinking
the generic message is always emitted. Adding an else is also
consistent with the pg_cancel approach. v6 adds else to make the
intent explicit.
- ereport(FATAL,
- (errcode(ERRCODE_ADMIN_SHUTDOWN),
- errmsg("terminating connection due to
administrator command")));
+ else
+ ereport(FATAL,
+ (errcode(ERRCODE_ADMIN_SHUTDOWN),
+ errmsg("terminating connection due to
administrator command")));
}
}
The _internal helper functions (pg_cancel_backend_internal,
pg_terminate_backend_internal) each have only one call site. What is
the intent behind keeping them separate, or can they be inlined
directly into pg_cancel_backend() and pg_terminate_backend()?
Andrey noted that:
> We have a race condition if many backends cancel same backend.
> Won't they mess each other's reason?
The spinlock in BackendMsgSet protects the write, but there is no
atomicity between writing the slot and delivering the signal. Another
backend can overwrite the slot between one writer's write and the
target reading it. Would it be valuable to define the behavior
explicitly, for example last-writer-wins, or document the limitation?
Andrey also raised this about translation:
> Keep in mind that Postgres literals are translated into many languages.
> So text ought to be clear enough for translators to build a sentence
> that precedes termination reason.
The current pattern:
> errmsg("terminating connection due to administrator command: %s", msg)
The colon-append pattern breaks in languages where the reason clause
is structured differently. Any suggestions on how to approach this, or
how similar cases are handled elsewhere in the codebase?
Also, parallel workers receive the generic message as they hit the
IsBackgroundWorker branch in ProcessInterrupts(), which bypasses
BackendMsgGet(). This is fine since parallel workers are ephemeral,
but should it be documented in the docs?
Patch attached.
--
Best Wishes,
Warda Bibi
Attachments:
[application/octet-stream] v6-0001-message-in-pg_terminate_backend-and-pg_cancel_backend.patch (8.8K, 2-v6-0001-message-in-pg_terminate_backend-and-pg_cancel_backend.patch)
download | inline diff:
From 3e166b0fb0cc9800a9a898240adb4750e4c6c705 Mon Sep 17 00:00:00 2001
From: Warda Bibi <[email protected]>
Date: Sat, 4 Apr 2026 20:12:58 +0500
Subject: [PATCH v6] message in pg_terminate_backend and pg_cancel_backend
Sometimes it is useful to terminate or cancel a backend with an
additional message explaining the reason, so the client sees something
more informative than the generic error text.
This patch adds an optional message argument to pg_terminate_backend()
and pg_cancel_backend(), folded into the existing functions via DEFAULT
parameters so no new catalog entries are needed:
pg_cancel_backend(pid integer, message text DEFAULT '')
pg_terminate_backend(pid integer, timeout int8 DEFAULT 0, message text DEFAULT '')
The message is stored in a dedicated shared memory region
(BackendMsgSlots, one slot per MaxBackends indexed by ProcNumber) and
read by the target backend in ProcessInterrupts() before the
ereport(FATAL/ERROR). Unicode-safe truncation is applied via
pg_mbcliplen() if the message exceeds BACKEND_MSG_MAX_LEN bytes, with
a NOTICE emitted to the caller.
Author: Daniel Gustafsson <[email protected]>
Author: Roman Khapov <[email protected]>
Reviewed-by: Kirill Reshke <[email protected]>
Reviewed-by: Jim Jones <[email protected]>
Reviewed-by: Andrey Borodin <[email protected]>
---
src/backend/catalog/system_functions.sql | 10 ++++++
src/backend/storage/ipc/signalfuncs.c | 32 ++----------------
src/backend/tcop/postgres.c | 8 ++---
src/backend/utils/misc/backend_msg.c | 41 +++++++++---------------
src/include/catalog/pg_proc.dat | 15 +++------
src/include/utils/backend_msg.h | 4 ++-
6 files changed, 41 insertions(+), 69 deletions(-)
diff --git a/src/backend/catalog/system_functions.sql b/src/backend/catalog/system_functions.sql
index 69699f8830a..7dbad6a279b 100644
--- a/src/backend/catalog/system_functions.sql
+++ b/src/backend/catalog/system_functions.sql
@@ -378,6 +378,16 @@ BEGIN ATOMIC
END;
+CREATE OR REPLACE FUNCTION
+ pg_cancel_backend(pid integer, message text DEFAULT '')
+ RETURNS boolean STRICT VOLATILE LANGUAGE INTERNAL AS 'pg_cancel_backend'
+ PARALLEL SAFE;
+
+CREATE OR REPLACE FUNCTION
+ pg_terminate_backend(pid integer, timeout int8 DEFAULT 0, message text DEFAULT '')
+ RETURNS boolean STRICT VOLATILE LANGUAGE INTERNAL AS 'pg_terminate_backend'
+ PARALLEL SAFE;
+
--
-- The default permissions for functions mean that anyone can execute them.
-- A number of functions shouldn't be executable by just anyone, but rather
diff --git a/src/backend/storage/ipc/signalfuncs.c b/src/backend/storage/ipc/signalfuncs.c
index 2501ec58658..eabb341a2ba 100644
--- a/src/backend/storage/ipc/signalfuncs.c
+++ b/src/backend/storage/ipc/signalfuncs.c
@@ -112,7 +112,7 @@ pg_signal_backend(int pid, int sig, const char *msg)
if (msg != NULL && msg[0] != '\0')
{
- int r = BackendMsgSet(pid, msg);
+ int r = BackendMsgSet(GetNumberFromPGProc(proc), msg);
if (r != -1 && r != strlen(msg))
ereport(NOTICE,
@@ -172,23 +172,11 @@ pg_cancel_backend_internal(pid_t pid, const char *msg)
Datum pg_cancel_backend(PG_FUNCTION_ARGS)
{
int pid;
-
- pid = PG_GETARG_INT32(0);
-
- return pg_cancel_backend_internal(pid, NULL);
-}
-
-Datum pg_cancel_backend_msg(PG_FUNCTION_ARGS)
-{
- int pid;
- char *msg;
+ char *msg;
Datum result;
pid = PG_GETARG_INT32(0);
msg = text_to_cstring(PG_GETARG_TEXT_PP(1));
- if (msg == NULL) {
- PG_RETURN_BOOL(false);
- }
result = pg_cancel_backend_internal(pid, msg);
@@ -313,26 +301,12 @@ Datum pg_terminate_backend(PG_FUNCTION_ARGS)
{
int pid;
int timeout_ms;
-
- pid = PG_GETARG_INT32(0);
- timeout_ms = PG_GETARG_INT64(1);
-
- return pg_terminate_backend_internal(pid, timeout_ms, NULL);
-}
-
-Datum pg_terminate_backend_msg(PG_FUNCTION_ARGS)
-{
- int pid;
- int timeout_ms;
- char *msg;
+ char *msg;
Datum result;
pid = PG_GETARG_INT32(0);
timeout_ms = PG_GETARG_INT64(1);
msg = text_to_cstring(PG_GETARG_TEXT_PP(2));
- if (msg == NULL) {
- PG_RETURN_BOOL(false);
- }
result = pg_terminate_backend_internal(pid, timeout_ms, msg);
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 92040d748f4..25d2857f69f 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -3402,10 +3402,10 @@ ProcessInterrupts(void)
(errcode(ERRCODE_ADMIN_SHUTDOWN),
errmsg("terminating connection due to administrator command: %s", msg)));
}
-
- ereport(FATAL,
- (errcode(ERRCODE_ADMIN_SHUTDOWN),
- errmsg("terminating connection due to administrator command")));
+ else
+ ereport(FATAL,
+ (errcode(ERRCODE_ADMIN_SHUTDOWN),
+ errmsg("terminating connection due to administrator command")));
}
}
diff --git a/src/backend/utils/misc/backend_msg.c b/src/backend/utils/misc/backend_msg.c
index 34d44069056..560aee148be 100644
--- a/src/backend/utils/misc/backend_msg.c
+++ b/src/backend/utils/misc/backend_msg.c
@@ -7,7 +7,7 @@
* Portions Copyright (c) 1996-2026, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
*
- * src/include/utils/misc/backend_msg.c
+ * src/backend/utils/misc/backend_msg.c
*
*--------------------------------------------------------------------
*/
@@ -86,7 +86,7 @@ void BackendMsgInit(int id)
on_shmem_exit(backend_msg_slot_clean, Int32GetDatum(0) /* not used */);
}
-int BackendMsgSet(pid_t pid, const char *msg)
+int BackendMsgSet(ProcNumber procno, const char *msg)
{
BackendMsgSlot *slot;
int len;
@@ -94,35 +94,26 @@ int BackendMsgSet(pid_t pid, const char *msg)
if (msg == NULL || msg[0] == '\0')
return 0;
- for (int i = 0; i < MaxBackends; ++i)
- {
- slot = &BackendMsgSlots[i];
-
- if (slot->pid == 0 || slot->pid != pid)
- continue;
-
- SpinLockAcquire(&slot->lock);
-
- if (slot->pid != pid)
- {
- SpinLockRelease(&slot->lock);
- break;
- }
+ slot = &BackendMsgSlots[procno];
- len = pg_mbcliplen(msg, strlen(msg), sizeof(slot->msg) - 1);
- memcpy(slot->msg, msg, len);
- slot->msg[len] = '\0';
+ SpinLockAcquire(&slot->lock);
+ if (slot->pid == 0)
+ {
SpinLockRelease(&slot->lock);
-
- return len;
+ ereport(LOG,
+ (errmsg("can't set message for missing backend, requested by %ld",
+ (long) MyProcPid)));
+ return -1;
}
- ereport(LOG,
- (errmsg("can't set message for missing backend %ld, requested by %ld",
- (long) pid, (long) MyProcPid)));
+ len = pg_mbcliplen(msg, strlen(msg), sizeof(slot->msg) - 1);
+ memcpy(slot->msg, msg, len);
+ slot->msg[len] = '\0';
- return -1;
+ SpinLockRelease(&slot->lock);
+
+ return len;
}
int BackendMsgGet(char *buf, int max_len)
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 2732dcb6867..3833fc4ba2c 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -6727,19 +6727,14 @@
{ oid => '2171', descr => 'cancel a server process\' current query',
proname => 'pg_cancel_backend', provolatile => 'v', prorettype => 'bool',
- proargtypes => 'int4', prosrc => 'pg_cancel_backend' },
-{ oid => '8223', descr => 'cancel a server process\' current query',
- proname => 'pg_cancel_backend', provolatile => 'v', prorettype => 'bool',
- proargtypes => 'int4 text', prosrc => 'pg_cancel_backend_msg' },
+ proargtypes => 'int4 text', proargnames => '{pid,message}',
+ proargdefaults => '{""}',
+ prosrc => 'pg_cancel_backend' },
{ oid => '2096', descr => 'terminate a server process',
- proname => 'pg_terminate_backend', provolatile => 'v', prorettype => 'bool',
- proargtypes => 'int4 int8', proargnames => '{pid,timeout}',
- proargdefaults => '{0}',
- prosrc => 'pg_terminate_backend' },
-{ oid => '8222', descr => 'terminate a server process',
proname => 'pg_terminate_backend', provolatile => 'v', prorettype => 'bool',
proargtypes => 'int4 int8 text', proargnames => '{pid,timeout,message}',
- prosrc => 'pg_terminate_backend_msg' },
+ proargdefaults => '{0,""}',
+ prosrc => 'pg_terminate_backend' },
{ oid => '2172', descr => 'prepare for taking an online backup',
proname => 'pg_backup_start', provolatile => 'v', proparallel => 'r',
prorettype => 'pg_lsn', proargtypes => 'text bool',
diff --git a/src/include/utils/backend_msg.h b/src/include/utils/backend_msg.h
index 7e53c59845f..5ccad83af11 100644
--- a/src/include/utils/backend_msg.h
+++ b/src/include/utils/backend_msg.h
@@ -15,12 +15,14 @@
#ifndef BACKEND_MSG_H
#define BACKEND_MSG_H
+#include "storage/proc.h"
+
#define BACKEND_MSG_MAX_LEN 128
extern void BackendMsgShmemInit(void);
extern Size BackendMsgShmemSize(void);
extern void BackendMsgInit(int id);
-extern int BackendMsgSet(pid_t pid, const char *msg);
+extern int BackendMsgSet(ProcNumber procno, const char *msg);
extern int BackendMsgGet(char *buf, int max_len);
extern bool BackendMsgIsSet(void);
--
2.51.0
view thread (15+ messages) latest in thread
reply
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: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Additional message in pg_terminate_backend
In-Reply-To: <CAJqHjGogwJd9+ESibKU1WYqhMK+7UaoTDF1oh99z9Q3wiEn0jw@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox