Re: Optional message to user when terminating/cancelling backend

On Sun, 3 Sep 2017 22:47:10 +0200
Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:

I have reviewed your latest patch.

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)

> 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.

> 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.

> 411 + * returns the length of message actually created. If the returned length

"the length of message" might be "the length of the message"

> 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.

> 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.

Regards,

--
Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp>

> > On 02 Sep 2017, at 02:21, Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com> wrote:
> >
> > On Fri, Jun 23, 2017 at 1:48 AM, 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!"
> >
> > FYI this no longer builds because commit 81c5e46c490e just stole your OIDs:
> >
> > make[3]: Entering directory
> > `/home/travis/build/postgresql-cfbot/postgresql/src/backend/catalog'
> > cd ../../../src/include/catalog && '/usr/bin/perl' ./duplicate_oids
> > 772
> > 972
> > make[3]: *** [postgres.bki] Error 1
>
> Thanks, I hadn’t spotted that yet. Attached is an updated patch using new OIDs
> to make it compile again.
>
> cheers ./daniel
>

--
Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp>