Re: [HACKERS] Optional message to user when terminating/cancelling backend

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.

> In 20170620200134(dot)ubnv4sked5seolyk(at)alap3(dot)anarazel(dot)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.

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.

--
Best,
Eren