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.