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.
Thanks!
Bertrand
