On Wed, Jun 20, 2018 at 02:52:23PM +0200, Magnus Hagander wrote:
> On Mon, Jun 18, 2018 at 6:42 AM, Michael Paquier <michael@paquier.xyz>
> wrote:
>> Okay, so this makes two votes in favor of keeping the messages simple
>> without context, shaped like "could not read file %s", with Robert and
>> Alvaro, and two votes for keeping some context with Andres and I.
>> Anybody else has an opinion to offer?
>>
>
> Count me in with Robert and Alvaro with a +0.5 :)
Thanks for your opinion.
>> Please note that I think that some messages should keep some context
>> anyway, for example the WAL-related information is useful. An example
>> is this one where the offset is good to know about:
>> + ereport(emode_for_corrupt_record(emode, targetPagePtr + reqLen),
>> + (errmsg("could not read from log segment %s, offset %u: read
>> %d bytes, expected %d",
>> + fname, readOff, r, XLOG_BLCKSZ)));
>
> Yeah, I think you'd need to make a call for the individual message to see
> how much it helps. In this one the context definitely does, in some others
> it doesn't.
Sure. I have looked at that and I am finishing with the updated version
attached.
I removed the portion about slru in the code, but I would like to add at
least a mention about it in the commit message. The simplifications are
also applied for the control file messages you changed as well in
cfb758b. Also in ReadControlFile(), we use "could not open control
file", so for consistency this should be replaced with a more simple
message. I noticed as well that the 2PC file handling loses errno a
couple of times, and that we had better keep the messages also
consistent if we do the simplification move... relmapper.c also gains
simplifications.
All the incorrect errno handling I found should be back-patched in my
opinion as we did so in the past with 2a11b188 for example. I am not
really willing to bother about the error message improvements on
anything else than HEAD (not v11, but v12), but... Opinions about all
that?
--
Michael