Horton

Thread

Commits

  1. Copy-edit text for the pg_terminate_backend() "timeout" parameter.

  2. Remove pg_wait_for_backend_termination().

  3. Add functions to wait for backend termination

  4. Fix typo in comment.

  1. A new function to wait for the backend exit after termination

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2020-10-21T13:02:15Z

    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.
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.

Attaching a WIP patch herewith.

Thoughts?

[1]
https://www.postgresql.org/message-id/flat/f31cc4da-a7ea-677f-cf64-a2f9db854bf5%40oss.nttdata.com

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

  2. Re: A new function to wait for the backend exit after termination

    Magnus Hagander <magnus@hagander.net> — 2020-10-21T13:13:36Z

    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/>
 Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/>

  3. Re: A new function to wait for the backend exit after termination

    David G. Johnston <david.g.johnston@gmail.com> — 2020-10-21T14:30:45Z

    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.

  4. Re: A new function to wait for the backend exit after termination

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2020-10-22T02:20:01Z

    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

  5. Re: A new function to wait for the backend exit after termination

    Andres Freund <andres@anarazel.de> — 2020-10-22T02:35:25Z

    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...

  6. Re: A new function to wait for the backend exit after termination

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2020-10-22T02:46:18Z

    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?

  7. Re: A new function to wait for the backend exit after termination

    David G. Johnston <david.g.johnston@gmail.com> — 2020-10-22T03:09:35Z

    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.

  8. Re: A new function to wait for the backend exit after termination

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2020-10-22T04:12:33Z

    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

  9. Re: A new function to wait for the backend exit after termination

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2020-10-28T11:50:46Z

    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

  10. Re: A new function to wait for the backend exit after termination

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-10-28T13:11:42Z

    
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

  11. Re: A new function to wait for the backend exit after termination

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2020-10-28T13:49:55Z

    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

  12. Re: A new function to wait for the backend exit after termination

    David G. Johnston <david.g.johnston@gmail.com> — 2020-10-28T14:21:09Z

    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.

  13. Re: A new function to wait for the backend exit after termination

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2020-10-29T05:13:57Z

    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

  14. Re: A new function to wait for the backend exit after termination

    David G. Johnston <david.g.johnston@gmail.com> — 2020-10-29T05:21:00Z

    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.

  15. Re: A new function to wait for the backend exit after termination

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2020-10-31T10:58:18Z

    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

  16. Re: A new function to wait for the backend exit after termination

    Muhammad Usama <m.usama@gmail.com> — 2020-11-30T14:39:31Z

    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

  17. Re: A new function to wait for the backend exit after termination

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2020-12-02T08:30:42Z

    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

  18. Re: A new function to wait for the backend exit after termination

    Muhammad Usama <m.usama@gmail.com> — 2020-12-02T13:46:43Z

    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


>
> >
> > The new status of this patch is: Ready for Committer
> >
>
> Thanks!
>
> With Regards,
> Bharath Rupireddy.
> EnterpriseDB: http://www.enterprisedb.com
>

  19. RE: A new function to wait for the backend exit after termination

    Hou, Zhijie <houzj.fnst@cn.fujitsu.com> — 2020-12-03T01:54:33Z

    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

  20. Re: A new function to wait for the backend exit after termination

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-12-03T02:01:34Z

    "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

  21. RE: A new function to wait for the backend exit after termination

    Hou, Zhijie <houzj.fnst@cn.fujitsu.com> — 2020-12-03T02:21:51Z

    > > +	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.

Thank you for pointing out that,
And sorry for did not think of it.

Yes, we can use %lld, (long long int) timeout.

Best regards,
houzj

  22. RE: A new function to wait for the backend exit after termination

    Hou, Zhijie <houzj.fnst@cn.fujitsu.com> — 2020-12-03T03:32:46Z

    > 
> 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.
> 

I am Sorry I forgot a possible typo comment.

