On Tue, Jan 3, 2023 at 5:36 AM vignesh C <vignes...@gmail.com> wrote:
> On Wed, 21 Dec 2022 at 11:04, Corey Huinker <corey.huin...@gmail.com>
> wrote:
> >
> > I've rebased and updated the patch to include documentation.
> >
> > Regression tests have been moved to a separate patchfile because error
> messages will vary by OS and configuration, so we probably can't do a
> stable regression test, but having them handy at least demonstrates the
> feature.
> >
> > On Sun, Dec 4, 2022 at 12:35 AM Corey Huinker <corey.huin...@gmail.com>
> wrote:
> >>
> >> Rebased. Still waiting on feedback before working on documentation.
> >>
> >> On Fri, Nov 4, 2022 at 5:23 AM Corey Huinker <corey.huin...@gmail.com>
> wrote:
> >>>
> >>> Oops, that sample output was from a previous run, should have been:
> >>>
> >>> -- SHELL_EXIT_CODE is undefined
> >>> \echo :SHELL_EXIT_CODE
> >>> :SHELL_EXIT_CODE
> >>> -- bad \!
> >>> \! borp
> >>> sh: line 1: borp: command not found
> >>> \echo :SHELL_EXIT_CODE
> >>> 127
> >>> -- bad backtick
> >>> \set var `borp`
> >>> sh: line 1: borp: command not found
> >>> \echo :SHELL_EXIT_CODE
> >>> 127
> >>> -- good \!
> >>> \! true
> >>> \echo :SHELL_EXIT_CODE
> >>> 0
> >>> -- play with exit codes
> >>> \! exit 4
> >>> \echo :SHELL_EXIT_CODE
> >>> 4
> >>> \set var `exit 3`
> >>> \echo :SHELL_EXIT_CODE
> >>> 3
> >>>
> >>>
> >>> On Fri, Nov 4, 2022 at 5:08 AM Corey Huinker <corey.huin...@gmail.com>
> wrote:
> >>>>
> >>>>
> >>>> Over in
> https://www.postgresql.org/message-id/eaf326ad693e74eba068f33a7f518...@oss.nttdata.com
> Justin Pryzby suggested that psql might need the ability to capture the
> shell exit code.
> >>>>
> >>>> This is a POC patch that does that, but doesn't touch on the
> ON_ERROR_STOP stuff.
> >>>>
> >>>> I've added some very rudimentary tests, but haven't touched the
> documentation, because I strongly suspect that someone will suggest a
> better name for the variable.
> >>>>
> >>>> But basically, it works like this
> >>>>
> >>>> -- SHELL_EXIT_CODE is undefined
> >>>> \echo :SHELL_EXIT_CODE
> >>>> :SHELL_EXIT_CODE
> >>>> -- bad \!
> >>>> \! borp
> >>>> sh: line 1: borp: command not found
> >>>> \echo :SHELL_EXIT_CODE
> >>>> 32512
> >>>> -- bad backtick
> >>>> \set var `borp`
> >>>> sh: line 1: borp: command not found
> >>>> \echo :SHELL_EXIT_CODE
> >>>> 127
> >>>> -- good \!
> >>>> \! true
> >>>> \echo :SHELL_EXIT_CODE
> >>>> 0
> >>>> -- play with exit codes
> >>>> \! exit 4
> >>>> \echo :SHELL_EXIT_CODE
> >>>> 1024
> >>>> \set var `exit 3`
> >>>> \echo :SHELL_EXIT_CODE
> >>>> 3
> >>>>
> >>>>
> >>>> Feedback welcome.
>
> CFBot shows some compilation errors as in [1], please post an updated
> version for the same:
> [02:35:49.924] psqlscanslash.l: In function ‘evaluate_backtick’:
> [02:35:49.924] psqlscanslash.l:822:11: error: implicit declaration of
> function ‘WIFSTOPPED’ [-Werror=implicit-function-declaration]
> [02:35:49.924] 822 | exit_code=WSTOPSIG(exit_code);
> [02:35:49.924] | ^~~~~~~~~~
> [02:35:49.924] psqlscanslash.l:823:14: error: implicit declaration of
> function ‘WSTOPSIG’ [-Werror=implicit-function-declaration]
> [02:35:49.924] 823 | }
> [02:35:49.924] | ^
> [02:35:49.924] cc1: all warnings being treated as errors
> [02:35:49.925] make[3]: *** [<builtin>: psqlscanslash.o] Error 1
>
> [1] - https://cirrus-ci.com/task/5424476720988160
>
> Regards,
> Vignesh
>
Thanks. I had left sys/wait.h out of psqlscanslash.
Attached is v3 of this patch, I've made the following changes:
1. pg_regress now creates an environment variable called PG_OS_TARGET,
which regression tests can use to manufacture os-specific commands. For our
purposes, this allows the regression test to manufacture a shell command
that has either "2> /dev/null" or "2> NUL". This seemed the least invasive
way to make this possible. If for some reason it becomes useful in general
psql scripting, then we can consider promoting it to a regular psql var.
2. There are now two psql variables, SHELL_EXIT_CODE, which has the return
code, and SHELL_ERROR, which is a true/false flag based on whether the exit
code was nonzero. These variables are the shell result analogues of
SQLSTATE and ERROR.
From 55d59e886fdddf341d70b158415a13bfe7c9858f Mon Sep 17 00:00:00 2001
From: coreyhuinker <corey.huin...@gmail.com>
Date: Tue, 3 Jan 2023 22:16:20 -0500
Subject: [PATCH 1/2] 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 40e6c231a3..56c59e0b10 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -595,6 +595,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.38.1
From cac8ccfdf0f0489150f9a485823082fe467f030f Mon Sep 17 00:00:00 2001
From: coreyhuinker <corey.huin...@gmail.com>
Date: Wed, 4 Jan 2023 01:55:37 -0500
Subject: [PATCH 2/2] 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 | 8 ++++++
src/bin/psql/help.c | 4 +++
src/bin/psql/psqlscanslash.l | 39 ++++++++++++++++++++++++++----
src/test/regress/expected/psql.out | 25 +++++++++++++++++++
src/test/regress/sql/psql.sql | 21 ++++++++++++++++
6 files changed, 113 insertions(+), 5 deletions(-)
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 8a5285da9a..4ad11eeffb 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4260,6 +4260,27 @@ bar
</listitem>
</varlistentry>
+ <varlistentry>
+ <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>
+ <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>
<term><varname>SHOW_ALL_RESULTS</varname></term>
<listitem>
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 00b89d941b..8e6ed7cbc0 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -4998,6 +4998,7 @@ static bool
do_shell(const char *command)
{
int result;
+ char exit_code_buf[32];
fflush(NULL);
if (!command)
@@ -5025,6 +5026,13 @@ do_shell(const char *command)
else
result = system(command);
+ snprintf(exit_code_buf, sizeof(exit_code_buf), "%d", WEXITSTATUS(result));
+ SetVariable(pset.vars, "SHELL_EXIT_CODE", exit_code_buf);
+ if (result == 0)
+ SetVariable(pset.vars, "SHELL_ERROR", "false");
+ else
+ 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..88931bd677 100644
--- a/src/bin/psql/psqlscanslash.l
+++ b/src/bin/psql/psqlscanslash.l
@@ -23,6 +23,9 @@
#include "fe_utils/conditional.h"
#include "libpq-fe.h"
+#include "settings.h"
+#include "variables.h"
+#include <sys/wait.h>
}
%{
@@ -774,6 +777,8 @@ evaluate_backtick(PsqlScanState state)
bool error = false;
char buf[512];
size_t result;
+ int exit_code = 0;
+ char exit_code_buf[32];
initPQExpBuffer(&cmd_output);
@@ -783,6 +788,7 @@ evaluate_backtick(PsqlScanState state)
{
pg_log_error("%s: %m", cmd);
error = true;
+ exit_code = -1;
}
if (!error)
@@ -790,7 +796,8 @@ evaluate_backtick(PsqlScanState state)
do
{
result = fread(buf, 1, sizeof(buf), fd);
- if (ferror(fd))
+ exit_code = ferror(fd);
+ if (exit_code)
{
pg_log_error("%s: %m", cmd);
error = true;
@@ -800,10 +807,25 @@ evaluate_backtick(PsqlScanState state)
} while (!feof(fd));
}
- if (fd && pclose(fd) == -1)
+ if (fd)
{
- pg_log_error("%s: %m", cmd);
- error = true;
+ int close_exit_code = 0;
+
+ /* Capture exit code for SHELL_EXIT_CODE */
+ close_exit_code = pclose(fd);
+ if (close_exit_code == -1)
+ {
+ pg_log_error("%s: %m", cmd);
+ error = true;
+ }
+ if (WIFEXITED(close_exit_code))
+ exit_code=WEXITSTATUS(close_exit_code);
+ else if(WIFSIGNALED(close_exit_code))
+ exit_code=WTERMSIG(close_exit_code);
+ else if(WIFSTOPPED(close_exit_code))
+ exit_code=WSTOPSIG(close_exit_code);
+ if (exit_code)
+ error = true;
}
if (PQExpBufferDataBroken(cmd_output))
@@ -824,7 +846,14 @@ 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_ERROR", "false");
}
+ else
+ SetVariable(pset.vars, "SHELL_ERROR", "true");
- termPQExpBuffer(&cmd_output);
+ /*
+ snprintf(exit_code_buf, sizeof(exit_code_buf), "%d", WEXITSTATUS(exit_code));
+ */
+ snprintf(exit_code_buf, sizeof(exit_code_buf), "%d", exit_code);
+ SetVariable(pset.vars, "SHELL_EXIT_CODE", exit_code_buf);
}
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index 8fc62cebd2..b2addffa70 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -1306,6 +1306,31 @@ execute q;
+----+-------------+
deallocate q;
+-- test 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 'nosuchcommand 2> NUL'
+\else
+ \set bad_cmd 'nosuchcommand 2> /dev/null'
+\endif
+\set bad_shbang '\\! ' :bad_cmd
+:bad_shbang
+\echo :SHELL_EXIT_CODE
+127
+\echo :SHELL_ERROR
+true
+\set SHELL_EXIT_CODE 'clear'
+\set SHELL_ERROR 'clear'
+\echo :SHELL_EXIT_CODE
+clear
+\echo :SHELL_ERROR
+clear
+\set nosuchvar `:bad_cmd`
+\echo :SHELL_EXIT_CODE
+127
+\echo :SHELL_ERROR
+true
-- 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 2da9665a19..be86ef6984 100644
--- a/src/test/regress/sql/psql.sql
+++ b/src/test/regress/sql/psql.sql
@@ -291,6 +291,27 @@ execute q;
deallocate q;
+-- test 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 'nosuchcommand 2> NUL'
+\else
+ \set bad_cmd 'nosuchcommand 2> /dev/null'
+\endif
+\set bad_shbang '\\! ' :bad_cmd
+
+:bad_shbang
+\echo :SHELL_EXIT_CODE
+\echo :SHELL_ERROR
+\set SHELL_EXIT_CODE 'clear'
+\set SHELL_ERROR 'clear'
+\echo :SHELL_EXIT_CODE
+\echo :SHELL_ERROR
+\set nosuchvar `:bad_cmd`
+\echo :SHELL_EXIT_CODE
+\echo :SHELL_ERROR
+
-- 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.38.1
