A new function to wait for the backend exit after termination

On Wed, Oct 21, 2020 at 3:02 PM Bharath Rupireddy <
bharath.rupireddyforpostgres@gmail.com> wrote:

Hi,

Currently pg_terminate_backend(), sends SIGTERM to the backend process but
doesn't ensure it's exit. There are chances that backends still are
running(even after pg_terminate_backend() is called) until the interrupts
are processed(using ProcessInterrupts()). This could cause problems
especially in testing, for instance in a sql file right after
pg_terminate_backend(), if any test case depends on the backend's
non-existence[1], but the backend is not terminated. As discussed in [1],
we have wait_pid()(see regress.c and sql/dblink.sql), but it's not usable
across the system. In [1], we thought it would be better to have functions
ensuring the backend's exit on the similar lines of pg_terminate_backend().

I propose to have two functions:

1. pg_terminate_backend_and_wait() -- which sends SIGTERM to the backend
and wait's until it's exit.

I think it would be nicer to have a pg_terminate_backend(pid, wait=false),
so a function with a second parameter which defaults to the current
behaviour of not waiting. And it might be a good idea to also give it a
timeout parameter?

2. pg_wait_backend() -- which waits for a given backend process. Note that
this function has to be used carefully after pg_terminate_backend(), if
used on a backend that's not ternmited it simply keeps waiting in a loop.

It seems this one also very much would need a timeout value.

And surely we should show some sort of wait event when it's waiting.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

On Wed, Oct 21, 2020 at 6:13 AM Magnus Hagander <magnus@hagander.net> wrote:

On Wed, Oct 21, 2020 at 3:02 PM Bharath Rupireddy <
bharath.rupireddyforpostgres@gmail.com> wrote:

Hi,

Currently pg_terminate_backend(), sends SIGTERM to the backend process
but doesn't ensure it's exit. There are chances that backends still are
running(even after pg_terminate_backend() is called) until the interrupts
are processed(using ProcessInterrupts()). This could cause problems
especially in testing, for instance in a sql file right after
pg_terminate_backend(), if any test case depends on the backend's
non-existence[1], but the backend is not terminated. As discussed in [1],
we have wait_pid()(see regress.c and sql/dblink.sql), but it's not usable
across the system. In [1], we thought it would be better to have functions
ensuring the backend's exit on the similar lines of pg_terminate_backend().

I propose to have two functions:

1. pg_terminate_backend_and_wait() -- which sends SIGTERM to the backend
and wait's until it's exit.

I think it would be nicer to have a pg_terminate_backend(pid, wait=false),
so a function with a second parameter which defaults to the current
behaviour of not waiting. And it might be a good idea to also give it a
timeout parameter?

Agreed on the overload, and the timeouts make sense too - with the caller
deciding whether a timeout results in a failure or a false return value.

2. pg_wait_backend() -- which waits for a given backend process. Note
that this function has to be used carefully after pg_terminate_backend(),
if used on a backend that's not ternmited it simply keeps waiting in a loop.

It seems this one also very much would need a timeout value.

Is there a requirement for waiting to be superuser only? You are not
affecting any session but your own during the waiting period.

I could imagine, in theory at least, wanting to wait for a backend to go
idle as well as for it disappearing. Scope creep in terms of this patch's
goal but worth at least considering now.

David J.

Thanks for the feedback.

On Wed, Oct 21, 2020 at 6:43 PM Magnus Hagander <magnus@hagander.net> wrote:

Currently pg_terminate_backend(), sends SIGTERM to the backend process but doesn't ensure it's exit. There are chances that backends still are running(even after pg_terminate_backend() is called) until the interrupts are processed(using ProcessInterrupts()). This could cause problems especially in testing, for instance in a sql file right after pg_terminate_backend(), if any test case depends on the backend's non-existence[1], but the backend is not terminated. As discussed in [1], we have wait_pid()(see regress.c and sql/dblink.sql), but it's not usable across the system. In [1], we thought it would be better to have functions ensuring the backend's exit on the similar lines of pg_terminate_backend().

I propose to have two functions:

1. pg_terminate_backend_and_wait() -- which sends SIGTERM to the backend and wait's until it's exit.

