Re: Add SHELL_EXIT_CODE to psql

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