On Sat, Mar 6, 2021 at 10:36 PM Magnus Hagander <mag...@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
v7-0001-pg_terminate_backend-with-wait-and-timeout.patch
Description: Binary data
