Re: pg_upgrade failure on Windows Server

On Wed, Mar 18, 2015 at 3:50 PM, Asif Naeem wrote:
> 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's name to more descriptive
> 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 ?

I have been poking at this patch for a couple of hours more
seriously... And yes I concur with your analysis that this may
complicate more the logic pg_ctl.c, and that PG_RESTRICT_EXEC may
interact badly with pg_ctl.

Also, I had a look at the stuff around write_stderr, trying to take it
using two approaches:
1) First using some status code as return value of
get_restricted_token and CreateRestrictedToken (total of 7 error
codes), but this is as well proving to be ugly, and would make the
user lose a lot of useful information about the error that happened
and is now flushed to stderr
2) Add write_stderr in libpqcommon, this is proving to be rather ugly
as this has a dependency with write_console in elog.c... (Note that
initdb has a local macro write_stderr)
So that's a bit disappointing, and we have a real problem with your
patch because reports related to restricted tokens are not written to
the event log when running PG instance as a service.

By the way, while reading your patch I found an additional issue, this
check has been removed:
- /* Verify that we found all functions */
- if (_IsProcessInJob == NULL || _CreateJobObject == NULL ||
_SetInformationJobObject == NULL || _AssignProcessToJobObject == NULL
|| _QueryInformationJobObject == NULL)
Because of this stuff removed process may crash if one or more
functions are not found.

After the stuff with LoadLibrary("KERNEL32.DLL"); simply write a
report to write_stderr and return if one of those functions is
missing, with the magic of IsWindowsXPOrGreater included of course.

Could you fix this with the ifdef WIN32 missing in restricted_token.h?

Now, after looking closely at this stuff, an option that we could
consider is dropping pg_ctl from the refactoring because what it does
is too low-level and even if pg_ctl uses restricted tokens, it does
not expect to have PG_RESTRICT_TOKEN set. So perhaps we could rename
restricted_token to something like env_restrict_token and mention in
the comments that PG_RESTRICT_TOKEN is at the center of the logic,
preventing the creation of a new token if process has already one.
Thoughts?

Regards,
--
Michael