Add SHELL_EXIT_CODE to psql

Over in
https://www.postgresql.org/message-id/[email protected]
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.

Oops, that sample output was from a previous run, should have been:

-- SHELL_EXIT_CODE is undefined
\echo :SHELL_EXIT_CODE
:SHELL_EXIT_CODE
-- bad \!
\! borp
sh: line 1: borp: command not found
\echo :SHELL_EXIT_CODE
127
-- bad backtick
\set var `borp`
sh: line 1: borp: command not found
\echo :SHELL_EXIT_CODE
127
-- good \!
\! true
\echo :SHELL_EXIT_CODE
0
-- play with exit codes
\! exit 4
\echo :SHELL_EXIT_CODE
4
\set var `exit 3`
\echo :SHELL_EXIT_CODE
3

On Fri, Nov 4, 2022 at 5:08 AM Corey Huinker <corey(dot)huinker(at)gmail(dot)com>
wrote:

>
> Over in
> https://www.postgresql.org/message-id/[email protected] 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.
>

Rebased. Still waiting on feedback before working on documentation.

On Fri, Nov 4, 2022 at 5:23 AM Corey Huinker <corey(dot)huinker(at)gmail(dot)com>
wrote:

> Oops, that sample output was from a previous run, should have been:
>
> -- SHELL_EXIT_CODE is undefined
> \echo :SHELL_EXIT_CODE
> :SHELL_EXIT_CODE
> -- bad \!
> \! borp
> sh: line 1: borp: command not found
> \echo :SHELL_EXIT_CODE
> 127
> -- bad backtick
> \set var `borp`
> sh: line 1: borp: command not found
> \echo :SHELL_EXIT_CODE
> 127
> -- good \!
> \! true
> \echo :SHELL_EXIT_CODE
> 0
> -- play with exit codes
> \! exit 4
> \echo :SHELL_EXIT_CODE
> 4
> \set var `exit 3`
> \echo :SHELL_EXIT_CODE
> 3
>
>
> On Fri, Nov 4, 2022 at 5:08 AM Corey Huinker <corey(dot)huinker(at)gmail(dot)com>
> wrote:
>
>>
>> Over in
>> https://www.postgresql.org/message-id/[email protected] 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.
>>
>

I've rebased and updated the patch to include documentation.

Regression tests have been moved to a separate patchfile because error
messages will vary by OS and configuration, so we probably can't do a
stable regression test, but having them handy at least demonstrates the
feature.

On Sun, Dec 4, 2022 at 12:35 AM Corey Huinker <corey(dot)huinker(at)gmail(dot)com>
wrote:

> Rebased. Still waiting on feedback before working on documentation.
>
> On Fri, Nov 4, 2022 at 5:23 AM Corey Huinker <corey(dot)huinker(at)gmail(dot)com>
> wrote:
>
>> Oops, that sample output was from a previous run, should have been:
>>
>> -- SHELL_EXIT_CODE is undefined
>> \echo :SHELL_EXIT_CODE
>> :SHELL_EXIT_CODE
>> -- bad \!
>> \! borp
>> sh: line 1: borp: command not found
>> \echo :SHELL_EXIT_CODE
>> 127
>> -- bad backtick
>> \set var `borp`
>> sh: line 1: borp: command not found
>> \echo :SHELL_EXIT_CODE
>> 127
>> -- good \!
>> \! true
>> \echo :SHELL_EXIT_CODE
>> 0
>> -- play with exit codes
>> \! exit 4
>> \echo :SHELL_EXIT_CODE
>> 4
>> \set var `exit 3`
>> \echo :SHELL_EXIT_CODE
>> 3
>>
>>
>> On Fri, Nov 4, 2022 at 5:08 AM Corey Huinker <corey(dot)huinker(at)gmail(dot)com>
>> wrote:
>>
>>>
>>> Over in
>>> https://www.postgresql.org/message-id/[email protected] 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.
>>>
>>

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.

