Re: [PATCHES] Infrastructure changes for recovery (v8)

On Mon, 2008-11-17 at 16:18 +0200, Heikki Linnakangas wrote:
> Simon Riggs wrote:
> > @@ -3845,6 +3850,52 @@ sigusr1_handler(SIGNAL_ARGS)
> >
> > PG_SETMASK(&BlockSig);
> >
> > + if (CheckPostmasterSignal(PMSIGNAL_RECOVERY_START))
> > + {
> > + Assert(pmState == PM_STARTUP);
> > +
> > + /*
> > + * Go to shutdown mode if a shutdown request was pending.
> > + */
> > + if (Shutdown > NoShutdown)
> > + {
> > + pmState = PM_WAIT_BACKENDS;
> > + /* PostmasterStateMachine logic does the rest */
> > + }
> > + else
> > + {
> > + /*
> > + * Startup process has entered recovery
> > + */
> > + pmState = PM_RECOVERY;
>
>
> Hmm, I smell a race condition here:
>
> 1. Startup process goes into consistent state, and signals postmaster
> 2. Startup process finishes WAL replay and dies
> 3. Postmaster wakes up in reaper(), noting that the startup process
> dies, and goes into PM_RUN mode.
> 4. The above signal handler for postmaster is run, causing an assertion
> failure, or putting postmaster back into PM_RECOVERY mode if assertions
> are disabled.
>
> Highly unlikely in practice, given how much code needs to run in the
> startup process between signaling the postmaster and exiting, but it
> seems theoretically possible. Do we care, and if we do, how can we fix it?

Might be possible - it does depend on the sequence of actions its true.
Agree not likely to happen, except as the result of another bug.

I'll change it to a test for

if (pmState == PM_STARTUP)
pmState = PM_RECOVERY;

The assertion was mainly for documentation, its not protecting anything
critical (IIRC).

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Training, Services and Support