Date: Fri, 3 Apr 2020 18:26:25 +0200
From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
To: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
 pgsql-bugs@lists.postgresql.org, michael@paquier.xyz
Subject: Re: [BUG] non archived WAL removed during production crash recovery
Message-ID: <20200403182625.0fccc6fd@firost>
In-Reply-To: <3d543ef2-ae02-5e59-3bfe-b79d00dab360@oss.nttdata.com>
References: <20200402.130443.1791362864704140391.horikyota.ntt@gmail.com>
	<20200402.130734.613319908534820379.horikyota.ntt@gmail.com>
	<03900fc5-fff4-109a-fe69-83de2c167929@oss.nttdata.com>
	<20200402.162323.770257425358491539.horikyota.ntt@gmail.com>
	<ca964b3a-61a0-902e-c7b3-3abbc01a921f@oss.nttdata.com>
	<20200402154915.4984cff2@firost>
	<3d543ef2-ae02-5e59-3bfe-b79d00dab360@oss.nttdata.com>
Organization: Dalibo
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="MP_/hFsTesUnE4wI_trfLOopy+I"
Precedence: bulk

--MP_/hFsTesUnE4wI_trfLOopy+I
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

On Thu, 2 Apr 2020 23:55:46 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

> On 2020/04/02 22:49, Jehan-Guillaume de Rorthais wrote:
>> On Thu, 2 Apr 2020 19:38:59 +0900
>> Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
[...]
>>> 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.

To some extends, it appears to me this sentence was relatively close to my
previous patch, as far as you exclude IN_CRASH_RECOVERY from the shortcut:

XLogArchiveCheckDone(const char *xlog)
{
    [...]
    if ( (inRecoveryState != IN_CRASH_RECOVERY) && (
          (inRecoveryState == NOT_IN_RECOVERY && !XLogArchivingActive()) ||
          (inRecoveryState == IN_ARCHIVE_RECOVERY && !XLogArchivingAlways())))
        return true;

Which means that only .done cleanup would occurs during CRASH_RECOVERY
and .ready files might be created if no .done exists. No matter the futur status
of the cluster: primary or standby. Normal shortcut will apply during first
checkpoint after the crash recovery step.

This should handle the case where a backup without backup_label (taken from a
snapshot or after a shutdown with immediate) is restored to build a standby.

Please, find in attachment a third version of my patch
"0001-v3-Fix-WAL-retention-during-production-crash-recovery.patch".

"0002-v1-Add-test-on-non-archived-WAL-during-crash-recovery.patch" is left
untouched. But I'm considering adding some more tests relative to this
discussion.

> BTW, now I'm thinking that the flag in shmem should be updated when
> the startup process sets StandbyModeRequested to true at the beginning
> of the recovery. That is,
> 
> - Add something like SharedStandbyModeRequested into XLogCtl. This field
>     should be initialized with false;
> - Set XLogCtl->SharedStandbyModeRequested to true when the startup
>     process detects the standby.signal file and sets the local variable
>     StandbyModeRequested to true.
> - Make XLogArchiveCheckDone() use XLogCtl->SharedStandbyModeRequested
>     to know whether the server is in standby mode or not.
> 
> Thought?

I try to avoid a new flag in memory with the proposal in attachment of this
email. It seems to me various combinaisons of booleans with subtle differences
around the same subject makes it a bit trappy and complicated to understand.

If my proposal is rejected, I'll be happy to volunteer to add
XLogCtl->SharedStandbyModeRequested though. It seems like a simple enough fix
as well.

Regards,

--MP_/hFsTesUnE4wI_trfLOopy+I
Content-Type: text/x-patch
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename=0001-v3-Fix-WAL-retention-during-production-crash-recovery.patch