Re: Crash by targetted recovery

At Thu, 5 Mar 2020 19:51:11 +0900, Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote in
>
>
> On 2020/03/05 12:08, Kyotaro Horiguchi wrote:
> > I understand that the reconnection for REDO record is useless. Ok I
> > take the !StandbyMode way.
> > The attached is the test script that is changed to count the added
> > test, and the slight revised main patch.
>
> Thanks for the patch!
>
> + /* Wal receiver should not active when entring XLOG_FROM_ARCHIVE */
> + Assert(!WalRcvStreaming());
>
> +1 to add this assertion check.
>
> Isn't it better to always check this while trying to read WAL from
> archive or pg_wal? So, what about the following change?
>
> {
> case XLOG_FROM_ARCHIVE:
> case XLOG_FROM_PG_WAL:
> + /*
> + * WAL receiver should not be running while trying to
> + * read WAL from archive or pg_wal.
> + */
> + Assert(!WalRcvStreaming());
> +
> /* Close any old file we might have open. */
> if (readFile >= 0)

(It seems retroverting to the first patch when I started this...)
The second place covers wider cases so I reverted the first place.

> + lastSourceFailed = false; /* We haven't failed on the new source */
>
> Is this really necessary? Since ReadRecord() always reset
> lastSourceFailed to false, it seems not necessary.

It's just to make sure. Actually lastSourceFailed is always false
when we get there. But when the source is switched, lastSourceFailed
should be changed to false as a matter of design. I'd like to do that
unless that harms.

> - else if (currentSource == 0)
> + else if (currentSource == 0 ||
>
> Though this is a *separate topic*, 0 should be XLOG_FROM_ANY?
> There are some places where 0 is used as the value of currentSource.
> IMO they should be updated so that XLOG_FROM_ANY is used instead of 0.

Yeah, I've thought that many times but have neglected since it is not
critical and trivial as a separate patch. I'd take the chance to do
that now. Another minor glitch is "int oldSource = currentSource;" it
is not debugger-friendly so I changed it to XLogSource. It is added
as a new patch file before the main patch.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center