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

On Mon, Apr 20, 2020 at 04:02:31PM +0900, Kyotaro Horiguchi wrote:
> At Sat, 18 Apr 2020 18:26:11 +0900, Michael Paquier <michael(at)paquier(dot)xyz> wrote in
> As the result, +1 to what v7 is doing and discussing on earlier
> removal of such WAL segments separately if needed.

Thanks for the extra review.

>> Yeah. We could try to do with "false" as command anyway, and see what
>> the buildfarm thinks. As the test is skipped on Windows, I would
>> assume that it does not matter much anyway. Let's see what others
>> think about this piece. I don't have plans to touch again this patch
>> until likely the middle of next week.
>
> Couldn't we use "/" as a globally-results-in-failure command? But
> that doesn't increment failed_count. The reason is pgarch_archiveXLog
> exits with FATAL for "is a directory" error. The comment asserts that
> we exit with FATAL for SIGINT or SIGQUIT and if so it is enough to
> check only exit-by-signal case. The following fix worked.

Yeah, I was working on this stuff today and I noticed this problem. I
was just going to send an email on the matter with a more portable
patch and you also just beat me to it with this one :)

So yes, using "false" may be a bad idea because we cannot rely on the
case where the command does not exist in an environment in this test.
After more testing, I have been hit hard about the fact that the
archiver exits immediately if an archive command cannot be found
(errcode = 127), and it does not report this failure back to
pg_stat_archiver, which would cause the test to wait until the timeout
of poll_query_until() kills the test. There is however an extra
method not mentioned yet on this thread: we know that cp/copy is
portable enough per the buildfarm, so let's use a copy command that we
know *will* fail. A simple way of doing this is a command where the
origin file does not exist.

> --- a/src/backend/postmaster/pgarch.c
> +++ b/src/backend/postmaster/pgarch.c
> @@ -595,7 +595,7 @@ pgarch_archiveXlog(char *xlog)
> * "command not found" type of error. If we overreact it's no big
> * deal, the postmaster will just start the archiver again.
> */
> - int lev = wait_result_is_any_signal(rc, true) ? FATAL : LOG;
> + int lev = wait_result_is_any_signal(rc, false) ? FATAL : LOG;
>
> if (WIFEXITED(rc))
> {
>
> I didn't tested it on Windows (I somehow broke my repo and it's too
> slow to clone.) but system("/") returned 1 and I think that result
> increments the counter.

No, this would be a behavior change, which is not acceptable in my
view. (By the way, just nuke your full repo if it does not work
anymore on Windows, this method works).

>> Indeed. The extra initialization was part of v4, and got removed as
>> of v5. Still, it seems to me that this part was not complete without
>> updating the shared memory field correctly at the beginning of the
>> REDO processing as the last version of the patch does.
>
> I may not be following the discussion, but I think it is reasonable
> that SharedRecoveryState is initialized as CRASH then moves to ARCHIVE
> as needed and finished by NONE. That transition also stables
> RecoveryInProgress().

Thought as well about that over the weekend, and that's still the best
option to me.

> I think it would be better be RECOVERY_STATE_DONE.

I like this suggestion better than the original in v7.

> By the way I noticed that RecoveryState is exactly a subset of
> DBState. And changes of SharedRecoveryState happens side-by-side with
> ControlFileData->state in most places. Coundn't we just usee
> ControlFile->state instead of SharedRecoveryState?

I actually found confusing to use the same thing, because then the
reader would thing that SharedRecoveryState could be set to more
values but we don't want that.

> By the way I found a typo.
>
> +# Recovery tests for the achiving with a standby partially check
> s/achiving/archiving/

Thanks, fixed.

Attached is an updated patch, where I tweaked more comments.

Jehan-Guillaume, who is your colleague who found originally about this
problem? We should credit him in the commit message.
--
Michael