Re: pg_upgrade failure on Windows Server

On Wed, Mar 18, 2015 at 1:09 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:

> 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.
>

Oops, good catch.

> 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?
>

Done.

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?
>

I do agree, pg_ctl refactoring is making current patch more complicated,
eventually we can come up with robust patch that include pg_ctl but result
patch could be big and touching lot of areas, I doubt that if such
complicated and big patch going to make its way into the repository. It
seems appropriate to do incremental work, we can refactor pg_ctl changes as
next effort. PFA update patch, it removes pg_ctl related code changes.
Please do let me know if I missed something. Thanks.

> Regards,
> --
> Michael
>