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 <bdrou...@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
