Date: Wed, 1 Apr 2020 18:17:35 +0200
From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
To: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: pgsql-bugs@lists.postgresql.org, Michael Paquier <michael@paquier.xyz>
Subject: Re: [BUG] non archived WAL removed during production crash recovery
Message-ID: <20200401181735.11100908@firost>
In-Reply-To: <d0ef12c5-7e36-b9b3-c8ea-2e98d69d1317@oss.nttdata.com>
References: <20200331172229.40ee00dc@firost>
	<d0ef12c5-7e36-b9b3-c8ea-2e98d69d1317@oss.nttdata.com>
Organization: Dalibo
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="MP_/h7vEWdLbZK.sp3iu_oxK3Oq"
Precedence: bulk

--MP_/h7vEWdLbZK.sp3iu_oxK3Oq
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

On Wed, 1 Apr 2020 17:27:22 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
[...]
> >    bool inRecovery = RecoveryInProgress();
> >    
> >    /*
> >     * The file is always deletable if archive_mode is "off".  On standbys
> >     * archiving is disabled if archive_mode is "on", and enabled with
> >     * "always".  On a primary, archiving is enabled if archive_mode is "on"
> >     * or "always".
> >     */
> >    if (!((XLogArchivingActive() && !inRecovery) ||
> >          (XLogArchivingAlways() && inRecovery)))
> >        return true;
> > 
> > Please find in attachment a patch that fix this issue using the following
> > test instead:
> > 
> >    if (!((XLogArchivingActive() && !StandbyModeRequested) ||
> >          (XLogArchivingAlways() && inRecovery)))
> >        return true;
> > 
> > I'm not sure if we should rely on StandbyModeRequested for the second part
> > of the test as well thought. What was the point to rely on
> > RecoveryInProgress() to get the recovery status from shared mem?  
> 
> Since StandbyModeRequested is the startup process-local variable,
> it basically cannot be used in XLogArchiveCheckDone() that other process
> may call.

Ok, you answered my wondering about using recovery status from shared mem. This
was obvious. Thanks for your help!

I was wondering if we could use "ControlFile->state != DB_IN_CRASH_RECOVERY".
It seems fine during crash recovery as the control file is updated before the
checkpoint, but it doesn't feel right for other code paths where the control
file might not be up-to-date on filesystem, right ?

> So another approach would be necessary... One straight idea is to
> add new shmem flag indicating whether we are in standby mode or not.

I was thinking about setting XLogCtlData->SharedRecoveryInProgress as an enum
using:

  enum RecoveryState
  {
    NOT_IN_RECOVERY = 0
    IN_CRASH_RECOVERY,
    IN_ARCHIVE_RECOVERY
  }

Please, find in attachment a patch implementing this.

Plus, I added a second commit to add one test in regard with this bug.

> Another is to make the startup process remove .ready file if necessary.

I'm not sure to understand this one.

Regards,

--MP_/h7vEWdLbZK.sp3iu_oxK3Oq
Content-Type: text/x-patch
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename=0001-v2-Fix-WAL-retention-during-production-crash-recovery.patch