Re: [BUG] non archived WAL removed during production crash recovery

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Jehan-Guillaume de Rorthais <jgdr(at)dalibo(dot)com>
Cc: Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, masao(dot)fujii(at)oss(dot)nttdata(dot)com, pgsql-bugs(at)lists(dot)postgresql(dot)org
Subject: Re: [BUG] non archived WAL removed during production crash recovery
Date: 2020-04-17 06:50:43
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-bugs pgsql-hackers

On Fri, Apr 17, 2020 at 12:07:39AM +0200, Jehan-Guillaume de Rorthais wrote:
> On Thu, 16 Apr 2020 17:11:00 +0900 Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>> Removing .done files does not matter as the segments related to them are
>> already gone.
> Not necessarily. There's a time windows between the moment the archiver set
> the .done file and when the checkpointer removes the associated WAL file.
> So, after a PANIC because of lack of space, WAL associated with .done files but
> not removed yet will be removed during the crash recovery.

Not sure that it is something that matters for this thread though, so
if necessary I think that it could be be discussed separately.

>> Even with that, why should we need to make the backend smarter about
>> the removal of .ready files during crash recovery. It seems to me
>> that we should keep them, and an operator could always come by himself
>> and do some manual cleanup to free some space in the pg_wal partition.
> We are agree on this.


> Unless I'm wrong, the empty string does not raise an error in pg_stat_archiver,
> and I wanted to add a test on this as well.

Exactly, it won't raise an error. Instead I switched to use a poll
query with pg_stat_file() and .ready files, but this has proved to
delay the test considerably if we did not create more segments. And
your approach has the merit to be more simple with only two segments
manipulated for the whole test. So I have tried first my idea,
noticed the mess it introduced, and just kept your approach.

> Thanks for your review! Let me know if you want me to add/change/fix some tests.

Thanks, I have worked more on the test, refactoring pieces related to
the segment names, adjusting some comments and fixing some of the
logic. Note that you introduced something incorrect at the creation
of $standby2 as you have been updating for

I have noticed an extra issue while looking at the backend pieces
today: at the beginning of the REDO loop we forgot one place where
SharedRecoveryState *has* to be updated to a correct state (around
the comment "Update pg_control to show that we are..." in xlog.c) as
the startup process may decide to switch the control file state to
update the new shared flag at this early stage. It did not matter
before because SharedRecoveryInProgress would be only "true" for both,
but that's not the case anymore as we need to make the difference
between crash recovery and archive recovery in the new flag. There is
no actual need to update SharedRecoveryState to RECOVERY_STATE_CRASH
as the initial shared memory state is RECOVERY_STATE_CRASH, but
updating the flag makes the code more consistent IMO so I updated it
anyway in the attached.

I have the feeling that I need to work a bit more on this patch, but
my impression is that we are getting to something committable here.


Attachment Content-Type Size
recovery-archives-v7.patch text/x-diff 14.8 KB

In response to


Browse pgsql-bugs by date

  From Date Subject
Next Message PG Bug reporting form 2020-04-17 07:38:04 BUG #16372: POSTGRES_DB docker-compose
Previous Message PG Bug reporting form 2020-04-17 05:39:21 BUG #16370: pgadmin broken

Browse pgsql-hackers by date

  From Date Subject
Next Message Laurenz Albe 2020-04-17 06:51:15 Re: It is not documented that pg_promote can exit standby mode
Previous Message Rajkumar Raghuwanshi 2020-04-17 06:39:51 Re: ERROR: could not open file "pg_tblspc/ issue with replication setup.