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

> On 28 Sep 2017, at 14:55, Yugo Nagata <nagata@sraoss.co.jp> wrote:
>
> On Sun, 3 Sep 2017 22:47:10 +0200
> Daniel Gustafsson <daniel@yesql.se> wrote:
>
> I have reviewed your latest patch.

Thanks!

> I can apply this to the master branch and build this successfully,
> and the behavior is as expected.
>
> However, here are some comments and suggestions.
>
>> 135 +               char   *buffer = palloc0(MAX_CANCEL_MSG);
>> 136 +
>> 137 +               GetCancelMessage(&buffer, MAX_CANCEL_MSG);
>> 138 +               ereport(ERROR,
>> 139 +                       (errcode(ERRCODE_QUERY_CANCELED),
>> 140 +                        errmsg("canceling statement due to user request: \"%s\"",
>> 141 +                               buffer)));
>
> The memory for buffer is allocated here, but I think we can do this
> in GetCancelMessage. Since the size of allocated memory is fixed
> to MAX_CANCEL_MSG, it isn't neccesary to pass this to the function.
> In addition, how about returning the message as the return value?
> That is, we can define GetCancelMessage as following;
>
>  char * GetCancelMessage(void)

I agree that it would be a much neater API, but it would mean pallocing inside
the spinlock wouldn’t it? That would be a no-no.

>> 180 +       r = SetBackendCancelMessage(pid, msg);
>> 181 +
>> 182 +       if (r != -1 && r != strlen(msg))
>> 183 +           ereport(NOTICE,
>> 184 +                   (errmsg("message is too long and has been truncated")));
>> 185 +   }
>
> We can this error handling in SetBackendCancelMessage. I think this is better
> because the truncation of message is done in this function. In addition,
> we should use pg_mbcliplen for this truncation as done in truncate_identifier.
> Else, multibyte character boundary is broken, and noises can slip in log
> messages.

Agreed on both points.  I did however leave the returnvalue as before even
though it’s not read in this coding.

>> 235 -   int         r = pg_signal_backend(PG_GETARG_INT32(0), SIGTERM);
>> 236 +   int r = pg_signal_backend(pid, SIGTERM, msg);
>
> This line includes an unnecessary indentation change.

That’s embarrasing, fixed.

>> 411 + * returns the length of message actually created. If the returned length
>
> "the length of message" might be "the length of the message”

Fixed.

>> 413 + * If the backend wasn't found and no message was set, -1 is returned. If two
>
> This comment is incorrect since this function returns 0 when no message was set.

Fixed.

>> 440 +           strlcpy(slot->message, message, sizeof(slot->message));
>> 441 +           slot->len = strlen(slot->message);
>> 442 +           message_len = slot->len;
>> 443 +           SpinLockRelease(&slot->mutex);
>> 444 +
>> 445 +           return message_len;
>
> This can return slot->len directly and the variable message_len is
> unnecessary. However, if we handle the "too long message" error
> in this function as suggested above, this does not have to
> return anything.

That would mean returning a variable which was set while holding the lock, when
the lock has been released.  That doesn’t seem like a good idea, even though it
may be of more theoretical importance here.

Attached patch is rebased over HEAD and addresses the above review comments.
Adding to the next commitfest as it was returned in a previous one.

cheers ./daniel