| From: | "Drouvot, Bertrand" <bdrouvot(at)amazon(dot)com> | 
|---|---|
| To: | Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> | 
| Cc: | Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com>, Masahiko Sawada <masahiko(dot)sawada(at)2ndquadrant(dot)com>, Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> | 
| Subject: | Re: Add Information during standby recovery conflicts | 
| Date: | 2020-11-28 11:08:24 | 
| Message-ID: | 28e04235-27e4-0697-68df-001d2b75bee6@amazon.com | 
| Views: | Whole Thread | Raw Message | Download mbox | Resend email | 
| Thread: | |
| Lists: | pgsql-hackers | 
Hi,
On 11/27/20 2:40 PM, Alvaro Herrera wrote:
> On 2020-Nov-27, Drouvot, Bertrand wrote:
>
>> +             if (nprocs > 0)
>> +             {
>> +                     ereport(LOG,
>> +                                     (errmsg("%s after %ld.%03d ms",
>> +                                                     get_recovery_conflict_desc(reason), msecs, usecs),
>> +                                      (errdetail_log_plural("Conflicting process: %s.",
>> +                                                                                "Conflicting processes: %s.",
>> +                                                                                nprocs, buf.data))));
>> +             }
>> +/* Return the description of recovery conflict */
>> +static const char *
>> +get_recovery_conflict_desc(ProcSignalReason reason)
>> +{
>> +     const char *reasonDesc = gettext_noop("unknown reason");
>> +
>> +     switch (reason)
>> +     {
>> +             case PROCSIG_RECOVERY_CONFLICT_BUFFERPIN:
>> +                     reasonDesc = gettext_noop("recovery is still waiting for recovery conflict on buffer pin");
>> +                     break;
> This doesn't work from a translation point of view.  First, you're
> building a sentence from parts, which is against policy.  Second, you're
> not actually invoking gettext to translate the string returned by
> get_recovery_conflict_desc.
>
> I think this needs to be rethought.  To handle the first problem I
> suggest to split the error message in two.  One phrase is the complain
> that recovery is waiting, and the other string is the reason for the
> wait.  Separate both either by splitting with a :, or alternatively put
> the other sentence in DETAIL.  (The latter would require to mix with the
> list of conflicting processes, which might be hard.)
>
> The first idea would look like this:
>
> LOG:  recovery still waiting after %ld.03d ms: for recovery conflict on buffer pin
> DETAIL:  Conflicting processes: 1, 2, 3.
>
> To achieve this, apart from editing the messages returned by
> get_recovery_conflict_desc, you'd need to ereport this way:
>
>      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.",
>
Thanks for your comments, I did not know that.
I've attached a new version that takes your comments into account.
Thanks!
Bertrand
| Attachment | Content-Type | Size | 
|---|---|---|
| v9-0005-Log-the-standby-recovery-conflict-waits.patch | text/plain | 18.0 KB | 
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Chapman Flack | 2020-11-28 16:10:59 | gcc -Wimplicit-fallthrough and pg_unreachable | 
| Previous Message | Alexander Korotkov | 2020-11-28 10:31:28 | Re: Improving spin-lock implementation on ARM. |