On Wed, Dec 28, 2022 at 5:59 AM Maxim Orlov <orlovmg(at)gmail(dot)com> 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.

>

On Fri, Dec 30, 2022 at 2:17 PM Corey Huinker <corey(dot)huinker(at)gmail(dot)com>
wrote:

> On Wed, Dec 28, 2022 at 5:59 AM Maxim Orlov <orlovmg(at)gmail(dot)com> 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.

On Sat, 31 Dec 2022 at 16:47, Corey Huinker <corey(dot)huinker(at)gmail(dot)com> 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.

On Sat, Dec 31, 2022 at 5:28 PM Isaac Morland <isaac(dot)morland(at)gmail(dot)com>
wrote:

> On Sat, 31 Dec 2022 at 16:47, Corey Huinker <corey(dot)huinker(at)gmail(dot)com>
> 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.

On Wed, 21 Dec 2022 at 11:04, Corey Huinker <corey(dot)huinker(at)gmail(dot)com> wrote:
>
> I've rebased and updated the patch to include documentation.
>
> Regression tests have been moved to a separate patchfile because error messages will vary by OS and configuration, so we probably can't do a stable regression test, but having them handy at least demonstrates the feature.
>
> On Sun, Dec 4, 2022 at 12:35 AM Corey Huinker <corey(dot)huinker(at)gmail(dot)com> wrote:
>>
>> Rebased. Still waiting on feedback before working on documentation.
>>
>> On Fri, Nov 4, 2022 at 5:23 AM Corey Huinker <corey(dot)huinker(at)gmail(dot)com> wrote:
>>>
>>> Oops, that sample output was from a previous run, should have been:
>>>
>>> -- SHELL_EXIT_CODE is undefined
>>> \echo :SHELL_EXIT_CODE
>>> :SHELL_EXIT_CODE
>>> -- bad \!
>>> \! borp
>>> sh: line 1: borp: command not found
>>> \echo :SHELL_EXIT_CODE
>>> 127
>>> -- bad backtick
>>> \set var `borp`
>>> sh: line 1: borp: command not found
>>> \echo :SHELL_EXIT_CODE
>>> 127
>>> -- good \!
>>> \! true
>>> \echo :SHELL_EXIT_CODE
>>> 0
>>> -- play with exit codes
>>> \! exit 4
>>> \echo :SHELL_EXIT_CODE
>>> 4
>>> \set var `exit 3`
>>> \echo :SHELL_EXIT_CODE
>>> 3
>>>
>>>
>>> On Fri, Nov 4, 2022 at 5:08 AM Corey Huinker <corey(dot)huinker(at)gmail(dot)com> wrote:
>>>>
>>>>
>>>> Over in https://www.postgresql.org/message-id/[email protected] Justin Pryzby suggested that psql might need the ability to capture the shell exit code.
>>>>
>>>> This is a POC patch that does that, but doesn't touch on the ON_ERROR_STOP stuff.
>>>>
>>>> I've added some very rudimentary tests, but haven't touched the documentation, because I strongly suspect that someone will suggest a better name for the variable.
>>>>
>>>> But basically, it works like this
>>>>
>>>> -- SHELL_EXIT_CODE is undefined
>>>> \echo :SHELL_EXIT_CODE
>>>> :SHELL_EXIT_CODE
>>>> -- bad \!
>>>> \! borp
>>>> sh: line 1: borp: command not found
>>>> \echo :SHELL_EXIT_CODE
>>>> 32512
>>>> -- bad backtick
>>>> \set var `borp`
>>>> sh: line 1: borp: command not found
>>>> \echo :SHELL_EXIT_CODE
>>>> 127
>>>> -- good \!
>>>> \! true
>>>> \echo :SHELL_EXIT_CODE
>>>> 0
>>>> -- play with exit codes
>>>> \! exit 4
>>>> \echo :SHELL_EXIT_CODE
>>>> 1024
>>>> \set var `exit 3`
>>>> \echo :SHELL_EXIT_CODE
>>>> 3
>>>>
>>>>
>>>> Feedback welcome.

