Thread
-
Additional message in pg_terminate_backend
Roman Khapov <rkhapov@yandex-team.ru> — 2025-12-13T07:44:46Z
Hi hackers! Recently I started working on patch for adding additional message from admin in pg_terminate_backend for one of our greenplum fork. The main idea is that there must be done little investigation every time you see 'FATAL: terminating connection due to administrator command’ to understand the reason, especially in cases where connection was terminated by another user. So it was decided to create some new functions, that allows to terminate connection with additional message. I did POC patches with the next main ideas: - lets add termReasonStr field in every PGPROC, that field can be used in ProcessInterrupts() - implementation of pg_terminate_backend/pg_cancel_backend should be accessible from extensions, so lets move it in pg_terminate_backend_impl/pg_cancel_backend_impl and add definitions for it somewhere - write simple extensions, which defines functions like pg_terminate_backend_msg, that sets termReasonStr and calls pg_terminate_backend_impl After patch and added extension, it is possible to do smth like: postgres=# select pg_terminate_backend_msg(pg_backend_pid(), 0, ’the message'); FATAL: terminating connection due to administrator command: the message The general question I want to ask: can this patches be useful for vanilla PostgreSQL? If so, there are some questions about patch improvements: - maybe the message can be delivered to backend by some other way than the field in struct PGPROC? termReasonStr field consumes some shared memory and are used in rare cases - names of all new funcs/fields/etc should be changed to some better names - new file signalfuncs.h seems like too complicated solutions to define *_impl functions, it can be done in some other files? ----- Best regards, Roman Khapov
-
Re: Additional message in pg_terminate_backend
Andrey Borodin <x4mmm@yandex-team.ru> — 2025-12-17T11:34:09Z
> On 13 Dec 2025, at 12:44, Roman Khapov <rkhapov@yandex-team.ru> wrote: > > Recently I started working on patch for adding additional message from admin > in pg_terminate_backend for one of our greenplum fork. The main idea is that there must be > done little investigation every time you see 'FATAL: terminating connection due to administrator command’ to understand the reason, especially in cases where connection was terminated by another > user. So it was decided to create some new functions, that allows to terminate connection with > additional message. Overall idea seems good to me. 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. > > I did POC patches with the next main ideas: > - lets add termReasonStr field in every PGPROC, that field can be used in ProcessInterrupts() > - implementation of pg_terminate_backend/pg_cancel_backend should be accessible from extensions, so lets move it in pg_terminate_backend_impl/pg_cancel_backend_impl and add definitions for it somewhere > - write simple extensions, which defines functions like pg_terminate_backend_msg, that sets termReasonStr and calls pg_terminate_backend_impl First thing that we need to do is to agree on API of the new feature. We do not need core-extension separation for this. My vote would be for having pg_cancel_backend(reason text)\pg_terminate_backend(reason text) along with parameterless versions. 32 bytes per PGPROC seems reasonable for a "reason". The patch doesn't seem to take care of cleaning "termReasonStr". Is it done elsewhere? We have a race condition if many backends cancel same backend. Won't they mess each other's reason? Thanks! Best regards, Andrey Borodin.
-
Re: Additional message in pg_terminate_backend
Daniel Gustafsson <daniel@yesql.se> — 2025-12-17T16:02:29Z
> On 13 Dec 2025, at 08:44, Roman Khapov <rkhapov@yandex-team.ru> wrote: > Recently I started working on patch for adding additional message from admin > in pg_terminate_backend for one of our greenplum fork. Greenplum already has support for passing a message in the terminate command doesnt it? Or at least it used to have but perhaps it was ripped out, my memory is getting a bit fuzzy. https://github.com/greenplum-db/gpdb-archive/commit/fa6c2d43d675aa05e2d9f797e3008f6fe075ee2c -- Daniel Gustafsson
-
Re: Additional message in pg_terminate_backend
Roman Khapov <rkhapov@yandex-team.ru> — 2025-12-20T09:26:40Z
> On 17 Dec 2025, at 21:02, Daniel Gustafsson <daniel@yesql.se> wrote: > > Greenplum already has support for passing a message in the terminate command > doesnt it? Or at least it used to have but perhaps it was ripped out, my > memory is getting a bit fuzzy. > > https://github.com/greenplum-db/gpdb-archive/commit/fa6c2d43d675aa05e2d9f797e3008f6fe075ee2c > Well, seems like I missed that patch.. Anyway, now I need same functionality in PostgreSQL, and your patch seems interesting, especially in part where you keep messages in separated shmem region. So I adopted your patch, maybe in that form it can be useful for PostgreSQL?
-
Re: Additional message in pg_terminate_backend
Kirill Reshke <reshkekirill@gmail.com> — 2025-12-20T10:32:34Z
On Sat, 20 Dec 2025 at 14:27, Roman Khapov <rkhapov@yandex-team.ru> wrote: > > > > On 17 Dec 2025, at 21:02, Daniel Gustafsson <daniel@yesql.se> wrote: > > > > Greenplum already has support for passing a message in the terminate command > > doesnt it? Or at least it used to have but perhaps it was ripped out, my > > memory is getting a bit fuzzy. > > > > https://github.com/greenplum-db/gpdb-archive/commit/fa6c2d43d675aa05e2d9f797e3008f6fe075ee2c > > > > Well, seems like I missed that patch.. > > Anyway, now I need same functionality in PostgreSQL, and your patch seems interesting, > especially in part where you keep messages in separated shmem region. > > So I adopted your patch, maybe in that form it can be useful for PostgreSQL? > > +CREATE OR REPLACE FUNCTION > + pg_terminate_backend_msg(pid integer, timeout int8 DEFAULT 0, msg text DEFAULT '') > + RETURNS boolean STRICT VOLATILE LANGUAGE INTERNAL AS 'pg_terminate_backend_msg' > + PARALLEL SAFE; I don't think we need to create a function with a name other than `pg_terminate_backend`. I also do not think we need pg_terminate_backend_msg as a wrapper to another function - all of this can be a single function, accepting different number of params, exampli gratia "bt_index_check" ``` Datum bt_index_check(PG_FUNCTION_ARGS) { Oid indrelid = PG_GETARG_OID(0); ... if (PG_NARGS() >= 2) args.heapallindexed = PG_GETARG_BOOL(1); if (PG_NARGS() >= 3) args.checkunique = PG_GETARG_BOOL(2); .... PG_RETURN_VOID(); } ``` -- Best regards, Kirill Reshke -
Re: Additional message in pg_terminate_backend
Jim Jones <jim.jones@uni-muenster.de> — 2026-02-01T00:08:31Z
Hi Roman, As pointed out by Kirill, there is no reason to create pg_terminate_backend_msg or pg_cancel_backend_msg. You can simply use the existing functions and expand them to use one extra parameter, e.g. CREATE OR REPLACE FUNCTION pg_terminate_backend(pid integer, timeout int8 DEFAULT 0, msg text DEFAULT '') RETURNS boolean STRICT VOLATILE LANGUAGE INTERNAL AS 'pg_terminate_backend' PARALLEL SAFE; Here I don't think we need to check PG_NARGS, since the function calls will always have a default value. Something like this perhaps: Datum pg_terminate_backend(PG_FUNCTION_ARGS) { int pid; int timeout; /* milliseconds */ char *msg; pid = PG_GETARG_INT32(0); timeout = PG_GETARG_INT64(1); msg = text_to_cstring(PG_GETARG_TEXT_PP(2)); return pg_terminate_backend_internal(pid, timeout, msg); } stpncpy() -> strlcpy() Documentation is missing -- assuming the feature design is already solid. Add backend_msg.c to meson.build. I rebased the patch (it was failing for quite some time) with some suggestions. Feel free to remove them and revert to your v2 if you disagree. I'll review the rest of code in the next days. Best, Jim -
Re: Additional message in pg_terminate_backend
Jim Jones <jim.jones@uni-muenster.de> — 2026-02-01T13:58:32Z
On 01/02/2026 01:08, Jim Jones wrote: > As pointed out by Kirill, there is no reason to create > pg_terminate_backend_msg or pg_cancel_backend_msg. You can simply use > the existing functions and expand them to use one extra parameter Since the message's size is limited to BACKEND_MSG_MAX_LEN, shouldn't you use it to limit msg at pg_terminate_backend[_msg]()? Something like: Datum pg_terminate_backend(PG_FUNCTION_ARGS) { int pid; int timeout; /* milliseconds */ char msg[BACKEND_MSG_MAX_LEN]; pid = PG_GETARG_INT32(0); timeout = PG_GETARG_INT64(1); text_to_cstring_buffer(PG_GETARG_TEXT_PP(2), msg, sizeof(msg)); return pg_terminate_backend_internal(pid, timeout, msg); } -
Re: Additional message in pg_terminate_backend
Roman Khapov <rkhapov@yandex-team.ru> — 2026-02-03T07:52:57Z
Hi Jim! Thanks for your review and rebase! > Since the message's size is limited to BACKEND_MSG_MAX_LEN, shouldn't > you use it to limit msg at pg_terminate_backend[_msg]()? Something like: > The message is truncated inside BackendMsgSet function, so I see a little point in truncating it at pg_terminate_backend.. Also, changing the stpncpy() to strlcpy() breaks the logic of returning result length of the message and NOTICE message about it, so I reverted this change. But this note make me think about adding test to truncation logic, so I added it in v4. -- Best regards, Roman Khapov
-
Re: Additional message in pg_terminate_backend
Jim Jones <jim.jones@uni-muenster.de> — 2026-02-03T08:21:57Z
On 03/02/2026 08:52, Roman Khapov wrote: > The message is truncated inside BackendMsgSet function, so I see a little > point in truncating it at pg_terminate_backend.. Thanks for the update! You might have a point there. One issue I see is with UTF8 character boundaries. Calculating len like this can lead to invalid characters, for instance: postgres=# SELECT pg_terminate_backend(pg_backend_pid(),0,repeat('🐘',1000)); NOTICE: message is too long, truncated to 127 FATAL: terminating connection due to administrator command: 🐘🐘🐘🐘🐘🐘🐘🐘🐘🐘🐘🐘🐘🐘🐘🐘🐘🐘🐘🐘🐘🐘🐘🐘🐘🐘🐘🐘🐘🐘🐘� server closed the connection unexpectedly This probably means the server terminated abnormally You might wanna take a look at other alternatives, such as pg_mbcliplen, for instance (pseudocode): len = strlen(msg); if (len >= sizeof(slot->msg)) len = pg_mbcliplen(msg, len, sizeof(slot->msg) - 1); memcpy(slot->msg, msg, len); slot->msg[len] = '\0'; Best, Jim -
Re: Additional message in pg_terminate_backend
Andrey Borodin <x4mmm@yandex-team.ru> — 2026-02-03T08:26:43Z
> On 3 Feb 2026, at 12:52, Roman Khapov <rkhapov@yandex-team.ru> wrote: > > <v4-0001-message-in-pg_terminate_backend-and-pg_cancel_bac.patch> Some notes on this version: 1. Did you mean BackendMsgShmemSize()? size = add_size(size, BackendStatusShmemSize()); 2. In docs: <function>pg_cancel_backend</function> ( <parameter>pid</parameter> <type>integer</type>, <parameter>message</parameter> <type>test</type> <literal>DEFAULT</literal> <literal>''</literal> ) Did you mean <type>text</type>? 3. Windows build failed [0] 4. In src/include/utils/misc/backend_msg.c identification is backend_msg.h Best regards, Andrey Borodin. [0] https://github.com/x4m/postgres_g/runs/62314358734 -
Re: Additional message in pg_terminate_backend
Roman Khapov <rkhapov@yandex-team.ru> — 2026-02-03T12:28:50Z
Hi again Jim, Andrew! Thanks for another round on review, updated the patch according to comments. Also, fix `make check` by updating pg_proc.data with new functions, similar to uuid7 way: defining _msg versions of the functions -- Best regards, Roman Khapov
-
Re: Additional message in pg_terminate_backend
Jim Jones <jim.jones@uni-muenster.de> — 2026-02-03T18:22:16Z
Hi Roman On 03/02/2026 13:28, Roman Khapov wrote: > Thanks for another round on review, updated the patch according to comments. > > Also, fix `make check` by updating pg_proc.data with new functions, > similar to uuid7 way: defining _msg versions of the functions Why did you decide to revert the pg_proc.dat and system_functions.sql changes from v3? In v3 I thought that adding a msg DEFAULT '' to the corresponding function in system_functions.sql ... --- a/src/backend/catalog/system_functions.sql +++ b/src/backend/catalog/system_functions.sql @@ -400,7 +400,12 @@ CREATE OR REPLACE FUNCTION PARALLEL SAFE; CREATE OR REPLACE FUNCTION - pg_terminate_backend(pid integer, timeout int8 DEFAULT 0) + pg_cancel_backend(pid integer, msg 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, msg text DEFAULT '') RETURNS boolean STRICT VOLATILE LANGUAGE INTERNAL AS 'pg_terminate_backend' PARALLEL SAFE; ... and updating pg_proc.dat accordingly would do the trick diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 5e5e33f64f..47aa30d716 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -6724,10 +6724,11 @@ { oid => '2171', descr => 'cancel a server process\' current query', proname => 'pg_cancel_backend', provolatile => 'v', prorettype => 'bool', - proargtypes => 'int4', prosrc => 'pg_cancel_backend' }, + proargtypes => 'int4 text', proargnames => '{pid,msg}', + 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}', + proargtypes => 'int4 int8 text', proargnames => '{pid,timeout,msg}', prosrc => 'pg_terminate_backend' }, { oid => '2172', descr => 'prepare for taking an online backup', proname => 'pg_backup_start', provolatile => 'v', proparallel => 'r', My rationale is based on the header in system_function.sql: ... * src/backend/catalog/system_functions.sql * * This file redefines certain built-in functions that are impractical * to fully define in pg_proc.dat. In most cases that's because they use * SQL-standard function bodies and/or default expressions... Am I missing something? Thanks Best, Jim -
Re: Additional message in pg_terminate_backend
warda Bibi <wardabibi221@gmail.com> — 2026-04-05T19:27:33Z
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 -
Re: Additional message in pg_terminate_backend
Jim Jones <jim.jones@uni-muenster.de> — 2026-04-09T11:07:12Z
Hi Warda and Prafulla On 05/04/2026 21:27, warda Bibi wrote: > Prafulla Ranadive and I reviewed this patch through the patch review > workshop. Overall, we find that the proposed feature is useful. Thanks for the nice review! Roman, are you still planning to work on this? The patch has been failing for several weeks. https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F6331 Best, Jim
-
Re: Additional message in pg_terminate_backend
Roman Khapov <rkhapov@yandex-team.ru> — 2026-04-10T06:05:08Z
Hi! Thanks Warda for review! Yes, I am planning to continue work on this patch, will send new version soon. -- Best regards, Roman Khapov > On 9 Apr 2026, at 16:07, Jim Jones <jim.jones@uni-muenster.de> wrote: > > Hi Warda and Prafulla > > On 05/04/2026 21:27, warda Bibi wrote: >> Prafulla Ranadive and I reviewed this patch through the patch review >> workshop. Overall, we find that the proposed feature is useful. > > Thanks for the nice review! > > Roman, are you still planning to work on this? The patch has been > failing for several weeks. > > https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F6331 > > Best, Jim >
-
Re: Additional message in pg_terminate_backend
Paul A Jungwirth <pj@illuminatedcomputing.com> — 2026-06-23T18:16:47Z
We talked about this patch for the Patch Review Workshop, and I thought I would share a few thoughts. On Sun, Apr 5, 2026 at 12:27 PM warda Bibi <wardabibi221@gmail.com> wrote: > > 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. I agree about adding defaults to the existing functions instead of introducing new ones. This doesn't need to be an extension, so there is no need to have separate functions. > --- 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; > + Are these entries still needed? After 759b03b24c we can set defaults with just pg_proc.dat. > 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). This seems like a nice improvement. But is there a race condition? What keeps the backend from being killed (or just finishing the connection) and the slot being re-used? v5 was checking the pid while holding the spinlock; perhaps we should still do that. > 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()? Since we aren't implementing this as an extension, I don't think there is any reason to keep the _internal helper functions. > 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? I think last-writer-wins is okay for canceling the same backend twice, but we should make sure the slot is still owned by the right pid. > 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? What if we emit msg as an errdetail? That seems to resolve the translation difficulties, and it makes the logs more structured. > 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? That seems okay. Agreed about documenting it. The v6 patch looks like an incremental patch on top of a previous patch (v5 I think). But v5, v4, and v3 no longer apply. Would you mind rebasing and putting all the changes into one patch? I think your suggestions are extensive enough that a single file would be easier to review. Yours, -- Paul ~{:-) pj@illuminatedcomputing.com -
Re: Additional message in pg_terminate_backend
warda Bibi <wardabibi221@gmail.com> — 2026-06-29T10:21:23Z
Hi Paul, Roman, and all, Thank you for the thorough review, Paul. I have put together v7 addressing all of the feedback below. On Mon, Jun 23, 2026, Paul A Jungwirth wrote: > Are these entries still needed? After 759b03b24c we can set defaults with just pg_proc.dat. Correct. v7 removes the system_functions.sql entries entirely; the DEFAULT values are now specified only in pg_proc.dat, which is sufficient after 759b03b24c. > What keeps the backend from being killed and the slot being re-used? v5 was checking the pid while holding the spinlock; perhaps we should still do that. BackendMsgSet() now acquires the slot spinlock before checking slot->pid == pid, ensuring that a slot reused by a new backend after the target exited cannot be written to. The O(1) procno indexing introduced in v6 is retained. > I don't think there is any reason to keep the _internal helper functions. pg_cancel_backend_internal() and pg_terminate_backend_internal() have been inlined into their respective callers, since each had only a single call site. > What if we emit msg as an errdetail? Done. Both paths now use errdetail("%s", msg) instead of appending the message to errmsg(). > 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? The documentation now notes that parallel workers always receive the generic termination message. > The v6 patch looks like an incremental patch on top of a previous > patch (v5 I think). But v5, v4, and v3 no longer apply. Would you mind > rebasing and putting all the changes into one patch? Done. v7 is a single self-contained patch rebased on the current master (b574fec00f2). Testing: All 7 subtests in src/test/modules/test_misc/t/014_backend_msg.pl pass on Linux (Ubuntu 24.04, arm64): t/014_backend_msg.pl .. ok Result: PASS Patch attached. Best Regards, Warda Bibi On Tue, Jun 23, 2026 at 11:17 PM Paul A Jungwirth < pj@illuminatedcomputing.com> wrote: > We talked about this patch for the Patch Review Workshop, and I > thought I would share a few thoughts. > > On Sun, Apr 5, 2026 at 12:27 PM warda Bibi <wardabibi221@gmail.com> wrote: > > > > 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. > > I agree about adding defaults to the existing functions instead of > introducing new ones. This doesn't need to be an extension, so there > is no need to have separate functions. > > > --- 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; > > + > > Are these entries still needed? After 759b03b24c we can set defaults > with just pg_proc.dat. > > > 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). > > This seems like a nice improvement. But is there a race condition? > What keeps the backend from being killed (or just finishing the > connection) and the slot being re-used? v5 was checking the pid while > holding the spinlock; perhaps we should still do that. > > > 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()? > > Since we aren't implementing this as an extension, I don't think there > is any reason to keep the _internal helper functions. > > > 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? > > I think last-writer-wins is okay for canceling the same backend twice, > but we should make sure the slot is still owned by the right pid. > > > 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? > > What if we emit msg as an errdetail? That seems to resolve the > translation difficulties, and it makes the logs more structured. > > > 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? > > That seems okay. Agreed about documenting it. > > The v6 patch looks like an incremental patch on top of a previous > patch (v5 I think). But v5, v4, and v3 no longer apply. Would you mind > rebasing and putting all the changes into one patch? I think your > suggestions are extensive enough that a single file would be easier to > review. > > Yours, > > -- > Paul ~{:-) > pj@illuminatedcomputing.com >