On Mon, Jan 9, 2023 at 10:01 AM Maxim Orlov <[email protected]> wrote:
> Hi!
>
> In overall, I think we move in the right direction. But we could make code
> better, should we?
>
> + /* 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;
> I think, it's better to add spaces around middle if block. It will be easy
> to read.
> Also, consider, adding spaces around assignment in this block.
>
Noted and will implement.
> + /*
> + snprintf(exit_code_buf, sizeof(exit_code_buf), "%d",
> WEXITSTATUS(exit_code));
> + */
> Probably, this is not needed.
>
Heh. Oops.
> > 1. pg_regress now creates an environment variable called PG_OS_TARGET
> Maybe, we can use env "OS"? I do not know much about Windows, but I think
> this is kind of standard environment variable there.
>
I chose a name that would avoid collisions with anything a user might
potentially throw into their environment, so if the var "OS" is fairly
standard is a reason to avoid using it. Also, going with our own env var
allows us to stay in perfect synchronization with the build's #ifdef WIN32
... and whatever #ifdefs may come in the future for new OSes. If there is
already an environment variable that does that for us, I would rather use
that, but I haven't found it.
The 0001 patch is unchanged from last time (aside from anything rebasing
might have done).
>
From cb2ac7393cf0ce0a7c336dfc749ff91142a88b09 Mon Sep 17 00:00:00 2001
From: coreyhuinker <[email protected]>
Date: Tue, 3 Jan 2023 22:16:20 -0500
Subject: [PATCH v4 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.39.0
From 9d4bec9e046a80f89ce484e2dc4f25c8f1d99e9c Mon Sep 17 00:00:00 2001
From: coreyhuinker <[email protected]>
Date: Mon, 9 Jan 2023 13:25:08 -0500
Subject: [PATCH v4 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 | 38 ++++++++++++++++++++++++++----
src/test/regress/expected/psql.out | 25 ++++++++++++++++++++
src/test/regress/sql/psql.sql | 21 +++++++++++++++++
6 files changed, 112 insertions(+), 5 deletions(-)
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 3f994a3592..711a6d24fd 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4264,6 +4264,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 b5201edf55..5d15c2143b 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -5005,6 +5005,7 @@ static bool
do_shell(const char *command)
{
int result;
+ char exit_code_buf[32];
fflush(NULL);
if (!command)
@@ -5032,6 +5033,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..f6f03bd816 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,27 @@ 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 +848,11 @@ 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", 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.39.0
