> On 22 Jun 2017, at 17:52, Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Thu, Jun 22, 2017 at 7:18 PM, Daniel Gustafsson <daniel@yesql.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.
Thanks!
> +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.
You’re probably right, I was thinking that someone might be interested in
knowing about truncation when extracting the message but I can’t really think
of a callsite which wouldn’t just pass in a large enough buffer in the first
place.
> +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()”
It doesn’t seem like a good idea to perform memory allocation inside a spinlock
in a signalled backend, that would probably at least require an LWLock wouldn’t
it? It seems safer to leave memory management to the signalled backend but it
may be paranoia on my part.
> + 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)
strlcpy(3) is defined as taking the size of the passed buffer and not the
copied string. Since we guarantee that slot->message is NUL terminated it
seems wise to stick to the API. Or did I misunderstand your comment?
cheers ./daniel