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

On Tue, Mar 16, 2021 at 10:38 AM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Mon, Mar 15, 2021 at 10:38 AM Fujii Masao
> <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
> > On 2021/03/15 12:27, Bharath Rupireddy wrote:
> > > On Sun, Mar 7, 2021 at 2:39 PM Bharath Rupireddy
> > > <bharath(dot)rupireddyforpostgres(at)gmail(dot)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/