Re: Add SHELL_EXIT_CODE to psql
Corey Huinker writes: > This is a follow up patch to apply the committed pattern to the various > piped output commands. Pushed with some changes: * You didn't update the documentation. * I thought this was way too many copies of the same logic. I made a subroutine to set these variables, and changed the original uses too. * You didn't change \w (exec_command_write) to set these variables. I'm assuming that was an oversight; if it was intentional, please explain why. I looked through psql's other uses of pclose() and system(), and found: * pager invocations * backtick evaluation within a prompt * \e (edit query buffer) I think that not changing these variables in those places is a good idea. For instance, if prompt display could change them then they'd never survive long enough to be useful; plus, having the behavior vary depending on your prompt settings seems pretty horrid. In general, these things are mainly useful to scripts, and I doubt that we want their interactive behavior to vary from scripted behavior, so setting them during interactive-only operations seems bad. regards, tom lane
Re: Add SHELL_EXIT_CODE to psql
Corey Huinker writes: > Following up here. This addendum patch clearly isn't as important as > anything currently trying to make it in before the feature freeze, but I > think the lack of setting the SHELL_ERROR and SHELL_EXIT_CODE vars on piped > commands would be viewed as a bug, so I'm hoping somebody can take a look > at it. I changed the CF entry back to Needs Review to remind us we're not done. regards, tom lane
Re: Add SHELL_EXIT_CODE to psql
On Fri, Mar 24, 2023 at 5:21 PM Corey Huinker wrote: > > > On Tue, Mar 21, 2023 at 3:33 PM Corey Huinker > wrote: > >> >>> As you had it, any nonzero result would prevent backtick substitution. >>> I'm not really sure how much thought went into the existing behavior, >>> but I am pretty sure that it's not part of this patch's charter to >>> change that. >>> >>> regards, tom lane >>> >> >> The changes all make sense, thanks! >> > > This is a follow up patch to apply the committed pattern to the various > piped output commands. > > I spotted this oversight in the > https://www.postgresql.org/message-id/CADkLM=dMG6AAWfeKvGnKOzz1O7ZNctFR1BzAA3K7-+XQxff=4...@mail.gmail.com > thread and, whether or not that feature gets in, we should probably apply > it to output pipes as well. > Following up here. This addendum patch clearly isn't as important as anything currently trying to make it in before the feature freeze, but I think the lack of setting the SHELL_ERROR and SHELL_EXIT_CODE vars on piped commands would be viewed as a bug, so I'm hoping somebody can take a look at it.
Re: Add SHELL_EXIT_CODE to psql
On Tue, Mar 21, 2023 at 3:33 PM Corey Huinker
wrote:
>
>> As you had it, any nonzero result would prevent backtick substitution.
>> I'm not really sure how much thought went into the existing behavior,
>> but I am pretty sure that it's not part of this patch's charter to
>> change that.
>>
>> regards, tom lane
>>
>
> The changes all make sense, thanks!
>
This is a follow up patch to apply the committed pattern to the various
piped output commands.
I spotted this oversight in the
https://www.postgresql.org/message-id/CADkLM=dMG6AAWfeKvGnKOzz1O7ZNctFR1BzAA3K7-+XQxff=4...@mail.gmail.com
thread and, whether or not that feature gets in, we should probably apply
it to output pipes as well.
From 9e29989372b7f42fcb7eac6dd15769ebe643abb5 Mon Sep 17 00:00:00 2001
From: coreyhuinker
Date: Fri, 24 Mar 2023 17:20:27 -0400
Subject: [PATCH v1] Have pipes set SHELL_ERROR and SHELL_EXIT_CODE.
---
src/bin/psql/common.c | 20 +---
src/bin/psql/copy.c | 4
2 files changed, 21 insertions(+), 3 deletions(-)
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index f907f5d4e8..3be24250ad 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -103,7 +103,13 @@ setQFout(const char *fname)
if (pset.queryFout && pset.queryFout != stdout && pset.queryFout != stderr)
{
if (pset.queryFoutPipe)
- pclose(pset.queryFout);
+ {
+ char buf[32];
+ int exit_code = pclose(pset.queryFout);
+ snprintf(buf, sizeof(buf), "%d", wait_result_to_exit_code(exit_code));
+ SetVariable(pset.vars, "SHELL_EXIT_CODE", buf);
+ SetVariable(pset.vars, "SHELL_ERROR", (exit_code == 0) ? "false" : "true");
+ }
else
fclose(pset.queryFout);
}
@@ -1652,7 +1658,11 @@ ExecQueryAndProcessResults(const char *query,
{
if (gfile_is_pipe)
{
- pclose(gfile_fout);
+ char buf[32];
+ int exit_code = pclose(gfile_fout);
+ snprintf(buf, sizeof(buf), "%d", wait_result_to_exit_code(exit_code));
+ SetVariable(pset.vars, "SHELL_EXIT_CODE", buf);
+ SetVariable(pset.vars, "SHELL_ERROR", (exit_code == 0) ? "false" : "true");
restore_sigpipe_trap();
}
else
@@ -1870,7 +1880,11 @@ ExecQueryUsingCursor(const char *query, double *elapsed_msec)
/* close \g argument file/pipe */
if (is_pipe)
{
- pclose(fout);
+ char buf[32];
+ int exit_code = pclose(fout);
+ snprintf(buf, sizeof(buf), "%d", wait_result_to_exit_code(exit_code));
+ SetVariable(pset.vars, "SHELL_EXIT_CODE", buf);
+ SetVariable(pset.vars, "SHELL_ERROR", (exit_code == 0) ? "false" : "true");
restore_sigpipe_trap();
}
else
diff --git a/src/bin/psql/copy.c b/src/bin/psql/copy.c
index 110d2d7269..00e01095fe 100644
--- a/src/bin/psql/copy.c
+++ b/src/bin/psql/copy.c
@@ -375,6 +375,7 @@ do_copy(const char *args)
{
if (options->program)
{
+ char buf[32];
int pclose_rc = pclose(copystream);
if (pclose_rc != 0)
@@ -391,6 +392,9 @@ do_copy(const char *args)
}
success = false;
}
+ snprintf(buf, sizeof(buf), "%d", wait_result_to_exit_code(pclose_rc));
+ SetVariable(pset.vars, "SHELL_EXIT_CODE", buf);
+ SetVariable(pset.vars, "SHELL_ERROR", (pclose_rc == 0) ? "false" : "true");
restore_sigpipe_trap();
}
else
--
2.39.2
Re: Add SHELL_EXIT_CODE to psql
> > > As you had it, any nonzero result would prevent backtick substitution. > I'm not really sure how much thought went into the existing behavior, > but I am pretty sure that it's not part of this patch's charter to > change that. > > regards, tom lane > The changes all make sense, thanks!
Re: Add SHELL_EXIT_CODE to psql
Corey Huinker writes:
> On Mon, Mar 20, 2023 at 1:01 PM Tom Lane wrote:
>> * 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.
Good point. So we'd better have it first pass through -1 literally,
else pclose() or system() failure will be reported as something
misleading (probably signal 127?).
Pushed with that change, some cosmetic adjustments, and one significant
logic change in do_backtick: I made it do
if (fd)
{
/*
* Although pclose's result always sets SHELL_EXIT_CODE, we
* historically have abandoned the backtick substitution only if it
* returns -1.
*/
exit_code = pclose(fd);
if (exit_code == -1)
{
pg_log_error("%s: %m", cmd);
error = true;
}
}
As you had it, any nonzero result would prevent backtick substitution.
I'm not really sure how much thought went into the existing behavior,
but I am pretty sure that it's not part of this patch's charter to
change that.
regards, tom lane
Re: Add SHELL_EXIT_CODE to psql
On Mon, Mar 20, 2023 at 1:01 PM Tom Lane wrote:
> Corey Huinker 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
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
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
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
+
+ SHELL_ERROR
+
+
+ true if the last shell command failed, false if
+ it succeeded. This applies to shell
Re: Add SHELL_EXIT_CODE to psql
Corey Huinker 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. * 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. regards, tom lane
Re: Add SHELL_EXIT_CODE to psql
I did a look at the patch, and it seems to be in a good condition. It implements declared functionality with no visible defects. As for test, I think it is possible to implement "signals" case in tap tests. But let the actual commiter decide does it worth it or not. -- Best regards, Maxim Orlov.
Re: Add SHELL_EXIT_CODE to psql
On Fri, Mar 10, 2023 at 3:49 PM Tom Lane wrote:
> I'm okay with adopting bash's rule, but then it should actually match
> bash --- signal N is reported as 128+N, not -N.
>
128+N is implemented.
Nonzero pclose errors of any kind will now overwrite an existing exit_code.
I didn't write a formal test for the signals, but instead tested it
manually:
[310444:16:54:32 EDT] corey=# \! sleep 1000
-- in another window, i found the pid of the sleep command and did a kill
-9 on it
[310444:16:55:15 EDT] corey=# \echo :SHELL_EXIT_CODE
137
137 = 128+9, so that checks out.
The OS-specific regression test has been modified. On Windows systems it
attempts to execute "CON", and on other systems it attempts to execute "/".
These are marginally better than "nosuchcommand" in that they should always
exist on their host OS and could never be a legit executable. I have no
opinion whether the test should live on past the development phase, but if
it does not then the 0001 patch is not needed.
From a5dddedcc7bf654b4b65c14ff7b89b9d83bb5c27 Mon Sep 17 00:00:00 2001
From: coreyhuinker
Date: Fri, 17 Mar 2023 06:43:24 -0400
Subject: [PATCH v9 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 fe578661eef7dbe33aec8f4eaa6dca80a1304c9b Mon Sep 17 00:00:00 2001
From: coreyhuinker
Date: Fri, 17 Mar 2023 06:44:43 -0400
Subject: [PATCH v9 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..8e1c0e3fc8 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 0;
+}
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 251863ddcbc4eecf4fa15e332724acbd3c652478 Mon Sep 17 00:00:00 2001
From: coreyhuinker
Date: Fri, 17 Mar 2023 16:46:57 -0400
Subject: [PATCH v9 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 | 35 +-
src/test/regress/expected/psql.out | 34 +
src/test/regress/sql/psql.sql | 30 +
6 files changed, 134 insertions(+), 5 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
+
+ SHELL_ERROR
+
+
+ true if the last shell command failed, false if
+ it succeeded. This applies to shell commands invoked via either \!
+ or `. See also SHELL_EXIT_CODE.
+
+
+
+
+
+
Re: Add SHELL_EXIT_CODE to psql
Justin Pryzby writes: > The exit status is one byte. I think you should define the status > variable along the lines of: > - 0 if successful; or, > - a positive number 1..255 indicating its exit status. or, > - a negative number N indicating it was terminated by signal -N; or, > - 256 if an internal error occurred (like pclose/ferror); > See bash(1). This would be a good behavior to start with, since it > ought to be familiar to everyone, and if it's good enough to write/run > shell scripts in, then it's got to be good enough for psql to run a > single command in. I'm okay with adopting bash's rule, but then it should actually match bash --- signal N is reported as 128+N, not -N. Not sure what to do about pclose/ferror cases. Maybe use -1? > I'm not sure why the shell uses 126-127 specially, though.. 127 is used similarly by system(3). I think both 126 and 127 might be specified by POSIX, but not sure. In any case, those are outside our jurisdiction. regards, tom lane
Re: Add SHELL_EXIT_CODE to psql
On Wed, Feb 22, 2023 at 03:04:33PM -0500, Corey Huinker wrote:
> +
> +/*
> + * 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 WTERMSIG(exit_status);
> + return 0;
> +}
This fails to distinguish between exiting with (say) code 1 and being
killed by signal 1.
> - if (ferror(fd))
> + exit_code = ferror(fd);
> + if (exit_code)
And this adds even more ambiguity with internal library/system calls, as
Tom said.
> + if (close_exit_code && !exit_code)
> + {
> + error = true;
> + exit_code = close_exit_code;
> + if (close_exit_code == -1)
> + pg_log_error("%s: %m", cmd);
I think if an error ocurrs in pclose(), then it should *always* be
reported. Knowing that we somehow failed while running the command is
more important than knowing how the command ran when we had a failure
while running it.
Note that for some tools, a nonzero exit code can be normal. Like diff
and grep.
The exit status is one byte. I think you should define the status
variable along the lines of:
- 0 if successful; or,
- a positive number 1..255 indicating its exit status. or,
- a negative number N indicating it was terminated by signal -N; or,
- 256 if an internal error occurred (like pclose/ferror);
See bash(1). This would be a good behavior to start with, since it
ought to be familiar to everyone, and if it's good enough to write/run
shell scripts in, then it's got to be good enough for psql to run a
single command in. I'm not sure why the shell uses 126-127 specially,
though..
EXIT STATUS
The exit status of an executed command is the value returned by the
waitpid system call or equivalent function. Exit statuses
fall between 0 and 255, though, as explained below, the shell may
use values above 125 specially. Exit statuses from shell
builtins and compound commands are also limited to this range. Under
certain circumstances, the shell will use special values to
indicate specific failure modes.
For the shell's purposes, a command which exits with a zero exit status
has succeeded. An exit status of zero indicates success.
A non-zero exit status indicates failure. When a command terminates on
a fatal signal N, bash uses the value of 128+N as the exit
status.
If a command is not found, the child process created to execute it
returns a status of 127. If a command is found but is not exe‐
cutable, the return status is 126.
If a command fails because of an error during expansion or redirection,
the exit status is greater than zero.
--
Justin
Re: Add SHELL_EXIT_CODE to psql
On Thu, Mar 2, 2023 at 1:35 PM Tom Lane wrote: > Corey Huinker writes: > > [ v9-0003-Add-psql-variables-SHELL_ERROR-and-SHELL_EXIT_COD.patch ] > > I took a brief look through this. I'm on board with the general > concept, but I think you've spent too much time on an ultimately > futile attempt to get a committable test case, and not enough time > I didn't want to give the impression that I was avoiding/dismissing the test case, and at some point I got curious how far we could push pg_regress. > on making the behavior correct/usable. In particular, it bothers > me that there doesn't appear to be any way to distinguish whether > a command failed on nonzero exit code versus a signal. Also, > That's an intriguing distinction, and one I hadn't considered, mostly because I assumed that any command with a set of exit codes rich enough to warrant inspection would have a separate exit code for i-was-terminated. It would certainly be possible to have two independent booleans, SHELL_ERROR and SHELL_SIGNAL. > I see that you're willing to report whatever ferror returns > (something quite unspecified in relevant standards, beyond > zero/nonzero) as an equally-non-distinguishable integer. > I had imagined users of this feature falling into two camps: 1. Users writing scripts for their specific environment, with the ability to know what exit codes are possible and different desired courses of action based on those codes. 2. Users in no specific environment, content with SHELL_ERROR boolean, who are probably just going to test for failure, and if so either \set a default or \echo a message and \quit. > > I'm tempted to suggest that we ought to be returning a string, > along the lines of "OK" or "exit code N" or "signal N" or > "I/O error". This though brings up the question of exactly > what you expect scripts to use the variable for. Maybe such > a definition would be unhelpful, but if so why? Maybe we > should content ourselves with adding the SHELL_ERROR boolean, and > leave the details to whatever we print in the error message? > Having a curated list of responses like "OK" and "exit code N" is also intriguing, but could be hard to unpack given psql's limited string manipulation capabilities. I think the SHELL_ERROR boolean solves most cases, but it was suggested in https://www.postgresql.org/message-id/20221102115801.gg16...@telsasoft.com that users might want more detail than that, even if that detail is unspecified and highly system dependent. > > As for the test case, I'm unimpressed with it because it's so > weak as to be nearly useless; plus it seems like a potential > security issue (what if "nosuchcommand" exists?). It's fine > to have it during development, I guess, but I'm inclined to > leave it out of the eventual commit. > > I have no objection to leaving it out. I think it proves that writing os-specific pg_regress tests is hard, and little else.
Re: Add SHELL_EXIT_CODE to psql
Corey Huinker writes: > [ v9-0003-Add-psql-variables-SHELL_ERROR-and-SHELL_EXIT_COD.patch ] I took a brief look through this. I'm on board with the general concept, but I think you've spent too much time on an ultimately futile attempt to get a committable test case, and not enough time on making the behavior correct/usable. In particular, it bothers me that there doesn't appear to be any way to distinguish whether a command failed on nonzero exit code versus a signal. Also, I see that you're willing to report whatever ferror returns (something quite unspecified in relevant standards, beyond zero/nonzero) as an equally-non-distinguishable integer. I'm tempted to suggest that we ought to be returning a string, along the lines of "OK" or "exit code N" or "signal N" or "I/O error". This though brings up the question of exactly what you expect scripts to use the variable for. Maybe such a definition would be unhelpful, but if so why? Maybe we should content ourselves with adding the SHELL_ERROR boolean, and leave the details to whatever we print in the error message? As for the test case, I'm unimpressed with it because it's so weak as to be nearly useless; plus it seems like a potential security issue (what if "nosuchcommand" exists?). It's fine to have it during development, I guess, but I'm inclined to leave it out of the eventual commit. regards, tom lane
Re: Add SHELL_EXIT_CODE to psql
On Wed, Feb 22, 2023 at 4:18 PM Thomas Munro wrote: > On Tue, Jan 31, 2023 at 9:23 AM Corey Huinker > wrote: > >> Unfortunately, there is a fail in FreeBSD > https://cirrus-ci.com/task/6466749487382528 > >> > >> Maybe, this patch is need to be rebased? > >> > > > > That failure is in postgres_fdw, which this code doesn't touch. > > > > I'm not able to get to > /tmp/cirrus-ci-build/build/testrun/postgres_fdw-running/regress/regression.out > - It's not in either of the browseable zip files and the 2nd zip file isn't > downloadable, so it's hard to see what's going on. > > Yeah, that'd be our current top flapping CI test[1]. These new > "*-running" tests (like the old "make installcheck") are only enabled > in the FreeBSD CI task currently, so that's why the failures only show > up there. I see[2] half a dozen failures of the "fdw_retry_check" > thing in the past ~24 hours. > > [1] > https://www.postgresql.org/message-id/flat/20221209001511.n3vqodxobqgscuil%40awork3.anarazel.de > [2] http://cfbot.cputube.org/highlights/regress.html Thanks for the explanation. I was baffled.
Re: Add SHELL_EXIT_CODE to psql
On Tue, Jan 31, 2023 at 9:23 AM Corey Huinker wrote: >> Unfortunately, there is a fail in FreeBSD >> https://cirrus-ci.com/task/6466749487382528 >> >> Maybe, this patch is need to be rebased? >> > > That failure is in postgres_fdw, which this code doesn't touch. > > I'm not able to get to > /tmp/cirrus-ci-build/build/testrun/postgres_fdw-running/regress/regression.out > - It's not in either of the browseable zip files and the 2nd zip file isn't > downloadable, so it's hard to see what's going on. Yeah, that'd be our current top flapping CI test[1]. These new "*-running" tests (like the old "make installcheck") are only enabled in the FreeBSD CI task currently, so that's why the failures only show up there. I see[2] half a dozen failures of the "fdw_retry_check" thing in the past ~24 hours. [1] https://www.postgresql.org/message-id/flat/20221209001511.n3vqodxobqgscuil%40awork3.anarazel.de [2] http://cfbot.cputube.org/highlights/regress.html
Re: Add SHELL_EXIT_CODE to psql
>
>
>
> The patch set seem to be in a good shape and pretty stable for quite a
> while.
> Could you add one more minor improvement, a new line after variables
> declaration?
>
As you wish. Attached.
>
> After this changes, I think, we make this patch RfC, shall we?
>
>
Fingers crossed.
From bb55e0fd1cd2de6fa25b7fc738dd6482d0c008c4 Mon Sep 17 00:00:00 2001
From: coreyhuinker
Date: Wed, 11 Jan 2023 17:27:23 -0500
Subject: [PATCH v9 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 6cd5998b9d..bf0ada4560 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 fe954f883f4ee65cbfe5dc2c20f012465d031ada Mon Sep 17 00:00:00 2001
From: coreyhuinker
Date: Thu, 12 Jan 2023 23:04:31 -0500
Subject: [PATCH v9 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..1eb7ff3fa2 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 WTERMSIG(exit_status);
+ return 0;
+}
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 0f91719c26b6201f4aaa436bd13141ba325e2f85 Mon Sep 17 00:00:00 2001
From: coreyhuinker
Date: Wed, 22 Feb 2023 14:55:34 -0500
Subject: [PATCH v9 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 | 34 +-
src/test/regress/expected/psql.out | 29 +
src/test/regress/sql/psql.sql | 25 ++
6 files changed, 123 insertions(+), 5 deletions(-)
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index dc6528dc11..3d8a7353b3 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4264,6 +4264,27 @@ bar
+
+ SHELL_ERROR
+
+
+ true if the last shell command failed, false if
+ it succeeded. This applies to shell commands invoked via either \!
+ or `. See also SHELL_EXIT_CODE.
+
+
+
+
+
+ SHELL_EXIT_CODE
+
+
+ The exit code return by the last shell command, invoked via either \! or `.
+ 0 means no error. See also SHELL_ERROR.
+
+
+
+
SHOW_ALL_RESULTS
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 955397ee9d..40ba2810ea 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -5032,6 +5032,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];
+
+
Re: Add SHELL_EXIT_CODE to psql
On Mon, 30 Jan 2023 at 23:23, Corey Huinker wrote: > > > I rebased, but there are no code differences. > The patch set seem to be in a good shape and pretty stable for quite a while. Could you add one more minor improvement, a new line after variables declaration? + int exit_code = wait_result_to_exit_code(result); + charbuf[32]; ...here + snprintf(buf, sizeof(buf), "%d", exit_code); + SetVariable(pset.vars, "SHELL_EXIT_CODE", buf); + SetVariable(pset.vars, "SHELL_ERROR", "true"); + charexit_code_buf[32]; ... and here + 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"); After this changes, I think, we make this patch RfC, shall we? -- Best regards, Maxim Orlov.
Re: Add SHELL_EXIT_CODE to psql
>
>
> Unfortunately, there is a fail in FreeBSD
> https://cirrus-ci.com/task/6466749487382528
>
> Maybe, this patch is need to be rebased?
>
>
That failure is in postgres_fdw, which this code doesn't touch.
I'm not able to get
to
/tmp/cirrus-ci-build/build/testrun/postgres_fdw-running/regress/regression.out
- It's not in either of the browseable zip files and the 2nd zip file isn't
downloadable, so it's hard to see what's going on.
I rebased, but there are no code differences.
From e99c7b8aa3b81725773c03583bcff50a88a58a38 Mon Sep 17 00:00:00 2001
From: coreyhuinker
Date: Wed, 11 Jan 2023 17:27:23 -0500
Subject: [PATCH v8 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 6cd5998b9d..bf0ada4560 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.1
From 496a84ccf758ed34d2c9bff0e9b458bef5fa9e58 Mon Sep 17 00:00:00 2001
From: coreyhuinker
Date: Thu, 12 Jan 2023 23:04:31 -0500
Subject: [PATCH v8 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..1eb7ff3fa2 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 WTERMSIG(exit_status);
+ return 0;
+}
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.1
From e3dc0f2117b52f9661a75e79097741ed6c0f3b20 Mon Sep 17 00:00:00 2001
From: coreyhuinker
Date: Mon, 23 Jan 2023 16:46:16 -0500
Subject: [PATCH v8 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 | 14
src/bin/psql/help.c| 4
src/bin/psql/psqlscanslash.l | 35 +-
src/test/regress/expected/psql.out | 29 +
src/test/regress/sql/psql.sql | 25 +
6 files changed, 122 insertions(+), 6 deletions(-)
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index dc6528dc11..3d8a7353b3 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4264,6 +4264,27 @@ bar
+
+ SHELL_ERROR
+
+
+ true if the last shell command failed, false if
+ it succeeded. This applies to shell commands invoked via either \!
+ or `. See also SHELL_EXIT_CODE.
+
+
+
+
+
+ SHELL_EXIT_CODE
+
+
+ The exit code return by the last shell command, invoked via either \! or `.
+ 0 means no error. See also SHELL_ERROR.
+
+
+
+
SHOW_ALL_RESULTS
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index b5201edf55..ea79d4fe57 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -5032,6 +5032,20 @@ do_shell(const char *command)
else
result = system(command);
+ if (result == 0)
+ {
+
Re: Add SHELL_EXIT_CODE to psql
Unfortunately, there is a fail in FreeBSD https://cirrus-ci.com/task/6466749487382528 Maybe, this patch is need to be rebased? -- Best regards, Maxim Orlov.
Re: Add SHELL_EXIT_CODE to psql
>
> Thanks! But CF bot still not happy. I think, we should address issues from
> here https://cirrus-ci.com/task/5391002618298368
>
Sure enough, exit codes are shell dependent...adjusted the tests to reflect
that.
From 237b892e5efe739bc8e75d4af30140520d445491 Mon Sep 17 00:00:00 2001
From: coreyhuinker
Date: Wed, 11 Jan 2023 17:27:23 -0500
Subject: [PATCH v7 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 6cd5998b9d..bf0ada4560 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.0
From 01b5d159e3bae3506b74eb22ec066601454fa442 Mon Sep 17 00:00:00 2001
From: coreyhuinker
Date: Thu, 12 Jan 2023 23:04:31 -0500
Subject: [PATCH v7 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..1eb7ff3fa2 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 WTERMSIG(exit_status);
+ return 0;
+}
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.0
From bc936340ba99f9b7d2bbdb36a24f2fcb531d720c Mon Sep 17 00:00:00 2001
From: coreyhuinker
Date: Mon, 23 Jan 2023 16:46:16 -0500
Subject: [PATCH v7 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 | 14
src/bin/psql/help.c| 4
src/bin/psql/psqlscanslash.l | 35 +-
src/test/regress/expected/psql.out | 29 +
src/test/regress/sql/psql.sql | 25 +
6 files changed, 122 insertions(+), 6 deletions(-)
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index dc6528dc11..3d8a7353b3 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4264,6 +4264,27 @@ bar
+
+ SHELL_ERROR
+
+
+ true if the last shell command failed, false if
+ it succeeded. This applies to shell commands invoked via either \!
+ or `. See also SHELL_EXIT_CODE.
+
+
+
+
+
+ SHELL_EXIT_CODE
+
+
+ The exit code return by the last shell command, invoked via either \! or `.
+ 0 means no error. See also SHELL_ERROR.
+
+
+
+
SHOW_ALL_RESULTS
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index b5201edf55..ea79d4fe57 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -5032,6 +5032,20 @@ 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,
Re: Add SHELL_EXIT_CODE to psql
On Mon, Jan 23, 2023 at 2:53 PM Robert Haas wrote: > On Mon, Jan 23, 2023 at 1:59 PM Corey Huinker > wrote: > > SHELL_ERROR is helpful in that it is a ready-made boolean that works for > \if tests in the same way that ERROR is set to true any time SQLSTATE is > nonzero. We don't yet have inline expressions for \if so the ready-made > boolean is a convenience. > > Oh, that seems a bit sad, but I guess it makes sense. > I agree, but there hasn't been much appetite for deciding what expressions would look like, or how we'd implement it. My instinct would be to not create our own expression engine, but instead integrate one that is already available. For my needs, the Unix `expr` command would be ideal (compares numbers and strings, can do regexes, can do complex expressions), but it's not cross platform.
Re: Add SHELL_EXIT_CODE to psql
On Mon, Jan 23, 2023 at 1:59 PM Corey Huinker wrote: > SHELL_ERROR is helpful in that it is a ready-made boolean that works for \if > tests in the same way that ERROR is set to true any time SQLSTATE is nonzero. > We don't yet have inline expressions for \if so the ready-made boolean is a > convenience. Oh, that seems a bit sad, but I guess it makes sense. > Or are you suggesting that I should have just set ERROR itself rather than > create SHELL_ERROR? No, I wasn't suggesting that. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add SHELL_EXIT_CODE to psql
On Fri, Jan 20, 2023 at 8:54 AM Robert Haas wrote: > On Wed, Jan 4, 2023 at 2:09 AM Corey Huinker > wrote: > > 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. > > Seems redundant. > SHELL_ERROR is helpful in that it is a ready-made boolean that works for \if tests in the same way that ERROR is set to true any time SQLSTATE is nonzero. We don't yet have inline expressions for \if so the ready-made boolean is a convenience. Or are you suggesting that I should have just set ERROR itself rather than create SHELL_ERROR?
Re: Add SHELL_EXIT_CODE to psql
On Wed, Jan 4, 2023 at 2:09 AM Corey Huinker wrote: > 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. Seems redundant. -- Robert Haas EDB: http://www.enterprisedb.com
Re: Add SHELL_EXIT_CODE to psql
On Fri, 13 Jan 2023 at 07:50, Corey Huinker wrote: > > I named it wait_result_to_exit_code(), but I welcome suggestions of a > better name. > Thanks! But CF bot still not happy. I think, we should address issues from here https://cirrus-ci.com/task/5391002618298368 -- Best regards, Maxim Orlov.
Re: Add SHELL_EXIT_CODE to psql
>
> I belive, we need proper includes.
>
Given that wait_error.c already seems to have the right includes worked out
for WEXITSTATUS/WIFSTOPPED/etc, I decided to just add a function there.
I named it wait_result_to_exit_code(), but I welcome suggestions of a
better name.
From 9e2827a6f955e7cebf87ca538fab113a359951b4 Mon Sep 17 00:00:00 2001
From: coreyhuinker
Date: Wed, 11 Jan 2023 17:27:23 -0500
Subject: [PATCH v6 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 6cd5998b9d..bf0ada4560 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.0
From 2c73156874bd0c222f414a2988271c4870d4293b Mon Sep 17 00:00:00 2001
From: coreyhuinker
Date: Thu, 12 Jan 2023 23:04:31 -0500
Subject: [PATCH v6 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..1eb7ff3fa2 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 WTERMSIG(exit_status);
+ return 0;
+}
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.0
From 9a969b50eec614cc181f020e25bd8dd00e7802f8 Mon Sep 17 00:00:00 2001
From: coreyhuinker
Date: Thu, 12 Jan 2023 23:28:14 -0500
Subject: [PATCH v6 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 | 14
src/bin/psql/help.c| 4
src/bin/psql/psqlscanslash.l | 35 +-
src/test/regress/expected/psql.out | 25 +
src/test/regress/sql/psql.sql | 21 ++
6 files changed, 114 insertions(+), 6 deletions(-)
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index dc6528dc11..3d8a7353b3 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4264,6 +4264,27 @@ bar
+
+ SHELL_ERROR
+
+
+ true if the last shell command failed, false if
+ it succeeded. This applies to shell commands invoked via either \!
+ or `. See also SHELL_EXIT_CODE.
+
+
+
+
+
+ SHELL_EXIT_CODE
+
+
+ The exit code return by the last shell command, invoked via either \! or `.
+ 0 means no error. See also SHELL_ERROR.
+
+
+
+
SHOW_ALL_RESULTS
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index b5201edf55..ea79d4fe57 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -5032,6 +5032,20 @@ 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),
Re: Add SHELL_EXIT_CODE to psql
Unfortunately, cirrus-ci still not happy
https://cirrus-ci.com/task/6502730475241472
[23:14:34.206] time make -s -j${BUILD_JOBS} world-bin
[23:14:43.174] psqlscanslash.l: In function ‘evaluate_backtick’:
[23:14:43.174] psqlscanslash.l:827:11: error: implicit declaration of
function ‘WIFSTOPPED’ [-Werror=implicit-function-declaration]
[23:14:43.174] 827 | exit_code = WSTOPSIG(close_exit_code);
[23:14:43.174] | ^~
[23:14:43.174] psqlscanslash.l:828:16: error: implicit declaration of
function ‘WSTOPSIG’ [-Werror=implicit-function-declaration]
[23:14:43.174] 828 |
[23:14:43.174] | ^
[23:14:43.174] cc1: all warnings being treated as errors
>
and on FreeBSD
[23:13:03.914] cc -o ...
[23:13:03.914] ld: error: undefined symbol: WEXITSTATUS
[23:13:03.914] >>> referenced by command.c:5036
(../src/bin/psql/command.c:5036)
[23:13:03.914] >>>
src/bin/psql/psql.p/command.c.o:(exec_command_shell_escape)
[23:13:03.914] cc: error: linker command failed with exit code 1 (use -v to
see invocation)
and on Windows
[23:19:51.870] meson-generated_.._psqlscanslash.c.obj : error LNK2019:
unresolved external symbol WIFSTOPPED referenced in function
evaluate_backtick
[23:19:52.197] meson-generated_.._psqlscanslash.c.obj : error LNK2019:
unresolved external symbol WSTOPSIG referenced in function evaluate_backtick
[23:19:52.197] src\bin\psql\psql.exe : fatal error LNK1120: 2 unresolved
externals
I belive, we need proper includes.
--
Best regards,
Maxim Orlov.
Re: Add SHELL_EXIT_CODE to psql
>
>
>
> The patch does not apply on top of HEAD as in [1], please post a rebased
> patch:
>
>
Conflict was due to the doc patch applying id tags to psql variable names.
I've rebased and added my own id tags to the two new variables.
From 9e2827a6f955e7cebf87ca538fab113a359951b4 Mon Sep 17 00:00:00 2001
From: coreyhuinker
Date: Wed, 11 Jan 2023 17:27:23 -0500
Subject: [PATCH v5 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 6cd5998b9d..bf0ada4560 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.0
From 4ed7ef0a21a541277250641c63a7bceea4c1ef62 Mon Sep 17 00:00:00 2001
From: coreyhuinker
Date: Wed, 11 Jan 2023 17:28:17 -0500
Subject: [PATCH v5 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 dc6528dc11..3d8a7353b3 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4264,6 +4264,27 @@ bar
+
+ SHELL_ERROR
+
+
+ true if the last shell command failed, false if
+ it succeeded. This applies to shell commands invoked via either \!
+ or `. See also SHELL_EXIT_CODE.
+
+
+
+
+
+ SHELL_EXIT_CODE
+
+
+ The exit code return by the last shell command, invoked via either \! or `.
+ 0 means no error. See also SHELL_ERROR.
+
+
+
+
SHOW_ALL_RESULTS
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
}
%{
@@ -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(_output);
@@ -783,6 +788,7 @@ evaluate_backtick(PsqlScanState state)
{
pg_log_error("%s: %m", cmd);
error = true;
+ exit_code = -1;
}
if (!error)
@@ -790,7
Re: Add SHELL_EXIT_CODE to psql
On Tue, 10 Jan 2023 at 00:06, Corey Huinker wrote:
>
> On Mon, Jan 9, 2023 at 10:01 AM Maxim Orlov 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).
The patch does not apply on top of HEAD as in [1], please post a rebased patch:
=== Applying patches on top of PostgreSQL commit ID
3c6fc58209f24b959ee18f5d19ef96403d08f15c ===
=== applying patch
./v4-0002-Add-psql-variables-SHELL_ERROR-and-SHELL_EXIT_COD.patch
patching file doc/src/sgml/ref/psql-ref.sgml
Hunk #1 FAILED at 4264.
1 out of 1 hunk FAILED -- saving rejects to file
doc/src/sgml/ref/psql-ref.sgml.rej
[1] - http://cfbot.cputube.org/patch_41_4073.log
Regards,
Vignesh
Re: Add SHELL_EXIT_CODE to psql
On Tue, Jan 10, 2023 at 3:54 AM Maxim Orlov wrote: > > > On Mon, 9 Jan 2023 at 21:36, Corey Huinker > wrote: > >> >> 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. >> >> Perhaps, I didn't make myself clear. Your solution is perfectly adapted > to our needs. > But all Windows since 2000 already have an environment variable > OS=Windows_NT. So, if env OS is defined and equal Windows_NT, this had to > be Windows. > May we use it in our case? I don't insist, just asking. > Ah, that makes more sense. I don't have a strong opinion on which we should use, and I would probably defer to someone who does windows (and possibly cygwin) builds more often than I do.
Re: Add SHELL_EXIT_CODE to psql
On Mon, 9 Jan 2023 at 21:36, Corey Huinker wrote: > > 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. > > Perhaps, I didn't make myself clear. Your solution is perfectly adapted to our needs. But all Windows since 2000 already have an environment variable OS=Windows_NT. So, if env OS is defined and equal Windows_NT, this had to be Windows. May we use it in our case? I don't insist, just asking. -- Best regards, Maxim Orlov.
Re: Add SHELL_EXIT_CODE to psql
On Mon, Jan 9, 2023 at 10:01 AM Maxim Orlov 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
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
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
+
+ SHELL_ERROR
+
+
+ true if the last shell command failed, false if
+ it succeeded. This applies to shell commands invoked via either \!
+ or `. See also SHELL_EXIT_CODE.
+
+
+
+
+
+ SHELL_EXIT_CODE
+
+
+ The exit code return by the last shell command, invoked via either \! or `.
+ 0 means no error. See also SHELL_ERROR.
+
+
+
+
SHOW_ALL_RESULTS
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)
Re: Add SHELL_EXIT_CODE to psql
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.
+ /*
+ snprintf(exit_code_buf, sizeof(exit_code_buf), "%d",
WEXITSTATUS(exit_code));
+ */
Probably, this is not needed.
> 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.
--
Best regards,
Maxim Orlov.
Re: Add SHELL_EXIT_CODE to psql
On Tue, Jan 3, 2023 at 5:36 AM vignesh C wrote:
> On Wed, 21 Dec 2022 at 11:04, Corey Huinker
> 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
> wrote:
> >>
> >> Rebased. Still waiting on feedback before working on documentation.
> >>
> >> On Fri, Nov 4, 2022 at 5:23 AM Corey Huinker
> 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
> 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]: *** [: 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
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
Re: Add SHELL_EXIT_CODE to psql
On Wed, 21 Dec 2022 at 11:04, Corey Huinker 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 wrote: >> >> Rebased. Still waiting on feedback before working on documentation. >> >> On Fri, Nov 4, 2022 at 5:23 AM Corey Huinker 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 >>> 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]: *** [: psqlscanslash.o] Error 1 [1] - https://cirrus-ci.com/task/5424476720988160 Regards, Vignesh
Re: Add SHELL_EXIT_CODE to psql
On Sat, Dec 31, 2022 at 5:28 PM Isaac Morland wrote: > On Sat, 31 Dec 2022 at 16:47, Corey Huinker > wrote: > >> >>> I wonder if there is value in setting up a psql on/off var >>> SHELL_ERROR_OUTPUT construct that when set to "off/false" >>> suppresses standard error via appending "2> /dev/null" (or "2> nul" if >>> #ifdef WIN32). At the very least, it would allow for tests like this to be >>> done with standard regression scripts. >>> >> >> Thinking on this some more a few ideas came up: >> >> 1. The SHELL_ERROR_OUTPUT var above. This is good for simple situations, >> but it would fail if the user took it upon themselves to redirect output, >> and suddenly "myprog 2> /dev/null" works fine on its own but will fail when >> we append our own "2> /dev/null" to it. >> > > Rather than attempting to append redirection directives to the command, > would it work to redirect stderr before invoking the shell? This seems to > me to be more reliable and it should allow an explicit redirection in the > shell command to still work. The difference between Windows and Unix then > becomes the details of what system calls we use to accomplish the > redirection (or maybe none, if an existing abstraction layer takes care of > that - I'm not really up on Postgres internals enough to know), rather than > what we append to the provided command. > > Inside psql, it's a call to the system() function which takes a single string. All output, stdout or stderr, is printed. So for the regression test we have to compose a command that is OS appropriate AND suppresses stderr.
Re: Add SHELL_EXIT_CODE to psql
On Sat, 31 Dec 2022 at 16:47, Corey Huinker wrote: > >> I wonder if there is value in setting up a psql on/off var >> SHELL_ERROR_OUTPUT construct that when set to "off/false" >> suppresses standard error via appending "2> /dev/null" (or "2> nul" if >> #ifdef WIN32). At the very least, it would allow for tests like this to be >> done with standard regression scripts. >> > > Thinking on this some more a few ideas came up: > > 1. The SHELL_ERROR_OUTPUT var above. This is good for simple situations, > but it would fail if the user took it upon themselves to redirect output, > and suddenly "myprog 2> /dev/null" works fine on its own but will fail when > we append our own "2> /dev/null" to it. > Rather than attempting to append redirection directives to the command, would it work to redirect stderr before invoking the shell? This seems to me to be more reliable and it should allow an explicit redirection in the shell command to still work. The difference between Windows and Unix then becomes the details of what system calls we use to accomplish the redirection (or maybe none, if an existing abstraction layer takes care of that - I'm not really up on Postgres internals enough to know), rather than what we append to the provided command.
Re: Add SHELL_EXIT_CODE to psql
On Fri, Dec 30, 2022 at 2:17 PM Corey Huinker wrote: > On Wed, Dec 28, 2022 at 5:59 AM Maxim Orlov wrote: > >> Hi! >> >> The patch is implementing what is declared to do. Shell return code is >> now accessible is psql var. >> Overall code is in a good condition. Applies with no errors on master. >> Unfortunately, regression tests are failing on the macOS due to the >> different shell output. >> > > That was to be expected. > > I wonder if there is value in setting up a psql on/off var > SHELL_ERROR_OUTPUT construct that when set to "off/false" > suppresses standard error via appending "2> /dev/null" (or "2> nul" if > #ifdef WIN32). At the very least, it would allow for tests like this to be > done with standard regression scripts. > Thinking on this some more a few ideas came up: 1. The SHELL_ERROR_OUTPUT var above. This is good for simple situations, but it would fail if the user took it upon themselves to redirect output, and suddenly "myprog 2> /dev/null" works fine on its own but will fail when we append our own "2> /dev/null" to it. 2. Exposing a PSQL var that identifies the shell snippet for "how to suppress standard error", which would be "2> /dev/null" for -nix systems and "2> NUL" for windows systems. This again, presumes that users will actually need this feature, and I'm not convinced that they will. 3. Exposing a PSQL var that is basically an "is this environment running on windows" and let them construct it from there. That gets complicated with WSL and the like, so I'm not wild about this, even though it would be comparatively simple to implement. 4. We could inject an environment variable into our own regression tests directly based on the "#ifdef WIN32" in our own code, and we can use a couple of \gset commands to construct the error-suppressed shell commands as needed. This seems like the best starting point, and we can always turn that env var into a real variable if it proves useful elsewhere.
Re: Add SHELL_EXIT_CODE to psql
On Wed, Dec 28, 2022 at 5:59 AM Maxim Orlov wrote: > Hi! > > The patch is implementing what is declared to do. Shell return code is now > accessible is psql var. > Overall code is in a good condition. Applies with no errors on master. > Unfortunately, regression tests are failing on the macOS due to the > different shell output. > That was to be expected. I wonder if there is value in setting up a psql on/off var SHELL_ERROR_OUTPUT construct that when set to "off/false" suppresses standard error via appending "2> /dev/null" (or "2> nul" if #ifdef WIN32). At the very least, it would allow for tests like this to be done with standard regression scripts. >
Re: Add SHELL_EXIT_CODE to psql
Hi! The patch is implementing what is declared to do. Shell return code is now accessible is psql var. Overall code is in a good condition. Applies with no errors on master. Unfortunately, regression tests are failing on the macOS due to the different shell output. @@ -1308,13 +1308,13 @@ deallocate q; -- test SHELL_EXIT_CODE \! nosuchcommand -sh: line 1: nosuchcommand: command not found +sh: nosuchcommand: command not found \echo :SHELL_EXIT_CODE 127 \set nosuchvar `nosuchcommand` -sh: line 1: nosuchcommand: command not found +sh: nosuchcommand: command not found \! nosuchcommand -sh: line 1: nosuchcommand: command not found +sh: nosuchcommand: command not found \echo :SHELL_EXIT_CODE 127 Since we do not want to test shell output in these cases, but only return code, what about using this kind of commands? postgres=# \! true > /dev/null 2>&1 postgres=# \echo :SHELL_EXIT_CODE 0 postgres=# \! false > /dev/null 2>&1 postgres=# \echo :SHELL_EXIT_CODE 1 postgres=# \! nosuchcommand > /dev/null 2>&1 postgres=# \echo :SHELL_EXIT_CODE 127 It is better to use spaces around "=". + if (WIFEXITED(exit_code)) + exit_code=WEXITSTATUS(exit_code); + else if(WIFSIGNALED(exit_code)) + exit_code=WTERMSIG(exit_code); + else if(WIFSTOPPED(exit_code)) + exit_code=WSTOPSIG(exit_code); -- Best regards, Maxim Orlov.
Re: Add SHELL_EXIT_CODE to psql
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
wrote:
> Rebased. Still waiting on feedback before working on documentation.
>
> On Fri, Nov 4, 2022 at 5:23 AM Corey Huinker
> 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
>> 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.
>>>
>>
From 443be6f64ca89bbd6367a011f2a98aa9324f27a9 Mon Sep 17 00:00:00 2001
From: coreyhuinker
Date: Wed, 21 Dec 2022 00:24:47 -0500
Subject: [PATCH 1/2] Make the exit code of shell commands executed via psql
visible via the variable SHELL_EXIT_CODE.
---
doc/src/sgml/ref/psql-ref.sgml | 9 +
src/bin/psql/command.c | 4
src/bin/psql/help.c| 2 ++
src/bin/psql/psqlscanslash.l | 24 +---
4 files changed, 36 insertions(+), 3 deletions(-)
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 8a5285da9a..d0c80b4528 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4260,6 +4260,15 @@ bar
+
+ SHELL_EXIT_CODE
+
+
+ The exit code return by the last shell command. 0 means no error.
+
+
+
+
SHOW_ALL_RESULTS
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index de6a3a71f8..f6d6a489a9 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,9 @@ 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 == 127 || result == -1)
{
pg_log_error("\\!: failed");
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index b4e0ec2687..caf13e2ed2 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -455,6 +455,8 @@ 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_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 a467b72144..ecbc3b9d8b 100644
--- a/src/bin/psql/psqlscanslash.l
+++ b/src/bin/psql/psqlscanslash.l
@@ -27,6 +27,8 @@
%{
#include "fe_utils/psqlscan_int.h"
+#include "settings.h"
+#include "variables.h"
/*
* We must have a typedef YYSTYPE for yylex's first argument, but this lexer
@@ -774,6 +776,8 @@ evaluate_backtick(PsqlScanState state)
bool error = false;
char buf[512];
size_t result;
+ int exit_code = 0;
+ char exit_code_buf[32];
initPQExpBuffer(_output);
@@ -783,6 +787,7 @@ evaluate_backtick(PsqlScanState state)
{
pg_log_error("%s: %m", cmd);
error = true;
+ exit_code = -1;
}
if (!error)
Re: Add SHELL_EXIT_CODE to psql
Rebased. Still waiting on feedback before working on documentation.
On Fri, Nov 4, 2022 at 5:23 AM Corey Huinker
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
> 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.
>>
>
From fef0e52706c9136659f0f8846bae289f0dbfb469 Mon Sep 17 00:00:00 2001
From: coreyhuinker
Date: Fri, 4 Nov 2022 04:45:39 -0400
Subject: [PATCH] POC: expose shell exit code as a psql variable
---
src/bin/psql/command.c | 4
src/bin/psql/help.c| 2 ++
src/bin/psql/psqlscanslash.l | 28 +---
src/test/regress/expected/psql.out | 11 +++
src/test/regress/sql/psql.sql | 7 +++
5 files changed, 49 insertions(+), 3 deletions(-)
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index de6a3a71f8..f6d6a489a9 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,9 @@ 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 == 127 || result == -1)
{
pg_log_error("\\!: failed");
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index b4e0ec2687..caf13e2ed2 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -455,6 +455,8 @@ 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_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 a467b72144..30e6f5dcd4 100644
--- a/src/bin/psql/psqlscanslash.l
+++ b/src/bin/psql/psqlscanslash.l
@@ -27,6 +27,8 @@
%{
#include "fe_utils/psqlscan_int.h"
+#include "settings.h"
+#include "variables.h"
/*
* We must have a typedef YYSTYPE for yylex's first argument, but this lexer
@@ -774,6 +776,8 @@ evaluate_backtick(PsqlScanState state)
bool error = false;
char buf[512];
size_t result;
+ int exit_code = 0;
+ char exit_code_buf[32];
initPQExpBuffer(_output);
@@ -783,6 +787,7 @@ evaluate_backtick(PsqlScanState state)
{
pg_log_error("%s: %m", cmd);
error = true;
+ exit_code = -1;
}
if (!error)
@@ -800,10 +805,25 @@ evaluate_backtick(PsqlScanState state)
} while (!feof(fd));
}
- if (fd && pclose(fd) == -1)
+ if (fd)
{
- pg_log_error("%s: %m", cmd);
- error = true;
+ exit_code = pclose(fd);
+ if (exit_code == -1)
+ {
+ pg_log_error("%s: %m", cmd);
+ error = true;
+ }
+ if (WIFEXITED(exit_code))
+ {
+ exit_code=WEXITSTATUS(exit_code);
+ }
+ else if(WIFSIGNALED(exit_code)) {
+ exit_code=WTERMSIG(exit_code);
+ }
+ else if(WIFSTOPPED(exit_code)) {
+ //If you need to act upon the process stopping, do it here.
+ exit_code=WSTOPSIG(exit_code);
+ }
}
if (PQExpBufferDataBroken(cmd_output))
@@ -826,5 +846,7 @@ evaluate_backtick(PsqlScanState state)
appendBinaryPQExpBuffer(output_buf, cmd_output.data, cmd_output.len);
}
+ snprintf(exit_code_buf, sizeof(exit_code_buf), "%d", exit_code);
+
Re: Add SHELL_EXIT_CODE to psql
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 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. >
