From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: masao.fujii@oss.nttdata.com
Cc: pgsql-bugs@lists.postgresql.org
Cc: michael@paquier.xyz
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: [BUG] non archived WAL removed during production crash recovery
Date: Tue, 14 Apr 2020 18:09:23 +0200
Message-ID: <20200414180923.759f4d9c@firost> (raw)
In-Reply-To: <20200410.110031.2261790267570514549.horikyota.ntt@gmail.com>
References: <20200408152603.4e527053@firost>
<20200409.112657.2065530970880154180.horikyota.ntt@gmail.com>
<20200409113512.22581b09@firost>
<20200410.110031.2261790267570514549.horikyota.ntt@gmail.com>
On Fri, 10 Apr 2020 11:00:31 +0900 (JST)
Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
[...]
> > > 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?
>
> No. I menat the v4 patch in [1].
>
> [1] https://www.postgresql.org/message-id/20200407171736.61906608%40firost
>
> Prior to appllying the patch (that is the commit 78ea..),
> XLogArchiveCheckDone() correctly returns true (= allow to remove it)
> for the same condition.
>
> The proposed patch does the folloing thing.
>
> if (!XLogArchivingActive() ||
> recoveryState == ARCHIVE_RECOVERING && !XLogArchivingAlways())
> return true;
>
> It doesn't return for the condition "recoveryState=CRASH_RECOVERING
> and archive_mode = on". Then the WAL files is mitakenly marked as
> ".ready" if not marked yet.
Indeed.
But .ready files are then deleted during the first restartpoint. I'm not sure
how to fix this behavior without making the code too complex.
This is discussed in my last answer to Michael few minutes ago as well.
> By the way, the code seems not following the convention a bit
> here. Let the inserting code be in the same style to the existing code
> around.
>
> + if ( ! XLogArchivingActive() )
>
> I think we don't put the spaces within the parentheses above.
Indeed, This is fixed in patch v5 sent few minutes ago.
> | ARCHIVE_RECOVERING/CRASH_RECOVERING/NOT_IN_RECOVERY
>
> The first two and the last one are in different style. *I* prefer them
> (if we use it) be "IN_ARCHIVE_RECOVERY/IN_CRASH_RECOVERY/NOT_IN_RECOVERY".
I like Michael's proposal. See v5 of the patch.
Thank you for your review!
Regards,
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: jgdr@dalibo.com, horikyota.ntt@gmail.com, masao.fujii@oss.nttdata.com, pgsql-bugs@lists.postgresql.org, michael@paquier.xyz, pgsql-hackers@lists.postgresql.org
Subject: Re: [BUG] non archived WAL removed during production crash recovery
In-Reply-To: <20200414180923.759f4d9c@firost>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox