]> granicus.if.org Git - postgresql/commitdiff
Clean up some psql issues around handling of the query output file.
authorTom Lane <tgl@sss.pgh.pa.us>
Thu, 3 Dec 2015 19:28:58 +0000 (14:28 -0500)
committerTom Lane <tgl@sss.pgh.pa.us>
Thu, 3 Dec 2015 19:29:28 +0000 (14:29 -0500)
Formerly, if "psql -o foo" failed to open the output file "foo", it would
print an error message but then carry on as though -o had not been
specified at all.  This seems contrary to expectation: a program that
cannot open its output file normally fails altogether.  Make psql do
exit(1) after reporting the error.

If "\o foo" failed to open "foo", it would print an error message but then
reset the output file to stdout, as if the argument had been omitted.
This is likewise pretty surprising behavior.  Make it keep the previous
output state, instead.

psql keeps SIGPIPE interrupts disabled when it is writing to a pipe, either
a pipe specified by -o/\o or a transient pipe opened for purposes such as
using a pager on query output.  The logic for this was too simple and could
sometimes re-enable SIGPIPE when a -o pipe was still active, thus possibly
leading to an unexpected psql crash later.

Fixing the last point required getting rid of the kluge in PrintQueryTuples
and ExecQueryUsingCursor whereby they'd transiently change the global
queryFout state, but that seems like good cleanup anyway.

Back-patch to 9.5 but not further; these are minor-enough issues that
changing the behavior in stable branches doesn't seem appropriate.

src/bin/psql/command.c
src/bin/psql/common.c
src/bin/psql/common.h
src/bin/psql/copy.c
src/bin/psql/print.c
src/bin/psql/print.h
src/bin/psql/startup.c

index 8eca4cf23bb643b9c5ec0c974a69b5f64c6a5997..6e8c62395c0735f434d961dfa02b9b673c38be8b 100644 (file)
@@ -1531,6 +1531,7 @@ exec_command(const char *cmd,
                                if (fname[0] == '|')
                                {
                                        is_pipe = true;
+                                       disable_sigpipe_trap();
                                        fd = popen(&fname[1], "w");
                                }
                                else
@@ -1565,6 +1566,9 @@ exec_command(const char *cmd,
                        }
                }
 
+               if (is_pipe)
+                       restore_sigpipe_trap();
+
                free(fname);
        }
 
index 3254a140b3a665c0e10b009373c83143b387cc3f..a287eeee1951f7f1e9de885de8b7b6cd3c9904d8 100644 (file)
 #include "mbprint.h"
 
 
-
 static bool ExecQueryUsingCursor(const char *query, double *elapsed_msec);
 static bool command_no_begin(const char *query);
 static bool is_select_command(const char *query);
 
+
 /*
- * setQFout
- * -- handler for -o command line option and \o command
+ * openQueryOutputFile --- attempt to open a query output file
  *
- * Tries to open file fname (or pipe if fname starts with '|')
- * and stores the file handle in pset)
- * Upon failure, sets stdout and returns false.
+ * fname == NULL selects stdout, else an initial '|' selects a pipe,
+ * else plain file.
+ *
+ * Returns output file pointer into *fout, and is-a-pipe flag into *is_pipe.
+ * Caller is responsible for adjusting SIGPIPE state if it's a pipe.
+ *
+ * On error, reports suitable error message and returns FALSE.
  */
 bool
