Date: Thu, 9 Apr 2020 11:35:12 +0200
From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: masao.fujii@oss.nttdata.com, pgsql-bugs@lists.postgresql.org,
 michael@paquier.xyz
Subject: Re: [BUG] non archived WAL removed during production crash recovery
Message-ID: <20200409113512.22581b09@firost>
In-Reply-To: <20200409.112657.2065530970880154180.horikyota.ntt@gmail.com>
References: <20200407171736.61906608@firost>
	<20200408.173909.874800874546158298.horikyota.ntt@gmail.com>
	<20200408152603.4e527053@firost>
	<20200409.112657.2065530970880154180.horikyota.ntt@gmail.com>
Organization: Dalibo
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Thu, 09 Apr 2020 11:26:57 +0900 (JST)
Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
[...]
> > > At Thu, 2 Apr 2020 15:49:15 +0200, Jehan-Guillaume de Rorthais
> > > <jgdr@dalibo.com> wrote in   
> > > > > Ok, so our *current* consensus seems the followings. Right?
> > > > > 
> > > > > - If archive_mode=off, any WAL files with .ready files are removed in
> > > > >     crash recovery, archive recoery and standby mode.    
> > > > 
> > > > yes    
> > > 
> > > If archive_mode = off no WAL files are marked as ".ready".  
> > 
> > Sure, on the primary side.
> > 
> > What if you build a standby from a backup with archive_mode=on with
> > some .ready files in there?   
> 
> Well. Backup doesn't have nothing in archive_status directory if it is
> taken by pg_basebackup. If the backup is created other way, it can
> have some (as Fujii-san mentioned).  Master with archive_mode != off
> and standby with archive_mode=always should archive WAL files that are
> not marked .done, but standby with archive_mode == on should not. The
> commit intended that

Unless I'm wrong, the commit avoids creating .ready files on standby when a WAL
has neither .done or .ready status file.

> but the mistake here is it thinks that inRecovery represents whether it is
> running as a standby or not, but actually it is true on primary during crash
> recovery.

Indeed.

> On the other hand, with the patch, standby with archive_mode=on
> wrongly archives WAL files during crash recovery.

"without the patch" you mean? You are talking about 78ea8b5daab, right?

> What we should check there is, as the commit was intended, not whether
> it is under crash or archive recovery, but whether it is running as
> primary or standby.

Yes.

> > > If it is "always", WAL files that are to be archived are
> > > marked as ".ready".  Finally, the condition reduces to:
> > > 
> > > If archiver is running, archive ".ready" files. Otherwise ignore
> > > ".ready" and just remove WAL files after use.  
> > > >     
> > > > > That is, WAL files with .ready files are removed when either
> > > > > archive_mode!=always in standby mode or archive_mode=off.    
> > > > 
> > > > sounds fine to me.    
> > > 
> > > That situation implies that archive_mode has been changed.  
> > 
> > Why? archive_mode may have been "always" on the primary when eg. a snapshot
> > has been created.  
> 
> .ready files are created only when archive_mode != off.

Yes, on a primary, they are created when archive_mode > off. On standby, when
archive_mode=always. If a primary had archive_mode=always, a standby created
from its backup will still have archive_mode=always, with no changes.
Maybe I miss your point, sorry.

Regards,