CFBot shows some compilation errors as in [1], please post an updated
version for the same:
[02:35:49.924] psqlscanslash.l: In function ‘evaluate_backtick’:
[02:35:49.924] psqlscanslash.l:822:11: error: implicit declaration of
function ‘WIFSTOPPED’ [-Werror=implicit-function-declaration]
[02:35:49.924] 822 | exit_code=WSTOPSIG(exit_code);
[02:35:49.924] | ^~~~~~~~~~
[02:35:49.924] psqlscanslash.l:823:14: error: implicit declaration of
function ‘WSTOPSIG’ [-Werror=implicit-function-declaration]
[02:35:49.924] 823 | }
[02:35:49.924] | ^
[02:35:49.924] cc1: all warnings being treated as errors
[02:35:49.925] make[3]: *** [<builtin>: psqlscanslash.o] Error 1

[1] - https://cirrus-ci.com/task/5424476720988160

Regards,
Vignesh

On Tue, Jan 3, 2023 at 5:36 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:

> On Wed, 21 Dec 2022 at 11:04, Corey Huinker <corey(dot)huinker(at)gmail(dot)com>
> wrote:
> >
> > I've rebased and updated the patch to include documentation.
> >
> > Regression tests have been moved to a separate patchfile because error
> messages will vary by OS and configuration, so we probably can't do a
> stable regression test, but having them handy at least demonstrates the
> feature.
> >
> > On Sun, Dec 4, 2022 at 12:35 AM Corey Huinker <corey(dot)huinker(at)gmail(dot)com>
> wrote:
> >>
> >> Rebased. Still waiting on feedback before working on documentation.
> >>
> >> On Fri, Nov 4, 2022 at 5:23 AM Corey Huinker <corey(dot)huinker(at)gmail(dot)com>
> wrote:
> >>>
> >>> Oops, that sample output was from a previous run, should have been:
> >>>
> >>> -- SHELL_EXIT_CODE is undefined
> >>> \echo :SHELL_EXIT_CODE
> >>> :SHELL_EXIT_CODE
> >>> -- bad \!
> >>> \! borp
> >>> sh: line 1: borp: command not found
> >>> \echo :SHELL_EXIT_CODE
> >>> 127
> >>> -- bad backtick
> >>> \set var `borp`
> >>> sh: line 1: borp: command not found
> >>> \echo :SHELL_EXIT_CODE
> >>> 127
> >>> -- good \!
> >>> \! true
> >>> \echo :SHELL_EXIT_CODE
> >>> 0
> >>> -- play with exit codes
> >>> \! exit 4
> >>> \echo :SHELL_EXIT_CODE
> >>> 4
> >>> \set var `exit 3`
> >>> \echo :SHELL_EXIT_CODE
> >>> 3
> >>>
> >>>
> >>> On Fri, Nov 4, 2022 at 5:08 AM Corey Huinker <corey(dot)huinker(at)gmail(dot)com>
> wrote:
> >>>>
> >>>>
> >>>> Over in
> https://www.postgresql.org/message-id/[email protected]
> Justin Pryzby suggested that psql might need the ability to capture the
> shell exit code.
> >>>>
> >>>> This is a POC patch that does that, but doesn't touch on the
> ON_ERROR_STOP stuff.
> >>>>
> >>>> I've added some very rudimentary tests, but haven't touched the
> documentation, because I strongly suspect that someone will suggest a
> better name for the variable.
> >>>>
> >>>> But basically, it works like this
> >>>>
> >>>> -- SHELL_EXIT_CODE is undefined
> >>>> \echo :SHELL_EXIT_CODE
> >>>> :SHELL_EXIT_CODE
> >>>> -- bad \!
> >>>> \! borp
> >>>> sh: line 1: borp: command not found
> >>>> \echo :SHELL_EXIT_CODE
> >>>> 32512
> >>>> -- bad backtick
> >>>> \set var `borp`
> >>>> sh: line 1: borp: command not found
> >>>> \echo :SHELL_EXIT_CODE
> >>>> 127
> >>>> -- good \!
> >>>> \! true
> >>>> \echo :SHELL_EXIT_CODE
> >>>> 0
> >>>> -- play with exit codes
> >>>> \! exit 4
> >>>> \echo :SHELL_EXIT_CODE
> >>>> 1024
> >>>> \set var `exit 3`
> >>>> \echo :SHELL_EXIT_CODE
> >>>> 3
> >>>>
> >>>>
> >>>> Feedback welcome.
>
> CFBot shows some compilation errors as in [1], please post an updated
> version for the same:
> [02:35:49.924] psqlscanslash.l: In function ‘evaluate_backtick’:
> [02:35:49.924] psqlscanslash.l:822:11: error: implicit declaration of
> function ‘WIFSTOPPED’ [-Werror=implicit-function-declaration]
> [02:35:49.924] 822 | exit_code=WSTOPSIG(exit_code);
> [02:35:49.924] | ^~~~~~~~~~
> [02:35:49.924] psqlscanslash.l:823:14: error: implicit declaration of
> function ‘WSTOPSIG’ [-Werror=implicit-function-declaration]
> [02:35:49.924] 823 | }
> [02:35:49.924] | ^
> [02:35:49.924] cc1: all warnings being treated as errors
> [02:35:49.925] make[3]: *** [<builtin>: psqlscanslash.o] Error 1
>
> [1] - https://cirrus-ci.com/task/5424476720988160
>
> Regards,
> Vignesh
>