-setQFout(const char *fname)
+openQueryOutputFile(const char *fname, FILE **fout, bool *is_pipe)
 {
-       bool            status = true;
-
-       /* Close old file/pipe */
-       if (pset.queryFout && pset.queryFout != stdout && pset.queryFout != stderr)
-       {
-               if (pset.queryFoutPipe)
-                       pclose(pset.queryFout);
-               else
-                       fclose(pset.queryFout);
-       }
-
-       /* If no filename, set stdout */
        if (!fname || fname[0] == '\0')
        {
-               pset.queryFout = stdout;
-               pset.queryFoutPipe = false;
+               *fout = stdout;
+               *is_pipe = false;
        }
        else if (*fname == '|')
        {
-               pset.queryFout = popen(fname + 1, "w");
-               pset.queryFoutPipe = true;
+               *fout = popen(fname + 1, "w");
+               *is_pipe = true;
        }
        else
        {
-               pset.queryFout = fopen(fname, "w");
-               pset.queryFoutPipe = false;
+               *fout = fopen(fname, "w");
+               *is_pipe = false;
        }
 
-       if (!(pset.queryFout))
+       if (*fout == NULL)
        {
                psql_error("%s: %s\n", fname, strerror(errno));
-               pset.queryFout = stdout;
-               pset.queryFoutPipe = false;
-               status = false;
+               return false;
        }
 
-       /* Direct signals */
-#ifndef WIN32
-       pqsignal(SIGPIPE, pset.queryFoutPipe ? SIG_IGN : SIG_DFL);
-#endif
-
-       return status;
+       return true;
 }
 
+/*
+ * setQFout
+ * -- handler for -o command line option and \o command
+ *
+ * On success, updates pset with the new output file and returns true.
+ * On failure, returns false without changing pset state.
+ */
+bool
+setQFout(const char *fname)
+{
+       FILE       *fout;
+       bool            is_pipe;
+
+       /* First make sure we can open the new output file/pipe */
+       if (!openQueryOutputFile(fname, &fout, &is_pipe))
+               return false;
+
+       /* Close old file/pipe */
+       if (pset.queryFout && pset.queryFout != stdout && pset.queryFout != stderr)
+       {
+               if (pset.queryFoutPipe)
+                       pclose(pset.queryFout);
+               else
+                       fclose(pset.queryFout);
+       }
+
+       pset.queryFout = fout;
+       pset.queryFoutPipe = is_pipe;
+
+       /* Adjust SIGPIPE handling appropriately: ignore signal if is_pipe */
+       set_sigpipe_trap_state(is_pipe);
+       restore_sigpipe_trap();
+
+       return true;
+}
 
 
 /*
  * Error reporting for scripts. Errors should look like
  *      psql:filename:lineno: message
- *
  */
 void
 psql_error(const char *fmt,...)
@@ -611,27 +629,23 @@ PrintQueryTuples(const PGresult *results)
        /* write output to \g argument, if any */
        if (pset.gfname)
        {
-               /* keep this code in sync with ExecQueryUsingCursor */
-               FILE       *queryFout_copy = pset.queryFout;
-               bool            queryFoutPipe_copy = pset.queryFoutPipe;
-
-               pset.queryFout = stdout;        /* so it doesn't get closed */
+               FILE       *fout;
+               bool            is_pipe;
 
-               /* open file/pipe */
-               if (!setQFout(pset.gfname))
-               {
-                       pset.queryFout = queryFout_copy;
-                       pset.queryFoutPipe = queryFoutPipe_copy;
+               if (!openQueryOutputFile(pset.gfname, &fout, &is_pipe))
                        return false;
-               }
-
-               printQuery(results, &my_popt, pset.queryFout, false, pset.logfile);
+               if (is_pipe)
+                       disable_sigpipe_trap();
 
-               /* close file/pipe, restore old setting */
-               setQFout(NULL);
+               printQuery(results, &my_popt, fout, false, pset.logfile);
 
-               pset.queryFout = queryFout_copy;
-               pset.queryFoutPipe = queryFoutPipe_copy;
+               if (is_pipe)
+               {
+                       pclose(fout);
+                       restore_sigpipe_trap();
+               }
+               else
+                       fclose(fout);
        }
        else
                printQuery(results, &my_popt, pset.queryFout, false, pset.logfile);
