Subject: Re: Crash by targetted recovery
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
References: <899463ee-549c-3f30-5752-ae1988ddc066@oss.nttdata.com>
 <20200227.152307.953966656438810060.horikyota.ntt@gmail.com>
 <ab80bd8c-1a3c-4ef2-e846-28258875f37a@oss.nttdata.com>
 <20200227.170530.2264412619413068664.horikyota.ntt@gmail.com>
From: Fujii Masao <masao.fujii@oss.nttdata.com>
Message-ID: <a74bfa5d-3a86-90b8-286e-9ff8822bd8e1@oss.nttdata.com>
Date: Thu, 27 Feb 2020 20:04:41 +0900
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:68.0)
 Gecko/20100101 Thunderbird/68.5.0
MIME-Version: 1.0
In-Reply-To: <20200227.170530.2264412619413068664.horikyota.ntt@gmail.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 2020/02/27 17:05, Kyotaro Horiguchi wrote:
> Thank you for the comment.
> At Thu, 27 Feb 2020 16:23:44 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in
>> On 2020/02/27 15:23, Kyotaro Horiguchi wrote:
>>>> I failed to understand why random access while reading from
>>>> stream is bad idea. Could you elaborate why?
>>> It seems to me the word "streaming" suggests that WAL record should be
>>> read sequentially. Random access, which means reading from arbitrary
>>> location, breaks a stream.  (But the patch doesn't try to stop wal
>>> sender if randAccess.)
>>>
>>>> Isn't it sufficient to set currentSource to 0 when disabling
>>>> StandbyMode?
>>> I thought that and it should work, but I hesitated to manipulate on
>>> currentSource in StartupXLOG. currentSource is basically a private
>>> state of WaitForWALToBecomeAvailable. ReadRecord modifies it but I
>>> think it's not good to modify it out of the the logic in
>>> WaitForWALToBecomeAvailable.
>>
>> If so, what about adding the following at the top of
>> WaitForWALToBecomeAvailable()?
>>
>>      if (!StandbyMode && currentSource == XLOG_FROM_STREAM)
>>           currentSource = 0;
> 
> It works virtually the same way. I'm happy to do that if you don't
> agree to using randAccess. But I'd rather do that in the 'if
> (!InArchiveRecovery)' section.

The approach using randAccess seems unsafe. Please imagine
the case where currentSource is changed to XLOG_FROM_ARCHIVE
because randAccess is true, while walreceiver is still running.
For example, this case can occur when the record at REDO
starting point is fetched with randAccess = true after walreceiver
is invoked to fetch the last checkpoint record. The situation
"currentSource != XLOG_FROM_STREAM while walreceiver is
  running" seems invalid. No?

So I think that the approach that I proposed is better.

Regards,

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