> On 20 Mar 2018, at 12:12, Eren Başak <e...@citusdata.com> wrote:
> I reviewed the patch. Here are my notes: Thanks! > The patch does not add new tests (maybe isolation tests) for the new feature. > Are there any opportunities to have tests for confirming the new behavior? Good point, not sure why I’ve forgotten to add this. No existing suite seemed to match well, so I added a new one for system administration functions like this one. > Not introduced with this patch, but the spacing between the functions is not > consistent in src/backend/storage/ipc/backend_signal.c. There are 2 newlines > after some functions (like pg_cancel_backend_msg) and 1 newline after others > (like pg_terminate_backend_msg). It would be nice to fix these while > refactoring. Fixed. > Another thing is that, in a similar manner, we could allow changing the error > code which might be useful for extensions. For example, Citus could use it to > cancel remote backends when it detects a distributed deadlock and changes the > error code to something retryable while doing so. For reference, see the hack > that Citus is currently using: > https://github.com/citusdata/citus/blob/81cbb7c223f2eb9cfa8903f1d28869b6f972ded1/src/backend/distributed/shared_library_init.c#L237 In 20170620200134.ubnv4sked5seo...@alap3.anarazel.de, Andres suggested the same thing. I don’t disagree, but I’m also not sure what the API would look like so I’d prefer to address that in a follow-up patch should this one get accepted. > + If the optional <literal>message</literal> parameter is set, the text > + will be appended to the error message returned to the signalled backend. > > I think providing more detail would be useful. For example we can provide an > example about how the error message looks like in its final form or what will > happen if the message is too long. Expanded the documentation a little and added a (contrived) example. > -pg_signal_backend(int pid, int sig) > +pg_signal_backend(int pid, int sig, char *msg) > > The new parameter (msg) is not mentioned in the function header comment. This > applies to pg_signal_backend, pg_cancel_backend_internal, > pg_terminate_backend_internal functions. Fixed. > +Datum > +pg_cancel_backend(PG_FUNCTION_ARGS) > +{ > + PG_RETURN_BOOL(pg_cancel_backend_internal(PG_GETARG_INT32(0), NULL)); > +} > + > +Datum > +pg_cancel_backend_msg(PG_FUNCTION_ARGS) > +{ > + pid_t pid; > + char *msg = NULL; > + > + if (PG_ARGISNULL(0)) > + ereport(ERROR, > + (errmsg("pid cannot be NULL"))); > + > + pid = PG_GETARG_INT32(0); > + > + if (PG_NARGS() == 2 && !PG_ARGISNULL(1)) > + msg = text_to_cstring(PG_GETARG_TEXT_PP(1)); > + > + PG_RETURN_BOOL(pg_cancel_backend_internal(pid, msg)); > +} > > The first function seem redundant here because the second one covers all the > cases. pg_cancel_backend() is defined proisstrict, while pg_cancel_backend_msg() is not. I think we must be able to perform the cancellation if the message is NULL, so made it two functions. Thinking more about this, I think pg_cancel_backend_msg() should handle a NULL pid in the same way as pg_cancel_backend() so fixed that as well. > + memset(slot->message, '\0', sizeof(slot->message)); > > SetBackendCancelMessage uses memset while BackendCancelShmemInit uses MemSet. > Not a big deal but it would be nice to be consistent and use postgres macro > versions for such calls. Good point, moved to MemSet() for both. > +int > +SetBackendCancelMessage(pid_t backend, char *message) > +{ > + BackendCancelShmemStruct *slot; > > The variable "slot" is declared outside of the foor loop but only used in the > inside, therefore it would be nicer to declare it inside the loop. Moved to inside the loop. > + BackendCancelInit(MyBackendId); > > Is the "normal multiuser case" the best place to initialize cancellation? For > example, can't we cancel background workers? If this is the right place, > maybe we should justify why this is the best place to initialize backend > cancellation memory part with a comment. I didn’t envision this being in another setting than the multiuser case, but it’s been clear throughout the thread that others have had good ideas around extended uses of this. Background workers can still be terminated/canceled, this only affects setting the message, and that is to me a multiuser feature. Renamed the functions BackendCancelMessage*() for clarity. > + char *buffer = palloc0(MAX_CANCEL_MSG); > > Why not use a static array like char[MAX_CANCEL_MSG], instead of pallocing? No specific reason, changed. > +/* > + * Return the configured cancellation message and its length. If the returned > + * length is greater than the size of the passed buffer, truncation has been > + * performed. The message is cleared on reading. > + */ > +int > +GetCancelMessage(char **buffer, size_t buf_len) ... > We never change what the buffer points to, therefore is it necessary to > declare `buffer` as char** instead of char*? Fixed > +extern int SetBackendCancelMessage(pid_t backend, char *message); > > Maybe rename backend to something like backend_pid. Makes sense, changed. > I think a function named GetStuff should not clear after getting the stuff. > Therefore either the function should be named something like > ConsumeCancelMessage or we should separate the "get" and "clear" concerns to > different functions. Good point, changed to ConsumeCancelMessage() > I don't think providing a single header comment for the functions below this > (CancelBackendMsgShmemSize, BackendCancelShmemInit, BackendCancelInit, > CleanupCancelBackend) is enough. More detailed comments about what each > function does would be useful. Comments expanded. > It took me some time to get used to the function names. The confusion was > about whether the function name tells "cancel the initialization process" or > "initialize backend cancellation memory”. Naming is hard. I’m not wed to any name used, and am open to any suggestions that can improve code clarity. Attached patches are rebased over HEAD with all of the above addressed. cheers ./daniel
0001-Refactor-backend-signalling-code-v8.patch
Description: Binary data
0002-Support-optional-message-in-backend-cancel-terminate-v8.patch
Description: Binary data
