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

Jehan-Guillaume de Rorthais <jgdr@dalibo.com>

From: Jehan-Guillaume de Rorthais <jgdr@dalibo.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>, masao.fujii@oss.nttdata.com, pgsql-bugs@lists.postgresql.org
Date: 2020-04-21T22:41:21Z
Lists: pgsql-bugs, pgsql-hackers
Hello,

I did another round of review of v8.

- LocalRecoveryInProgress = xlogctl->SharedRecoveryInProgress;
+ LocalRecoveryInProgress = (xlogctl->SharedRecoveryState !=
  RECOVERY_STATE_DONE);

Do we need to acquire info_lck to look at the state here, as we do in
GetRecoveryState()? Why is it missing from previous code where
SharedRecoveryInProgress was protected by info_lck as well?

Plus, the new line length overflow the 80-column, but I'm not sure where to
break this line.

+if ($Config{osname} eq 'MSWin32')
+{
+
+	# some Windows Perls at least don't like IPC::Run's start/kill_kill
regime.
+	plan skip_all => "Test fails on Windows perl";
+}

In fact, this was inherited from 011_crash_recovery.pl where I originally
added some tests. As 020_archive_status.pl doesn't use IPC::Run, the comment is
wrong. But I wonder if this whole block is really needed. Unfortunately I can't
test on MSWin32 :/

On Tue, 21 Apr 2020 11:15:01 +0900
Michael Paquier <michael@paquier.xyz> wrote:

> > Indeed, Benoît Lobréau reported this behavior to me.  
> 
> Noted.  Thanks for the information.  I don't think that I have ever
> met Benoît in person, do I?

I don't think so.

> Tell him that I owe him one beer or a beverage of his choice when we meet
> IRL, and that he had better use this message-id to make me keep my promise :)

I told him (but I'm sure he was reading anyway :)).

Regards,



Commits

  1. Fix interpolation in test name.

  2. Add more TAP coverage for archive status with crash recovery of standbys

  3. Fix handling of WAL segments ready to be archived during crash recovery

  4. Fix WAL recycling on standbys depending on archive_mode