Re: pg_upgrade failure on Windows Server

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 to
> 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 with
> 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 = 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=/usr --with-gxx-include-dir=/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.

 #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.
Regards,
--
Michael