I think it would be nicer to have a pg_terminate_backend(pid, wait=false), so a function with a second parameter which defaults to the current behaviour of not waiting. And it might be a good idea to also give it a timeout parameter?

+1 to have pg_terminate_backend(pid, wait=false, timeout), timeout in
milliseconds only valid if wait = true.

2. pg_wait_backend() -- which waits for a given backend process. Note that this function has to be used carefully after pg_terminate_backend(), if used on a backend that's not ternmited it simply keeps waiting in a loop.

It seems this one also very much would need a timeout value.

And surely we should show some sort of wait event when it's waiting.

Yes for this function too we can have a timeout value.
pg_wait_backend(pid, timeout), timeout in milliseconds.

I think we can use WaitLatch with the given timeout and with a new
wait event type WAIT_EVENT_BACKEND_SHUTDOWN instead of pg_usleep for
achieving the given timeout mechanism. With WaitLatch we would also
get the waiting event in stats. Thoughts?

rc = WaitLatch(MyLatch,
WL_LATCH_SET | WL_POSTMASTER_DEATH, timeout,
WAIT_EVENT_BACKEND_SHUTDOWN);

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On 2020-10-21 15:13:36 +0200, Magnus Hagander wrote:

It seems this one also very much would need a timeout value.

I'm not really against that, but I wonder if we just end up
reimplementing statement timeout...

Thanks for the feedback.

On Wed, Oct 21, 2020 at 8:01 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:

On Wed, Oct 21, 2020 at 6:13 AM Magnus Hagander <magnus@hagander.net> wrote:

I think it would be nicer to have a pg_terminate_backend(pid, wait=false), so a function with a second parameter which defaults to the current behaviour of not waiting. And it might be a good idea to also give it a timeout parameter?

Agreed on the overload, and the timeouts make sense too - with the caller deciding whether a timeout results in a failure or a false return value.

If the backend is terminated within the user specified timeout then
the function returns true, otherwise false.

2. pg_wait_backend() -- which waits for a given backend process. Note that this function has to be used carefully after pg_terminate_backend(), if used on a backend that's not ternmited it simply keeps waiting in a loop.

It seems this one also very much would need a timeout value.

Is there a requirement for waiting to be superuser only? You are not affecting any session but your own during the waiting period.

IIUC, in the same patch instead of returning an error in case of
non-superusers, do we need to wait for user provided timeout
milliseconds until the current user becomes superuser and then throw
error if still non-superuser, and proceed further if superuser?

Do we need to have a new function that waits until a current
non-superuser in a session becomes superuser?

Something else?

I could imagine, in theory at least, wanting to wait for a backend to go idle as well as for it disappearing. Scope creep in terms of this patch's goal but worth at least considering now.

IIUC, do we need a new option, something like pg_wait_backend(pid,
timeout, waituntil) where "waituntil" if specified "idle" waits until
the given backend goes to idle mode, or "termination" waits until
termination?

If my understanding is wrong, could you please explain more?

On Wednesday, October 21, 2020, Bharath Rupireddy <
bharath.rupireddyforpostgres@gmail.com> wrote:

Thanks for the feedback.

On Wed, Oct 21, 2020 at 8:01 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:

On Wed, Oct 21, 2020 at 6:13 AM Magnus Hagander <magnus@hagander.net>

wrote:

I think it would be nicer to have a pg_terminate_backend(pid,

wait=false), so a function with a second parameter which defaults to the
current behaviour of not waiting. And it might be a good idea to also give
it a timeout parameter?

Agreed on the overload, and the timeouts make sense too - with the

caller deciding whether a timeout results in a failure or a false return
value.

If the backend is terminated within the user specified timeout then
the function returns true, otherwise false.

I’m suggesting an option for the second case to fail instead of returning
false.

2. pg_wait_backend() -- which waits for a given backend process. Note

that this function has to be used carefully after pg_terminate_backend(),
if used on a backend that's not ternmited it simply keeps waiting in a loop.

It seems this one also very much would need a timeout value.

Is there a requirement for waiting to be superuser only? You are not

affecting any session but your own during the waiting period.

IIUC, in the same patch instead of returning an error in case of
non-superusers, do we need to wait for user provided timeout
milliseconds until the current user becomes superuser and then throw
error if still non-superuser, and proceed further if superuser?

Do we need to have a new function that waits until a current
non-superuser in a session becomes superuser?

Something else?

Not sure how that would even be possible mid-statement. I was suggesting
removing the superuser check altogether and letting any user execute “wait”.

I could imagine, in theory at least, wanting to wait for a backend to go

idle as well as for it disappearing. Scope creep in terms of this patch's
goal but worth at least considering now.

IIUC, do we need a new option, something like pg_wait_backend(pid,
timeout, waituntil) where "waituntil" if specified "idle" waits until
the given backend goes to idle mode, or "termination" waits until
termination?

If my understanding is wrong, could you please explain more?

Yes, this describes what i was thinking.

David J.

On Thu, Oct 22, 2020 at 8:39 AM David G. Johnston
<david.g.johnston@gmail.com> wrote:

If the backend is terminated within the user specified timeout then
the function returns true, otherwise false.

I’m suggesting an option for the second case to fail instead of returning false.

That seems fine.

I could imagine, in theory at least, wanting to wait for a backend to go idle as well as for it disappearing. Scope creep in terms of this patch's goal but worth at least considering now.

IIUC, do we need a new option, something like pg_wait_backend(pid,
timeout, waituntil) where "waituntil" if specified "idle" waits until
the given backend goes to idle mode, or "termination" waits until
termination?

If my understanding is wrong, could you please explain more?

Yes, this describes what i was thinking.

+1.

I will implement these functionality and post a new patch soon.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Oct 21, 2020 at 6:43 PM Magnus Hagander <magnus@hagander.net> wrote:

I think it would be nicer to have a pg_terminate_backend(pid, wait=false), so a function with a second parameter which defaults to the current behaviour of not waiting. And it might be a good idea to also give it a timeout parameter?

Done.

2. pg_wait_backend() -- which waits for a given backend process. Note that this function has to be used carefully after pg_terminate_backend(), if used on a backend that's not ternmited it simply keeps waiting in a loop.

It seems this one also very much would need a timeout value.

Done.

And surely we should show some sort of wait event when it's waiting.

Added two wait events.

If the backend is terminated within the user specified timeout then
the function returns true, otherwise false.

I’m suggesting an option for the second case to fail instead of returning false.

Done.

I could imagine, in theory at least, wanting to wait for a backend to go idle as well as for it disappearing. Scope creep in terms of this patch's goal but worth at least considering now.

IIUC, do we need a new option, something like pg_wait_backend(pid,
timeout, waituntil) where "waituntil" if specified "idle" waits until
the given backend goes to idle mode, or "termination" waits until
termination?

Done.

Attaching a v2 patch herewith.

Thoughts and feedback are welcome.

Below things are still pending, which I plan to work on soon:

1. More testing and addition of test cases into the regression test suite.
2. Addition of the new function information into the docs.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On 2020/10/28 20:50, Bharath Rupireddy wrote:

On Wed, Oct 21, 2020 at 6:43 PM Magnus Hagander <magnus@hagander.net> wrote:

I think it would be nicer to have a pg_terminate_backend(pid, wait=false), so a function with a second parameter which defaults to the current behaviour of not waiting. And it might be a good idea to also give it a timeout parameter?

Done.

2. pg_wait_backend() -- which waits for a given backend process. Note that this function has to be used carefully after pg_terminate_backend(), if used on a backend that's not ternmited it simply keeps waiting in a loop.

It seems this one also very much would need a timeout value.

Done.

And surely we should show some sort of wait event when it's waiting.

Added two wait events.

If the backend is terminated within the user specified timeout then
the function returns true, otherwise false.

I’m suggesting an option for the second case to fail instead of returning false.

Done.

I prefer that false is returned when the timeout happens,
like pg_promote() does.

I could imagine, in theory at least, wanting to wait for a backend to go idle as well as for it disappearing. Scope creep in terms of this patch's goal but worth at least considering now.

IIUC, do we need a new option, something like pg_wait_backend(pid,
timeout, waituntil) where "waituntil" if specified "idle" waits until
the given backend goes to idle mode, or "termination" waits until
termination?

Isn't this wait-for-idle mode fragile? Because there is no guarantee
that the backend is still in idle state when pg_wait_backend(idle) returns.

Done.

