Re: Add Information during standby recovery conflicts - Mailing list pgsql-hackers

Hi,

On 11/27/20 6:04 AM, Masahiko Sawada wrote:
> Thank you for updating the patch! The patch works fine and looks good
> to me except for the following small comments:
>
> +/*
> + * Log the recovery conflict.
> + *
> + * waitStart is the timestamp when the caller started to wait. This
> function also
> + * reports the details about the conflicting process ids if *waitlist
> is not NULL.
> + */
> +void
> +LogRecoveryConflict(ProcSignalReason reason, TimestampTz waitStart,
> +                                       TimestampTz cur_ts,
> VirtualTransactionId *waitlist)
>
> I think it's better to explain cur_ts as well in the function comment.
>
> Regarding the function arguments, 'waitStart' is camel case whereas
> 'cur_ts' is snake case and 'waitlist' is using only lower cases. I
> think we should unify them.
>
> ---
> -ResolveRecoveryConflictWithLock(LOCKTAG locktag)
> +ResolveRecoveryConflictWithLock(LOCKTAG locktag, bool logged_recovery_conflict)
>
> The function argument name 'logged_recovery_conflict' sounds a bit
> redundant to me as this function is used only for recovery conflict
> resolution. How about 'need_log' or something? Also it’s better to
> explain it in the function comment.

Thanks for reviewing!

I have addressed your comments in the new attached version.

Thanks

Bertrand