Date: Fri, 28 Feb 2020 17:28:06 +0900 (JST)
Message-Id: <20200228.172806.1107809554616271723.horikyota.ntt@gmail.com>
To: michael@paquier.xyz
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Make mesage at end-of-recovery less scary.
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
In-Reply-To: <20200228073318.GD2688@paquier.xyz>
References: <20200228.160100.2210969269596489579.horikyota.ntt@gmail.com>
	<20200228073318.GD2688@paquier.xyz>
User-Agent: Mew version 6.8 on Emacs 26.1
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Precedence: bulk

Thank you for the comments.

At Fri, 28 Feb 2020 16:33:18 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Fri, Feb 28, 2020 at 04:01:00PM +0900, Kyotaro Horiguchi wrote:
> > Hello, this is a followup thread of [1].
> > 
> > # I didn't noticed that the thread didn't cover -hackers..
> > 
> > When recovery of any type ends, we see several kinds of error messages
> > that says "WAL is broken".
> 
> Have you considered an error context here?  Your patch leads to a bit
> of duplication with the message a bit down of what you are changing
> where the end of local pg_wal is reached.

It is a DEBUG message and it is for the time moving from crash
recovery to archive recovery. I could remove that but decided to leave
it for tracability.

> > +	* reached the end of WAL.  Otherwise something's really wrong and
> > +	* we report just only the errormsg if any. If we don't receive
> 
> This sentence sounds strange to me.  Or you meant "Something is wrong,
> so use errormsg as report if it is set"?

The whole comment there follows.
| recovery. If we get here during recovery, we can assume that we
| reached the end of WAL.  Otherwise something's really wrong and
| we report just only the errormsg if any. If we don't receive
| errormsg here, we already logged something.  We don't emit
| "reached end of WAL" in muted messages.

"Othhersise" means "other than the case of recovery".  "Just only the
errmsg" means "show the message not as a part the message "reached end
of WAL".

> > +			 * Note: errormsg is alreay translated.
> 
> Typo here.

Thanks. Will fix along with "rached".

> > +	if (StandbyMode)
> > +		ereport(actual_emode,
> > +			(errmsg ("rached end of WAL at %X/%X on timeline %u in %s during streaming replication",
> 
> StandbyMode happens also with only WAL archiving, depending on if
> primary_conninfo is set or not.

Right. I'll fix it. Maybe to "during standby mode".

> > +	(errmsg ("rached end of WAL at %X/%X on timeline %u in %s during crash recovery",
> 
> FWIW, you are introducing three times the same typo, in the same
> word, in three different messages.

They're copy-pasto.  I refrained from constructing an error message
from multiple nonindipendent parts.  Are you suggesting to do so?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center