Hi,
I have reviewed the v8 patches.
I can confirm that the patches apply and pass the tests as of 03/27/2018 11:00am (UTC).
I didn't test whether the docs compile but they look good.
I have briefly tested the patch again and can confirm that the patch does what is intended.
The v8 patches address almost all of my review notes on v7 patch, thanks Daniel for that.
I have some more questions, notes and views on the patch but have no strong opinions at this moment. So I am fine with whatever decision is made:
I see you have removed extra newlines from backend_signal.c. However, I realized that there is still one extra newline after pg_reload_conf.
That's fine for me, although I would prefer to get the ability to specify the error code sooner than later. My main question is that I am not sure whether the community prefers to ship two similar (in use case) changes on an API in a single patch or fine with two patches. Can that be a problem if the subsequent patch is released in a different postgres version? I am not sure.
> 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.
I agree that we should preserve the old usage as well. What I don't understand is that, can't we remove proisstrict from pg_cancel_backend and copy the body of pg_cancel_backend_msg into pg_cancel_backend. This way, I think we can accomplish the same thing without introducing two new functions.
+Datum
+pg_terminate_backend_msg(PG_FUNCTION_ARGS)
+{
+ pid_t pid;
+ char *msg = NULL;
+
+ if (PG_ARGISNULL(0))
+ ereport(ERROR,
+ (errmsg("pid cannot be NULL")));
pg_terminate_backend_msg errors out if the pid is null but pg_cancel_backend_msg returns null and I think we should have consistency between these two in this regard.
