Re: Add Information during standby recovery conflicts

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