@@ -1199,10 +1213,10 @@ ExecQueryUsingCursor(const char *query, double *elapsed_msec)
        PGresult   *results;
        PQExpBufferData buf;
        printQueryOpt my_popt = pset.popt;
-       FILE       *queryFout_copy = pset.queryFout;
-       bool            queryFoutPipe_copy = pset.queryFoutPipe;
+       FILE       *fout;
+       bool            is_pipe;
+       bool            is_pager = false;
        bool            started_txn = false;
-       bool            did_pager = false;
        int                     ntuples;
        int                     fetch_count;
        char            fetch_cmd[64];
@@ -1268,21 +1282,22 @@ ExecQueryUsingCursor(const char *query, double *elapsed_msec)
        /* prepare to write output to \g argument, if any */
        if (pset.gfname)
        {
-               /* keep this code in sync with PrintQueryTuples */
-               pset.queryFout = stdout;        /* so it doesn't get closed */
-
-               /* open file/pipe */
-               if (!setQFout(pset.gfname))
+               if (!openQueryOutputFile(pset.gfname, &fout, &is_pipe))
                {
-                       pset.queryFout = queryFout_copy;
-                       pset.queryFoutPipe = queryFoutPipe_copy;
                        OK = false;
                        goto cleanup;
                }
+               if (is_pipe)
+                       disable_sigpipe_trap();
+       }
+       else
+       {
+               fout = pset.queryFout;
+               is_pipe = false;                /* doesn't matter */
        }
 
        /* clear any pre-existing error indication on the output stream */
-       clearerr(pset.queryFout);
+       clearerr(fout);
 
        for (;;)
        {
@@ -1302,12 +1317,10 @@ ExecQueryUsingCursor(const char *query, double *elapsed_msec)
                if (PQresultStatus(results) != PGRES_TUPLES_OK)
                {
                        /* shut down pager before printing error message */
-                       if (did_pager)
+                       if (is_pager)
                        {
-                               ClosePager(pset.queryFout);
-                               pset.queryFout = queryFout_copy;
-                               pset.queryFoutPipe = queryFoutPipe_copy;
-                               did_pager = false;
+                               ClosePager(fout);
+                               is_pager = false;
                        }
 
                        OK = AcceptResult(results);
@@ -1331,17 +1344,17 @@ ExecQueryUsingCursor(const char *query, double *elapsed_msec)
                        /* this is the last result set, so allow footer decoration */
                        my_popt.topt.stop_table = true;
                }
-               else if (pset.queryFout == stdout && !did_pager)
+               else if (fout == stdout && !is_pager)
                {
                        /*
                         * If query requires multiple result sets, hack to ensure that
                         * only one pager instance is used for the whole mess
                         */
-                       pset.queryFout = PageOutput(INT_MAX, &(my_popt.topt));
-                       did_pager = true;
+                       fout = PageOutput(INT_MAX, &(my_popt.topt));
+                       is_pager = true;
                }
 
-               printQuery(results, &my_popt, pset.queryFout, did_pager, pset.logfile);
+               printQuery(results, &my_popt, fout, is_pager, pset.logfile);
 
                PQclear(results);
 
@@ -1355,7 +1368,7 @@ ExecQueryUsingCursor(const char *query, double *elapsed_msec)
                 * the pager dies/exits/etc, there's no sense throwing more data at
                 * it.
                 */
-               flush_error = fflush(pset.queryFout);
+               flush_error = fflush(fout);
 
                /*
                 * Check if we are at the end, if a cancel was pressed, or if there
@@ -1365,24 +1378,25 @@ ExecQueryUsingCursor(const char *query, double *elapsed_msec)
                 * stop bothering to pull down more data.
                 */
                if (ntuples < fetch_count || cancel_pressed || flush_error ||
-                       ferror(pset.queryFout))
+                       ferror(fout))
                        break;
        }
 