Thanks. I had left sys/wait.h out of psqlscanslash.

Attached is v3 of this patch, I've made the following changes:

1. pg_regress now creates an environment variable called PG_OS_TARGET,
which regression tests can use to manufacture os-specific commands. For our
purposes, this allows the regression test to manufacture a shell command
that has either "2> /dev/null" or "2> NUL". This seemed the least invasive
way to make this possible. If for some reason it becomes useful in general
psql scripting, then we can consider promoting it to a regular psql var.

2. There are now two psql variables, SHELL_EXIT_CODE, which has the return
code, and SHELL_ERROR, which is a true/false flag based on whether the exit
code was nonzero. These variables are the shell result analogues of
SQLSTATE and ERROR.

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.

On Mon, Jan 9, 2023 at 10:01 AM Maxim Orlov <orlovmg(at)gmail(dot)com> 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).

>

On Mon, 9 Jan 2023 at 21:36, Corey Huinker <corey(dot)huinker(at)gmail(dot)com> 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.

On Tue, Jan 10, 2023 at 3:54 AM Maxim Orlov <orlovmg(at)gmail(dot)com> wrote:

>
>
> On Mon, 9 Jan 2023 at 21:36, Corey Huinker <corey(dot)huinker(at)gmail(dot)com>
> 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.

On Tue, 10 Jan 2023 at 00:06, Corey Huinker <corey(dot)huinker(at)gmail(dot)com> wrote:
>
> On Mon, Jan 9, 2023 at 10:01 AM Maxim Orlov <orlovmg(at)gmail(dot)com> 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

>
>
>
> 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.

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.

>
> 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.

On Fri, 13 Jan 2023 at 07:50, Corey Huinker <corey(dot)huinker(at)gmail(dot)com> 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.

On Wed, Jan 4, 2023 at 2:09 AM Corey Huinker <corey(dot)huinker(at)gmail(dot)com> 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

On Fri, Jan 20, 2023 at 8:54 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Wed, Jan 4, 2023 at 2:09 AM Corey Huinker <corey(dot)huinker(at)gmail(dot)com>
> 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?

On Mon, Jan 23, 2023 at 1:59 PM Corey Huinker <corey(dot)huinker(at)gmail(dot)com> 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

