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

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.