Re: pg_upgrade failure on Windows Server - Mailing list pgsql-bugs
From | Michael Paquier |
---|---|
Subject | Re: pg_upgrade failure on Windows Server |
Date | |
Msg-id | CAB7nPqTUbuJuXK1iOox_AFuZYZxUqHeE1VH0Zov4jMe+rnq-JA@mail.gmail.com Whole thread Raw |
In response to | Re: pg_upgrade failure on Windows Server (Asif Naeem <anaeem.it@gmail.com>) |
Responses |
Re: pg_upgrade failure on Windows Server
(Asif Naeem <anaeem.it@gmail.com>)
|
List | pgsql-bugs |
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
pgsql-bugs by date: