Re: [HACKERS] Optional message to user when terminating/cancelling backend

[Daniel Gustafsson]

> On 28 Sep 2017, at 14:55, Yugo Nagata  wrote:
> 
> On Sun, 3 Sep 2017 22:47:10 +0200
> Daniel Gustafsson  wrote:
> 
> I have reviewed your latest patch. 
> 
> I can apply this to the master branch and build this successfully,
> and the behavior is as expected. 
> 
> However, here are some comments and suggestions.

Thanks for the review!  As I haven’t had time to address the comments for a new
versin of this patch, I’m closing this as Returned with Feedback and will
re-submit for the next commitfest.

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

Re: [HACKERS] Optional message to user when terminating/cancelling backend

[Yugo Nagata]

On Sun, 3 Sep 2017 22:47:10 +0200
Daniel Gustafsson  wrote:

I have reviewed your latest patch. 

I can apply this to the master branch and build this successfully,
and the behavior is as expected. 

However, here are some comments and suggestions.

> 135 +   char   *buffer = palloc0(MAX_CANCEL_MSG);
> 136 +
> 137 +   GetCancelMessage(, MAX_CANCEL_MSG);
> 138 +   ereport(ERROR,
> 139 +   (errcode(ERRCODE_QUERY_CANCELED),
> 140 +errmsg("canceling statement due to user request: 
> \"%s\"",
> 141 +   buffer)));

The memory for buffer is allocated here, but I think we can do this
in GetCancelMessage. Since the size of allocated memory is fixed
to MAX_CANCEL_MSG, it isn't neccesary to pass this to the function.
In addition, how about returning the message as the return value?
That is, we can define GetCancelMessage as following;

  char * GetCancelMessage(void)

