Re: PostmasterIsAlive() in recovery (non-USE_POST_MASTER_DEATH_SIGNAL builds)

On Mon, Nov 16, 2020 at 8:56 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> On Thu, Sep 24, 2020 at 05:55:17PM +1200, Thomas Munro wrote:
> > Right, RestoreArchivedFile() uses system(), so I guess it can hang
> > around for a long time after unexpected postmaster exit on every OS if
> > the command waits. To respond to various kinds of important
> > interrupts, I suppose that'd ideally use something like
> > OpenPipeStream() and a typical WaitLatch() loop with CFI(). I'm not
> > sure what our policy is or should be for exiting while we have running
> > subprocesses. I guess that is a separate issue.
>
> - if (IsUnderPostmaster && !PostmasterIsAlive())
> + if (IsUnderPostmaster &&
> +#ifndef USE_POSTMASTER_DEATH_SIGNAL
> + count++ % 1024 == 0 &&
> +#endif
> + !PostmasterIsAlive())
> That's pretty hack-ish, still efficient. Could we consider a
> different approach like something relying on
> PostmasterIsAliveInternal() with repetitive call handling? This may
> not be the only place where we care about that, particularly for
> non-core code.

As far as I know there aren't any other places that do polling of
PostmasterIsAlive() in a loop like this. All others have been removed
from core code: either they already had a WaitLatch() or similar so it
we just had to add WL_EXIT_ON_PM_DEATH, or they do pure CPU-bound and
don't actively try to detect postmaster death. That's why it seems
utterly insane that here we try to detect it X million times per
second.

> No objections with the two changes from pg_usleep() to WaitLatch() so
> they could be applied separately first.

I thought about committing that first part, and got as far as
splitting the patch into two (see attached), but then I re-read
Fujii-san's message about the speed of promotion and realised that we
really should have something like a condition variable for walRcvState
changes. I'll look into that.