Re: [HACKERS] Optional message to user when terminating/cancelling backend - Mailing list pgsql-hackers
| From | Eren Başak |
|---|---|
| Subject | Re: [HACKERS] Optional message to user when terminating/cancelling backend |
| Date | |
| Msg-id | CAFNTstPcstV8Brqkg00a84V72b_FfnLinhu2C2Top+QssmwFhg@mail.gmail.com Whole thread Raw |
| In response to | Re: [HACKERS] Optional message to user when terminating/cancelling backend (Daniel Gustafsson <daniel@yesql.se>) |
| Responses |
Re: [HACKERS] Optional message to user when terminating/cancellingbackend
(Christoph Berg <myon@debian.org>)
Re: [HACKERS] Optional message to user when terminating/cancelling backend (Daniel Gustafsson <daniel@yesql.se>) |
| List | pgsql-hackers |
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:
+ 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@yesql.se> wrote:
> On 26 Jan 2018, at 00:05, Daniel Gustafsson <daniel@yesql.se> wrote:
>
>> On 24 Jan 2018, at 16:45, Alvaro Herrera <alvherre@alvh.no-ip.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
pgsql-hackers by date: