Re: Making psql error out on output failures

On 2020-01-16 5:20 a.m., Daniel Verite wrote:
> David Zhang wrote:
>
>> If I change the error log message like below, where "%m" is used to pass the
>> value of strerror(errno), "could not write to output file:" is copied from
>> function "WRITE_ERROR_EXIT".
>> - pg_log_error("Error printing tuples");
>> + pg_log_error("could not write to output file: %m");
>> then the output message is something like below, which, I believe, is more
>> consistent with pg_dump.
> The problem is that errno may not be reliable to tell us what was
> the problem that leads to ferror(fout) being nonzero, since it isn't
> saved at the point of the error and the output code may have called
> many libc functions between the first occurrence of the output error
> and when pg_log_error() is called.
>
> The linux manpage on errno warns specifically about this:
> <quote from "man errno">
> NOTES
> A common mistake is to do
>
> if (somecall() == -1) {
> printf("somecall() failed\n");
> if (errno == ...) { ... }
> }
>
> where errno no longer needs to have the value it had upon return
> from somecall()
> (i.e., it may have been changed by the printf(3)). If the value of
> errno should be
> preserved across a library call, it must be saved:
> </quote>
>
> This other bit from the POSIX spec [1] is relevant:
>
> "The value of errno shall be defined only after a call to a function
> for which it is explicitly stated to be set and until it is changed
> by the next function call or if the application assigns it a value."
>
> To use errno in a way that complies with the above, the psql code
> should be refactored. I don't know if having a more precise error
> message justifies that refactoring. I've elaborated a bit about that
> upthread with the initial submission.

Yes, I agree with you. For case 2 "select repeat('111', 1000000) \g
/mnt/ramdisk/file", it can be easily fixed with more accurate error
message similar to pg_dump, one example could be something like below.
But for case 1 "psql -d postgres  -At -c "select repeat('111', 1000000)"
> /mnt/ramdisk/file" , it may require a lot of refactoring work.

diff --git a/src/port/snprintf.c b/src/port/snprintf.c
index 8fd997553e..e6c239fd9f 100644
--- a/src/port/snprintf.c
+++ b/src/port/snprintf.c
@@ -309,8 +309,10 @@ flushbuffer(PrintfTarget *target)

                written = fwrite(target->bufstart, 1, nc, target->stream);
                target->nchars += written;
-               if (written != nc)
+               if (written != nc) {
                        target->failed = true;
+                       fprintf(stderr, "could not write to output file:
%s\n", strerror(errno));
+               }

> Besides, I'm not even
> sure that errno is necessarily set on non-POSIX platforms when fputc
> or fputs fails.
Verified, fputs does set the errno at least in Ubuntu Linux.
> That's why this patch uses the safer approach to emit a generic
> error message.
>
> [1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/errno.html
>
>
> Best regards,
--
David

Software Engineer
Highgo Software Inc. (Canada)
www.highgo.ca