Re: pg_upgrade failure on Windows Server - Mailing list pgsql-bugs

Thank you Michael.

On Wed, Mar 18, 2015 at 5:57 AM, Michael Paquier <michael.paquier@gmail.com=
>
wrote:

> On Wed, Mar 18, 2015 at 5:15 AM, Asif Naeem <anaeem.it@gmail.com> wrote:
> > Thank you for useful suggestions, PFA patch, for pg_ctl.c and
> pg_regress.c
> > it relies on CreateRestrictedProcess() function now.
>
> Thanks for the updated version.
>
> > src/common/restricted_token.c
> >>
> >>      #define write_stderr(fmt, ...) fprintf(stderr, fmt, __VA_ARGS__)
>
> Oops, I forgot this stuff with pg_ctl. It is not nice to duplicate
> this declaration in restricted_token.c.
>
> > security.c write_stderr() implementation seems correct, that relies on
> > pgwin32_is_service() function but it seems little expensive operation t=
o
> > write error messages. pg_ctl.c write_stderr() implementation depend on
> > isatty() to detect if it is running as service, I doubt that if it is
> right
> > way to to do so. Any suggestion how to tackle this situation, along wit=
h
> > this can we make common routine of write_stderr() function that others
> use
> > it instead of defining their own?
>
> I think that we should rip out the dependency with write_stderr and
> have get_restricted_token return a state code instead, with something
> like that:
> typedef enum RestrictedTokenState
> {
>      PG_RESTRICTED_TOKEN_OK =3D 0,
>      PG_RESTRICTED_TOKEN_FAIL_EXECUTE,
>      PG_RESTRICTED_TOKEN_ERROR_PLATFORM,
>      [...]
> } RestrictedTokenState;
>
> Using that, caller can simply error out something with the error code.
> We could have some documentation as well about that... But let's get
> the code nicely done first.
>
> > Please do let me know if I missed something or more information is
> required.
>
> Some more comments:
>
> I am getting a compilation failure when compiling on a non-Windows
> environment:
> Configured with: --prefix=3D/usr
> --with-gxx-include-dir=3D/usr/include/c++/4.2.1
> In file included from restricted_token.c:23:
> ../../src/include/common/restricted_token.h:21:1: error: unknown type
> name 'HANDLE'
> HANDLE  CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION
> *processInfo, const char *progname);
> ^
> ../../src/include/common/restricted_token.h:21:43: error: unknown type
> name 'PROCESS_INFORMATION'
> HANDLE  CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION
> *processInfo, const char *progname);
>                                            ^
> 2 errors generated.
> make[2]: *** [restricted_token.o] Error 1
> make[2]: *** Waiting for unfinished jobs....
> make[1]: *** [all-common-recurse] Error 2
> make: *** [all-src-recurse] Error 2
>
> This is generated because CreateRestrictedProcess is missing #ifdef
> WIN32 in restricted_token.h.
>

Oops. Sorry I missed that.


>  #include "common/username.h"
> +#include "common/restricted_token.h"
> Be careful of the include file ordering.
>
> It seems backward to me to make CreateRestrictedProcess available in
> restricted_token.h. What I got in mind first was a common API like
> this one for all the callers:
> get_restricted_token(const char *progname, const char *cmd, bool no_wait)=
;
> no_wait controlling WaitForSingleObject() in get_restricted_token.
>
> On top of that, it seems useful to me to use PG_RESTRICT_EXEC to
> ensure that we do not try twice to get a token when we already have
> one.
>

At the moment there is a need to use restricted process logic in pg_regress
and pg_ctl in the following functions i.e.

src/test/regress/pg_regress.c

> /*
>  * Spawn a process to execute the given shell command; don't wait for it
>  *
>  * Returns the process ID (or HANDLE) so we can wait for it later
>  */
> PID_TYPE
> spawn_process(const char *cmdline)
> {
> ..
>

src/bin/pg_ctl/pg_ctl.c

> static void WINAPI
> pgwin32_ServiceMain(DWORD argc, LPTSTR *argv)
> {
> ..
>

src/bin/pg_ctl/pg_ctl.c

> /*
>  * start/test/stop routines
>  */
> static int
> start_postmaster(void)
> {
> ...
>

If spawn_process() is going to use get_restricted_token probably we need to
change its name too.

For pg_ctl and pg_regress, I tried to refactor only related code that
CreateRestrictedProcess was already doing so I thought to use it for these
utilities. Please do let me know if I missed something, Is it not going to
be more complicated if we use get_restricted_token/PG_RESTRICT_EXEC logic
there ?, at the moment get_restricted_token implementation seems like
re-execute ourselves with restricted token and exit as child process
terminates, If we want to change it to execute an another program with
restricted token, is't there a need to change it=E2=80=99s name to more des=
criptive
one ?. If we depend on PG_RESTRICT_EXEC environment variable execute once
logic, what if any of the parent program already ran
get_restricted_token(), e.g. pg_upgrade internally run pg_ctl ?

Please do let me know if I missed something or more information is
required. Thanks.

Regards,
> --
> Michael
>