On Fri, 1 Mar 2024 at 06:19, Jelte Fennema-Nio <postgres@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