On Sun, 3 Sep 2017 22:47:10 +0200
Daniel Gustafsson <dan...@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 <nag...@sraoss.co.jp>
> > On 02 Sep 2017, at 02:21, Thomas Munro <thomas.mu...@enterprisedb.com>
> > wrote:
> >
> > On Fri, Jun 23, 2017 at 1:48 AM, Daniel Gustafsson <dan...@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 <nag...@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
