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

On Fri, Dec 4, 2020 at 10:13 AM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Fri, Dec 4, 2020 at 2:02 PM Hou, Zhijie <houzj(dot)fnst(at)cn(dot)fujitsu(dot)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/