From: Drouvot, Bertrand <bdrouvot@amazon.com>
To: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Fujii Masao <masao.fujii@oss.nttdata.com>
Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: Add Information during standby recovery conflicts
Date: Tue, 17 Nov 2020 09:23:13 +0100
Message-ID: <9682a167-af46-2e58-7f77-c87edaf80fde@amazon.com> (raw)
In-Reply-To: <CAD21AoCMP6QPZDK2uqHmG_tV=Uwo5_8yKK1uKec6PnzN30YQkw@mail.gmail.com>
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>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: bdrouvot@amazon.com, sawada.mshk@gmail.com, masao.fujii@oss.nttdata.com, masahiko.sawada@2ndquadrant.com, horikyota.ntt@gmail.com, alvherre@alvh.no-ip.org
Subject: Re: Add Information during standby recovery conflicts
In-Reply-To: <9682a167-af46-2e58-7f77-c87edaf80fde@amazon.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox