Making psql error out on output failures

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Making psql error out on output failures

Daniel Verite
  Hi,

When exporting data with psql -c "..." >file or select ... \g file inside
psql,
post-creation output errors are silently ignored.
The problem can be seen easily by creating a small ramdisk and
filling it over capacity:

$ sudo mount -t tmpfs -o rw,size =1M tmpfs /mnt/ramdisk

$ psql -d postgres -At \
  -c "select repeat('abc', 1000000)" > /mnt/ramdisk/file

$ echo $?
0

$ ls -l /mnt/ramdisk/file
-rw-r--r-- 1 daniel daniel 1048576 Dec 16 15:57 /mnt/ramdisk/file

$ df -h /mnt/ramdisk/
Filesystem Size  Used Avail Use% Mounted on
tmpfs 1.0M  1.0M     0 100% /mnt/ramdisk

The output that should be 3M byte long is truncated as expected, but
we got no error message and no error code from psql, which
is obviously not nice.

The reason is that PrintQuery() and the code below it in
fe_utils/print.c call puts(), putc(), fprintf(),... without checking their
return values or the result of ferror() on the output stream.
If we made them do that and had the printing bail out at the first error,
that would be a painful change, since there are a lot of such calls:
$ egrep -w '(fprintf|fputs|fputc)' fe_utils/print.c | wc -l
326
and the call sites are in functions that have no error reporting paths
anyway.

To start the discussion, here's a minimal patch that checks ferror() in
PrintQueryTuples() to raise an error when the printing is done
(better late than never).
The error message is generic as opposed to using errno, as
I don't know whether errno has been clobbered at this point.
OTOH, I think that the error indicator on the output stream is not
cleared by successful writes after some other previous writes have failed.

Are there opinions on whether this should be addressed simply like
in the attached, or a large refactoring of print.c to bail out at the first
error would be better, or other ideas on how to proceed?


Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

psql-raise-printing-errors.diff (986 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Making psql error out on output failures

David Z
Hi Daniel,
I agree with you if psql output doesn't indicate any error when the disk is full, then it is obviously not nice. In some situations, people may end up lost data permanently.
However, after I quickly applied your idea/patch to "commit bf65f3c8871bcc95a3b4d5bcb5409d3df05c8273 (HEAD -> REL_12_STABLE, origin/REL_12_STABLE)", and I found the behaviours/results are different.

Here is the steps and output,
$ sudo mkdir -p /mnt/ramdisk
$ sudo mount -t tmpfs -o rw,size=1M tmpfs /mnt/ramdisk

Test-1: delete the "file", and run psql command from a terminal directly,
$ rm /mnt/ramdisk/file
$ psql -d postgres  -At -c "select repeat('111', 1000000)" > /mnt/ramdisk/file
Error printing tuples
then dump the file,
$ rm /mnt/ramdisk/file
$ hexdump -C /mnt/ramdisk/file
00000000  31 31 31 31 31 31 31 31  31 31 31 31 31 31 31 31  |1111111111111111|
*
00100000

Test-2: delete the "file", run the command within psql console,
$ rm /mnt/ramdisk/file
$ psql -d postgres
psql (12.1)
Type "help" for help.

postgres=# select repeat('111', 1000000) \g /mnt/ramdisk/file
Error printing tuples
postgres=#
Then dump the file again,
$ hexdump -C /mnt/ramdisk/file
00000000  20 20 20 20 20 20 20 20  20 20 20 20 20 20 20 20  |                |
*
00100000

As you can see the content are different after applied the patch.

David

The new status of this patch is: Waiting on Author
Reply | Threaded
Open this post in threaded view
|

Re: Making psql error out on output failures

Daniel Verite
        David Z wrote:

> $ psql -d postgres  -At -c "select repeat('111', 1000000)" >
> /mnt/ramdisk/file

The -A option selects the unaligned output format and -t
switches to the "tuples only" mode (no header, no footer).

> Test-2: delete the "file", run the command within psql console,
> $ rm /mnt/ramdisk/file
> $ psql -d postgres

In this invocation there's no -A and -t, so the beginning of the
output is going to be a left padded column name that is not in the
other output.
The patch is not involved in that difference.


Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


Reply | Threaded
Open this post in threaded view
|

Re: Making psql error out on output failures

David Zhang
Right, the file difference is caused by "-At".

On the other side, in order to keep the output message more consistent with other tools, I did a litter bit more investigation on pg_dump to see how it handles this situation. Here is my findings.
pg_dump using WRITE_ERROR_EXIT to throw the error message when "(bytes_written != size * nmemb)", where WRITE_ERROR_EXIT calls fatal("could not write to output file: %m") and then "pg_log_generic(PG_LOG_ERROR, __VA_ARGS__)". After ran a quick test in the same situation, I got message like below,
$ pg_dump -h localhost -p 5432  -d postgres -t psql_error -f /mnt/ramdisk/file
pg_dump: error: could not write to output file: No space left on device

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.
$ psql -d postgres  -t -c "select repeat('111', 1000000)" -o /mnt/ramdisk/file
could not write to output file: No space left on device
$ psql -d postgres  -t -c "select repeat('111', 1000000)" > /mnt/ramdisk/file
could not write to output file: No space left on device

Hope the information will help.

David
---
Highgo Software Inc. (Canada)
www.highgo.ca
Reply | Threaded
Open this post in threaded view
|

Re: Making psql error out on output failures

Daniel Verite
        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. Besides, I'm not even
sure that errno is necessarily set on non-POSIX platforms when fputc
or fputs fails.
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,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


Reply | Threaded
Open this post in threaded view
|

Re: Making psql error out on output failures

David Zhang
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




Reply | Threaded
Open this post in threaded view
|

Re: Making psql error out on output failures

Daniel Verite
        David Zhang wrote:

> 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.

I don't quite see why you make that distinction? The relevant bits of
code are common, it's all the code in src/fe_utils/print.c called
from PrintQuery().

Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite