From: Fujii Masao <masao.fujii@oss.nttdata.com>
To: Drouvot, Bertrand <bdrouvot@amazon.com>
To: Masahiko Sawada <sawada.mshk@gmail.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: Wed, 18 Nov 2020 00:44:39 +0900
Message-ID: <0c8a89ae-27bf-2c31-e74a-cf04033d7daf@oss.nttdata.com> (raw)
In-Reply-To: <9682a167-af46-2e58-7f77-c87edaf80fde@amazon.com>
References: <CA+fd4k4DTjgNXrgbQ7=2hVidG+inzqJUU2BziV=sL7yR0BmC=A@mail.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>
<9682a167-af46-2e58-7f77-c87edaf80fde@amazon.com>
On 2020/11/17 17:23, Drouvot, Bertrand wrote:
>
> 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!
Thanks for updating the patch! Here are review comments.
+ Controls whether a log message is produced when the startup process
+ is waiting longer than <varname>deadlock_timeout</varname>
+ for recovery conflicts.
But a log message can be produced also when the backend is waiting
for recovery conflict. Right? If yes, this description needs to be corrected.
+ for recovery conflicts. This is useful in determining if recovery
+ conflicts prevents the recovery from applying WAL.
"prevents" should be "prevent"?
+ TimestampDifference(waitStart, GetCurrentTimestamp(), &secs, &usecs);
+ msecs = secs * 1000 + usecs / 1000;
GetCurrentTimestamp() is basically called before LogRecoveryConflict()
is called. So isn't it better to avoid calling GetCurrentTimestamp() newly in
LogRecoveryConflict() and to reuse the timestamp that we got?
It's helpful to avoid the waste of cycles.
+ while (VirtualTransactionIdIsValid(*vxids))
+ {
+ PGPROC *proc = BackendIdGetProc(vxids->backendId);
BackendIdGetProc() can return NULL if the backend is not active
at that moment. This case should be handled.
+ case PROCSIG_RECOVERY_CONFLICT_BUFFERPIN:
+ reasonDesc = gettext_noop("recovery is still waiting recovery conflict on buffer pin");
It's natural to use "waiting for recovery" rather than "waiting recovery"?
+ /* Also, set deadlock timeout for logging purpose if necessary */
+ if (log_recovery_conflict_waits)
+ {
+ timeouts[cnt].id = STANDBY_TIMEOUT;
+ timeouts[cnt].type = TMPARAM_AFTER;
+ timeouts[cnt].delay_ms = DeadlockTimeout;
+ cnt++;
+ }
This needs to be executed only when the message has not been logged yet.
Right?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
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: masao.fujii@oss.nttdata.com, bdrouvot@amazon.com, sawada.mshk@gmail.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: <0c8a89ae-27bf-2c31-e74a-cf04033d7daf@oss.nttdata.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