Attaching a v2 patch herewith.

Thoughts and feedback are welcome.

Thanks for the patch!

When the specified timeout is negative, the following error is thrown *after*
SIGTERM is signaled to the target backend. This seems strange to me.
The timeout value should be verified at the beginning of the function, instead.

ERROR: timeout cannot be negative

pg_terminate_backend(xxx, false) failed with the following error. I think
it's more helpful if the function can work even without the timeout value.
That is, what about redefining the function in src/backend/catalog/system_views.sql
and specifying the DEFAULT values for the arguments "wait" and "timeout"?
The similar function "pg_promote" would be good reference to you.

ERROR: function pg_terminate_backend(integer, boolean) does not exist at character 8

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Thanks for the comments.

On Wed, Oct 28, 2020 at 6:41 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

I prefer that false is returned when the timeout happens,
like pg_promote() does.

Earlier it was suggested to error out on timeout. Since users can not
guess on time it takes to terminate or become idle, throwing error
seems to be odd on timeout. And also in case if the given pid is not a
backend pid, we are throwing a warning and returning false but not
error. Similarly we can return false on timeout, if required a
warning. Thoughts?

IIUC, do we need a new option, something like pg_wait_backend(pid,
timeout, waituntil) where "waituntil" if specified "idle" waits until
the given backend goes to idle mode, or "termination" waits until
termination?

Isn't this wait-for-idle mode fragile? Because there is no guarantee
that the backend is still in idle state when pg_wait_backend(idle) returns.

Yeah this can happen. By the time pg_wait_backend returns we could
have the idle state of the backend changed. Looks like this is also a
problem with the existing pgstat_get_backend_current_activity()
function. There we have a comment saying below and the function
returns a pointer to the current activity string. Maybe we could have
similar comments about the usage in the document?

* It is the caller's responsibility to invoke this only for backends whose
* state is expected to remain stable while the result is in use.

Does this problem exist even if we use pg_stat_activity()?

When the specified timeout is negative, the following error is thrown *after*
SIGTERM is signaled to the target backend. This seems strange to me.
The timeout value should be verified at the beginning of the function, instead.

ERROR: timeout cannot be negative

Okay. I will change that.

pg_terminate_backend(xxx, false) failed with the following error. I think
it's more helpful if the function can work even without the timeout value.
That is, what about redefining the function in src/backend/catalog/system_views.sql
and specifying the DEFAULT values for the arguments "wait" and "timeout"?
The similar function "pg_promote" would be good reference to you.

ERROR: function pg_terminate_backend(integer, boolean) does not exist at character 8

Yeah. This seems good. I will have false as default value for the wait
parameter. I have defined the timeout to be in milliseconds, then how
about having a default value of 100 milliseconds?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Oct 28, 2020 at 6:50 AM Bharath Rupireddy <
bharath.rupireddyforpostgres@gmail.com> wrote:

Thanks for the comments.

On Wed, Oct 28, 2020 at 6:41 PM Fujii Masao <masao.fujii@oss.nttdata.com>
wrote:

I prefer that false is returned when the timeout happens,
like pg_promote() does.

Earlier it was suggested to error out on timeout.

For consideration. I'll give a point for being consistent with other
existing functions, and it wouldn't be hard to extend should we want to add
the option later, so while the more flexible API seems better on its face
limiting ourselves to boolean false isn't a big deal to me; especially as
I've yet to write code that would make use of this feature.

Since users can not

guess on time it takes to terminate or become idle, throwing error
seems to be odd on timeout.

I don't see how the one follows from the other.

And also in case if the given pid is not a

backend pid, we are throwing a warning and returning false but not
error.

Similarly we can return false on timeout, if required a

warning. Thoughts?

IMO, if there are multiple ways to return false then all of them should
emit a notice or warning describing which of the false conditions was hit.

IIUC, do we need a new option, something like pg_wait_backend(pid,
timeout, waituntil) where "waituntil" if specified "idle" waits until
the given backend goes to idle mode, or "termination" waits until
termination?

Isn't this wait-for-idle mode fragile? Because there is no guarantee
that the backend is still in idle state when pg_wait_backend(idle)

returns.

