Re: BUG #13888: pg_dump write error

On Tue, Jan 26, 2016 at 11:35 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>> On Tue, Jan 26, 2016 at 1:08 AM, Alvaro Herrera
>> <alvherre(at)2ndquadrant(dot)com> wrote:
>>> Yeah, I noticed this and similar lacks of error checks in pg_dump in
>>> code review, which I didn't get around to patching. Care to submit a
>>> patch?
>
>> Indeed, with a closer look there are things like tarWrite that can
>> return 0 and trigger WRITE_ERROR_EXIT with the same thing. Couldn't we
>> simply check for errno = 0 and generate a more generic error message
>> instead? Or are you willing at replacing all those things with just
>> exit_horribly()?
>
> I do not understand these claims that there isn't an error check there.
> There surely is. But fwrite() didn't set errno.

Yep, and the error message provided let's the user think that there
has been a failure, with a success. I am wondering about something
like that:
-#define WRITE_ERROR_EXIT \
+#define WRITE_ERROR_EXIT(errno) \
do { \
- exit_horribly(modulename, "could not write to output
file: %s\n", \
- strerror(errno)); \
+ if (errno != 0) \
+ exit_horribly(modulename, "could not write to
output file: %s\n", \
+ strerror(errno)); \
+ else \
+ exit_horribly(modulename, "could not write to
output file\n"); \
} while (0)
A more invasive approach would be to actually replace most of the
READ/WRITE_ERROR_EXIT stuff by more appropriate error checks with
customized error messages. And I would prefer that, this helps having
a deeper understanding of exactly where the error occurred when
running pg_dump.

> The real question is why did he get a short write in the first place.
> We don't make any attempt to support filesystems that require retries,
> which seems to be what is going on here. Should we?

I would keep the code simple, we don't do that in the backend I recall.
--
Michael