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

On Mon, Jun 11, 2018 at 03:17:15PM -0700, Andres Freund wrote:
> On 2018-06-11 18:11:05 -0400, Alvaro Herrera wrote:
>> As for the messages, I propose to make them more regular, i.e. always
>> use the wording "could not read from file "%s": read %d, expected %zu",
>> avoiding variations such as
>> not enough data in file \"%s\": %d read, %d expected"
>> could not read compressed file \"%s\": read %d out of %zu
>> could not read any data from log segment %s, offset %u, length %lu
>> and others that appear in other places. (In the last case, I even go as
>> far as proposing "read %d, expected %zu" where the the first %d is
>> constant zero. Extra points if the sprintf format specifiers are always
>> the same (say %zu) instead of, say, %d in odd places.
>
>> I would go as far as suggesting to remove qualifiers that indicate what
>> the file is for (such as "relation mapping file"); relying on the path
>> as indicator of what's going wrong should be sufficient, since it's an
>> error that affects internals anyway, not anything that users can do much
>> about. Keep variations to a minimum, to ease translator's work;
>> sometimes it's hard enough to come up with good translations for things
>> like "relation mapping file" in the first place, and they don't help the
>> end user.
>
> If we go there, why not wrap the read/write/etc calls into a wrapper,
> including the error handling?

On this thread have been spotted 17 code paths involved: 13 for the
backend and 4 for src/bin. For the frontend part the error handling of
each of of them is slightly different (pg_rewind uses pg_fatal,
pg_waldump uses fatal_error) so I don't think that for this part another
wrapper would bring more clarity. For the backend part, 7 are working
on transient files and 6 are not, which would make the wrapper a tad
more complex if we would need for example a set of flags in input to
control if the fd passed down by the caller should use
CloseTransientFile() before emitting an error or not. If the balance
was way more in favor of one or the other though...

> I'm not quite convinced that "relation mapping file" doesn't provide any
> information. It's likley to be easier to search for than a specific
> filename, particularly if there's oids or such in the name...

Agreed. I also quite like the message mentioning directly 2PC files as
well. I think that we could gain by making all end messages more
consistent, as the markers used and the style of each message is
slightly different, so I would suggest something like that instead to
gain in consistency:
if (readBytes < 0)
ereport(elevel, "could not blah: %m");
else
ereport(elevel, "could not blah: %d read, expected %zu");

My point is that if we use the same markers and the same end messages,
then those are easier to grep for, and callers are still free to provide
the head of error messages the way they want depending on the situation.
--
Michael