I was thinking this would be useful for orchestration. However, as you
say, its a pretty fragile method. I withdraw the suggestion. What I would
replace it with is a pg_wait_for_notify(payload_test) function that allows
an SQL user to plug itself into the listen/notify feature and pause the
session until a notification arrives. The session it is coordinating with
would simply notify just before ending its script/transaction.

David J.

On Wed, Oct 28, 2020 at 7:51 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:

And also in case if the given pid is not a
backend pid, we are throwing a warning and returning false but not
error.

Similarly we can return false on timeout, if required a
warning. Thoughts?

IMO, if there are multiple ways to return false then all of them should emit a notice or warning describing which of the false conditions was hit.

Currently there are two possibilities in pg_teriminate_backend where a
warning is thrown and false is returned. 1. when the process with a
given pid is not a backend 2. when we can not send the SIGTERM to the
given backend.

I will add another case to throw the warning and return false when
timeout occurs.

IIUC, do we need a new option, something like pg_wait_backend(pid,
timeout, waituntil) where "waituntil" if specified "idle" waits until
the given backend goes to idle mode, or "termination" waits until
termination?

Isn't this wait-for-idle mode fragile? Because there is no guarantee
that the backend is still in idle state when pg_wait_backend(idle) returns.

I was thinking this would be useful for orchestration. However, as you say, its a pretty fragile method. I withdraw the suggestion.

So, pg_wait_backend(pid, timeout) waits until the backend with a given
pid is terminated?

What I would replace it with is a pg_wait_for_notify(payload_test) function that allows an SQL user to plug itself into the listen/notify feature and pause the session until a notification arrives. The session it is coordinating with would >simply notify just before ending its script/transaction.

Why does one session need to listen and wait until another session
notifies? If my understanding is wrong, could you please elaborate on
the above point, the usage and the use case?

For consideration. I'll give a point for being consistent with other existing functions, and it wouldn't be hard to extend should we want to add the option later, so while the more flexible API seems better on its face limiting ourselves to >boolean false isn't a big deal to me; especially as I've yet to write code that would make use of this feature.

I see that this pg_wait_backend(pid, timeout) functionality can be
right away used in two places, one in dblink.sql where wait_pid is
being used, second in postgres_fdw.sql where
terminate_backend_and_wait() is being used. However we can make these
changes as part of another patch set after the proposed two new
functions are finalized and reviewed.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Oct 28, 2020 at 10:14 PM Bharath Rupireddy <
bharath.rupireddyforpostgres@gmail.com> wrote:

On Wed, Oct 28, 2020 at 7:51 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:

I was thinking this would be useful for orchestration. However, as you

say, its a pretty fragile method. I withdraw the suggestion.

So, pg_wait_backend(pid, timeout) waits until the backend with a given
pid is terminated?

Yes. The original proposal.

What I would replace it with is a pg_wait_for_notify(payload_test)

function that allows an SQL user to plug itself into the listen/notify
feature and pause the session until a notification arrives. The session it
is coordinating with would >simply notify just before ending its
script/transaction.

Why does one session need to listen and wait until another session
notifies? If my understanding is wrong, could you please elaborate on
the above point, the usage and the use case?

Theory, but I imagine writing an isolation test like test script where the
two sessions wait for notifications instead of sleep for random amounts of
time.

More generally, psql is very powerful but doesn't allow scripting to plug
into pub/sub. I don't have a concrete use case for why it should but the
capability doesn't seem far-fetched.

I'm not saying this is something that is needed, rather it would seem more
useful than wait_for_idle.

David J.

On Wed, Oct 28, 2020 at 6:41 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

I prefer that false is returned when the timeout happens,
like pg_promote() does.

Done.

When the specified timeout is negative, the following error is thrown *after*
SIGTERM is signaled to the target backend. This seems strange to me.
The timeout value should be verified at the beginning of the function, instead.

ERROR: timeout cannot be negative

I'm not throwing error for this case, instead a warning and returning
false. This is to keep it consistent with other cases such as the
given pid is not a backend pid.

Attaching the v3 patch. I tried to address the review comments
received so far and added documentation. I tested the patch locally
here. I saw that we don't have any test cases for existing
pg_terminate_backend(), do we need to add test cases into regression
suites for these two new functions?

Please review the v3 patch and let me know comments.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: not tested

