On Mon, Mar 20, 2023 at 1:01 PM Tom Lane <t...@sss.pgh.pa.us> wrote:
> Corey Huinker <corey.huin...@gmail.com> writes:
> > 128+N is implemented.
>
> I think this mostly looks OK, but:
>
> * I still say there is no basis whatever for believing that the result
> of ferror() is an exit code, errno code, or anything else with
> significance beyond zero-or-not. Feeding it to wait_result_to_exit_code
> as you've done here is not going to do anything but mislead people in
> a platform-dependent way. Probably should set exit_code to -1 if
> ferror reports trouble.
>
Agreed. Sorry, I read your comment wrong the last time or I would have
already made it so.
>
> * Why do you have wait_result_to_exit_code defaulting to return 0
> if it doesn't recognize the code as either WIFEXITED or WIFSIGNALED?
> That seems pretty misleading; again -1 would be a better idea.
>
That makes sense as well. Given that WIFSIGNALED is currently defined as
the negation of WIFEXITED, whatever default result we have here is
basically a this-should-never-happen. That might be something we want to
catch with an assert, but I'm fine with a -1 for now.
From 1ff2b5ba4b82efca0edf60a9fb1cdf8307471eee Mon Sep 17 00:00:00 2001
From: coreyhuinker <corey.huin...@gmail.com>
Date: Fri, 17 Mar 2023 06:43:24 -0400
Subject: [PATCH v11 1/3] Add PG_OS_TARGET environment variable to enable
OS-specific changes to regression tests.
---
src/test/regress/pg_regress.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 7b23cc80dc..333aaab139 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -594,6 +594,17 @@ initialize_environment(void)
#endif
}
+ /*
+ * Regression tests that invoke OS commands must occasionally use
+ * OS-specific syntax (example: NUL vs /dev/null). This env var allows
+ * those tests to adjust commands accordingly.
+ */
+#if defined(WIN32)
+ setenv("PG_OS_TARGET", "WIN32", 1);
+#else
+ setenv("PG_OS_TARGET", "OTHER", 1);
+#endif
+
/*
* Set translation-related settings to English; otherwise psql will
* produce translated messages and produce diffs. (XXX If we ever support
--
2.39.2
From 3d9c43cfec7f60785c1f1fa1aaa3e1b48224ef98 Mon Sep 17 00:00:00 2001
From: coreyhuinker <corey.huin...@gmail.com>
Date: Mon, 20 Mar 2023 16:05:10 -0400
Subject: [PATCH v11 2/3] Add wait_result_to_exit_code().
---
src/common/wait_error.c | 15 +++++++++++++++
src/include/port.h | 1 +
2 files changed, 16 insertions(+)
diff --git a/src/common/wait_error.c b/src/common/wait_error.c
index 4a3c3c61af..80b94cae51 100644
--- a/src/common/wait_error.c
+++ b/src/common/wait_error.c
@@ -127,3 +127,18 @@ wait_result_is_any_signal(int exit_status, bool include_command_not_found)
return true;
return false;
}
+
+/*
+ * Return the POSIX exit code (0 to 255) that corresponds to the argument.
+ * The argument is a return code returned by wait(2) or waitpid(2), which
+ * also applies to pclose(3) and system(3).
+ */
+int
+wait_result_to_exit_code(int exit_status)
+{
+ if (WIFEXITED(exit_status))
+ return WEXITSTATUS(exit_status);
+ if (WIFSIGNALED(exit_status))
+ return 128 + WTERMSIG(exit_status);
+ return -1;
+}
diff --git a/src/include/port.h b/src/include/port.h
index e66193bed9..1b119a3c56 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -495,6 +495,7 @@ extern char *escape_single_quotes_ascii(const char *src);
extern char *wait_result_to_str(int exitstatus);
extern bool wait_result_is_signal(int exit_status, int signum);
extern bool wait_result_is_any_signal(int exit_status, bool include_command_not_found);
+extern int wait_result_to_exit_code(int exit_status);
/*
* Interfaces that we assume all Unix system have. We retain individual macros
--
2.39.2
From bf65f36fd57977acaac57972425d1717a205cb72 Mon Sep 17 00:00:00 2001
From: coreyhuinker <corey.huin...@gmail.com>
Date: Mon, 20 Mar 2023 16:05:23 -0400
Subject: [PATCH v11 3/3] Add psql variables SHELL_ERROR and SHELL_EXIT_CODE
which report the success or failure of the most recent psql OS-command
executed via the \! command or `backticks`. These variables are roughly
analogous to ERROR and SQLSTATE, but for OS level operations instead of SQL
commands.
---
doc/src/sgml/ref/psql-ref.sgml | 21 ++++++++++++++++++
src/bin/psql/command.c | 15 +++++++++++++
src/bin/psql/help.c | 4 ++++
src/bin/psql/psqlscanslash.l | 33 +++++++++++++++++++++++++----
src/test/regress/expected/psql.out | 34 ++++++++++++++++++++++++++++++
src/test/regress/sql/psql.sql | 30 ++++++++++++++++++++++++++
6 files changed, 133 insertions(+), 4 deletions(-)
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 7b8ae9fac3..e6e307fd18 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4267,6 +4267,27 @@ bar
</listitem>
</varlistentry>
+ <varlistentry id="app-psql-variables-shell-error">
+ <term><varname>SHELL_ERROR</varname></term>
+ <listitem>
+ <para>
+ <literal>true</literal> if the last shell command failed, <literal>false</literal> if
+ it succeeded. This applies to shell commands invoked via either <literal>\!</literal>
+ or <literal>`</literal>. See also <varname>SHELL_EXIT_CODE</varname>.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry id="app-psql-variables-exit-code">
+ <term><varname>SHELL_EXIT_CODE</varname></term>
+ <listitem>
+ <para>
+ The exit code return by the last shell command, invoked via either <literal>\!</literal> or <literal>`</literal>.
+ 0 means no error. See also <varname>SHELL_ERROR</varname>.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="app-psql-variables-show-all-results">
<term><varname>SHOW_ALL_RESULTS</varname></term>
<listitem>
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 61ec049f05..aa44d4f7f5 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -5041,6 +5041,21 @@ do_shell(const char *command)
else
result = system(command);
+ if (result == 0)
+ {
+ SetVariable(pset.vars, "SHELL_EXIT_CODE", "0");
+ SetVariable(pset.vars, "SHELL_ERROR", "false");
+ }
+ else
+ {
+ int exit_code = wait_result_to_exit_code(result);
+ char buf[32];
+
+ snprintf(buf, sizeof(buf), "%d", exit_code);
+ SetVariable(pset.vars, "SHELL_EXIT_CODE", buf);
+ SetVariable(pset.vars, "SHELL_ERROR", "true");
+ }
+
if (result == 127 || result == -1)
{
pg_log_error("\\!: failed");
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index e45c4aaca5..6d5226f793 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -455,6 +455,10 @@ helpVariables(unsigned short int pager)
" show all results of a combined query (\\;) instead of only the last\n");
HELP0(" SHOW_CONTEXT\n"
" controls display of message context fields [never, errors, always]\n");
+ HELP0(" SHELL_ERROR\n"
+ " true if last shell command failed, else false\n");
+ HELP0(" SHELL_EXIT_CODE\n"
+ " Exit code of the last shell command\n");
HELP0(" SINGLELINE\n"
" if set, end of line terminates SQL commands (same as -S option)\n");
HELP0(" SINGLESTEP\n"
diff --git a/src/bin/psql/psqlscanslash.l b/src/bin/psql/psqlscanslash.l
index 8449ee1a52..dc60c29c52 100644
--- a/src/bin/psql/psqlscanslash.l
+++ b/src/bin/psql/psqlscanslash.l
@@ -23,6 +23,7 @@
#include "fe_utils/conditional.h"
#include "libpq-fe.h"
+#include "settings.h"
}
%{
@@ -774,6 +775,7 @@ evaluate_backtick(PsqlScanState state)
bool error = false;
char buf[512];
size_t result;
+ int exit_code = 0;
initPQExpBuffer(&cmd_output);
@@ -783,6 +785,7 @@ evaluate_backtick(PsqlScanState state)
{
pg_log_error("%s: %m", cmd);
error = true;
+ exit_code = -1;
}
if (!error)
@@ -794,16 +797,29 @@ evaluate_backtick(PsqlScanState state)
{
pg_log_error("%s: %m", cmd);
error = true;
+ exit_code = -1;
break;
}
appendBinaryPQExpBuffer(&cmd_output, buf, result);
} while (!feof(fd));
}
- if (fd && pclose(fd) == -1)
+ if (fd)
{
- pg_log_error("%s: %m", cmd);
- error = true;
+ int close_exit_code = pclose(fd);
+
+ /*
+ * Any error reported on pclose() takes precedence over a pre-existing
+ * error from during the run.
+ */
+
+ if (close_exit_code)
+ {
+ error = true;
+ exit_code = close_exit_code;
+ if (close_exit_code == -1)
+ pg_log_error("%s: %m", cmd);
+ }
}
if (PQExpBufferDataBroken(cmd_output))
@@ -824,7 +840,16 @@ evaluate_backtick(PsqlScanState state)
cmd_output.data[cmd_output.len - 1] == '\n')
cmd_output.len--;
appendBinaryPQExpBuffer(output_buf, cmd_output.data, cmd_output.len);
+ SetVariable(pset.vars, "SHELL_EXIT_CODE", "0");
+ SetVariable(pset.vars, "SHELL_ERROR", "false");
}
+ else
+ {
+ char exit_code_buf[32];
- termPQExpBuffer(&cmd_output);
+ snprintf(exit_code_buf, sizeof(exit_code_buf), "%d",
+ wait_result_to_exit_code(exit_code));
+ SetVariable(pset.vars, "SHELL_EXIT_CODE", exit_code_buf);
+ SetVariable(pset.vars, "SHELL_ERROR", "true");
+ }
}
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index c00e28361c..f54a8ac481 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -1306,6 +1306,40 @@ execute q;
+----+-------------+
deallocate q;
+-- test SHELL_ERROR / SHELL_EXIT_CODE
+\getenv pg_os_target PG_OS_TARGET
+SELECT :'pg_os_target' = 'WIN32' AS windows_target \gset
+\if :windows_target
+ \set bad_cmd 'CON 2> NUL'
+ \set expected_exit_code 1
+\else
+ \set bad_cmd '/ 2> /dev/null'
+ \set expected_exit_code 126
+\endif
+\set bad_shbang '\\! ' :bad_cmd
+:bad_shbang
+\echo :SHELL_ERROR
+true
+\pset format unaligned
+SELECT CASE
+ WHEN :'SHELL_EXIT_CODE' = :'expected_exit_code' THEN 'OK'
+ ELSE :'SHELL_EXIT_CODE'
+END AS exit_code_check;
+exit_code_check|OK
+\set SHELL_ERROR 'clear'
+\set SHELL_EXIT_CODE 'clear'
+\echo :SHELL_ERROR
+clear
+\echo :SHELL_EXIT_CODE
+clear
+\set nosuchvar `:bad_cmd`
+\echo :SHELL_ERROR
+true
+SELECT CASE
+ WHEN :'SHELL_EXIT_CODE' = :'expected_exit_code' THEN 'OK'
+ ELSE :'SHELL_EXIT_CODE'
+END AS exit_code_check;
+exit_code_check|OK
-- test single-line header and data
prepare q as select repeat('x',2*n) as "0123456789abcdef", repeat('y',20-2*n) as "0123456789" from generate_series(1,10) as n;
\pset linestyle ascii
diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql
index 961783d6ea..c11739b736 100644
--- a/src/test/regress/sql/psql.sql
+++ b/src/test/regress/sql/psql.sql
@@ -291,6 +291,36 @@ execute q;
deallocate q;
+-- test SHELL_ERROR / SHELL_EXIT_CODE
+\getenv pg_os_target PG_OS_TARGET
+SELECT :'pg_os_target' = 'WIN32' AS windows_target \gset
+\if :windows_target
+ \set bad_cmd 'CON 2> NUL'
+ \set expected_exit_code 1
+\else
+ \set bad_cmd '/ 2> /dev/null'
+ \set expected_exit_code 126
+\endif
+\set bad_shbang '\\! ' :bad_cmd
+
+:bad_shbang
+\echo :SHELL_ERROR
+\pset format unaligned
+SELECT CASE
+ WHEN :'SHELL_EXIT_CODE' = :'expected_exit_code' THEN 'OK'
+ ELSE :'SHELL_EXIT_CODE'
+END AS exit_code_check;
+\set SHELL_ERROR 'clear'
+\set SHELL_EXIT_CODE 'clear'
+\echo :SHELL_ERROR
+\echo :SHELL_EXIT_CODE
+\set nosuchvar `:bad_cmd`
+\echo :SHELL_ERROR
+SELECT CASE
+ WHEN :'SHELL_EXIT_CODE' = :'expected_exit_code' THEN 'OK'
+ ELSE :'SHELL_EXIT_CODE'
+END AS exit_code_check;
+
-- test single-line header and data
prepare q as select repeat('x',2*n) as "0123456789abcdef", repeat('y',20-2*n) as "0123456789" from generate_series(1,10) as n;
--
2.39.2
