Subject: Re: Add Information during standby recovery conflicts
To: Masahiko Sawada <sawada.mshk@gmail.com>
CC: Fujii Masao <masao.fujii@oss.nttdata.com>, Masahiko Sawada
	<masahiko.sawada@2ndquadrant.com>, Kyotaro Horiguchi
	<horikyota.ntt@gmail.com>, Alvaro Herrera <alvherre@alvh.no-ip.org>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
References: 
 <CA+fd4k4DTjgNXrgbQ7=2hVidG+inzqJUU2BziV=sL7yR0BmC=A@mail.gmail.com>
 <20201015.121338.832998229451118046.horikyota.ntt@gmail.com>
 <CA+fd4k53s4-P1PuebNgGysO8uHmiitKWxKXcFYgLU07iUx0AsQ@mail.gmail.com>
 <20201015.145210.622238854316424234.horikyota.ntt@gmail.com>
 <CA+fd4k5JjcphpPw8oorRGAC5v4z-Bn-6dJ9=vRtX3O7gp+5YYg@mail.gmail.com>
 <c51826fa-d9d4-4f99-d837-1872ce2d402a@amazon.com>
 <CA+fd4k4jOQV51-LJ=B3b7R2mZ+W3AhFkM3XaqyYcqAx_nUk6aw@mail.gmail.com>
 <9493cda6-ac77-6aff-28ea-4ff7774ba78c@oss.nttdata.com>
 <CA+fd4k6xVsPLvV2YO9PKceCBqdxvxQJL7sPS1nmVA-iAk2n58A@mail.gmail.com>
 <c141f3fc-2cff-a8f5-08b5-0aafa173c949@oss.nttdata.com>
 <6416172a-8c7d-84da-2600-f5f41c61db18@amazon.com>
 <CAD21AoC2K8=Du2hLhWGXwq31CH6K7pWxDYAiz4Tmzs5yyfMohA@mail.gmail.com>
 <e2819497-ff8b-831c-e6aa-6f1e81b1e709@amazon.com>
 <CAD21AoDuSCvQip1MBwFa+gAaBiT4E-X=sQJVP-ayP=VZPkimtg@mail.gmail.com>
 <a2003b61-3169-0523-af37-44876248b547@amazon.com>
 <CAD21AoCMP6QPZDK2uqHmG_tV=Uwo5_8yKK1uKec6PnzN30YQkw@mail.gmail.com>
From: "Drouvot, Bertrand" <bdrouvot@amazon.com>
Message-ID: <9682a167-af46-2e58-7f77-c87edaf80fde@amazon.com>
Date: Tue, 17 Nov 2020 09:23:13 +0100
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:78.0)
 Gecko/20100101 Thunderbird/78.4.3
MIME-Version: 1.0
In-Reply-To: 
 <CAD21AoCMP6QPZDK2uqHmG_tV=Uwo5_8yKK1uKec6PnzN30YQkw@mail.gmail.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Precedence: bulk


On 11/17/20 2:09 AM, Masahiko Sawada wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On Mon, Nov 16, 2020 at 4:55 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote:
>> Hi,
>>
>> On 11/16/20 6:44 AM, Masahiko Sawada wrote:
>>> Thank you for updating the patch.
>>>
>>> Here are review comments.
>>>
>>> +           if (report_waiting && (!logged_recovery_conflict ||
>>> new_status == NULL))
>>> +               ts = GetCurrentTimestamp();
>>>
>>> The condition will always be true if log_recovery_conflict_wait is
>>> false and report_waiting is true, leading to unnecessary calling of
>>> GetCurrentTimestamp().
>>>
>>> ---
>>> +   <para>
>>> +    You can control whether a log message is produced when the startup process
>>> +    is waiting longer than <varname>deadlock_timeout</varname> for recovery
>>> +    conflicts. This is controled by the <xref
>>> linkend="guc-log-recovery-conflict-waits"/>
>>> +    parameter.
>>> +   </para>
>>>
>>> s/controled/controlled/
>>>
>>> ---
>>>       if (report_waiting)
>>>           waitStart = GetCurrentTimestamp();
>>>
>>> Similarly, we have the above code but we don't need to call
>>> GetCurrentTimestamp() if update_process_title is false, even if
>>> report_waiting is true.
>>>
>>> I've attached the patch that fixes the above comments. It can be
>>> applied on top of your v8 patch.
>> Thanks for the review and the associated fixes!
>>
>> I've attached a new version that contains your fixes.
>>
> Thank you for updating the patch.
>
> I have other comments:
>
> +   <para>
> +    You can control whether a log message is produced when the startup process
> +    is waiting longer than <varname>deadlock_timeout</varname> for recovery
> +    conflicts. This is controlled by the
> +    <xref linkend="guc-log-recovery-conflict-waits"/> parameter.
> +   </para>
>
> It would be better to use 'WAL replay' instead of 'the startup
> process' for consistency with circumjacent descriptions. What do you
> think?

Agree that the wording is more appropriate.

>
> ---
> @@ -1260,6 +1262,8 @@ ProcSleep(LOCALLOCK *locallock, LockMethod
> lockMethodTable)
>    else
>    enable_timeout_after(DEADLOCK_TIMEOUT, DeadlockTimeout);
>    }
> + else
> +   standbyWaitStart = GetCurrentTimestamp();
>
> I think we can add a check of log_recovery_conflict_waits to avoid
> unnecessary calling of GetCurrentTimestamp().
>
> I've attached the updated version patch including the above comments
> as well as adding some assertions. Please review it.
>
That looks all good to me.

Thanks a lot for your precious help!

Bertrand