I have tested the patch against current master branch (commit:6742e14959a3033d946ab3d67f5ce4c99367d332)
Both functions work without a problem and as expected.
Just a tiny comment/suggestion.
specifying a -ve timeout in pg_terminate_backed rightly throws an error,
I am not sure if it would be right or a wrong approach but I guess we can ignore -ve
timeout in pg_terminate_backend function when wait (second argument) is false.

e.g. pg_terminate_backend(12320, false,-1); -- ignore -1 timout since wait is false

The new status of this patch is: Ready for Committer

On Mon, Nov 30, 2020 at 8:10 PM Muhammad Usama <m.usama@gmail.com> wrote:

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: not tested

I have tested the patch against current master branch (commit:6742e14959a3033d946ab3d67f5ce4c99367d332)
Both functions work without a problem and as expected.

Thanks!

Just a tiny comment/suggestion.
specifying a -ve timeout in pg_terminate_backed rightly throws an error,
I am not sure if it would be right or a wrong approach but I guess we can ignore -ve
timeout in pg_terminate_backend function when wait (second argument) is false.

e.g. pg_terminate_backend(12320, false,-1); -- ignore -1 timout since wait is false

IMO, that's not a good idea. I see it this way, for any function first
the input args have to be validated. If okay, then follows the use of
those args and the main functionality. I can also see pg_promote(),
which first does the input timeout validation throwing error if it is
<= 0.

We can retain the existing behaviour.

The new status of this patch is: Ready for Committer

Thanks!

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Wed, Dec 2, 2020 at 1:30 PM Bharath Rupireddy <
bharath.rupireddyforpostgres@gmail.com> wrote:

On Mon, Nov 30, 2020 at 8:10 PM Muhammad Usama <m.usama@gmail.com> wrote:

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: not tested

I have tested the patch against current master branch

(commit:6742e14959a3033d946ab3d67f5ce4c99367d332)

Both functions work without a problem and as expected.

Thanks!

Just a tiny comment/suggestion.
specifying a -ve timeout in pg_terminate_backed rightly throws an error,
I am not sure if it would be right or a wrong approach but I guess we

can ignore -ve

timeout in pg_terminate_backend function when wait (second argument) is

false.

e.g. pg_terminate_backend(12320, false,-1); -- ignore -1 timout since

wait is false

IMO, that's not a good idea. I see it this way, for any function first
the input args have to be validated. If okay, then follows the use of
those args and the main functionality. I can also see pg_promote(),
which first does the input timeout validation throwing error if it is
<= 0.

We can retain the existing behaviour.

Agreed!

Thanks
Best regards
Muhammad Usama

Hi

I take a look into the patch, and here some comments.

1.
+
+	ereport(WARNING,
+			(errmsg("could not wait for the termination of the backend with PID %d within %ld milliseconds",
+					pid, timeout)));
+

The code use %ld to print int64 type.
How about use INT64_FORMAT, which looks more appropriate.

2.
+	if (timeout <= 0)
+	{
+		ereport(WARNING,
+				(errmsg("timeout cannot be negative or zero: %ld", timeout)));
+		PG_RETURN_BOOL(r);
+	}

The same as 1.

3.
+pg_terminate_backend_and_wait(PG_FUNCTION_ARGS)
+{
+	int 	pid = PG_GETARG_DATUM(0);

+pg_wait_backend(PG_FUNCTION_ARGS)
+{
+	int		pid = PG_GETARG_INT32(0);

The code use different macro to get pid,
How about use PG_GETARG_INT32(0) for each one.

I changed the status to 'wait on anthor'.
The others of the patch LGTM,
I think it can be changed to Ready for Committer again, when this comment is confirmed.

Best regards,
houzj

"Hou, Zhijie" <houzj.fnst@cn.fujitsu.com> writes:

+	ereport(WARNING,
+			(errmsg("could not wait for the termination of the backend with PID %d within %ld milliseconds",
+					pid, timeout)));

The code use %ld to print int64 type.
How about use INT64_FORMAT, which looks more appropriate.

This is a translatable message, so INT64_FORMAT is no good -- we need
something that is the same across platforms. The current project standard
for this problem is to use "%lld" and explicitly cast the argument to long
long int to match that.

regards, tom lane