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(at)yesql(dot)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(at)sraoss(dot)co(dot)jp>