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 message 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(>mutex);
+ strlcpy(*buffer, (const char *) slot->message, buf_len);
+ msg_length = slot->len;
+ slot->len = 0;
+ slot->message[0] = '\0';
+ SpinLockRelease(>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,
