Optional message to user when terminating/cancelling backend

Started by Daniel Gustafssonalmost 9 years ago79 messageshackers
Jump to latest
#1Daniel Gustafsson
daniel@yesql.se

When terminating, or cancelling, a backend it’s currently not possible to let
the signalled session know *why* it was dropped. This has nagged me in the
past and now it happened to come up again, so I took a stab at this. The
attached patch implements the ability to pass an optional text message to the
signalled session which is included in the error message:

SELECT pg_terminate_backend(<pid> [, message]);
SELECT pg_cancel_backend(<pid> [, message]);

Right now the message is simply appended on the error message, not sure if
errdetail or errhint would be better? Calling:

select pg_terminate_backend(<pid>, 'server rebooting');

..leads to:

FATAL: terminating connection due to administrator command: "server rebooting"

Omitting the message invokes the command just like today.

The message is stored in a new shmem area which is checked when the session is
aborted. To keep things simple a small buffer is kept per backend for the
message. If deemed too costly, keeping a central buffer from which slabs are
allocated can be done (but seemed rather complicated for little gain compared
to the quite moderate memory spend.)

cheers ./daniel

Attachments:

terminate_msg_v2.patchapplication/octet-stream; name=terminate_msg_v2.patchDownload+281-20
#2Pavel Stehule
pavel.stehule@gmail.com
In reply to: Daniel Gustafsson (#1)
Re: Optional message to user when terminating/cancelling backend

2017-06-19 20:24 GMT+02:00 Daniel Gustafsson <daniel@yesql.se>:

When terminating, or cancelling, a backend it’s currently not possible to
let
the signalled session know *why* it was dropped. This has nagged me in the
past and now it happened to come up again, so I took a stab at this. The
attached patch implements the ability to pass an optional text message to
the
signalled session which is included in the error message:

SELECT pg_terminate_backend(<pid> [, message]);
SELECT pg_cancel_backend(<pid> [, message]);

Right now the message is simply appended on the error message, not sure if
errdetail or errhint would be better? Calling:

select pg_terminate_backend(<pid>, 'server rebooting');

..leads to:

FATAL: terminating connection due to administrator command: "server
rebooting"

Omitting the message invokes the command just like today.

The message is stored in a new shmem area which is checked when the
session is
aborted. To keep things simple a small buffer is kept per backend for the
message. If deemed too costly, keeping a central buffer from which slabs
are
allocated can be done (but seemed rather complicated for little gain
compared
to the quite moderate memory spend.)

cheers ./daniel

+1

very good idea

Pavel

Show quoted text

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#3Satyanarayana Narlapuram
Satyanarayana.Narlapuram@microsoft.com
In reply to: Pavel Stehule (#2)
Re: Optional message to user when terminating/cancelling backend

+1.

This really helps PostgreSQL Azure service as well. When we are doing the upgrades to the service, instead of abruptly terminating the sessions we can provide this message.

Thanks,
Satya

From: pgsql-hackers-owner@postgresql.org [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Pavel Stehule
Sent: Monday, June 19, 2017 11:41 AM
To: Daniel Gustafsson <daniel@yesql.se>
Cc: PostgreSQL mailing lists <pgsql-hackers@postgresql.org>
Subject: Re: [HACKERS] Optional message to user when terminating/cancelling backend

2017-06-19 20:24 GMT+02:00 Daniel Gustafsson <daniel@yesql.se<mailto:daniel@yesql.se>>:
When terminating, or cancelling, a backend it’s currently not possible to let
the signalled session know *why* it was dropped. This has nagged me in the
past and now it happened to come up again, so I took a stab at this. The
attached patch implements the ability to pass an optional text message to the
signalled session which is included in the error message:

SELECT pg_terminate_backend(<pid> [, message]);
SELECT pg_cancel_backend(<pid> [, message]);

Right now the message is simply appended on the error message, not sure if
errdetail or errhint would be better? Calling:

select pg_terminate_backend(<pid>, 'server rebooting');

..leads to:

FATAL: terminating connection due to administrator command: "server rebooting"

Omitting the message invokes the command just like today.

The message is stored in a new shmem area which is checked when the session is
aborted. To keep things simple a small buffer is kept per backend for the
message. If deemed too costly, keeping a central buffer from which slabs are
allocated can be done (but seemed rather complicated for little gain compared
to the quite moderate memory spend.)

cheers ./daniel

+1

very good idea

Pavel

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org<mailto:pgsql-hackers@postgresql.org>)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers&lt;https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.postgresql.org%2Fmailpref%2Fpgsql-hackers&amp;data=02%7C01%7CSatyanarayana.Narlapuram%40microsoft.com%7C8f211f53d54c4d6308d308d4b742f824%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636334945639579811&amp;sdata=VH8ljYHI%2BaxoNdM7pYLpfEqa%2FbXWf0dRptoNzxElbaA%3D&amp;reserved=0&gt;

#4Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Satyanarayana Narlapuram (#3)
Re: Optional message to user when terminating/cancelling backend

Satyanarayana Narlapuram wrote:

+1.

This really helps PostgreSQL Azure service as well. When we are doing
the upgrades to the service, instead of abruptly terminating the
sessions we can provide this message.

I think you mean "in addition to" rather than "instead of".

Unless you have a lot of users running psql manually, I don't see how
this is actually very useful or actionable. What would the user do with
the information? Hopefully your users already trust that you'd keep the
downtime to the minimum possible.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#5David G. Johnston
david.g.johnston@gmail.com
In reply to: Alvaro Herrera (#4)
Re: Optional message to user when terminating/cancelling backend

On Tue, Jun 20, 2017 at 11:54 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Satyanarayana Narlapuram wrote:

Unless you have a lot of users running psql manually, I don't see how
this is actually very useful or actionable. What would the user do with
the information? Hopefully your users already trust that you'd keep the
downtime to the minimum possible.

Why wouldn't this be useful in application logs? Spurious dropped
connections during application execution would be alarming. Seeing a
message from the DBA when looking into those would be a painless and
quick way to alleviate stress.

pg_cancel_backend(<pid>, 'please try not to leave sessions in an "idle
in transaction" state...') would also seem like a useful message to
communicate; to user or application. Sure, some of this can, and
maybe would also need to, be done out-of-band but this communication
channel seems worthy enough to at least evaluate the provided
implementation.

David J.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#6Satyanarayana Narlapuram
Satyanarayana.Narlapuram@microsoft.com
In reply to: David G. Johnston (#5)
Re: Optional message to user when terminating/cancelling backend

Agree with David on the general usefulness of this channel. Not that Azure has this implementation or proposal today, but as a managed service this channel of communication is worth. For example, DBA / service can set a policy that if certain session exceeds the resource usage DBA can kill it and communicate the same. For example, too many locks, lot of IO activity, large open transactions etc. The messages will help application developer to tune their workloads appropriately.

Thanks,
Satya

-----Original Message-----
From: David G. Johnston [mailto:david.g.johnston@gmail.com]
Sent: Tuesday, June 20, 2017 12:44 PM
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Satyanarayana Narlapuram <Satyanarayana.Narlapuram@microsoft.com>; Pavel Stehule <pavel.stehule@gmail.com>; Daniel Gustafsson <daniel@yesql.se>; PostgreSQL mailing lists <pgsql-hackers@postgresql.org>
Subject: Re: [HACKERS] Optional message to user when terminating/cancelling backend

On Tue, Jun 20, 2017 at 11:54 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

Satyanarayana Narlapuram wrote:

Unless you have a lot of users running psql manually, I don't see how
this is actually very useful or actionable. What would the user do
with the information? Hopefully your users already trust that you'd
keep the downtime to the minimum possible.

Why wouldn't this be useful in application logs? Spurious dropped connections during application execution would be alarming. Seeing a message from the DBA when looking into those would be a painless and quick way to alleviate stress.

pg_cancel_backend(<pid>, 'please try not to leave sessions in an "idle in transaction" state...') would also seem like a useful message to communicate; to user or application. Sure, some of this can, and maybe would also need to, be done out-of-band but this communication channel seems worthy enough to at least evaluate the provided implementation.

David J.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#7Andres Freund
andres@anarazel.de
In reply to: Daniel Gustafsson (#1)
Re: Optional message to user when terminating/cancelling backend

Hi,

On 2017-06-19 20:24:43 +0200, Daniel Gustafsson wrote:

When terminating, or cancelling, a backend it’s currently not possible to let
the signalled session know *why* it was dropped. This has nagged me in the
past and now it happened to come up again, so I took a stab at this. The
attached patch implements the ability to pass an optional text message to the
signalled session which is included in the error message:

SELECT pg_terminate_backend(<pid> [, message]);
SELECT pg_cancel_backend(<pid> [, message]);

Right now the message is simply appended on the error message, not sure if
errdetail or errhint would be better? Calling:

select pg_terminate_backend(<pid>, 'server rebooting');

..leads to:

FATAL: terminating connection due to administrator command: "server rebooting"

Omitting the message invokes the command just like today.

For extensions it'd also be useful if it'd be possible to overwrite the
error code. E.g. for citus there's a distributed deadlock detector,
running out of process because there's no way to interrupt lock waits
locally, and we've to do some ugly hacking to generate proper error
messages and code from another session.

- Andres

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#8Michael Paquier
michael@paquier.xyz
In reply to: Daniel Gustafsson (#1)
Re: Optional message to user when terminating/cancelling backend

On Tue, Jun 20, 2017 at 3:24 AM, Daniel Gustafsson <daniel@yesql.se> wrote:

The message is stored in a new shmem area which is checked when the session is
aborted. To keep things simple a small buffer is kept per backend for the
message. If deemed too costly, keeping a central buffer from which slabs are
allocated can be done (but seemed rather complicated for little gain compared
to the quite moderate memory spend.)

I think that you are right to take the approach with a per-backend
slot. This will avoid complications related to entry removals and
locking issues. There would be scaling issues as well if things get
very signaled for a lot of backends.

+#define MAX_CANCEL_MSG 128
That looks enough.

+           LWLockAcquire(BackendCancelLock, LW_EXCLUSIVE);
+
+           strlcpy(slot->message, message, sizeof(slot->message));
+           slot->len = strlen(message);
Why not using one spin lock per slot and save it in BackendCancelShmemStruct?
+   pid_t       pid = PG_GETARG_INT32(0);
+   char       *msg = text_to_cstring(PG_GETARG_TEXT_PP(1));
+
+   PG_RETURN_BOOL(pg_terminate_backend_internal(pid, msg));
It would be more solid to add some error handling for messages that
are too long, or at least truncate the message if too long.
-- 
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#9Yugo Nagata
nagata@sraoss.co.jp
In reply to: Daniel Gustafsson (#1)
Re: Optional message to user when terminating/cancelling backend

Hi,

Here are some comments for the patch.

+Datum
+pg_cancel_backend(PG_FUNCTION_ARGS)
+{
+   PG_RETURN_BOOL(pg_cancel_backend_internal(PG_GETARG_INT32(0), NULL));
+}
+Datum
+pg_cancel_backend_msg(PG_FUNCTION_ARGS)
+{
+   pid_t       pid = PG_GETARG_INT32(0);
+   char       *msg = text_to_cstring(PG_GETARG_TEXT_PP(1));
+
+   PG_RETURN_BOOL(pg_cancel_backend_internal(pid, msg));
+}

It would be better to insert a blank line between these functions.

+/*
+ * Sets a cancellation message for the backend with the specified pid and
+ * returns the length of message actually created. If the returned length
+ * is equal to the length of the message parameter, truncation may have
+ * occurred. If the backend wasn't found and no message was set, -1 is
+ * returned.
+ */

It seems to me that this comment is incorrect.

"If the returned length is not equal to the length of the message parameter,"

is right, isn't it?

In addition, the last statement would be

"If the backend wasn't found, -1 is returned. Otherwize, if no message was set,
0 is returned."

+           strlcpy(slot->message, message, sizeof(slot->message));
+           slot->len = strlen(message);
+
+           LWLockRelease(BackendCancelLock);
+           return slot->len;

If SetBackendCancelMessage() has to return the length of message actually created,
slot->len should be strlen(slot->message) instead of strlen(message).
In the current code, when the return value and slot->len is always set
to the length of the passed message parameter.

Regards,

On Mon, 19 Jun 2017 20:24:43 +0200
Daniel Gustafsson <daniel@yesql.se> wrote:

When terminating, or cancelling, a backend it’s currently not possible to let
the signalled session know *why* it was dropped. This has nagged me in the
past and now it happened to come up again, so I took a stab at this. The
attached patch implements the ability to pass an optional text message to the
signalled session which is included in the error message:

SELECT pg_terminate_backend(<pid> [, message]);
SELECT pg_cancel_backend(<pid> [, message]);

Right now the message is simply appended on the error message, not sure if
errdetail or errhint would be better? Calling:

select pg_terminate_backend(<pid>, 'server rebooting');

..leads to:

FATAL: terminating connection due to administrator command: "server rebooting"

Omitting the message invokes the command just like today.

The message is stored in a new shmem area which is checked when the session is
aborted. To keep things simple a small buffer is kept per backend for the
message. If deemed too costly, keeping a central buffer from which slabs are
allocated can be done (but seemed rather complicated for little gain compared
to the quite moderate memory spend.)

cheers ./daniel

--
Yugo Nagata <nagata@sraoss.co.jp>

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#10Yugo Nagata
nagata@sraoss.co.jp
In reply to: Michael Paquier (#8)
Re: Optional message to user when terminating/cancelling backend

On Wed, 21 Jun 2017 12:06:33 +0900
Michael Paquier <michael.paquier@gmail.com> wrote:

On Tue, Jun 20, 2017 at 3:24 AM, Daniel Gustafsson <daniel@yesql.se> wrote:

The message is stored in a new shmem area which is checked when the session is
aborted. To keep things simple a small buffer is kept per backend for the
message. If deemed too costly, keeping a central buffer from which slabs are
allocated can be done (but seemed rather complicated for little gain compared
to the quite moderate memory spend.)

I think that you are right to take the approach with a per-backend
slot. This will avoid complications related to entry removals and
locking issues. There would be scaling issues as well if things get
very signaled for a lot of backends.

+#define MAX_CANCEL_MSG 128
That looks enough.

+           LWLockAcquire(BackendCancelLock, LW_EXCLUSIVE);
+
+           strlcpy(slot->message, message, sizeof(slot->message));
+           slot->len = strlen(message);
Why not using one spin lock per slot and save it in BackendCancelShmemStruct?

+1

I found an example that a spin lock is used during strlcpy in WalReceiverMain().

+   pid_t       pid = PG_GETARG_INT32(0);
+   char       *msg = text_to_cstring(PG_GETARG_TEXT_PP(1));
+
+   PG_RETURN_BOOL(pg_terminate_backend_internal(pid, msg));
It would be more solid to add some error handling for messages that
are too long, or at least truncate the message if too long.

I agree that error handling for too long messages is needed.
Although long messages are truncated in SetBackendCancelMessage(),
it is better to inform users that the message they can read was
truncated one. Or, maybe we should prohibit too long message
is passed in pg_teminate_backend()

--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

--
Yugo Nagata <nagata@sraoss.co.jp>

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#11Daniel Gustafsson
daniel@yesql.se
In reply to: Yugo Nagata (#10)
Re: Optional message to user when terminating/cancelling backend

On 21 Jun 2017, at 16:30, Yugo Nagata <nagata@sraoss.co.jp> wrote:

On Wed, 21 Jun 2017 12:06:33 +0900
Michael Paquier <michael.paquier@gmail.com> wrote:

On Tue, Jun 20, 2017 at 3:24 AM, Daniel Gustafsson <daniel@yesql.se> wrote:

The message is stored in a new shmem area which is checked when the session is
aborted. To keep things simple a small buffer is kept per backend for the
message. If deemed too costly, keeping a central buffer from which slabs are
allocated can be done (but seemed rather complicated for little gain compared
to the quite moderate memory spend.)

I think that you are right to take the approach with a per-backend
slot. This will avoid complications related to entry removals and
locking issues. There would be scaling issues as well if things get
very signaled for a lot of backends.

+#define MAX_CANCEL_MSG 128
That looks enough.

+           LWLockAcquire(BackendCancelLock, LW_EXCLUSIVE);
+
+           strlcpy(slot->message, message, sizeof(slot->message));
+           slot->len = strlen(message);
Why not using one spin lock per slot and save it in BackendCancelShmemStruct?

+1

I found an example that a spin lock is used during strlcpy in WalReceiverMain().

Yeah I agree as well, will fix.

+   pid_t       pid = PG_GETARG_INT32(0);
+   char       *msg = text_to_cstring(PG_GETARG_TEXT_PP(1));
+
+   PG_RETURN_BOOL(pg_terminate_backend_internal(pid, msg));
It would be more solid to add some error handling for messages that
are too long, or at least truncate the message if too long.

I agree that error handling for too long messages is needed.
Although long messages are truncated in SetBackendCancelMessage(),
it is better to inform users that the message they can read was
truncated one. Or, maybe we should prohibit too long message
is passed in pg_teminate_backend()

The message is truncated in SetBackendCancelMessage() for safety, but
pg_{cancel|terminate}_backend() could throw an error on too long message, or
warning truncation, to the caller as well. Personally I think a warning is the
appropriate response, but I don’t really have a strong opinion.

cheers ./daniel

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#12Michael Paquier
michael@paquier.xyz
In reply to: Daniel Gustafsson (#11)
Re: Optional message to user when terminating/cancelling backend

On Wed, Jun 21, 2017 at 11:42 PM, Daniel Gustafsson <daniel@yesql.se> wrote:

The message is truncated in SetBackendCancelMessage() for safety, but
pg_{cancel|terminate}_backend() could throw an error on too long message, or
warning truncation, to the caller as well. Personally I think a warning is the
appropriate response, but I don’t really have a strong opinion.

And a NOTICE? That's what happens for relation name truncation. You
are right that having a check in SetBackendCancelMessage() makes the
most sense as bgworkers could just call the low level API. Isn't the
concept actually closer to just a backend message? This slot could be
used for other purposes than cancellation.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#13Yugo Nagata
nagata@sraoss.co.jp
In reply to: Michael Paquier (#12)
Re: Optional message to user when terminating/cancelling backend

On Thu, 22 Jun 2017 09:24:54 +0900
Michael Paquier <michael.paquier@gmail.com> wrote:

On Wed, Jun 21, 2017 at 11:42 PM, Daniel Gustafsson <daniel@yesql.se> wrote:

The message is truncated in SetBackendCancelMessage() for safety, but
pg_{cancel|terminate}_backend() could throw an error on too long message, or
warning truncation, to the caller as well. Personally I think a warning is the
appropriate response, but I don’t really have a strong opinion.

And a NOTICE? That's what happens for relation name truncation. You
are right that having a check in SetBackendCancelMessage() makes the
most sense as bgworkers could just call the low level API. Isn't the
concept actually closer to just a backend message? This slot could be
used for other purposes than cancellation.

+1 for NOTICE. The message truncation seems to be a kind of helpful
information rather than a likely problem as long as pg_terminated_backend
exits successfully.

https://www.postgresql.org/docs/10/static/runtime-config-logging.html#runtime-config-severity-levels

--
Michael

--
Yugo Nagata <nagata@sraoss.co.jp>

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#14Daniel Gustafsson
daniel@yesql.se
In reply to: Yugo Nagata (#13)
Re: Optional message to user when terminating/cancelling backend

On 22 Jun 2017, at 10:24, Yugo Nagata <nagata@sraoss.co.jp> wrote:

On Thu, 22 Jun 2017 09:24:54 +0900
Michael Paquier <michael.paquier@gmail.com> wrote:

On Wed, Jun 21, 2017 at 11:42 PM, Daniel Gustafsson <daniel@yesql.se> wrote:

The message is truncated in SetBackendCancelMessage() for safety, but
pg_{cancel|terminate}_backend() could throw an error on too long message, or
warning truncation, to the caller as well. Personally I think a warning is the
appropriate response, but I don’t really have a strong opinion.

And a NOTICE? That's what happens for relation name truncation. You
are right that having a check in SetBackendCancelMessage() makes the
most sense as bgworkers could just call the low level API. Isn't the
concept actually closer to just a backend message? This slot could be
used for other purposes than cancellation.

+1 for NOTICE. The message truncation seems to be a kind of helpful
information rather than a likely problem as long as pg_terminated_backend
exits successfully.

https://www.postgresql.org/docs/10/static/runtime-config-logging.html#runtime-config-severity-levels

Good point. I’ve attached a new version which issues a NOTICE on truncation
and also addresses all other comments so far in this thread. Thanks a lot for
the early patch reviews!

cheers ./daniel

Attachments:

terminate_msg_v3.patchapplication/octet-stream; name=terminate_msg_v3.patchDownload+314-20
#15Dilip Kumar
dilipbalaut@gmail.com
In reply to: Daniel Gustafsson (#14)
Re: Optional message to user when terminating/cancelling backend

On Thu, Jun 22, 2017 at 7:18 PM, Daniel Gustafsson <daniel@yesql.se> wrote:

Good point. I’ve attached a new version which issues a NOTICE on truncation
and also addresses all other comments so far in this thread. Thanks a lot for
the early patch reviews!

cheers ./daniel

I have done an initial review of the patch. I have some comments/suggestions.

+int
+GetCancelMessage(char **buffer, size_t buf_len)
+{
+ volatile BackendCancelShmemStruct *slot = MyCancelSlot;
+ int msg_length = 0;
+

Returned value of this function is never used, better to convert it to
just void.

-------

+bool
+HasCancelMessage(void)
+{
+ volatile BackendCancelShmemStruct *slot = MyCancelSlot;
+/*
+ * Return the configured cancellation message and its length. If the returned
+ * length is greater than the size of the passed buffer, truncation has been
+ * performed. The message is cleared on reading.
+ */
+int
+GetCancelMessage(char **buffer, size_t buf_len)

I think it will be good to merge these two function where
GetCancelMessage will first check whether it has the message or not
if it does then allocate the buffer of size slot->len and copy the
slot message to allocated buffer otherwise just return NULL.

So it will look like "char *GetCancelMessage()"

---------

+ SpinLockAcquire(&slot->mutex);
+ strlcpy(*buffer, (const char *) slot->message, buf_len);

strlcpy(*buffer, (const char *) slot->message, slot->len);

Isn't it better to copy only upto slot->len, seems like it will always
be <= buf_len, and if not then
we can do min(buf_len, slot->len)

----

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#16Daniel Gustafsson
daniel@yesql.se
In reply to: Dilip Kumar (#15)
Re: Optional message to user when terminating/cancelling backend

On 22 Jun 2017, at 17:52, Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Thu, Jun 22, 2017 at 7:18 PM, Daniel Gustafsson <daniel@yesql.se> wrote:

Good point. I’ve attached a new version which issues a NOTICE on truncation
and also addresses all other comments so far in this thread. Thanks a lot for
the early patch reviews!

cheers ./daniel

I have done an initial review of the patch. I have some comments/suggestions.

Thanks!

+int
+GetCancelMessage(char **buffer, size_t buf_len)
+{
+ volatile BackendCancelShmemStruct *slot = MyCancelSlot;
+ int msg_length = 0;
+

Returned value of this function is never used, better to convert it to
just void.

You’re probably right, I was thinking that someone might be interested in
knowing about truncation when extracting the message but I can’t really think
of a callsite which wouldn’t just pass in a large enough buffer in the first
place.

+bool
+HasCancelMessage(void)
+{
+ volatile BackendCancelShmemStruct *slot = MyCancelSlot;
+/*
+ * Return the configured cancellation message and its length. If the returned
+ * length is greater than the size of the passed buffer, truncation has been
+ * performed. The message is cleared on reading.
+ */
+int
+GetCancelMessage(char **buffer, size_t buf_len)

I think it will be good to merge these two function where
GetCancelMessage will first check whether it has the message or not
if it does then allocate the buffer of size slot->len and copy the
slot message to allocated buffer otherwise just return NULL.

So it will look like "char *GetCancelMessage()”

It doesn’t seem like a good idea to perform memory allocation inside a spinlock
in a signalled backend, that would probably at least require an LWLock wouldn’t
it? It seems safer to leave memory management to the signalled backend but it
may be paranoia on my part.

+ SpinLockAcquire(&slot->mutex);
+ strlcpy(*buffer, (const char *) slot->message, buf_len);

strlcpy(*buffer, (const char *) slot->message, slot->len);

Isn't it better to copy only upto slot->len, seems like it will always
be <= buf_len, and if not then
we can do min(buf_len, slot->len)

strlcpy(3) is defined as taking the size of the passed buffer and not the
copied string. Since we guarantee that slot->message is NUL terminated it
seems wise to stick to the API. Or did I misunderstand your comment?

cheers ./daniel

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#17Joel Jacobson
joel@trustly.com
In reply to: Alvaro Herrera (#4)
Re: Optional message to user when terminating/cancelling backend

+1

On Tue, Jun 20, 2017 at 8:54 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Unless you have a lot of users running psql manually, I don't see how
this is actually very useful or actionable. What would the user do with
the information? Hopefully your users already trust that you'd keep the
downtime to the minimum possible.

I think this feature would be useful for PgTerminator
(https://github.com/trustly/pgterminator)
a tool which automatically kills unprotected processes that could
potentially be the reason why

X number of protected important processes have been waiting for >Y seconds.

When I'm guilty of locking this in the production DB and get killed by
PgTerminator,
it would be nice to know the reason, e.g. that it was PgTerminator
that killed me
and what process I was blocking.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#18Joshua D. Drake
jd@commandprompt.com
In reply to: Joel Jacobson (#17)
Re: Optional message to user when terminating/cancelling backend

On 06/26/2017 07:15 AM, Joel Jacobson wrote:

+1

On Tue, Jun 20, 2017 at 8:54 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Unless you have a lot of users running psql manually, I don't see how
this is actually very useful or actionable. What would the user do with
the information? Hopefully your users already trust that you'd keep the
downtime to the minimum possible.

I think this feature would be useful for PgTerminator
(https://github.com/trustly/pgterminator)
a tool which automatically kills unprotected processes that could
potentially be the reason why

X number of protected important processes have been waiting for >Y seconds.

When I'm guilty of locking this in the production DB and get killed by
PgTerminator,
it would be nice to know the reason, e.g. that it was PgTerminator
that killed me
and what process I was blocking.

And not just the pid but literally "what".

jD

--
Command Prompt, Inc. || http://the.postgres.company/ || @cmdpromptinc

PostgreSQL Centered full stack support, consulting and development.
Advocate: @amplifypostgres || Learn: https://pgconf.us
***** Unless otherwise stated, opinions are my own. *****

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#19Thomas Munro
thomas.munro@gmail.com
In reply to: Daniel Gustafsson (#14)
Re: Optional message to user when terminating/cancelling backend

On Fri, Jun 23, 2017 at 1:48 AM, Daniel Gustafsson <daniel@yesql.se> wrote:

Good point. I’ve attached a new version which issues a NOTICE on truncation
and also addresses all other comments so far in this thread. Thanks a lot for
the early patch reviews!

FYI this no longer builds because commit 81c5e46c490e just stole your OIDs:

make[3]: Entering directory
`/home/travis/build/postgresql-cfbot/postgresql/src/backend/catalog'
cd ../../../src/include/catalog && '/usr/bin/perl' ./duplicate_oids
772
972
make[3]: *** [postgres.bki] Error 1

--
Thomas Munro
http://www.enterprisedb.com

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#20Daniel Gustafsson
daniel@yesql.se
In reply to: Thomas Munro (#19)
Re: Optional message to user when terminating/cancelling backend

On 02 Sep 2017, at 02:21, Thomas Munro <thomas.munro@enterprisedb.com> wrote:

On Fri, Jun 23, 2017 at 1:48 AM, Daniel Gustafsson <daniel@yesql.se> wrote:

Good point. I’ve attached a new version which issues a NOTICE on truncation
and also addresses all other comments so far in this thread. Thanks a lot for
the early patch reviews!

FYI this no longer builds because commit 81c5e46c490e just stole your OIDs:

make[3]: Entering directory
`/home/travis/build/postgresql-cfbot/postgresql/src/backend/catalog'
cd ../../../src/include/catalog && '/usr/bin/perl' ./duplicate_oids
772
972
make[3]: *** [postgres.bki] Error 1

Thanks, I hadn’t spotted that yet. Attached is an updated patch using new OIDs
to make it compile again.

cheers ./daniel

Attachments:

terminate_msg_v4.patchapplication/octet-stream; name=terminate_msg_v4.patchDownload+314-20
#21Yugo Nagata
nagata@sraoss.co.jp
In reply to: Daniel Gustafsson (#20)
#22Daniel Gustafsson
daniel@yesql.se
In reply to: Yugo Nagata (#21)
#23Daniel Gustafsson
daniel@yesql.se
In reply to: Yugo Nagata (#21)
#24Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Daniel Gustafsson (#23)
#25Michael Paquier
michael@paquier.xyz
In reply to: Alvaro Herrera (#24)
#26Daniel Gustafsson
daniel@yesql.se
In reply to: Alvaro Herrera (#24)
#27Daniel Gustafsson
daniel@yesql.se
In reply to: Daniel Gustafsson (#26)
#28Eren Başak
eren@citusdata.com
In reply to: Daniel Gustafsson (#27)
#29Christoph Berg
myon@debian.org
In reply to: Eren Başak (#28)
#30Daniel Gustafsson
daniel@yesql.se
In reply to: Eren Başak (#28)
#31Eren Başak
eren@citusdata.com
In reply to: Daniel Gustafsson (#30)
#32Daniel Gustafsson
daniel@yesql.se
In reply to: Eren Başak (#31)
#33Peter Eisentraut
peter_e@gmx.net
In reply to: Daniel Gustafsson (#32)
#34Daniel Gustafsson
daniel@yesql.se
In reply to: Peter Eisentraut (#33)
#35Michael Paquier
michael@paquier.xyz
In reply to: Daniel Gustafsson (#34)
#36Daniel Gustafsson
daniel@yesql.se
In reply to: Michael Paquier (#35)
#37Michael Paquier
michael@paquier.xyz
In reply to: Daniel Gustafsson (#36)
#38Daniel Gustafsson
daniel@yesql.se
In reply to: Michael Paquier (#37)
#39Andres Freund
andres@anarazel.de
In reply to: Andres Freund (#7)
#40Daniel Gustafsson
daniel@yesql.se
In reply to: Andres Freund (#39)
#41Andres Freund
andres@anarazel.de
In reply to: Daniel Gustafsson (#40)
#42Daniel Gustafsson
daniel@yesql.se
In reply to: Andres Freund (#41)
#43Thomas Munro
thomas.munro@gmail.com
In reply to: Daniel Gustafsson (#42)
#44Michael Paquier
michael@paquier.xyz
In reply to: Thomas Munro (#43)
#45Pavel Stehule
pavel.stehule@gmail.com
In reply to: Daniel Gustafsson (#42)
#46Daniel Gustafsson
daniel@yesql.se
In reply to: Pavel Stehule (#45)
#47Daniel Gustafsson
daniel@yesql.se
In reply to: Michael Paquier (#44)
#48Daniel Gustafsson
daniel@yesql.se
In reply to: Thomas Munro (#43)
#49Daniel Gustafsson
daniel@yesql.se
In reply to: Daniel Gustafsson (#48)
#50Heikki Linnakangas
heikki.linnakangas@enterprisedb.com
In reply to: Daniel Gustafsson (#46)
#51Daniel Gustafsson
daniel@yesql.se
In reply to: Heikki Linnakangas (#50)
#52Pavel Stehule
pavel.stehule@gmail.com
In reply to: Daniel Gustafsson (#51)
#53Daniel Gustafsson
daniel@yesql.se
In reply to: Pavel Stehule (#52)
#54Pavel Stehule
pavel.stehule@gmail.com
In reply to: Daniel Gustafsson (#53)
#55Andres Freund
andres@anarazel.de
In reply to: Daniel Gustafsson (#51)
#56Daniel Gustafsson
daniel@yesql.se
In reply to: Andres Freund (#55)
#57Thomas Munro
thomas.munro@gmail.com
In reply to: Daniel Gustafsson (#49)
#58Tom Lane
tgl@sss.pgh.pa.us
In reply to: Thomas Munro (#57)
#59Thomas Munro
thomas.munro@gmail.com
In reply to: Tom Lane (#58)
#60Tom Lane
tgl@sss.pgh.pa.us
In reply to: Thomas Munro (#59)
#61Daniel Gustafsson
daniel@yesql.se
In reply to: Tom Lane (#60)
#62Daniel Gustafsson
daniel@yesql.se
In reply to: Tom Lane (#58)
#63Pavel Stehule
pavel.stehule@gmail.com
In reply to: Daniel Gustafsson (#62)
#64Michael Paquier
michael@paquier.xyz
In reply to: Daniel Gustafsson (#61)
#65Michael Paquier
michael@paquier.xyz
In reply to: Michael Paquier (#64)
#66Daniel Gustafsson
daniel@yesql.se
In reply to: Michael Paquier (#65)
#67Michael Paquier
michael@paquier.xyz
In reply to: Daniel Gustafsson (#66)
#68Thomas Munro
thomas.munro@gmail.com
In reply to: Daniel Gustafsson (#66)
#69Michael Paquier
michael@paquier.xyz
In reply to: Thomas Munro (#68)
#70Daniel Gustafsson
daniel@yesql.se
In reply to: Michael Paquier (#69)
#71Michael Paquier
michael@paquier.xyz
In reply to: Daniel Gustafsson (#70)
#72Daniel Gustafsson
daniel@yesql.se
In reply to: Michael Paquier (#71)
#73Michael Paquier
michael@paquier.xyz
In reply to: Daniel Gustafsson (#72)
#74Daniel Gustafsson
daniel@yesql.se
In reply to: Michael Paquier (#73)
#75Michael Paquier
michael@paquier.xyz
In reply to: Daniel Gustafsson (#74)
#76Daniel Gustafsson
daniel@yesql.se
In reply to: Michael Paquier (#75)
#77Tom Lane
tgl@sss.pgh.pa.us
In reply to: Daniel Gustafsson (#76)
#78Dmitry Dolgov
9erthalion6@gmail.com
In reply to: Tom Lane (#77)
#79Michael Paquier
michael@paquier.xyz
In reply to: Dmitry Dolgov (#78)