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

From: Daniel Gustafsson <daniel(at)yesql(dot)se>
To: Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp>
Cc: Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [HACKERS] Optional message to user when terminating/cancelling backend
Date: 2018-01-24 13:50:30
Message-ID: 0078CDD5-E83B-40C1-A819-617055B136D5@yesql.se
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> On 28 Sep 2017, at 14:55, Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
>
> On Sun, 3 Sep 2017 22:47:10 +0200
> Daniel Gustafsson <daniel(at)yesql(dot)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

Attachment Content-Type Size
terminate_msg_v5.patch application/octet-stream 17.3 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Alvaro Herrera 2018-01-24 13:58:14 Re: proposal: alternative psql commands quit and exit
Previous Message Peter Eisentraut 2018-01-24 13:21:21 Re: documentation is now XML