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

On Sun, 3 Sep 2017 22:47:10 +0200
Daniel Gustafsson <daniel@yesql.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@sraoss.co.jp>



> > On 02 Sep 2017, at 02:21, Thomas Munro <thomas.munro@enterprisedb.com> wrote:
> > 
> > On Fri, Jun 23, 2017 at 1:48 AM, 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!"
> > 
> > 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@sraoss.co.jp>


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers