Re: Fix some error handling for read() and errno

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Magnus Hagander <magnus(at)hagander(dot)net>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Robbie Harwood <rharwood(at)redhat(dot)com>, Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, hlinnaka <hlinnaka(at)iki(dot)fi>
Subject: Re: Fix some error handling for read() and errno
Date: 2018-06-21 06:32:01
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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(at)paquier(dot)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

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

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

Attachment Content-Type Size
read-errno-handling-v5.patch text/x-diff 21.5 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Langote 2018-06-21 06:35:50 bug with expression index on partition
Previous Message Amit Khandekar 2018-06-21 06:21:17 Re: server crashed with TRAP: FailedAssertion("!(!parallel_aware || pathnode->path.parallel_safe)"