On Mon, Jan 23, 2023 at 2:53 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Mon, Jan 23, 2023 at 1:59 PM Corey Huinker <corey(dot)huinker(at)gmail(dot)com>
> 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.

>
> 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.

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.

>
>
> 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.

On Mon, 30 Jan 2023 at 23:23, Corey Huinker <corey(dot)huinker(at)gmail(dot)com> 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);
+ char buf[32];
...here
+ snprintf(buf, sizeof(buf), "%d", exit_code);
+ SetVariable(pset.vars, "SHELL_EXIT_CODE", buf);
+ SetVariable(pset.vars, "SHELL_ERROR", "true");

+ char exit_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.

>
>
>
> 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.

On Tue, Jan 31, 2023 at 9:23 AM Corey Huinker <corey(dot)huinker(at)gmail(dot)com> 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

On Wed, Feb 22, 2023 at 4:18 PM Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:

> On Tue, Jan 31, 2023 at 9:23 AM Corey Huinker <corey(dot)huinker(at)gmail(dot)com>
> 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.

Corey Huinker <corey(dot)huinker(at)gmail(dot)com> 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

On Thu, Mar 2, 2023 at 1:35 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Corey Huinker <corey(dot)huinker(at)gmail(dot)com> 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/[email protected]
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.

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

Justin Pryzby <pryzby(at)telsasoft(dot)com> 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

On Fri, Mar 10, 2023 at 3:49 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> 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.

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.

Corey Huinker <corey(dot)huinker(at)gmail(dot)com> writes:
> 128+N is implemented.

I think this mostly looks OK, but:

* I still say there is no basis whatever for believing that the result
of ferror() is an exit code, errno code, or anything else with
significance beyond zero-or-not. Feeding it to wait_result_to_exit_code
as you've done here is not going to do anything but mislead people in
a platform-dependent way. Probably should set exit_code to -1 if
ferror reports trouble.

* 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

On Mon, Mar 20, 2023 at 1:01 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Corey Huinker <corey(dot)huinker(at)gmail(dot)com> writes:
> > 128+N is implemented.
>
> I think this mostly looks OK, but:
>
> * I still say there is no basis whatever for believing that the result
> of ferror() is an exit code, errno code, or anything else with
> significance beyond zero-or-not. Feeding it to wait_result_to_exit_code
> as you've done here is not going to do anything but mislead people in
> a platform-dependent way. Probably should set exit_code to -1 if
> ferror reports trouble.
>

Agreed. Sorry, I read your comment wrong the last time or I would have
already made it so.

>
> * Why do you have wait_result_to_exit_code defaulting to return 0
> if it doesn't recognize the code as either WIFEXITED or WIFSIGNALED?
> That seems pretty misleading; again -1 would be a better idea.
>

That makes sense as well. Given that WIFSIGNALED is currently defined as
the negation of WIFEXITED, whatever default result we have here is
basically a this-should-never-happen. That might be something we want to
catch with an assert, but I'm fine with a -1 for now.

Corey Huinker <corey(dot)huinker(at)gmail(dot)com> writes:
> On Mon, Mar 20, 2023 at 1:01 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> 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

>
>
> 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!

On Tue, Mar 21, 2023 at 3:33 PM Corey Huinker <corey(dot)huinker(at)gmail(dot)com>
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=4Q@mail.gmail.com
thread and, whether or not that feature gets in, we should probably apply
it to output pipes as well.

On Fri, Mar 24, 2023 at 5:21 PM Corey Huinker <corey(dot)huinker(at)gmail(dot)com>
wrote:

>
>
> On Tue, Mar 21, 2023 at 3:33 PM Corey Huinker <corey(dot)huinker(at)gmail(dot)com>
> 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=4Q@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.

Corey Huinker <corey(dot)huinker(at)gmail(dot)com> 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

Corey Huinker <corey(dot)huinker(at)gmail(dot)com> 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