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

Hi,

I reviewed the patch. Here are my notes:

I can confirm that the patches apply and pass the tests as of 03/19/2018 @
11:00am (UTC).

I didn't test whether the docs compile or look good.

I have briefly tested and can confirm that the patch does what is intended.
Here are my comments about the patch:

The patch does not add new tests (maybe isolation tests) for the new
feature. Are there any opportunities to have tests for confirming the new
behavior? At least some regression tests like the following:

SELECT pg_cancel_backend(pg_backend_pid());
ERROR: 57014: canceling statement due to user request

SELECT pg_cancel_backend(pg_backend_pid(), 'message');
ERROR: 57014: canceling statement due to user request: "message"

Not introduced with this patch, but the spacing between the functions is
not consistent in src/backend/storage/ipc/backend_signal.c. There are 2
newlines after some functions (like pg_cancel_backend_msg) and 1 newline
after others (like pg_terminate_backend_msg). It would be nice to fix these
while refactoring.

I also thought about whether the patch should allow the message to be
completely overwritten, instead of appending to the existing one and I
think it is fine. Also, adding the admin message to the HINT or DETAIL part
of the error message would make sense but these are ignored by some clients
so it is fine this way.

Another thing is that, in a similar manner, we could allow changing the
error code which might be useful for extensions. For example, Citus could
use it to cancel remote backends when it detects a distributed deadlock and
changes the error code to something retryable while doing so. For
reference, see the hack that Citus is currently using:
https://github.com/citusdata/citus/blob/81cbb7c223f2eb9cfa8903f1d28869b6f972ded1/src/backend/distributed/shared_library_init.c#L237

+ If the optional <literal>message</literal> parameter is set, the text
+ will be appended to the error message returned to the signalled
backend.

I think providing more detail would be useful. For example we can provide
an example about how the error message looks like in its final form or what
will happen if the message is too long.

-pg_signal_backend(int pid, int sig)
+pg_signal_backend(int pid, int sig, char *msg)

The new parameter (msg) is not mentioned in the function header comment.
This applies to pg_signal_backend, pg_cancel_backend_internal,
pg_terminate_backend_internal functions.

+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;
+ char *msg = NULL;
+
+ if (PG_ARGISNULL(0))
+ ereport(ERROR,
+ (errmsg("pid cannot be NULL")));
+
+ pid = PG_GETARG_INT32(0);
+
+ if (PG_NARGS() == 2 && !PG_ARGISNULL(1))
+ msg = text_to_cstring(PG_GETARG_TEXT_PP(1));
+
+ PG_RETURN_BOOL(pg_cancel_backend_internal(pid, msg));
+}

The first function seem redundant here because the second one covers all
the cases.

+ memset(slot->message, '\0', sizeof(slot->message));

SetBackendCancelMessage uses memset while BackendCancelShmemInit uses
MemSet. Not a big deal but it would be nice to be consistent and use
postgres macro versions for such calls.

+int
+SetBackendCancelMessage(pid_t backend, char *message)
+{
+ BackendCancelShmemStruct *slot;

The variable "slot" is declared outside of the foor loop but only used in
the inside, therefore it would be nicer to declare it inside the loop.

+ BackendCancelInit(MyBackendId);

Is the "normal multiuser case" the best place to initialize cancellation?
For example, can't we cancel background workers? If this is the right
place, maybe we should justify why this is the best place to initialize
backend cancellation memory part with a comment.

+ char *buffer = palloc0(MAX_CANCEL_MSG);

Why not use a static array like char[MAX_CANCEL_MSG], instead of pallocing?

+/*
+ * 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)
+{
+ volatile BackendCancelShmemStruct *slot = MyCancelSlot;
+ int msg_length = 0;
+
+ if (slot != NULL && slot->len > 0)
+ {
+ SpinLockAcquire(&slot->mutex);
+ strlcpy(*buffer, (const char *) slot->message, buf_len);
+ msg_length = slot->len;
+ slot->len = 0;
+ slot->message[0] = '\0';
+ SpinLockRelease(&slot->mutex);
+ }
+
+ return msg_length;
+}

We never change what the buffer points to, therefore is it necessary to
declare `buffer` as char** instead of char*?

+extern int SetBackendCancelMessage(pid_t backend, char *message);

Maybe rename backend to something like backend_pid.

+/*
+ * 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 a function named GetStuff should not clear after getting the stuff.
Therefore either the function should be named something like
ConsumeCancelMessage or we should separate the "get" and "clear" concerns
to different functions.

+/*
+ * The following routines handle registering an optional message when
+ * cancelling, or terminating a backend. The message will be stored in
+ * shared memory and is limited to MAX_CANCEL_MSG characters including
+ * the NULL terminator.
+ *
+ * Access to the message slots is protected by spinlocks.
+ */

I don't think providing a single header comment for the functions below
this (CancelBackendMsgShmemSize, BackendCancelShmemInit, BackendCancelInit,
CleanupCancelBackend) is enough. More detailed comments about what each
function does would be useful.

+BackendCancelInit(int backend_id)

It took me some time to get used to the function names. The confusion was
about whether the function name tells "cancel the initialization process"
or "initialize backend cancellation memory".

--- /dev/null
+++ b/src/include/storage/backend_signal.h

First, I thought that this split should be done in the first patch, but I
realized that everything that has been moved from
src/backend/utils/adt/misc.c to src/backend/storage/ipc/backend_signal.c
has been declared in src/backend/utils/fmgrprotos.h

Best,
Eren

On Tue, Mar 6, 2018 at 12:20 AM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:

> > On 26 Jan 2018, at 00:05, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
> >
> >> On 24 Jan 2018, at 16:45, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
> wrote:
>
> >> Maybe have two patches, 0001 creates the files moving the contents over,
> >> then 0002 adds your new stuff on top.
> >
> > The two attached patches implements this.
>
> Attached are rebased patches to cope with the recent pgproc changes. I
> also
> made the function cope with NULL messages, not because it’s a sensible
> value
> but I can see this function being fed from a sub-SELECT which could return
> NULL.
>
> As per the previous mail, 0001 refactors the signal code according to
> Alvaros
> suggestion and 0002 implements $subject on top of the refactoring.
>
> cheers ./daniel
>
>