Thread: Add SHELL_EXIT_CODE to psql

On Wed, 21 Dec 2022 at 11:04, Corey Huinker <corey.huinker@gmail.com> wrote:
>
> I've rebased and updated the patch to include documentation.
>
> Regression tests have been moved to a separate patchfile because error messages will vary by OS and configuration, so
weprobably 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.huinker@gmail.com> wrote:
>>
>> Rebased. Still waiting on feedback before working on documentation.
>>
>> On Fri, Nov 4, 2022 at 5:23 AM Corey Huinker <corey.huinker@gmail.com> wrote:
>>>
>>> Oops, that sample output was from a previous run, should have been:
>>>
>>> -- SHELL_EXIT_CODE is undefined
>>> \echo :SHELL_EXIT_CODE
>>> :SHELL_EXIT_CODE
>>> -- bad \!
>>> \! borp
>>> sh: line 1: borp: command not found
>>> \echo :SHELL_EXIT_CODE
>>> 127
>>> -- bad backtick
>>> \set var `borp`
>>> sh: line 1: borp: command not found
>>> \echo :SHELL_EXIT_CODE
>>> 127
>>> -- good \!
>>> \! true
>>> \echo :SHELL_EXIT_CODE
>>> 0
>>> -- play with exit codes
>>> \! exit 4
>>> \echo :SHELL_EXIT_CODE
>>> 4
>>> \set var `exit 3`
>>> \echo :SHELL_EXIT_CODE
>>> 3
>>>
>>>
>>> On Fri, Nov 4, 2022 at 5:08 AM Corey Huinker <corey.huinker@gmail.com> wrote:
>>>>
>>>>
>>>> Over in https://www.postgresql.org/message-id/eaf326ad693e74eba068f33a7f518039@oss.nttdata.com Justin Pryzby
suggestedthat 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
someonewill 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, 10 Jan 2023 at 00:06, Corey Huinker <corey.huinker@gmail.com> wrote:
>
> On Mon, Jan 9, 2023 at 10:01 AM Maxim Orlov <orlovmg@gmail.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
variablethere. 
>
>
> I chose a name that would avoid collisions with anything a user might potentially throw into their environment, so if
thevar "OS" is fairly standard is a reason to avoid using it. Also, going with our own env var allows us to stay in
perfectsynchronization with the build's #ifdef WIN32 ... and whatever #ifdefs may come in the future for new OSes. If
thereis 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

On Wed, Jan 4, 2023 at 2:09 AM Corey Huinker <corey.huinker@gmail.com> wrote:
> 2. There are now two psql variables, SHELL_EXIT_CODE, which has the return code, and SHELL_ERROR, which is a
true/falseflag based on whether the exit code was nonzero. These variables are the shell result analogues of SQLSTATE
andERROR.
 

Seems redundant.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On Mon, Jan 23, 2023 at 1:59 PM Corey Huinker <corey.huinker@gmail.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
totrue 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 Tue, Jan 31, 2023 at 9:23 AM Corey Huinker <corey.huinker@gmail.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
eitherof 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

Corey Huinker <corey.huinker@gmail.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 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
statusesfrom shell
 
       builtins and compound commands are also limited to this range.  Under certain circumstances, the shell will use
specialvalues  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
indicatessuccess.
 
       A non-zero exit status indicates failure.  When a command terminates on a fatal signal N, bash uses the value of
128+Nas 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
foundbut 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@telsasoft.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

Corey Huinker <corey.huinker@gmail.com> writes:
> 128+N is implemented.

I think this mostly looks OK, but:

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

* 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

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

Corey Huinker <corey.huinker@gmail.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.huinker@gmail.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