Subject: Re: [PATCHES] Infrastructure changes for recovery (v8)
From: Simon Riggs <simon@2ndQuadrant.com>
To: Heikki Linnakangas <heikki.linnakangas@enterprisedb.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
In-Reply-To: <49217D3F.9050603@enterprisedb.com>
References: <1222815151.4445.1397.camel@ebony.2ndQuadrant>
	<48EC9CC5.6060803@enterprisedb.com>
	<1223468660.4747.294.camel@ebony.2ndQuadrant>
	<1223472898.4747.310.camel@ebony.2ndQuadrant>
	<49217D3F.9050603@enterprisedb.com>
Content-Type: text/plain
Date: Mon, 17 Nov 2008 15:39:42 +0000
Message-Id: <1226936382.3790.19.camel@ebony.2ndQuadrant>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit


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