Date: Fri, 27 Nov 2020 10:40:57 -0300
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: "Drouvot, Bertrand" <bdrouvot@amazon.com>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>,
	Fujii Masao <masao.fujii@oss.nttdata.com>,
	Masahiko Sawada <masahiko.sawada@2ndquadrant.com>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: Add Information during standby recovery conflicts
Message-ID: <20201127134057.GA15122@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <61a9ad81-9436-dbf2-22a5-044843aa6ba3@amazon.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

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.",