Re: BUG #13888: pg_dump write error

On Tue, Jan 26, 2016 at 11:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> On Tue, Jan 26, 2016 at 1:08 AM, Alvaro Herrera
>> <alvherre@2ndquadrant.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