-       /* close \g argument file/pipe, restore old setting */
        if (pset.gfname)
        {
-               /* keep this code in sync with PrintQueryTuples */
-               setQFout(NULL);
-
-               pset.queryFout = queryFout_copy;
-               pset.queryFoutPipe = queryFoutPipe_copy;
+               /* close \g argument file/pipe */
+               if (is_pipe)
+               {
+                       pclose(fout);
+                       restore_sigpipe_trap();
+               }
+               else
+                       fclose(fout);
        }
-       else if (did_pager)
+       else if (is_pager)
        {
-               ClosePager(pset.queryFout);
-               pset.queryFout = queryFout_copy;
-               pset.queryFoutPipe = queryFoutPipe_copy;
+               /* close transient pager */
+               ClosePager(fout);
        }
 
 cleanup:
index caf31d19b89d3ba03c1034c213ca82b7e2e29891..62a602632a128105c2b24c6cea369abce5513a81 100644 (file)
@@ -16,6 +16,7 @@
 
 #define atooid(x)  ((Oid) strtoul((x), NULL, 10))
 
+extern bool openQueryOutputFile(const char *fname, FILE **fout, bool *is_pipe);
 extern bool setQFout(const char *fname);
 
 extern void psql_error(const char *fmt,...) pg_attribute_printf(1, 2);
index c0fc28373a93a7e7b9f13268eea67a4df18d05af..a09408bf5504e7ab09f15d9627447bc952a6f012 100644 (file)
@@ -37,7 +37,7 @@
  * where 'filename' can be one of the following:
  *     '<file path>' | PROGRAM '<command>' | stdin | stdout | pstdout | pstdout
  * and 'query' can be one of the following:
- *  SELECT | UPDATE | INSERT | DELETE
+ *     SELECT | UPDATE | INSERT | DELETE
  *
  * An undocumented fact is that you can still write BINARY before the
  * tablename; this is a hangover from the pre-7.3 syntax.  The options
@@ -312,9 +312,7 @@ do_copy(const char *args)
                                fflush(stdout);
                                fflush(stderr);
                                errno = 0;
-#ifndef WIN32
-                               pqsignal(SIGPIPE, SIG_IGN);
-#endif
+                               disable_sigpipe_trap();
                                copystream = popen(options->file, PG_BINARY_W);
                        }
                        else
@@ -399,9 +397,7 @@ do_copy(const char *args)
                                }
                                success = false;
                        }
