So I took a look at this patch. The conflict is with 2fe3bdbd691
committed by Peter Eisentraut which added error checks for pipes.
Afaics the behaviour is now for pipe commands returning non-zero to
cause an error *always* regardless of the setting of ON_ERROR_STOP.
I'm not entirely sure that's sensible actually. If you write to a pipe
that ends in grep and it happens to produce no matching rows you may
actually be quite surprised when that causes your script to fail...
But if you remove that failing hunk the resulting patch does apply. I
don't see any tests so ... I don't know if the behaviour is still
sensible. A quick read gives me the impression that now it's actually
inconsistent in the other direction where it stops sometimes more
often than the user might expect.
I also don't understand the difference between ON_ERROR_STOP_ON and
ON_ERROR_STOP_ALL. Nor why we would want ON_ERROR_STOP_SHELL which
stops only on shell errors, rather than, say, ON_ERROR_STOP_SQL to do
the converse which would at least match the historical behaviour?
From 1f0feb9daf106721cb7fcba39ef7d663178f8ed1 Mon Sep 17 00:00:00 2001
From: Greg Stark <st...@mit.edu>
Date: Mon, 20 Mar 2023 14:25:22 -0400
Subject: [PATCH v5] on error stop for shell
---
doc/src/sgml/ref/psql-ref.sgml | 7 +++-
src/bin/psql/command.c | 20 ++++++++++--
src/bin/psql/common.c | 58 +++++++++++++++++++++++++++++++---
src/bin/psql/psqlscanslash.l | 29 +++++++++++++++--
src/bin/psql/settings.h | 10 +++++-
src/bin/psql/startup.c | 29 +++++++++++++----
6 files changed, 134 insertions(+), 19 deletions(-)
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 7b8ae9fac3..856bb5716f 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4194,7 +4194,12 @@ bar
<para>
By default, command processing continues after an error. When this
variable is set to <literal>on</literal>, processing will instead stop
- immediately. In interactive mode,
+ immediately upon an error in SQL query. When this variable is set to
+ <literal>shell</literal>, a nonzero exit status of a shell command,
+ which indicates failure, is interpreted as an error that stops the processing.
+ When this variable is set to <literal>all</literal>, errors from both
+ SQL queries and shell commands can stop the processing.
+ In interactive mode,
<application>psql</application> will return to the command prompt;
otherwise, <application>psql</application> will exit, returning
error code 3 to distinguish this case from fatal error
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 61ec049f05..9fbf2522ea 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -34,6 +34,7 @@
#include "describe.h"
#include "fe_utils/cancel.h"
#include "fe_utils/print.h"
+#include "fe_utils/psqlscan_int.h"
#include "fe_utils/string_utils.h"
#include "help.h"
#include "input.h"
@@ -2394,9 +2395,13 @@ exec_command_set(PsqlScanState scan_state, bool active_branch)
*/
char *newval;
char *opt;
+ PQExpBuffer output_buf = scan_state->output_buf;
opt = psql_scan_slash_option(scan_state,
OT_NORMAL, NULL, false);
+ if (output_buf->len >= output_buf->maxlen
+ && (pset.on_error_stop == PSQL_ERROR_STOP_SHELL || pset.on_error_stop == PSQL_ERROR_STOP_ALL))
+ success = false;
newval = pg_strdup(opt ? opt : "");
free(opt);
@@ -5041,10 +5046,19 @@ do_shell(const char *command)
else
result = system(command);
- if (result == 127 || result == -1)
+ if (result != 0)
{
- pg_log_error("\\!: failed");
- return false;
+ if (result < 0)
+ pg_log_error("could not execute command: %m");
+ else
+ {
+ char *reason = wait_result_to_str(result);
+
+ pg_log_error("%s: %s", command, reason ? reason : "");
+ free(reason);
+ }
+ if (pset.on_error_stop == PSQL_ERROR_STOP_SHELL || pset.on_error_stop == PSQL_ERROR_STOP_ALL)
+ return false;
}
return true;
}
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index f907f5d4e8..c87090a75d 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -94,6 +94,7 @@ setQFout(const char *fname)
{
FILE *fout;
bool is_pipe;
+ bool status = true;
/* First make sure we can open the new output file/pipe */
if (!openQueryOutputFile(fname, &fout, &is_pipe))
@@ -103,7 +104,24 @@ setQFout(const char *fname)
if (pset.queryFout && pset.queryFout != stdout && pset.queryFout != stderr)
{
if (pset.queryFoutPipe)
- pclose(pset.queryFout);
+ {
+ int pclose_rc = pclose(pset.queryFout);
+ if (pclose_rc != 0)
+ {
+ if (pclose_rc < 0)
+ pg_log_error("could not close pipe to external command: %m");
+ else
+ {
+ char *reason = wait_result_to_str(pclose_rc);
+
+ pg_log_error("command failure: %s",
+ reason ? reason : "");
+ free(reason);
+ }
+ if ((pset.on_error_stop == PSQL_ERROR_STOP_SHELL || pset.on_error_stop == PSQL_ERROR_STOP_ALL))
+ status = false;
+ }
+ }
else
fclose(pset.queryFout);
}
@@ -115,7 +133,7 @@ setQFout(const char *fname)
set_sigpipe_trap_state(is_pipe);
restore_sigpipe_trap();
- return true;
+ return status;
}
@@ -1385,7 +1403,7 @@ DescribeQuery(const char *query, double *elapsed_msec)
* For other commands, the results are processed normally, depending on their
* status.
*
- * Returns 1 on complete success, 0 on interrupt and -1 or errors. Possible
+ * Returns 1 on complete success, 0 on interrupt and -1 on errors. Possible
* failure modes include purely client-side problems; check the transaction
* status for the server-side opinion.
*
@@ -1652,7 +1670,22 @@ ExecQueryAndProcessResults(const char *query,
{
if (gfile_is_pipe)
{
- pclose(gfile_fout);
+ int pclose_rc = pclose(gfile_fout);
+ if (pclose_rc != 0)
+ {
+ if (pclose_rc < 0)
+ pg_log_error("could not close pipe to external command: %m");
+ else
+ {
+ char *reason = wait_result_to_str(pclose_rc);
+
+ pg_log_error("%s: %s", pset.gfname + 1,
+ reason ? reason : "");
+ free(reason);
+ }
+ if (pset.on_error_stop == PSQL_ERROR_STOP_SHELL || pset.on_error_stop == PSQL_ERROR_STOP_ALL)
+ success = false;
+ }
restore_sigpipe_trap();
}
else
@@ -1870,7 +1903,22 @@ ExecQueryUsingCursor(const char *query, double *elapsed_msec)
/* close \g argument file/pipe */
if (is_pipe)
{
- pclose(fout);
+ int pclose_rc = pclose(fout);
+ if (pclose_rc != 0)
+ {
+ if (pclose_rc < 0)
+ pg_log_error("could not close pipe to external command: %m");
+ else
+ {
+ char *reason = wait_result_to_str(pclose_rc);
+
+ pg_log_error("%s: %s", pset.gfname + 1,
+ reason ? reason : "");
+ free(reason);
+ }
+ if (pset.on_error_stop == PSQL_ERROR_STOP_SHELL || pset.on_error_stop == PSQL_ERROR_STOP_ALL)
+ OK = false;
+ }
restore_sigpipe_trap();
}
else
diff --git a/src/bin/psql/psqlscanslash.l b/src/bin/psql/psqlscanslash.l
index 8449ee1a52..5ea38742ff 100644
--- a/src/bin/psql/psqlscanslash.l
+++ b/src/bin/psql/psqlscanslash.l
@@ -55,6 +55,10 @@ static int backtick_start_offset;
#define LEXRES_OK 1 /* OK completion of backslash argument */
+/* command execution in evaluate_backtick() results in error*/
+#define CMD_ERR -1
+
+
static void evaluate_backtick(PsqlScanState state);
#define ECHO psqlscan_emit(cur_state, yytext, yyleng)
@@ -800,10 +804,23 @@ evaluate_backtick(PsqlScanState state)
} while (!feof(fd));
}
- if (fd && pclose(fd) == -1)
+ if (fd)
{
- pg_log_error("%s: %m", cmd);
- error = true;
+ int pclose_rc = pclose(fd);
+ if (pclose_rc != 0)
+ {
+ if (pclose_rc < 0)
+ pg_log_error("%s: %m", cmd);
+ else
+ {
+ char *reason = wait_result_to_str(pclose_rc);
+
+ pg_log_error("%s: %s", cmd,
+ reason ? reason : "");
+ free(reason);
+ }
+ error = true;
+ }
}
if (PQExpBufferDataBroken(cmd_output))
@@ -825,6 +842,12 @@ evaluate_backtick(PsqlScanState state)
cmd_output.len--;
appendBinaryPQExpBuffer(output_buf, cmd_output.data, cmd_output.len);
}
+ /* If error, set len to a value greater than or equal
+ * to maxlen to indicate error in command execution.
+ * This can help with ON_ERROR_STOP.
+ */
+ else
+ output_buf->len = CMD_ERR;
termPQExpBuffer(&cmd_output);
}
diff --git a/src/bin/psql/settings.h b/src/bin/psql/settings.h
index 73d4b393bc..93acf6d9dd 100644
--- a/src/bin/psql/settings.h
+++ b/src/bin/psql/settings.h
@@ -54,6 +54,14 @@ typedef enum
PSQL_ERROR_ROLLBACK_ON
} PSQL_ERROR_ROLLBACK;
+typedef enum
+{
+ PSQL_ERROR_STOP_OFF,
+ PSQL_ERROR_STOP_ON,
+ PSQL_ERROR_STOP_SHELL,
+ PSQL_ERROR_STOP_ALL
+} PSQL_ERROR_STOP;
+
typedef enum
{
PSQL_COMP_CASE_PRESERVE_UPPER,
@@ -133,7 +141,6 @@ typedef struct _psqlSettings
* functions.
*/
bool autocommit;
- bool on_error_stop;
bool quiet;
bool singleline;
bool singlestep;
@@ -145,6 +152,7 @@ typedef struct _psqlSettings
PSQL_ECHO echo;
PSQL_ECHO_HIDDEN echo_hidden;
PSQL_ERROR_ROLLBACK on_error_rollback;
+ PSQL_ERROR_STOP on_error_stop;
PSQL_COMP_CASE comp_case;
HistControl histcontrol;
const char *prompt1;
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 5a28b6f713..acf197c1a3 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -877,12 +877,6 @@ autocommit_hook(const char *newval)
return ParseVariableBool(newval, "AUTOCOMMIT", &pset.autocommit);
}
-static bool
-on_error_stop_hook(const char *newval)
-{
- return ParseVariableBool(newval, "ON_ERROR_STOP", &pset.on_error_stop);
-}
-
static bool
quiet_hook(const char *newval)
{
@@ -1036,6 +1030,29 @@ on_error_rollback_hook(const char *newval)
return true;
}
+static bool
+on_error_stop_hook(const char *newval)
+{
+ Assert(newval != NULL); /* else substitute hook messed up */
+ if (pg_strcasecmp(newval, "shell") == 0)
+ pset.on_error_stop = PSQL_ERROR_STOP_SHELL;
+ else if (pg_strcasecmp(newval, "all") == 0)
+ pset.on_error_stop = PSQL_ERROR_STOP_ALL;
+ else
+ {
+ bool on_off;
+
+ if (ParseVariableBool(newval, NULL, &on_off))
+ pset.on_error_stop = on_off ? PSQL_ERROR_STOP_ON : PSQL_ERROR_STOP_OFF;
+ else
+ {
+ PsqlVarEnumError("ON_ERROR_STOP", newval, "on, off, shell, all");
+ return false;
+ }
+ }
+ return true;
+}
+
static char *
comp_keyword_case_substitute_hook(char *newval)
{
--
2.39.2
