Re: Add SHELL_EXIT_CODE to psql - Mailing list pgsql-hackers

From Corey Huinker
Subject Re: Add SHELL_EXIT_CODE to psql
Date
Msg-id CADkLM=ekdTJwprfJZU43_Px6P8Y0ZTPNhc9j6K8BqE8snuoJBQ@mail.gmail.com
Whole thread Raw
In response to Re: Add SHELL_EXIT_CODE to psql  (Maxim Orlov <orlovmg@gmail.com>)
Responses Re: Add SHELL_EXIT_CODE to psql
Re: Add SHELL_EXIT_CODE to psql
List pgsql-hackers
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 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).
Attachment

pgsql-hackers by date:

Previous
From: Nathan Bossart
Date:
Subject: Re: Allow +group in pg_ident.conf
Next
From: Tom Lane
Date:
Subject: Re: [PATCH] random_normal function