+{ oid => '16386', descr => 'terminate a backend process and wait for it\'s exit or until timeout occurs'

Does the following change looks better?

Wait for it\'s exit => Wait for its exit

Best regards,
houzj

  23. Re: A new function to wait for the backend exit after termination

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2020-12-03T03:56:08Z

    Thanks for the review.

On Thu, Dec 3, 2020 at 7:24 AM Hou, Zhijie <houzj.fnst@cn.fujitsu.com> wrote:
>
> 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.
>

Changed it to use %lld and typecasting timeout to (long long int) as
suggested by Tom.

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

Changed.

>
> 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.
>

Changed.

> I am Sorry I forgot a possible typo comment.
>
> +{ oid => '16386', descr => 'terminate a backend process and wait for it\'s exit or until timeout occurs'
>
> Does the following change looks better?
>
> Wait for it\'s exit => Wait for its exit
>

Changed.

>
> 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.
>

Attaching v4 patch. Please have a look.

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

  24. RE: A new function to wait for the backend exit after termination

    Hou, Zhijie <houzj.fnst@cn.fujitsu.com> — 2020-12-04T03:13:42Z

    Hi,

-        however only superusers can terminate superuser backends.
+        however only superusers can terminate superuser backends. When no
+        <parameter>wait</parameter> and <parameter>timeout</parameter> are
+        provided, only SIGTERM is sent to the backend with the given process
+        ID and <literal>false</literal> is returned immediately. But the

I test the case when no wait and timeout are provided.
True is returned as the following which seems different from the doc.

postgres=# select pg_terminate_backend(pid);
 pg_terminate_backend 
----------------------
 t
(1 row)

Best regards,
houzj

  25. Re: A new function to wait for the backend exit after termination

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2020-12-04T06:29:51Z

    On Fri, Dec 4, 2020 at 8:44 AM Hou, Zhijie <houzj.fnst@cn.fujitsu.com>
wrote:
>
> -        however only superusers can terminate superuser backends.
> +        however only superusers can terminate superuser backends. When no
> +        <parameter>wait</parameter> and <parameter>timeout</parameter>
are
> +        provided, only SIGTERM is sent to the backend with the given
process
> +        ID and <literal>false</literal> is returned immediately. But the
>
> I test the case when no wait and timeout are provided.
> True is returned as the following which seems different from the doc.
>
> postgres=# select pg_terminate_backend(pid);
>  pg_terminate_backend
> ----------------------
>  t
> (1 row)
>

Thanks for pointing that out. I reworded that statement. Attaching v5
patch. Please have a look.

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

  26. RE: A new function to wait for the backend exit after termination

    Hou, Zhijie <houzj.fnst@cn.fujitsu.com> — 2020-12-04T08:31:40Z

    Hi,

When test pg_terminate_backend_and_wait with parallel query.
I noticed that the function is not defined as parallel safe.

I am not very familiar with the standard about whether a function should be parallel safe.
But I found the following function are all defined as parallel safe:

pg_promote
pg_terminate_backend(integer)
pg_sleep*

Is there a reason why pg_terminate_backend_and_wait are not parallel safe ?
(I'm sorry if I miss something in previous mails.)

Best regards,
houzj

  27. Re: A new function to wait for the backend exit after termination

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2020-12-04T09:13:28Z

    On Fri, Dec 4, 2020 at 2:02 PM Hou, Zhijie <houzj.fnst@cn.fujitsu.com> wrote:
>
> Hi,
>
> When test pg_terminate_backend_and_wait with parallel query.
> I noticed that the function is not defined as parallel safe.
>
> I am not very familiar with the standard about whether a function should be parallel safe.
> But I found the following function are all defined as parallel safe:
>
> pg_promote
> pg_terminate_backend(integer)
> pg_sleep*
>
> Is there a reason why pg_terminate_backend_and_wait are not parallel safe ?
> (I'm sorry if I miss something in previous mails.)
>

I'm not quite sure of a use case where existing pg_terminate_backend()
or for that matter the new pg_terminate_backend_and_wait() and
pg_wait_backend() will ever get used from parallel workers. Having
said that, I marked the new functions as parallel safe to keep it the
way it is with existing pg_terminate_backend().

postgres=# select proparallel, proname, prosrc from pg_proc where
proname IN ('pg_wait_backend', 'pg_terminate_backend');
 proparallel |       proname        |            prosrc
-------------+----------------------+-------------------------------
 s           | pg_terminate_backend | pg_terminate_backend
 s           | pg_wait_backend      | pg_wait_backend
 s           | pg_terminate_backend | pg_terminate_backend_and_wait
(3 rows)

Attaching v6 patch. Please have a look.

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

  28. Re: A new function to wait for the backend exit after termination

    Hou, Zhijie <houzj.fnst@cn.fujitsu.com> — 2020-12-04T09:29:01Z

    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

Thanks for the new patch, the patch LGTM and works as expected

The new status of this patch is: Ready for Committer

  29. Re: A new function to wait for the backend exit after termination

    Magnus Hagander <magnus@hagander.net> — 2021-03-06T17:06:38Z

    On Fri, Dec 4, 2020 at 10:13 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Fri, Dec 4, 2020 at 2:02 PM Hou, Zhijie <houzj.fnst@cn.fujitsu.com> wrote:
> >
> > Hi,
> >
> > When test pg_terminate_backend_and_wait with parallel query.
> > I noticed that the function is not defined as parallel safe.
> >
> > I am not very familiar with the standard about whether a function should be parallel safe.
> > But I found the following function are all defined as parallel safe:
> >
> > pg_promote
> > pg_terminate_backend(integer)
> > pg_sleep*
> >
> > Is there a reason why pg_terminate_backend_and_wait are not parallel safe ?
> > (I'm sorry if I miss something in previous mails.)
> >
>
> I'm not quite sure of a use case where existing pg_terminate_backend()
> or for that matter the new pg_terminate_backend_and_wait() and
> pg_wait_backend() will ever get used from parallel workers. Having
> said that, I marked the new functions as parallel safe to keep it the
> way it is with existing pg_terminate_backend().
>
> postgres=# select proparallel, proname, prosrc from pg_proc where
> proname IN ('pg_wait_backend', 'pg_terminate_backend');
>  proparallel |       proname        |            prosrc
> -------------+----------------------+-------------------------------
>  s           | pg_terminate_backend | pg_terminate_backend
>  s           | pg_wait_backend      | pg_wait_backend
>  s           | pg_terminate_backend | pg_terminate_backend_and_wait
> (3 rows)
>
> Attaching v6 patch. Please have a look.

Taking another look at this patch. Here are a few more comments:

For pg_terminate_backend, wouldn't it be easier to just create one
function that has a default for wait and a default for timeout?
Instead of having one version that takes one argument, and another
version that takes 3? Seems that would also simplify the
implementation by not having to set things up and call indirectly?

pg_wait_backend() "checks the existence of the session", and "returns
true on success". It's unclear from that what's considered a success.
Also, technically, it only checks for the existence of the backend and
not the session inside, I think?

But also the fact is that it returns true when the backend is *gone*,
which I think is a very strange definition of "success". In fact,
isn't pg_wait_backend() is a pretty bad name for a function that does
this? Maybe pg_wait_for_backend_termination()? (the internal function
has a name that more matches what it does, but the SQL function does
not)

Why is the for(;;) loop in pg_wait_until_termination not a do {}
while(remainingtime > 0)?

The wait event needs to be added to the list in the documentation.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

  30. Re: A new function to wait for the backend exit after termination

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-03-07T09:09:36Z

    On Sat, Mar 6, 2021 at 10:36 PM Magnus Hagander <magnus@hagander.net> wrote:
>
> > Attaching v6 patch. Please have a look.
>
> Taking another look at this patch. Here are a few more comments:

Thanks for the review comments.

> For pg_terminate_backend, wouldn't it be easier to just create one
> function that has a default for wait and a default for timeout?
> Instead of having one version that takes one argument, and another
> version that takes 3? Seems that would also simplify the
> implementation by not having to set things up and call indirectly?

Done.

> pg_wait_backend() "checks the existence of the session", and "returns
> true on success". It's unclear from that what's considered a success.
> Also, technically, it only checks for the existence of the backend and
> not the session inside, I think?
> But also the fact is that it returns true when the backend is *gone*,
> which I think is a very strange definition of "success".

Yes, it only checks the existence of the backend process. Changed the
phrasing a bit to make things clear.

> In fact, isn't pg_wait_backend() is a pretty bad name for a function that does
> this? Maybe pg_wait_for_backend_termination()? (the internal function
> has a name that more matches what it does, but the SQL function does
> not)

pg_wait_for_backend_termination LGTM, so changed pg_wait_backend to that name.

> Why is the for(;;) loop in pg_wait_until_termination not a do {}
> while(remainingtime > 0)?

Done.

> The wait event needs to be added to the list in the documentation.

Added to monitoring.sgml's IPC wait event type.

Attaching v7 patch for further review.

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

  31. Re: A new function to wait for the backend exit after termination

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-03-15T03:27:57Z

    On Sun, Mar 7, 2021 at 2:39 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
> Attaching v7 patch for further review.

Attaching v8 patch after rebasing on to the latest master.

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

  32. Re: A new function to wait for the backend exit after termination

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-03-15T05:08:15Z

    
On 2021/03/15 12:27, Bharath Rupireddy wrote:
> On Sun, Mar 7, 2021 at 2:39 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
>> Attaching v7 patch for further review.
> 
> Attaching v8 patch after rebasing on to the latest master.

Thanks for rebasing the patch!

-       WAIT_EVENT_XACT_GROUP_UPDATE
+       WAIT_EVENT_XACT_GROUP_UPDATE,
+       WAIT_EVENT_BACKEND_TERMINATION

These should be listed in alphabetical order.

In pg_wait_until_termination's do-while loop, ResetLatch() should be called. Otherwise, it would enter busy-loop after any signal arrives. Because the latch is kept set and WaitLatch() always exits immediately in that case.

+	/*
+	 * Wait in steps of waittime milliseconds until this function exits or
+	 * timeout.
+	 */
+	int64	waittime = 10;

10 ms per cycle seems too frequent?

+			ereport(WARNING,
+					(errmsg("timeout cannot be negative or zero: %lld",
+							(long long int) timeout)));
+
+			result = false;

IMO the parameter should be verified before doing the actual thing.

Why is WARNING thrown in this case? Isn't it better to throw ERROR like pg_promote() does?

Regards,

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

  33. Re: A new function to wait for the backend exit after termination

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-03-16T09:38:04Z

    On Mon, Mar 15, 2021 at 10:38 AM Fujii Masao
<masao.fujii@oss.nttdata.com> wrote:
> On 2021/03/15 12:27, Bharath Rupireddy wrote:
> > On Sun, Mar 7, 2021 at 2:39 PM Bharath Rupireddy
> > <bharath.rupireddyforpostgres@gmail.com> wrote:
> >> Attaching v7 patch for further review.
> >
> > Attaching v8 patch after rebasing on to the latest master.
>
> Thanks for rebasing the patch!

Thanks for reviewing.

> -       WAIT_EVENT_XACT_GROUP_UPDATE
> +       WAIT_EVENT_XACT_GROUP_UPDATE,
> +       WAIT_EVENT_BACKEND_TERMINATION
>
> These should be listed in alphabetical order.

Done.

> In pg_wait_until_termination's do-while loop, ResetLatch() should be called. Otherwise, it would enter busy-loop after any signal arrives. Because the latch is kept set and WaitLatch() always exits immediately in that case.

Done.

> +       /*
> +        * Wait in steps of waittime milliseconds until this function exits or
> +        * timeout.
> +        */
> +       int64   waittime = 10;
>
> 10 ms per cycle seems too frequent?

Increased it to 100msec.

> +                       ereport(WARNING,
> +                                       (errmsg("timeout cannot be negative or zero: %lld",
> +                                                       (long long int) timeout)));
> +
> +                       result = false;
>
> IMO the parameter should be verified before doing the actual thing.

Done.

> Why is WARNING thrown in this case? Isn't it better to throw ERROR like pg_promote() does?

Done.

Attaching v9 patch for further review.

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

  34. Re: A new function to wait for the backend exit after termination

    Magnus Hagander <magnus@hagander.net> — 2021-03-16T16:18:31Z

    On Tue, Mar 16, 2021 at 10:38 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Mon, Mar 15, 2021 at 10:38 AM Fujii Masao
> <masao.fujii@oss.nttdata.com> wrote:
> > On 2021/03/15 12:27, Bharath Rupireddy wrote:
> > > On Sun, Mar 7, 2021 at 2:39 PM Bharath Rupireddy
> > > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > >> Attaching v7 patch for further review.
> > >
> > > Attaching v8 patch after rebasing on to the latest master.
> >
> > Thanks for rebasing the patch!
>
> Thanks for reviewing.
>
> > -       WAIT_EVENT_XACT_GROUP_UPDATE
> > +       WAIT_EVENT_XACT_GROUP_UPDATE,
> > +       WAIT_EVENT_BACKEND_TERMINATION
> >
> > These should be listed in alphabetical order.
>
> Done.
>
> > In pg_wait_until_termination's do-while loop, ResetLatch() should be called. Otherwise, it would enter busy-loop after any signal arrives. Because the latch is kept set and WaitLatch() always exits immediately in that case.
>
> Done.
>
> > +       /*
> > +        * Wait in steps of waittime milliseconds until this function exits or
> > +        * timeout.
> > +        */
> > +       int64   waittime = 10;
> >
> > 10 ms per cycle seems too frequent?
>
> Increased it to 100msec.
>
> > +                       ereport(WARNING,
> > +                                       (errmsg("timeout cannot be negative or zero: %lld",
> > +                                                       (long long int) timeout)));
> > +
> > +                       result = false;
> >
> > IMO the parameter should be verified before doing the actual thing.
>
> Done.
>
> > Why is WARNING thrown in this case? Isn't it better to throw ERROR like pg_promote() does?
>
> Done.
>
> Attaching v9 patch for further review.

Almost there :)


Does it really make sense that pg_wait_for_backend_termination()
defaults to waiting *100 milliseconds*, and then logs a warning? That
seems extremely short if I'm explicitly asking it to wait.

I'd argue that 100ms is too short for pg_terminate_backend() as well,
but I think it's a bit more reasonable there.

Wait events should be in alphabetical order in pgstat_get_wait_ipc()
as well, not just in the header (which was adjusted per Fujii's
comment)


+                       (errmsg("could not wait for the termination of
the backend with PID %d within %lld milliseconds",

That's not true though? The wait succeeded, it just timed out? Isn't
itm ore like "backend with PID %d did not terminate within %lld
milliseconds"?


-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

  35. Re: A new function to wait for the backend exit after termination

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-03-17T01:31:39Z

    On Tue, Mar 16, 2021 at 9:48 PM Magnus Hagander <magnus@hagander.net> wrote:
> Does it really make sense that pg_wait_for_backend_termination()
> defaults to waiting *100 milliseconds*, and then logs a warning? That
> seems extremely short if I'm explicitly asking it to wait.

I increased the default wait timeout to 5seconds.

> Wait events should be in alphabetical order in pgstat_get_wait_ipc()
> as well, not just in the header (which was adjusted per Fujii's
> comment)

Done.

>
> +                       (errmsg("could not wait for the termination of
> the backend with PID %d within %lld milliseconds",
>
> That's not true though? The wait succeeded, it just timed out? Isn't
> itm ore like "backend with PID %d did not terminate within %lld
> milliseconds"?

Looks better. Done.

Attaching v10 patch for further review.

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

  36. Re: A new function to wait for the backend exit after termination

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2021-03-17T02:58:47Z

    At Wed, 17 Mar 2021 07:01:39 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in 
> Attaching v10 patch for further review.

The time-out mechanism doesn't count remainingtime as expected,
concretely it does the following.

do {
  kill();
  WaitLatch(WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH, waittime);
  ResetLatch(MyLatch);
  remainingtime -= waittime;
} while (remainingtime > 0);

So, the WaitLatch doesn't consume as much time as the set waittime in
case of latch set. remainingtime reduces faster than the real at the
iteration.

It wouldn't happen actually but I concern about PID recycling. We can
make sure to get rid of the fear by checking for our BEENTRY instead
of PID.  However, it seems to me that some additional function is
needed in pgstat.c so that we can check the realtime value of
PgBackendStatus, which might be too much.


+	/* If asked to wait, check whether the timeout value is valid or not. */
+	if (wait && pid != MyProcPid)
+	{
+		timeout = PG_GETARG_INT64(2);
+
+		if (timeout <= 0)
+			ereport(ERROR,
+					(errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+					 errmsg("\"timeout\" must not be negative or zero")));

This means that pg_terminate_backend accepts negative timeouts when
terminating myself, which looks odd.

Is there any reason to reject 0 as timeout?

+	 * Wait only if requested and the termination is successful. Self
+	 * termination is allowed but waiting is not.
+	 */
+	if (wait && pid != MyProcPid && result)
+		result = pg_wait_until_termination(pid, timeout);

Why don't we wait for myself to be terminated?  There's no guarantee
that myself will be terminated without failure.  (I agree that that is
not so useful, but I think there's no reason not to do so.)


The first suggested signature for pg_terminate_backend() with timeout
was pg_terminate_backend(pid, timeout).  The current signature (pid,
wait?, timeout) looks redundant.  Maybe the reason for rejecting 0
astimeout is pg_terminate_backend(pid, true, 0) looks odd but it we
can wait forever in that case (as other features does).  On the other
hand pg_terminate_backend(pid, false, 100) is apparently odd but this
patch doesn't seem to reject it.  If there's no considerable reason
for the current signature, I would suggest that:

pg_terminate_backend(pid, timeout), where it waits forever if timeout
is zero and waits for the timeout if positive. Negative values are not
accepted.

That being said, I didn't find the disucssion about allowing default
timeout value by separating the boolean, if it is the consensus on
this thread, sorry for the noise.


+				ereport(WARNING,
+						(errmsg("could not check the existence of the backend with PID %d: %m",
+								pid)));
+				return false;

I think this is worth ERROR. We can avoid this handling if we look
into PgBackendEntry instead.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  37. Re: A new function to wait for the backend exit after termination

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-03-17T09:03:45Z

    On Wed, Mar 17, 2021 at 8:28 AM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
>
> At Wed, 17 Mar 2021 07:01:39 +0530, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote in
> > Attaching v10 patch for further review.
>
> The time-out mechanism doesn't count remainingtime as expected,
> concretely it does the following.
>
> do {
>   kill();
>   WaitLatch(WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH, waittime);
>   ResetLatch(MyLatch);
>   remainingtime -= waittime;
> } while (remainingtime > 0);
>
> So, the WaitLatch doesn't consume as much time as the set waittime in
> case of latch set. remainingtime reduces faster than the real at the
> iteration.

WaitLatch can exit without waiting for the waittime duration whenever
the MyLatch is set (SetLatch). Now the question is how frequently
SetLatch can get called in a backend? For instance, if we keep calling
pg_reload_conf in any of the backends in the cluster, then the
SetLatch will be called and the timeout in pg_wait_until_termination
will be reached fastly.  I see that this problem can also exist in
case of pg_promote function. Similarly it may exist in other places
where we have WaitLatch for timeouts.

IMO, the frequency of SetLatch calls may not be that much in real time
scenarios. If at all, the latch gets set too frequently, then the
terminate and wait functions might timeout earlier. But is it a
critical problem to worry about? (IMHO, it's not that critical) If
yes, we might as well need to fix it (I don't know how?) in other
critical areas like pg_promote?

> It wouldn't happen actually but I concern about PID recycling. We can
> make sure to get rid of the fear by checking for our BEENTRY instead
> of PID.  However, it seems to me that some additional function is
> needed in pgstat.c so that we can check the realtime value of
> PgBackendStatus, which might be too much.

The aim of the wait logic is to ensure that the process is gone from
the system processes that is why using kill(), not it's entries are
gone from the shared memory.

> +       /* If asked to wait, check whether the timeout value is valid or not. */
> +       if (wait && pid != MyProcPid)
> +       {
> +               timeout = PG_GETARG_INT64(2);
> +
> +               if (timeout <= 0)
> +                       ereport(ERROR,
> +                                       (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
> +                                        errmsg("\"timeout\" must not be negative or zero")));
>
> This means that pg_terminate_backend accepts negative timeouts when
> terminating myself, which looks odd.

I will change this.

> Is there any reason to reject 0 as timeout?

Actually, timeout 0 should mean that "don't wait" and we can error out
on negative values. Thoughts?

> +        * Wait only if requested and the termination is successful. Self
> +        * termination is allowed but waiting is not.
> +        */
> +       if (wait && pid != MyProcPid && result)
> +               result = pg_wait_until_termination(pid, timeout);
>
> Why don't we wait for myself to be terminated?  There's no guarantee
> that myself will be terminated without failure.  (I agree that that is
> not so useful, but I think there's no reason not to do so.)

We could programmatically allow it to wait in case of self termination
and it doesn't make any difference to the user, they would see
"Terminating connection due to administrator command" FATAL error. I
can remove pid != MyProcPid.

> The first suggested signature for pg_terminate_backend() with timeout
> was pg_terminate_backend(pid, timeout).  The current signature (pid,
> wait?, timeout) looks redundant.  Maybe the reason for rejecting 0
> astimeout is pg_terminate_backend(pid, true, 0) looks odd but it we
> can wait forever in that case (as other features does).  On the other
> hand pg_terminate_backend(pid, false, 100) is apparently odd but this
> patch doesn't seem to reject it.  If there's no considerable reason
> for the current signature, I would suggest that:
>
> pg_terminate_backend(pid, timeout), where it waits forever if timeout
> is zero and waits for the timeout if positive. Negative values are not
> accepted.

So, as stated above, how about a timeout 0 (which is default) telling
"don't wait", negative error out, a positive milliseconds value
indicating that we should wait after termination?

And for pg_wait_for_backend_termination timeout 0 or negative, we error out?

IMO, the above semantics are better than timeout 0 meaning "wait
forever". Thoughts?

> +                               ereport(WARNING,
> +                                               (errmsg("could not check the existence of the backend with PID %d: %m",
> +                                                               pid)));
> +                               return false;
>
> I think this is worth ERROR. We can avoid this handling if we look
> into PgBackendEntry instead.

I will change it to ERROR.

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

  38. Re: A new function to wait for the backend exit after termination

    Zhihong Yu <zyu@yugabyte.com> — 2021-03-17T09:19:23Z

    Hi,
w.r.t. WaitLatch(), if its return value is WL_TIMEOUT, we know the
specified timeout has elapsed.
It seems WaitLatch() can be enhanced to also return the actual duration of
the wait.
This way, the caller can utilize the duration directly.

As for other places where WaitLatch() is called, similar change can be
applied on a per-case basis (with separate patches, not under this topic).

Cheers

On Wed, Mar 17, 2021 at 2:04 AM Bharath Rupireddy <
bharath.rupireddyforpostgres@gmail.com> wrote:

> On Wed, Mar 17, 2021 at 8:28 AM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> >
> > At Wed, 17 Mar 2021 07:01:39 +0530, Bharath Rupireddy <
> bharath.rupireddyforpostgres@gmail.com> wrote in
> > > Attaching v10 patch for further review.
> >
> > The time-out mechanism doesn't count remainingtime as expected,
> > concretely it does the following.
> >
> > do {
> >   kill();
> >   WaitLatch(WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH, waittime);
> >   ResetLatch(MyLatch);
> >   remainingtime -= waittime;
> > } while (remainingtime > 0);
> >
> > So, the WaitLatch doesn't consume as much time as the set waittime in
> > case of latch set. remainingtime reduces faster than the real at the
> > iteration.
>
> WaitLatch can exit without waiting for the waittime duration whenever
> the MyLatch is set (SetLatch). Now the question is how frequently
> SetLatch can get called in a backend? For instance, if we keep calling
> pg_reload_conf in any of the backends in the cluster, then the
> SetLatch will be called and the timeout in pg_wait_until_termination
> will be reached fastly.  I see that this problem can also exist in
> case of pg_promote function. Similarly it may exist in other places
> where we have WaitLatch for timeouts.
>
> IMO, the frequency of SetLatch calls may not be that much in real time
> scenarios. If at all, the latch gets set too frequently, then the
> terminate and wait functions might timeout earlier. But is it a
> critical problem to worry about? (IMHO, it's not that critical) If
> yes, we might as well need to fix it (I don't know how?) in other
> critical areas like pg_promote?
>
> > It wouldn't happen actually but I concern about PID recycling. We can
> > make sure to get rid of the fear by checking for our BEENTRY instead
> > of PID.  However, it seems to me that some additional function is
> > needed in pgstat.c so that we can check the realtime value of
> > PgBackendStatus, which might be too much.
>
> The aim of the wait logic is to ensure that the process is gone from
> the system processes that is why using kill(), not it's entries are
> gone from the shared memory.
>
> > +       /* If asked to wait, check whether the timeout value is valid or
> not. */
> > +       if (wait && pid != MyProcPid)
> > +       {
> > +               timeout = PG_GETARG_INT64(2);
> > +
> > +               if (timeout <= 0)
> > +                       ereport(ERROR,
> > +
>  (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
> > +                                        errmsg("\"timeout\" must not be
> negative or zero")));
> >
> > This means that pg_terminate_backend accepts negative timeouts when
> > terminating myself, which looks odd.
>
> I will change this.
>
> > Is there any reason to reject 0 as timeout?
>
> Actually, timeout 0 should mean that "don't wait" and we can error out
> on negative values. Thoughts?
>
> > +        * Wait only if requested and the termination is successful. Self
> > +        * termination is allowed but waiting is not.
> > +        */
> > +       if (wait && pid != MyProcPid && result)
> > +               result = pg_wait_until_termination(pid, timeout);
> >
> > Why don't we wait for myself to be terminated?  There's no guarantee
> > that myself will be terminated without failure.  (I agree that that is
> > not so useful, but I think there's no reason not to do so.)
>
> We could programmatically allow it to wait in case of self termination
> and it doesn't make any difference to the user, they would see
> "Terminating connection due to administrator command" FATAL error. I
> can remove pid != MyProcPid.
>
> > The first suggested signature for pg_terminate_backend() with timeout
> > was pg_terminate_backend(pid, timeout).  The current signature (pid,
> > wait?, timeout) looks redundant.  Maybe the reason for rejecting 0
> > astimeout is pg_terminate_backend(pid, true, 0) looks odd but it we
> > can wait forever in that case (as other features does).  On the other
> > hand pg_terminate_backend(pid, false, 100) is apparently odd but this
> > patch doesn't seem to reject it.  If there's no considerable reason
> > for the current signature, I would suggest that:
> >
> > pg_terminate_backend(pid, timeout), where it waits forever if timeout
> > is zero and waits for the timeout if positive. Negative values are not
> > accepted.
>
> So, as stated above, how about a timeout 0 (which is default) telling
> "don't wait", negative error out, a positive milliseconds value
> indicating that we should wait after termination?
>
> And for pg_wait_for_backend_termination timeout 0 or negative, we error
> out?
>
> IMO, the above semantics are better than timeout 0 meaning "wait
> forever". Thoughts?
>
> > +                               ereport(WARNING,
> > +                                               (errmsg("could not check
> the existence of the backend with PID %d: %m",
> > +                                                               pid)));
> > +                               return false;
> >
> > I think this is worth ERROR. We can avoid this handling if we look
> > into PgBackendEntry instead.
>
> I will change it to ERROR.
>
> With Regards,
> Bharath Rupireddy.
> EnterpriseDB: http://www.enterprisedb.com
>
>
>

  39. Re: A new function to wait for the backend exit after termination

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2021-03-18T07:16:54Z

    
On 2021/03/17 11:58, Kyotaro Horiguchi wrote:
> The first suggested signature for pg_terminate_backend() with timeout
> was pg_terminate_backend(pid, timeout).  The current signature (pid,
> wait?, timeout) looks redundant.  Maybe the reason for rejecting 0
> astimeout is pg_terminate_backend(pid, true, 0) looks odd but it we
> can wait forever in that case (as other features does).

I'm afraid that "waiting forever" can cause something like deadlock situation,
as follows. We have no mechanism to detect this for now.

1. backend 1 took the lock on the relation A.
2. backend 2 took the lock on the relation B.
3. backend 1 tries to take the lock on the relation B and is waiting for
    the lock to be released.
4. backend 2 accidentally executes pg_wait_for_backend_termination() with
    the pid of backend 1, and then is waiting for backend 1 to be terminated.

Regards,

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

  40. Re: A new function to wait for the backend exit after termination

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-03-18T07:41:37Z

    On Thu, Mar 18, 2021 at 12:46 PM Fujii Masao
<masao.fujii@oss.nttdata.com> wrote:
> On 2021/03/17 11:58, Kyotaro Horiguchi wrote:
> > The first suggested signature for pg_terminate_backend() with timeout
> > was pg_terminate_backend(pid, timeout).  The current signature (pid,
> > wait?, timeout) looks redundant.  Maybe the reason for rejecting 0
> > astimeout is pg_terminate_backend(pid, true, 0) looks odd but it we
> > can wait forever in that case (as other features does).
>
> I'm afraid that "waiting forever" can cause something like deadlock situation,
> as follows. We have no mechanism to detect this for now.
>
> 1. backend 1 took the lock on the relation A.
> 2. backend 2 took the lock on the relation B.
> 3. backend 1 tries to take the lock on the relation B and is waiting for
>     the lock to be released.
> 4. backend 2 accidentally executes pg_wait_for_backend_termination() with
>     the pid of backend 1, and then is waiting for backend 1 to be terminated.

Yeah this can happen.

So, as stated upthread, how about a timeout 0 (which is default)
telling "don't wait", erroring out on negative value and when
specified a positive milliseconds value, then wait for that amount of
time. With this semantics, we can remove the wait flag for
pg_terminate_backend(pid, 0). Thoughts?

And for pg_wait_for_backend_termination timeout 0 or negative, we
error out. Thoughts?

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

  41. Re: A new function to wait for the backend exit after termination

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-03-19T06:07:26Z

    On Thu, Mar 18, 2021 at 1:11 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Thu, Mar 18, 2021 at 12:46 PM Fujii Masao
> <masao.fujii@oss.nttdata.com> wrote:
> > On 2021/03/17 11:58, Kyotaro Horiguchi wrote:
> > > The first suggested signature for pg_terminate_backend() with timeout
> > > was pg_terminate_backend(pid, timeout).  The current signature (pid,
> > > wait?, timeout) looks redundant.  Maybe the reason for rejecting 0
> > > astimeout is pg_terminate_backend(pid, true, 0) looks odd but it we
> > > can wait forever in that case (as other features does).
> >
> > I'm afraid that "waiting forever" can cause something like deadlock situation,
> > as follows. We have no mechanism to detect this for now.
> >
> > 1. backend 1 took the lock on the relation A.
> > 2. backend 2 took the lock on the relation B.
> > 3. backend 1 tries to take the lock on the relation B and is waiting for
> >     the lock to be released.
> > 4. backend 2 accidentally executes pg_wait_for_backend_termination() with
> >     the pid of backend 1, and then is waiting for backend 1 to be terminated.
>
> Yeah this can happen.
>
> So, as stated upthread, how about a timeout 0 (which is default)
> telling "don't wait", erroring out on negative value and when
> specified a positive milliseconds value, then wait for that amount of
> time. With this semantics, we can remove the wait flag for
> pg_terminate_backend(pid, 0). Thoughts?
>
> And for pg_wait_for_backend_termination timeout 0 or negative, we
> error out. Thoughts?

Attaching v11 patch that removed the wait boolean flag in the
pg_terminate_backend and timeout 0 indicates "no wait", negative value
"errors out", positive value "waits for those many milliseconds". Also
addressed other review comments that I received upthread. Please
review v11 further.

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

  42. Re: A new function to wait for the backend exit after termination

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-04-05T03:21:03Z

    On Fri, Mar 19, 2021 at 11:37 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
> Attaching v11 patch that removed the wait boolean flag in the
> pg_terminate_backend and timeout 0 indicates "no wait", negative value
> "errors out", positive value "waits for those many milliseconds". Also
> addressed other review comments that I received upthread. Please
> review v11 further.

Attaching v12 patch after rebasing onto the latest master.

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

  43. Re: A new function to wait for the backend exit after termination

    Magnus Hagander <magnus@hagander.net> — 2021-04-08T09:41:17Z

    On Mon, Apr 5, 2021 at 5:21 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Fri, Mar 19, 2021 at 11:37 AM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> > Attaching v11 patch that removed the wait boolean flag in the
> > pg_terminate_backend and timeout 0 indicates "no wait", negative value
> > "errors out", positive value "waits for those many milliseconds". Also
> > addressed other review comments that I received upthread. Please
> > review v11 further.
>
> Attaching v12 patch after rebasing onto the latest master.

I've applied this patch with some minor changes.

I rewrote some parts of the documentation to make it more focused on
the end user rather than the implementation. I also made a small
simplification in pg_terminate_backend() which removes the "wait"
variable (seems like a bit of a leftover since the time when it was a
separate argument). And picked a correct oid for the function (oids
8000-9999 should be used for new patches, 16386 is in the user area of
oids)

Thanks!

-- 
 Magnus Hagander
 Me: https://www.hagander.net/
 Work: https://www.redpill-linpro.com/

  44. Re: A new function to wait for the backend exit after termination

    Noah Misch <noah@leadboat.com> — 2021-06-01T03:48:58Z

    On Thu, Apr 08, 2021 at 11:41:17AM +0200, Magnus Hagander wrote:
> I've applied this patch with some minor changes.

I wondered if the new pg_wait_for_backend_termination() could replace
regress.c:wait_pid().  I think it can't, because the new function requires the
backend to still be present in the procarray:

	proc = BackendPidGetProc(pid);

	if (proc == NULL)
	{
		ereport(WARNING,
				(errmsg("PID %d is not a PostgreSQL server process", pid)));

		PG_RETURN_BOOL(false);
	}

	PG_RETURN_BOOL(pg_wait_until_termination(pid, timeout));

If a backend has left the procarray but not yet left the kernel process table,
this function will issue the warning and not wait for the final exit.  Given
that limitation, is pg_wait_for_backend_termination() useful enough?  If
waiting for procarray departure is enough, should pg_wait_until_termination()
check BackendPidGetProc(pid) instead of kill(0, pid), so it can return
earlier?  I can see the value of adding the pg_terminate_backend() timeout
argument, in any case.

  45. Re: A new function to wait for the backend exit after termination

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-06-01T07:55:24Z

    On Tue, Jun 1, 2021 at 9:19 AM Noah Misch <noah@leadboat.com> wrote:
>
> On Thu, Apr 08, 2021 at 11:41:17AM +0200, Magnus Hagander wrote:
> > I've applied this patch with some minor changes.

Thanks for taking a look at this function.

> I wondered if the new pg_wait_for_backend_termination() could replace
> regress.c:wait_pid().

I was earlier thinking of replacing the wait_pid() with the new
function but arrived at a similar conclusion as yours.

> I think it can't, because the new function requires the
> backend to still be present in the procarray:
>
>         proc = BackendPidGetProc(pid);
>
>         if (proc == NULL)
>         {
>                 ereport(WARNING,
>                                 (errmsg("PID %d is not a PostgreSQL server process", pid)));
>
>                 PG_RETURN_BOOL(false);
>         }
>
>         PG_RETURN_BOOL(pg_wait_until_termination(pid, timeout));
>
> If a backend has left the procarray but not yet left the kernel process table,
> this function will issue the warning and not wait for the final exit.

Yes, if the backend is not in procarray but still in the kernel
process table, it emits a warning "PID %d is not a PostgreSQL server
process" and returns false.

> Given that limitation, is pg_wait_for_backend_termination() useful enough?  If
> waiting for procarray departure is enough, should pg_wait_until_termination()
> check BackendPidGetProc(pid) instead of kill(0, pid), so it can return
> earlier?

We can just remove BackendPidGetProc(pid) in
pg_wait_for_backend_termination. With this change, we can get rid of
the wait_pid() from regress.c. But, my concern is that the
pg_wait_for_backend_termination() can also check non-postgres server
process pid. Is this okay? In that case, this function becomes a
generic(OS level function) rather than a postgres server specific
function. I'm not sure if all agree to that. Thoughts?

> I can see the value of adding the pg_terminate_backend() timeout
> argument, in any case.

True. We can leave pg_terminate_backend() as is.

With Regards,
Bharath Rupireddy.

  46. Re: A new function to wait for the backend exit after termination

    Noah Misch <noah@leadboat.com> — 2021-06-05T01:32:36Z

    On Tue, Jun 01, 2021 at 01:25:24PM +0530, Bharath Rupireddy wrote:
> On Tue, Jun 1, 2021 at 9:19 AM Noah Misch <noah@leadboat.com> wrote:
> > Given that limitation, is pg_wait_for_backend_termination() useful enough?  If
> > waiting for procarray departure is enough, should pg_wait_until_termination()
> > check BackendPidGetProc(pid) instead of kill(0, pid), so it can return
> > earlier?
> 
> We can just remove BackendPidGetProc(pid) in
> pg_wait_for_backend_termination. With this change, we can get rid of
> the wait_pid() from regress.c. But, my concern is that the
> pg_wait_for_backend_termination() can also check non-postgres server
> process pid. Is this okay?

It may or may not be okay.  I would not feel good about it.

> In that case, this function becomes a
> generic(OS level function) rather than a postgres server specific
> function. I'm not sure if all agree to that. Thoughts?

My preference is to remove pg_wait_for_backend_termination().  The use case
that prompted this thread used pg_terminate_backend(pid, 180000); it doesn't
need pg_wait_for_backend_termination().

  47. Re: A new function to wait for the backend exit after termination

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-06-05T06:36:46Z

    On Sat, Jun 5, 2021 at 7:02 AM Noah Misch <noah@leadboat.com> wrote:
>
> On Tue, Jun 01, 2021 at 01:25:24PM +0530, Bharath Rupireddy wrote:
> > On Tue, Jun 1, 2021 at 9:19 AM Noah Misch <noah@leadboat.com> wrote:
> > > Given that limitation, is pg_wait_for_backend_termination() useful enough?  If
> > > waiting for procarray departure is enough, should pg_wait_until_termination()
> > > check BackendPidGetProc(pid) instead of kill(0, pid), so it can return
> > > earlier?
> >
> > We can just remove BackendPidGetProc(pid) in
> > pg_wait_for_backend_termination. With this change, we can get rid of
> > the wait_pid() from regress.c. But, my concern is that the
> > pg_wait_for_backend_termination() can also check non-postgres server
> > process pid. Is this okay?
>
> It may or may not be okay.  I would not feel good about it.
>
> > In that case, this function becomes a
> > generic(OS level function) rather than a postgres server specific
> > function. I'm not sure if all agree to that. Thoughts?
>
> My preference is to remove pg_wait_for_backend_termination().  The use case
> that prompted this thread used pg_terminate_backend(pid, 180000); it doesn't
> need pg_wait_for_backend_termination().

I was earlier thinking that the function
pg_wait_for_backend_termination() will be useful:
1) If the user wants to pg_terminate_backend(<<pid>>); and
pg_wait_for_backend_termination(<<pid>>, <<timeout>>); separately. It
seems like the proc array entry will be removed as part of SITERM
processing (see [1]) and the BackendPidGetProc will return NULL. So,
it's not useful here.
2) If the user wants to pg_wait_for_backend_termination(<<pid>>,
<<timeout>>);, thinking that some event might cause the backend to be
terminated within the <<timeout>>. So, it's still useful here.

[1]
(gdb) bt
#0  ProcArrayRemove (proc=0x55b27f26356c
<CleanupInvalidationState+278>, latestXid=32764)
    at procarray.c:526
#1  0x000055b27f281c9d in RemoveProcFromArray (code=1, arg=0) at proc.c:812
#2  0x000055b27f2542ce in shmem_exit (code=1) at ipc.c:272
#3  0x000055b27f2540d5 in proc_exit_prepare (code=1) at ipc.c:194
#4  0x000055b27f254022 in proc_exit (code=1) at ipc.c:107
#5  0x000055b27f449479 in errfinish (filename=0x55b27f61cd65
"postgres.c", lineno=3191,
    funcname=0x55b27f61e770 <__func__.40727> "ProcessInterrupts") at elog.c:666
#6  0x000055b27f29097e in ProcessInterrupts () at postgres.c:3191
#7  0x000055b27f28cbf0 in ProcessClientReadInterrupt (blocked=true) at
postgres.c:499

With Regards,
Bharath Rupireddy.

  48. Re: A new function to wait for the backend exit after termination

    Noah Misch <noah@leadboat.com> — 2021-06-05T19:08:01Z

    On Sat, Jun 05, 2021 at 12:06:46PM +0530, Bharath Rupireddy wrote:
> On Sat, Jun 5, 2021 at 7:02 AM Noah Misch <noah@leadboat.com> wrote:
> > On Tue, Jun 01, 2021 at 01:25:24PM +0530, Bharath Rupireddy wrote:
> > > On Tue, Jun 1, 2021 at 9:19 AM Noah Misch <noah@leadboat.com> wrote:
> > > > Given that limitation, is pg_wait_for_backend_termination() useful enough?  If
> > > > waiting for procarray departure is enough, should pg_wait_until_termination()
> > > > check BackendPidGetProc(pid) instead of kill(0, pid), so it can return
> > > > earlier?
> > >
> > > We can just remove BackendPidGetProc(pid) in
> > > pg_wait_for_backend_termination. With this change, we can get rid of
> > > the wait_pid() from regress.c. But, my concern is that the
> > > pg_wait_for_backend_termination() can also check non-postgres server
> > > process pid. Is this okay?
> >
> > It may or may not be okay.  I would not feel good about it.
> >
> > > In that case, this function becomes a
> > > generic(OS level function) rather than a postgres server specific
> > > function. I'm not sure if all agree to that. Thoughts?
> >
> > My preference is to remove pg_wait_for_backend_termination().  The use case
> > that prompted this thread used pg_terminate_backend(pid, 180000); it doesn't
> > need pg_wait_for_backend_termination().
> 
> I was earlier thinking that the function
> pg_wait_for_backend_termination() will be useful:
> 1) If the user wants to pg_terminate_backend(<<pid>>); and
> pg_wait_for_backend_termination(<<pid>>, <<timeout>>); separately. It
> seems like the proc array entry will be removed as part of SITERM
> processing (see [1]) and the BackendPidGetProc will return NULL. So,
> it's not useful here.
> 2) If the user wants to pg_wait_for_backend_termination(<<pid>>,
> <<timeout>>);, thinking that some event might cause the backend to be
> terminated within the <<timeout>>. So, it's still useful here.

That is factual.  That pg_wait_for_backend_termination() appears to be useful
for (1) but isn't useful for (1) reduces its value.  I think it reduces the
value slightly below zero.  Relevant to that, if a user doesn't care about the
distinction between "backend has left the procarray" and "backend's PID has
left the kernel process table", that user can poll pg_stat_activity to achieve
the same level of certainty that pg_wait_for_backend_termination() offers.

  49. Re: A new function to wait for the backend exit after termination

    Justin Pryzby <pryzby@telsasoft.com> — 2021-06-12T01:54:08Z

    On Sat, Jun 05, 2021 at 12:08:01PM -0700, Noah Misch wrote:
> > > My preference is to remove pg_wait_for_backend_termination().  The use case
> > > that prompted this thread used pg_terminate_backend(pid, 180000); it doesn't
> > > need pg_wait_for_backend_termination().

Is this an Opened Issue ?

-- 
Justin

  50. Re: A new function to wait for the backend exit after termination

    Noah Misch <noah@leadboat.com> — 2021-06-12T04:37:50Z

    On Fri, Jun 11, 2021 at 08:54:08PM -0500, Justin Pryzby wrote:
> On Sat, Jun 05, 2021 at 12:08:01PM -0700, Noah Misch wrote:
> > > > My preference is to remove pg_wait_for_backend_termination().  The use case
> > > > that prompted this thread used pg_terminate_backend(pid, 180000); it doesn't
> > > > need pg_wait_for_backend_termination().
> 
> Is this an Opened Issue ?

An Open Item?  Not really, since there's no objective defect.  Nonetheless,
the attached is what I'd like to use.

  51. Re: A new function to wait for the backend exit after termination

    Justin Pryzby <pryzby@telsasoft.com> — 2021-06-12T05:12:12Z

    On Fri, Jun 11, 2021 at 09:37:50PM -0700, Noah Misch wrote:
> On Fri, Jun 11, 2021 at 08:54:08PM -0500, Justin Pryzby wrote:
> > On Sat, Jun 05, 2021 at 12:08:01PM -0700, Noah Misch wrote:
> > > > > My preference is to remove pg_wait_for_backend_termination().  The use case
> > > > > that prompted this thread used pg_terminate_backend(pid, 180000); it doesn't
> > > > > need pg_wait_for_backend_termination().
> > 
> > Is this an Opened Issue ?
> 
> An Open Item?  Not really, since there's no objective defect.  Nonetheless,
> the attached is what I'd like to use.

I think of this as a list of stuff to avoid forgetting that needs to be
addressed or settled before the release.

If the value of the new function is marginal, it may be good to remove it, else
we're committed to supporting it.

Even if it's not removed, the descriptions should be cleaned up.

| src/include/catalog/pg_proc.dat-  descr => 'terminate a backend process and if timeout is specified, wait for its exit or until timeout occurs',
=> I think doesn't need to change or mention the optional timeout at all

| src/include/catalog/pg_proc.dat-{ oid => '2137', descr => 'wait for a backend process exit or timeout occurs',
=> should just say "wait for a backend process to exit".  The timeout has a default.

  52. Re: A new function to wait for the backend exit after termination

    Noah Misch <noah@leadboat.com> — 2021-06-12T15:21:39Z

    On Sat, Jun 12, 2021 at 12:12:12AM -0500, Justin Pryzby wrote:
> Even if it's not removed, the descriptions should be cleaned up.
> 
> | src/include/catalog/pg_proc.dat-  descr => 'terminate a backend process and if timeout is specified, wait for its exit or until timeout occurs',
> => I think doesn't need to change or mention the optional timeout at all

Agreed, these strings generally give less detail.  I can revert that to the
v13 wording, 'terminate a server process'.

  53. Re: A new function to wait for the backend exit after termination

    Justin Pryzby <pryzby@telsasoft.com> — 2021-06-12T18:27:43Z

    On Sat, Jun 12, 2021 at 08:21:39AM -0700, Noah Misch wrote:
> On Sat, Jun 12, 2021 at 12:12:12AM -0500, Justin Pryzby wrote:
> > Even if it's not removed, the descriptions should be cleaned up.
> > 
> > | src/include/catalog/pg_proc.dat-  descr => 'terminate a backend process and if timeout is specified, wait for its exit or until timeout occurs',
> > => I think doesn't need to change or mention the optional timeout at all
> 
> Agreed, these strings generally give less detail.  I can revert that to the
> v13 wording, 'terminate a server process'.

Maybe you'd also update the release notes.

I suggest some edits from the remaining parts of the original patch.

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index fbc80c1403..b7383bc8aa 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -24998,7 +24998,7 @@ SELECT collation for ('foo' COLLATE "de_DE");
         milliseconds) and greater than zero, the function waits until the
         process is actually terminated or until the given time has passed. If
         the process is terminated, the function
-        returns <literal>true</literal>.  On timeout a warning is emitted and
+        returns <literal>true</literal>.  On timeout, a warning is emitted and
         <literal>false</literal> is returned.
        </para></entry>
       </row>
diff --git a/src/backend/storage/ipc/signalfuncs.c b/src/backend/storage/ipc/signalfuncs.c
index 837699481c..f12c417854 100644
--- a/src/backend/storage/ipc/signalfuncs.c
+++ b/src/backend/storage/ipc/signalfuncs.c
@@ -187,12 +187,12 @@ pg_wait_until_termination(int pid, int64 timeout)
 }
 
 /*
- * Signal to terminate a backend process. This is allowed if you are a member
- * of the role whose process is being terminated. If timeout input argument is
- * 0 (which is default), then this function just signals the backend and
- * doesn't wait. Otherwise it waits until given the timeout milliseconds or no
- * process has the given PID and returns true. On timeout, a warning is emitted
- * and false is returned.
+ * Send a signal to terminate a backend process. This is allowed if you are a
+ * member of the role whose process is being terminated. If the timeout input
+ * argument is 0, then this function just signals the backend and returns true.
+ * If timeout is nonzero, then it waits until no process has the given PID; if
+ * the process ends within the timeout, true is returned, and if the timeout is
+ * exceeded, a warning is emitted and false is returned.
  *
  * Note that only superusers can signal superuser-owned processes.
  */
@@ -201,7 +201,7 @@ pg_terminate_backend(PG_FUNCTION_ARGS)
 {
 	int			pid;
 	int			r;
-	int			timeout;
+	int			timeout; /* milliseconds */
 
 	pid = PG_GETARG_INT32(0);
 	timeout = PG_GETARG_INT64(1);

  54. Re: A new function to wait for the backend exit after termination

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-06-14T14:04:59Z

    On Sat, Jun 12, 2021 at 10:07 AM Noah Misch <noah@leadboat.com> wrote:
>
> On Fri, Jun 11, 2021 at 08:54:08PM -0500, Justin Pryzby wrote:
> > On Sat, Jun 05, 2021 at 12:08:01PM -0700, Noah Misch wrote:
> > > > > My preference is to remove pg_wait_for_backend_termination().  The use case
> > > > > that prompted this thread used pg_terminate_backend(pid, 180000); it doesn't
> > > > > need pg_wait_for_backend_termination().
> >
> > Is this an Opened Issue ?
>
> An Open Item?  Not really, since there's no objective defect.  Nonetheless,
> the attached is what I'd like to use.

Thanks. +1 to remove the pg_wait_for_backend_termination function. The
patch basically looks good to me. I'm attaching an updated patch. I
corrected a minor typo in the commit message, took docs and code
comment changes suggested by Justin Pryzby.

Please note that release notes still have the headline "Add functions
to wait for backend termination" of the original commit that added the
pg_wait_for_backend_termination. With the removal of it, I'm not quite
sure if we retain the the commit message or tweak it to something like
"Add optional timeout parameter to pg_terminate_backend".
<!--
Author: Magnus Hagander <magnus@hagander.net>
2021-04-08 [aaf043257] Add functions to wait for backend termination
-->

With Regards,
Bharath Rupireddy.

  55. Re: A new function to wait for the backend exit after termination

    Noah Misch <noah@leadboat.com> — 2021-06-14T16:40:27Z

    On Sat, Jun 12, 2021 at 01:27:43PM -0500, Justin Pryzby wrote:
> On Sat, Jun 12, 2021 at 08:21:39AM -0700, Noah Misch wrote:
> > On Sat, Jun 12, 2021 at 12:12:12AM -0500, Justin Pryzby wrote:
> > > Even if it's not removed, the descriptions should be cleaned up.
> > > 
> > > | src/include/catalog/pg_proc.dat-  descr => 'terminate a backend process and if timeout is specified, wait for its exit or until timeout occurs',
> > > => I think doesn't need to change or mention the optional timeout at all
> > 
> > Agreed, these strings generally give less detail.  I can revert that to the
> > v13 wording, 'terminate a server process'.
> 
> Maybe you'd also update the release notes.

What part of the notes did you expect to change that the patch did not change?

> I suggest some edits from the remaining parts of the original patch.

These look good.  I will push them when I push the other part.

  56. Re: A new function to wait for the backend exit after termination

    Justin Pryzby <pryzby@telsasoft.com> — 2021-06-14T16:46:34Z

    On Mon, Jun 14, 2021 at 09:40:27AM -0700, Noah Misch wrote:
> > > Agreed, these strings generally give less detail.  I can revert that to the
> > > v13 wording, 'terminate a server process'.

...

> > Maybe you'd also update the release notes.
> 
> What part of the notes did you expect to change that the patch did not change?

Sorry, I didn't notice that your patch already adjusted the v14 notes.

Note that Bharath also corrected your commit message to say "unable *to*", and
revert the verbose pg_proc.dat descr change.

Thanks,
-- 
Justin

  57. Re: A new function to wait for the backend exit after termination

    Noah Misch <noah@leadboat.com> — 2021-06-15T00:40:40Z

    On Mon, Jun 14, 2021 at 07:34:59PM +0530, Bharath Rupireddy wrote:
> Thanks. +1 to remove the pg_wait_for_backend_termination function. The
> patch basically looks good to me. I'm attaching an updated patch. I
> corrected a minor typo in the commit message, took docs and code
> comment changes suggested by Justin Pryzby.

Pushed as two commits.  I used your log message typo fix.  Here were the diffs
in your v2 and not in an earlier patch:

> -+       Add an optional wait parameter to <link
> ++       Add an optional timeout parameter to <link

I used this.

> -+	int			timeout; /* milliseconds */
> ++	int			timeout;  /* milliseconds */

pgindent chooses a third option, so I ran pgindent instead of using this.

> Please note that release notes still have the headline "Add functions
> to wait for backend termination" of the original commit that added the
> pg_wait_for_backend_termination. With the removal of it, I'm not quite
> sure if we retain the the commit message or tweak it to something like
> "Add optional timeout parameter to pg_terminate_backend".
> <!--
> Author: Magnus Hagander <magnus@hagander.net>
> 2021-04-08 [aaf043257] Add functions to wait for backend termination
> -->

That part is supposed to mirror "git log --pretty=format:%s", no matter what
happens later.  The next set of release note updates might add my latest
commit (5f1df62) to this SGML comment, on another line.