On Fri, 1 Mar 2024 at 06:19, Jelte Fennema-Nio <postg...@jeltef.nl> wrote: > This is a preliminary review. I'll look at this more closely soon.
Some more thoughts after looking some more at the proposed changes +#define EXTENDED_CANCEL_REQUEST_CODE PG_PROTOCOL(1234,5677) nit: I think the code should be 1234,5679 because it's the newer message type, so to me it makes more sense if the code is a larger number. + * FIXME: we used to use signal_child. I believe kill() is + * maybe even more correct, but verify that. signal_child seems to be the correct one, not kill. signal_child has this relevant comment explaining why it's better than plain kill: * On systems that have setsid(), each child process sets itself up as a * process group leader. For signals that are generally interpreted in the * appropriate fashion, we signal the entire process group not just the * direct child process. This allows us to, for example, SIGQUIT a blocked * archive_recovery script, or SIGINT a script being run by a backend via * system(). +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. > While we're at it, switch to using atomics in pmsignal.c for the state > field. That feels easier to reason about than volatile > pointers. I feel like this refactor would benefit from being a separate commit. That would make it easier to follow which change to pmsignal.c is necessary for what. + MyCancelKeyLength = (MyProcPort != NULL && MyProcPort->extended_query_cancel) ? MAX_CANCEL_KEY_LENGTH : 4; I think we should be doing this check the opposite way, i.e. only fall back to the smaller key when explicitly requested: + MyCancelKeyLength = (MyProcPort != NULL && MyProcPort->old_query_cancel) ? 4 : MAX_CANCEL_KEY_LENGTH; That way we'd get the more secure, longer key length for non-backend processes such as background workers. + case EOF: + /* We'll come back when there is more data */ + return PGRES_POLLING_READING; Nice catch, I'll go steal this for my patchset which adds all the necessary changes to be able to do a protocol bump[1]. + int be_pid; /* PID of backend --- needed for XX cancels */ Seems like you accidentally added XX to the comment in this line. [1]: https://www.postgresql.org/message-id/flat/CAGECzQSr2%3DJPJHNN06E_jTF2%2B0E60K%3DhotyBw5wY%3Dq9Wvmt7DQ%40mail.gmail.com#359e4222eb161da37124be1a384f8d92