> 180 +   r = SetBackendCancelMessage(pid, msg);
> 181 +
> 182 +   if (r != -1 && r != strlen(msg))
> 183 +   ereport(NOTICE,
> 184 +   (errmsg("message is too long and has been 
> truncated")));
> 185 +   }

We can this error handling in SetBackendCancelMessage. I think this is better
because the truncation of message is done in this function. In addition, 
we should use pg_mbcliplen for this truncation as done in truncate_identifier. 
Else, multibyte character boundary is broken, and noises can slip in log
messages.

> 235 -   int r = pg_signal_backend(PG_GETARG_INT32(0), SIGTERM);
> 236 +   int r = pg_signal_backend(pid, SIGTERM, msg);

This line includes an unnecessary indentation change.

> 411 + * returns the length of message actually created. If the returned length

"the length of message" might be "the length of the message"

> 413 + * If the backend wasn't found and no message was set, -1 is returned. 
> If two

This comment is incorrect since this function returns 0 when no message was set.

> 440 +   strlcpy(slot->message, message, sizeof(slot->message));
> 441 +   slot->len = strlen(slot->message);
> 442 +   message_len = slot->len;
> 443 +   SpinLockRelease(>mutex);
> 444 +
> 445 +   return message_len;

This can return slot->len directly and the variable message_len is
unnecessary. However, if we handle the "too long message" error
in this function as suggested above, this does not have to
return anything.

Regards,


-- 
Yugo Nagata 



> > On 02 Sep 2017, at 02:21, Thomas Munro  
> > wrote:
> > 
> > On Fri, Jun 23, 2017 at 1:48 AM, Daniel Gustafsson  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
> 


-- 
Yugo Nagata 


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

Re: [HACKERS] Optional message to user when terminating/cancelling backend

[Daniel Gustafsson]

> On 02 Sep 2017, at 02:21, Thomas Munro  wrote:
> 
> On Fri, Jun 23, 2017 at 1:48 AM, Daniel Gustafsson  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



terminate_msg_v4.patch
Description: Binary data

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

Re: [HACKERS] Optional message to user when terminating/cancelling backend

[Thomas Munro]

On Fri, Jun 23, 2017 at 1:48 AM, Daniel Gustafsson  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

Re: [HACKERS] Optional message to user when terminating/cancelling backend

[Joshua D. Drake]

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

+1

On Tue, Jun 20, 2017 at 8:54 PM, Alvaro Herrera
 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

Re: [HACKERS] Optional message to user when terminating/cancelling backend

[Joel Jacobson]

+1

On Tue, Jun 20, 2017 at 8:54 PM, Alvaro Herrera
 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

Re: [HACKERS] Optional message to user when terminating/cancelling backend

[Daniel Gustafsson]

> On 22 Jun 2017, at 17:52, Dilip Kumar  wrote:
> 
> On Thu, Jun 22, 2017 at 7:18 PM, Daniel Gustafsson  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(>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

Re: [HACKERS] Optional message to user when terminating/cancelling backend

[Dilip Kumar]

On Thu, Jun 22, 2017 at 7:18 PM, Daniel Gustafsson  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(>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

Re: [HACKERS] Optional message to user when terminating/cancelling backend

[Daniel Gustafsson]

> On 22 Jun 2017, at 10:24, Yugo Nagata  wrote:
> 
> On Thu, 22 Jun 2017 09:24:54 +0900
> Michael Paquier  wrote:
> 
>> On Wed, Jun 21, 2017 at 11:42 PM, Daniel Gustafsson  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



terminate_msg_v3.patch
Description: Binary data

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

Re: [HACKERS] Optional message to user when terminating/cancelling backend

[Yugo Nagata]

On Thu, 22 Jun 2017 09:24:54 +0900
Michael Paquier  wrote:

> On Wed, Jun 21, 2017 at 11:42 PM, Daniel Gustafsson  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 


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

Re: [HACKERS] Optional message to user when terminating/cancelling backend

[Michael Paquier]

On Wed, Jun 21, 2017 at 11:42 PM, Daniel Gustafsson  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

Re: [HACKERS] Optional message to user when terminating/cancelling backend

[Daniel Gustafsson]

> On 21 Jun 2017, at 16:30, Yugo Nagata  wrote:
> 
> On Wed, 21 Jun 2017 12:06:33 +0900
> Michael Paquier  wrote:
> 
>> On Tue, Jun 20, 2017 at 3:24 AM, Daniel Gustafsson  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

Re: [HACKERS] Optional message to user when terminating/cancelling backend

[Yugo Nagata]

On Wed, 21 Jun 2017 12:06:33 +0900
Michael Paquier  wrote:

> On Tue, Jun 20, 2017 at 3:24 AM, Daniel Gustafsson  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 


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

Re: [HACKERS] Optional message to user when terminating/cancelling backend

[Yugo Nagata]

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  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( [, message]);
>   SELECT pg_cancel_backend( [, 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(, '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 


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

Re: [HACKERS] Optional message to user when terminating/cancelling backend

[Michael Paquier]

On Tue, Jun 20, 2017 at 3:24 AM, Daniel Gustafsson  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

Re: [HACKERS] Optional message to user when terminating/cancelling backend

[Satyanarayana Narlapuram]

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.johns...@gmail.com] 
Sent: Tuesday, June 20, 2017 12:44 PM
To: Alvaro Herrera <alvhe...@2ndquadrant.com>
Cc: Satyanarayana Narlapuram <satyanarayana.narlapu...@microsoft.com>; Pavel 
Stehule <pavel.steh...@gmail.com>; Daniel Gustafsson <dan...@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 <alvhe...@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(, '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

Re: [HACKERS] Optional message to user when terminating/cancelling backend

[Andres Freund]

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( [, message]);
>   SELECT pg_cancel_backend( [, 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(, '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

Re: [HACKERS] Optional message to user when terminating/cancelling backend

[David G. Johnston]

On Tue, Jun 20, 2017 at 11:54 AM, Alvaro Herrera
 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(, '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

Re: [HACKERS] Optional message to user when terminating/cancelling backend

[Alvaro Herrera]

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 Herrerahttps://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

Re: [HACKERS] Optional message to user when terminating/cancelling backend

[Satyanarayana Narlapuram]

+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-ow...@postgresql.org 
[mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Pavel Stehule
Sent: Monday, June 19, 2017 11:41 AM
To: Daniel Gustafsson <dan...@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 
<dan...@yesql.se<mailto:dan...@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( [, message]);
  SELECT pg_cancel_backend( [, 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(, '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<https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.postgresql.org%2Fmailpref%2Fpgsql-hackers=02%7C01%7CSatyanarayana.Narlapuram%40microsoft.com%7C8f211f53d54c4d6308d308d4b742f824%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636334945639579811=VH8ljYHI%2BaxoNdM7pYLpfEqa%2FbXWf0dRptoNzxElbaA%3D=0>

Re: [HACKERS] Optional message to user when terminating/cancelling backend

[Pavel Stehule]

2017-06-19 20:24 GMT+02:00 Daniel Gustafsson :

> 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( [, message]);
>   SELECT pg_cancel_backend( [, 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(, '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)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

[HACKERS] Optional message to user when terminating/cancelling backend

[Daniel Gustafsson]

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( [, message]);
  SELECT pg_cancel_backend( [, 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(, '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



terminate_msg_v2.patch
Description: Binary data

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