Re: Add Information during standby recovery conflicts

Hi,

On 11/30/20 4:41 AM, Masahiko Sawada wrote:
> On Sun, Nov 29, 2020 at 3:47 PM Drouvot, Bertrand <bdrouvot@amazon.com> wrote:
>> Hi Alvaro,
>>
>> On 11/28/20 6:36 PM, Alvaro Herrera wrote:
>>> Hi Bertrand,
>>>
>>> On 2020-Nov-28, Drouvot, Bertrand wrote:
>>>
>>>> +             if (nprocs > 0)
>>>> +             {
>>>> +                     ereport(LOG,
>>>> +                                     (errmsg("recovery still waiting after %ld.%03d ms: %s",
>>>> +                                                     msecs, usecs, _(get_recovery_conflict_desc(reason))),
>>>> +                                      (errdetail_log_plural("Conflicting process: %s.",
>>>> +                                                                                "Conflicting processes: %s.",
>>>> +                                                                                nprocs, buf.data))));
>>>> +             }
>>>> +             else
>>>> +             {
>>>> +                     ereport(LOG,
>>>> +                                     (errmsg("recovery still waiting after %ld.%03d ms: %s",
>>>> +                                                     msecs, usecs, _(get_recovery_conflict_desc(reason)))));
>>>> +             }
>>>> +
>>>> +             pfree(buf.data);
>>>> +     }
>>>> +     else
>>>> +             ereport(LOG,
>>>> +                             (errmsg("recovery still waiting after %ld.%03d ms: %s",
>>>> +                                             msecs, usecs, _(get_recovery_conflict_desc(reason)))));
>>>> +}
>>> Another trivial stylistic point is that you can collapse all these
>>> ereport calls into one, with something like
>>>
>>>     ereport(LOG,
>>>             errmsg("recovery still waiting after ...", opts),
>>>             waitlist != NULL ? errdetail_log_plural("foo bar baz", opts) : 0);
>>>
>>> where the "waitlist" has been constructed beforehand, or is set to NULL
>>> if there's no process list.
>> Nice!
>>
>>>> +     switch (reason)
>>>> +     {
>>>> +             case PROCSIG_RECOVERY_CONFLICT_BUFFERPIN:
>>>> +                     reasonDesc = gettext_noop("for recovery conflict on buffer pin");
>>>> +                     break;
>>> Pure bikeshedding after discussing this with my pillow: I think I'd get
>>> rid of the initial "for" in these messages.
>> both comments implemented in the new attached version.
>>
> Thank you for updating the patch!
>
> +       /* Also, set deadlock timeout for logging purpose if necessary */
> +       if (log_recovery_conflict_waits && !need_log)
> +       {
> +           timeouts[cnt].id = STANDBY_TIMEOUT;
> +           timeouts[cnt].type = TMPARAM_AFTER;
> +           timeouts[cnt].delay_ms = DeadlockTimeout;
> +           cnt++;
> +       }
>
> You changed to 'need_log' but this condition seems not correct. I
> think we need to set this timeout when both log_recovery_conflict and
> need_log is true.

Nice catch!

In fact it behaves correctly, that's jut the 'need_log' name that is 
miss leading: I renamed it to 'already_logged' in the new attached version.

> The rest of the patch looks good to me.

Great!

Thanks

Bertrand