Hi,
On 11/17/20 4:44 PM, Fujii Masao wrote:
>
> 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.
Thanks for looking at it!
I don't think so, only the startup process should write those new log
messages.
What makes you think that would not be the case?
>
>
> + for recovery conflicts. This is useful in determining if
> recovery
> + conflicts prevents the recovery from applying WAL.
>
> "prevents" should be "prevent"?
Indeed: fixed in the new attached patch.
>
>
> + 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.
>
good catch! fixed in the new attached patch.
>
> + 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.
>
handled in the new attached patch.
>
> + 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"?
>
I would be tempted to say so, the new patch makes use of "waiting for".
>
> + /* 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?
>
good catch: fixed in the new attached patch.
Bertrand
