Re: Crash by targetted recovery

On 2020/03/06 10:29, Kyotaro Horiguchi wrote:
> 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.

Thanks for updating the patch that way.
Not sure which patch you're mentioning, though.

Regarding 0003 patch, I added a bit more detail comments into
the patch so that we can understand the code more easily.
Updated version of 0003 patch attached. Barring any objection,
at first, I plan to commit this patch.

>> + 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.

OK.

>> - 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.

There seems to be more other places where XLogSource and
XLOG_FROM_XXX are not used yet. For example, the initial values
of readSource and XLogReceiptSource, the type of argument
"source" in XLogFileReadAnyTLI() and XLogFileRead(), etc.
These also should be updated?

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters