> On 21 Jun 2017, at 16:30, Yugo Nagata <nag...@sraoss.co.jp> wrote:
> 
> On Wed, 21 Jun 2017 12:06:33 +0900
> Michael Paquier <michael.paqu...@gmail.com> wrote:
> 
>> On Tue, Jun 20, 2017 at 3:24 AM, Daniel Gustafsson <dan...@yesql.se> wrote:
>>> The message is stored in a new shmem area which is checked when the session 
>>> is
>>> aborted.  To keep things simple a small buffer is kept per backend for the
>>> message.  If deemed too costly, keeping a central buffer from which slabs 
>>> are
>>> allocated can be done (but seemed rather complicated for little gain 
>>> compared
>>> to the quite moderate memory spend.)
>> 
>> I think that you are right to take the approach with a per-backend
>> slot. This will avoid complications related to entry removals and
>> locking issues. There would be scaling issues as well if things get
>> very signaled for a lot of backends.
>> 
>> +#define MAX_CANCEL_MSG 128
>> That looks enough.
>> 
>> +           LWLockAcquire(BackendCancelLock, LW_EXCLUSIVE);
>> +
>> +           strlcpy(slot->message, message, sizeof(slot->message));
>> +           slot->len = strlen(message);
>> Why not using one spin lock per slot and save it in BackendCancelShmemStruct?
> 
> +1
> 
> I found an example that a spin lock is used during strlcpy in 
> WalReceiverMain().

Yeah I agree as well, will fix.

>> +   pid_t       pid = PG_GETARG_INT32(0);
>> +   char       *msg = text_to_cstring(PG_GETARG_TEXT_PP(1));
>> +
>> +   PG_RETURN_BOOL(pg_terminate_backend_internal(pid, msg));
>> It would be more solid to add some error handling for messages that
>> are too long, or at least truncate the message if too long.
> 
> I agree that error handling for too long messages is needed.
> Although long messages are truncated in SetBackendCancelMessage(),
> it is better to inform users that the message they can read was
> truncated one. Or, maybe we should prohibit too long message
> is passed in pg_teminate_backend()

The message is truncated in SetBackendCancelMessage() for safety, but
pg_{cancel|terminate}_backend() could throw an error on too long message, or
warning truncation, to the caller as well.  Personally I think a warning is the
appropriate response, but I don’t really have a strong opinion.

cheers ./daniel

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

Reply via email to