On Mon, Nov 9, 2020 at 2:49 PM Drouvot, Bertrand <bdrou...@amazon.com> wrote: > > Hi, > > On 11/6/20 3:21 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 Fri, Oct 30, 2020 at 6:02 PM Drouvot, Bertrand <bdrou...@amazon.com> > > wrote: > >> Hi, > >> > >> On 10/30/20 4:25 AM, Fujii Masao 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 2020/10/30 10:29, Masahiko Sawada wrote: > >>>> , > >>>> > >>>> On Thu, 29 Oct 2020 at 00:16, Fujii Masao > >>>> <masao.fu...@oss.nttdata.com> wrote: > >>>>> I read v8 patch. Here are review comments. > >>>> Thank you for your review. > >>>> > > Thank you for updating the patch! > > > > Looking at the latest version patch > > (v8-0002-Log-the-standby-recovery-conflict-waits.patch), I think it > > doesn't address some comments from Fujii-san. > > > >>>>> When recovery conflict with buffer pin happens, log message is output > >>>>> every deadlock_timeout. Is this intentional behavior? If yes, IMO > >>>>> that's > >>>>> not good because lots of messages can be output. > >>>> Agreed. > > I think the latest patch doesn't fix the above comment. Log message > > for recovery conflict on buffer pin is logged every deadlock_timeout. > > > >>>> if we were to log the recovery conflict only once in bufferpin > >>>> conflict case, we would log it multiple times only in lock conflict > >>>> case. So I guess it's better to do that in all conflict cases. > >>> Yes, I agree that this behavior basically should be consistent between > >>> all cases. > > The latest patch seems not to address this comment as well. > > Oh, I missed those ones, thanks for the feedback. > > New version attached, so that recovery conflict will be logged only once > also for buffer pin and lock cases.
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.
Regards,
--
Masahiko Sawada
EnterpriseDB: https://www.enterprisedb.com/
fix_masahiko2.patch
Description: Binary data
