On 04/07/2024 13:50, Jelte Fennema-Nio wrote:
> On Thu, 4 Jul 2024 at 12:35, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>>
>> On 04/07/2024 13:32, Heikki Linnakangas wrote:
>>> Here's a new version of the first patch.
>>
>> Sorry, forgot attachment.
>
> It seems you undid the following earlier change. Was that on purpose?
> If not, did you undo any other earlier changes by accident?
>
>>> +SendCancelRequest(int backendPID, int32 cancelAuthCode)
>>>
>>> I think this name of the function is quite confusing, it's not sending
>>> a cancel request, it is processing one. It sends a SIGINT.
>>
>> Heh, well, it's sending the cancel request signal to the right backend,
>> but I see your point. Renamed to ProcessCancelRequest.
Ah, I made that change as part of the second patch earlier, so I didn't
consider it now.
I don't feel strongly about it, but I think SendCancelRequest() actually
feels a little better, in procsignal.c. It's more consistent with the
existing SendProcSignal() function.
There was indeed another change in the second patch that I missed:
> + /* If we have setsid(), signal the backend's whole process group */
> +#ifdef HAVE_SETSID
> + kill(-backendPID, SIGINT);
> +#else
> kill(backendPID, SIGINT);
> +#endif
I'm not sure that's really required, when sending SIGINT to a backend
process. A backend process shouldn't have any child processes, and if it
did, it's not clear what good SIGINT will do them. But I guess it makes
sense to do it anyway, for consistency with pg_cancel_backend(), which
also signals the whole process group.
--
Heikki Linnakangas
Neon (https://neon.tech)