-#ifndef WIN32
-                       pqsignal(SIGPIPE, SIG_DFL);
-#endif
+                       restore_sigpipe_trap();
                }
                else
                {
index 190f2bc5d854ecedab990204dd2bcafa12a74fbb..05d4b3162c3441530a8d3cb286a23ba65ed5b827 100644 (file)
  */
 volatile bool cancel_pressed = false;
 
+/*
+ * Likewise, the sigpipe_trap and pager open/close functions are here rather
+ * than in common.c so that this file can be used by non-psql programs.
+ */
+static bool always_ignore_sigpipe = false;
+
+
 /* info for locale-aware numeric formatting; set up by setDecimalLocale() */
 static char *decimal_point;
 static int     groupdigits;
@@ -2775,10 +2782,61 @@ print_troff_ms_vertical(const printTableContent *cont, FILE *fout)
 
 
 /********************************/
-/* Public functions            */
+/* Public functions                            */
 /********************************/
 
 
+/*
+ * disable_sigpipe_trap
+ *
+ * Turn off SIGPIPE interrupt --- call this before writing to a temporary
+ * query output file that is a pipe.
+ *
+ * No-op on Windows, where there's no SIGPIPE interrupts.
+ */
+void
+disable_sigpipe_trap(void)
+{
+#ifndef WIN32
+       pqsignal(SIGPIPE, SIG_IGN);
+#endif
+}
+
+/*
+ * restore_sigpipe_trap
+ *
+ * Restore normal SIGPIPE interrupt --- call this when done writing to a
+ * temporary query output file that was (or might have been) a pipe.
+ *
+ * Note: within psql, we enable SIGPIPE interrupts unless the permanent query
+ * output file is a pipe, in which case they should be kept off.  This
+ * approach works only because psql is not currently complicated enough to
+ * have nested usages of short-lived output files.  Otherwise we'd probably
+ * need a genuine save-and-restore-state approach; but for now, that would be
+ * useless complication.  In non-psql programs, this always enables SIGPIPE.
+ *
+ * No-op on Windows, where there's no SIGPIPE interrupts.
+ */
+void
+restore_sigpipe_trap(void)
+{
+#ifndef WIN32
+       pqsignal(SIGPIPE, always_ignore_sigpipe ? SIG_IGN : SIG_DFL);
+#endif
+}
+
+/*
+ * set_sigpipe_trap_state
+ *
+ * Set the trap state that restore_sigpipe_trap should restore to.
+ */
+void
+set_sigpipe_trap_state(bool ignore)
+{
+       always_ignore_sigpipe = ignore;
+}
+
+
 /*
  * PageOutput
  *
@@ -2792,9 +2850,6 @@ PageOutput(int lines, const printTableOpt *topt)
        /* check whether we need / can / are supposed to use pager */
        if (topt && topt->pager && isatty(fileno(stdin)) && isatty(fileno(stdout)))
        {
-               const char *pagerprog;
-               FILE       *pagerpipe;
-
 #ifdef TIOCGWINSZ
                unsigned short int pager = topt->pager;
                int                     min_lines = topt->pager_min_lines;
@@ -2807,20 +2862,19 @@ PageOutput(int lines, const printTableOpt *topt)
                if (result == -1
                        || (lines >= screen_size.ws_row && lines >= min_lines)
                        || pager > 1)
-               {
 #endif
+               {
+                       const char *pagerprog;
+                       FILE       *pagerpipe;
+
                        pagerprog = getenv("PAGER");
                        if (!pagerprog)
                                pagerprog = DEFAULT_PAGER;
-#ifndef WIN32
-                       pqsignal(SIGPIPE, SIG_IGN);
-#endif
+                       disable_sigpipe_trap();
                        pagerpipe = popen(pagerprog, "w");
                        if (pagerpipe)
                                return pagerpipe;
-#ifdef TIOCGWINSZ
                }
-#endif
        }
 
        return stdout;
@@ -2848,9 +2902,7 @@ ClosePager(FILE *pagerpipe)
                        fprintf(pagerpipe, _("Interrupted\n"));
 
                pclose(pagerpipe);
-#ifndef WIN32
-               pqsignal(SIGPIPE, SIG_DFL);
-#endif
+               restore_sigpipe_trap();
        }
 }
 
index df514cffb0c770ae07bc63303ba8057de6c833bd..fd56598426da62390e7428cd6dcba1cbf457bebb 100644 (file)
@@ -167,6 +167,10 @@ extern const printTextFormat pg_asciiformat_old;
 extern const printTextFormat pg_utf8format;
 
 
+extern void disable_sigpipe_trap(void);
+extern void restore_sigpipe_trap(void);
+extern void set_sigpipe_trap_state(bool ignore);
+
 extern FILE *PageOutput(int lines, const printTableOpt *topt);
 extern void ClosePager(FILE *pagerpipe);
 
index 7aa997d479c8be02ad8c7b2180b3ff8c3b5c3709..4e5021a43df80fae16ab3b2a4cf34434e52dc6c0 100644 (file)
@@ -461,7 +461,8 @@ parse_psql_options(int argc, char *argv[], struct adhoc_opts * options)
                                options->no_readline = true;
                                break;
                        case 'o':
-                               setQFout(optarg);
+                               if (!setQFout(optarg))
+                                       exit(EXIT_FAILURE);
                                break;
                        case 'p':
                                options->port = pg_strdup(optarg);