Re: Optional message to user when terminating/cancelling backend

On Thu, Jun 22, 2017 at 7:18 PM, Daniel Gustafsson <daniel(at)yesql(dot)se> 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(&slot->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