Re: BufFileRead() error signalling

On Mon, Jan 27, 2020 at 10:09:30AM -0500, Robert Haas wrote:
> I recognize that your commit message explains this change by saying
> that this code will now never be reached except as a result of a short
> read, but I don't think that will necessarily be clear to future
> readers of those code, or people who get the error message. It seems
> like if we're going to do do this, the error messages ought to be
> changed to complain about a short read rather than an inability to
> read for unspecified reasons. However, I wonder why we don't make
> BufFileRead() throw all of the errors including complaining about
> short reads. I would like to confess my undying (and probably
> unrequited) love for the following code from md.c:
>
> errmsg("could not read block
> %u in file \"%s\": read only %d of %d bytes",
>
> Now that is an error message! I am not confused! I don't know why that
> happened, but I sure know what happened!

I was briefly looking at 0001, and count -1 from me for the
formulation of the error messages used in those patches.

> I think we should aim for that kind of style everywhere, so that
> complaints about reading and writing files are typically reported as
> either of these:
>
> could not read file "%s": %m
> could not read file "%s": read only %d of %d bytes

That's actually not the best fit, because this does not take care of
the pluralization of the second message if you have only 1 byte to
read ;)

A second point to take into account is that the unification of error
messages makes for less translation work, which is always welcome.
Those points have been discussed on this thread:
https://www.postgresql.org/message-id/20180520000522.GB1603@paquier.xyz

The related commit is 811b6e3, and the pattern we agreed on for a
partial read was:
"could not read file \"%s\": read %d of %zu"

Then, if the syscall had an error we'd fall down to that:
"could not read file \"%s\": %m"
--
Michael