Thread: pg_upgrade failure on Windows Server

Hi,

It is been observed that pg_upgrade fail to upgrade on Windows Server
machine if being run as Administrator user (other local users works fine),
It fail with the following error message i.e.

> ""c:\pg93\bin/pg_ctl" -w -l "pg_upgrade_server.log" -D "c:\pgdata93" -o
> "-p 3333 -b -c autovacuum_multixact_freeze_max_age=3D2000000000  -c
> synchronous_commit=3Doff -c fsync=3Doff -c full_page_writes=3Doff" start =
>>
> "pg_upgrade_server_start.log" 2>&1"
> *failure*
> There were problems executing """c:\pg93\bin/pg_ctl" -w -l
> "pg_upgrade_server.log" -D "c:\pgdata93" -o "-p 3333 -b -c
> autovacuum_multixact_freeze_max_age=3D2000000000  -c synchronous_commit=
=3Doff
> -c fsync=3Doff -c full_page_writes=3Doff" start >>
> "pg_upgrade_server_start.log" 2>&1""
> Consult the last few lines of "pg_upgrade_server_start.log" or
> "pg_upgrade_server.log" for
> the probable cause of the failure.
> connection to database failed: could not connect to server: Connection
> refused (0x0000274D/10061)
>         Is the server running on host "localhost" (127.0.0.1) and accepti=
ng
>         TCP/IP connections on port 3333?
>
> could not connect to new postmaster started with the command:
> "c:\pg93\bin/pg_ctl" -w -l "pg_upgrade_server.log" -D "c:\pgdata93" -o "-=
p
> 3333 -b -c autovacuum_multixact_freeze_max_age=3D2000000000  -c
> synchronous_commit=3Doff -c fsync=3Doff -c full_page_writes=3Doff" start
> Failure, exiting


pg_upgrade_server.log

> PANIC:  could not open control file "global/pg_control": Permission denie=
d


"global/pg_control=E2=80=9D file in data directory owned by
"BUILTIN\Administrators=E2=80=9D that was owned by =E2=80=9CAdministrator=
=E2=80=9D user before
pg_upgrade. I have verified it on Windows Server 2003/2008 R2/2012.

Steps to reproduce the issue :

> 1. Login as Administrator on Windows Server 2012 or Windows Server 2008
> 2. On command prompt run "c:\pg92\bin\initdb -D c:\pgdata92"
> 3. On command prompt run "c:\pg93\bin\initdb -D c:\pgdata93"
> 4. On command prompt run  "c:\pg93\bin\pg_upgrade -d c:\pgdata92 -D
> c:\pgdata93 -b c:\pg92\bin -B c:\pg93\bin -p 2222 -P 3333 -v"


Issue is reproducible with PG 9.2, 9.3 and 9.4. It is caused by
pg_resetxlog utility (It is internally used by pg_upgrade) that fail to
keep correct file permissions/ownership (as required by dbserver process)
while rewriting "global/pg_control=E2=80=9D in data directory. pg_resetxlog=
 utility
don=E2=80=99t sandbox and make changes in data directory, when it is being =
run as
Administrator ; it create files that are owned by "BUILTIN\Administrators=
=E2=80=9D
group. pg_ctl fails because it is using CreateRestrictedProcess() to
sandbox and give up administrator privileges that make it fail to open
"global/pg_control=E2=80=9D file (that is now owned by "BUILTIN\Administrat=
ors=E2=80=9D
group) in write mode i.e.

2014-12-19 03:19:34 PST PANIC: 42501: could not open control file
> "global/pg_control": Permission denied
> 2014-12-19 03:19:34 PST LOCATION: ReadControlFile,
> src\backend\access\transam\xlog.c:4458


I think Domain users that are members of Administrators group suffer from
same problem. On Windows Server 2003, changing Group Policy =E2=80=9CSystem
objects: Default owner for objects created by members of the Administrators
group=E2=80=9D to =E2=80=9CObject Creator=E2=80=9D fixes the problem. Micro=
soft obsolete this Group
Policy in later version of Windows Server. I have tried a workaround (
http://support.microsoft.com/kb/947721) on Windows Server 2008 R2 but it
seems did not worked for me. As it is default behavior for Windows Server,
I would suggest to use CreateRestrictedProcess function (sandbox or
restricted tokens approach) in pg_resetxlog utility or explicitly change
ownership to current user instead of "BUILTIN\Administrators=E2=80=9D group=
 if
required (If you agree with the approach I can submit patch for this).
Please do let me know if I missed something or more information is
required. Thanks.

Regards,
Muhammad Asif Naeem

On Tue, Jan 20, 2015 at 10:41:41PM +0500, Asif Naeem wrote:
> Above setting is available in Windows Server 2003, If I change this policy
> setting from “Administrators group" to "object creator", pg_upgrade work
> without issue. Unfortunately it got obsolete in later version of Windows
> Server. On Windows Server 2008, there seems workaround available i.e. http://
> support.microsoft.com/kb/947721 but it did not worked for me.
>
> PFA patch. To run pg_upgrade (and its child processes) without administrative
> privileges (when available), I have copied restricted token logic from src/bin/
> initdb/initdb.c, that enables pg_upgrade to rerun itself with removed
> Administrators group privileges, this way it create files with the ownership of
> user running pg_upgrade. With the patch applied, pg_upgrade (or its child
> process) don’t create files with administrators group owner and works fine
> without issue.
>
> As pg_resetxlog change file in data directory, it do suffer from same
> permission issue, it also require similar changes. PFA for pg_resetxlog fix.
> Please do let me know if I missed something or more information is required.
> Thanks.

This sounds like the exact right patch.  However, since it has a lot of
Windows-specific code, and we don't have any Windows experts, I am not
sure how this can be applied.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

Bruce Momjian wrote:

> This sounds like the exact right patch.  However, since it has a lot of
> Windows-specific code, and we don't have any Windows experts, I am not
> sure how this can be applied.

Are you saying we will remove the Windows port?  That sounds awesome,
thanks!  If you need help, I will volunteer on the spot, just LMK.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Jan 21, 2015 at 03:31:42PM -0300, Alvaro Herrera wrote:
> Bruce Momjian wrote:
>
> > This sounds like the exact right patch.  However, since it has a lot of
> > Windows-specific code, and we don't have any Windows experts, I am not
> > sure how this can be applied.
>
> Are you saying we will remove the Windows port?  That sounds awesome,
> thanks!  If you need help, I will volunteer on the spot, just LMK.

:-)

Well, I _am_ saying that historically patches that touch the innards of
the Windows API are rarely applied as we can't evaluate or maintain the
code.  I can probably come up with an example if you want.  The
Windows-specif code we do carry is required and was developed by people
that are no longer as involved.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

On Thu, Jan 22, 2015 at 12:06 AM, Bruce Momjian <bruce@momjian.us> wrote:
>
> On Wed, Jan 21, 2015 at 03:31:42PM -0300, Alvaro Herrera wrote:
> > Bruce Momjian wrote:
> >
> > > This sounds like the exact right patch.  However, since it has a lot
of
> > > Windows-specific code, and we don't have any Windows experts, I am not
> > > sure how this can be applied.
> >
> > Are you saying we will remove the Windows port?  That sounds awesome,
> > thanks!  If you need help, I will volunteer on the spot, just LMK.
>
> :-)
>
> Well, I _am_ saying that historically patches that touch the innards of
> the Windows API are rarely applied as we can't evaluate or maintain the
> code.  I can probably come up with an example if you want.

I think it is true to a great extent that Windows patches receive less
attention, however in many cases the patch finally do get committed.
I think the right thing for this patch is that Author should submit it to
next CF, so that it could be tracked and reviewed, once it is reviewed
by some one having Windows access, it should be taken care by
Committer.


> The
> Windows-specif code we do carry is required and was developed by people
> that are no longer as involved.
>

I have seen many a times that once committer's (those who are not generally
involved in Windows development) get reasonable confidence about patch
and the review done for the same, they commit the patch.  It happens both
for the patches where I was Author and where I was Reviewer, although I
agree that it takes more time.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Jan 22, 2015 at 12:31 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Thu, Jan 22, 2015 at 12:06 AM, Bruce Momjian <bruce@momjian.us> wrote:
>>
>> On Wed, Jan 21, 2015 at 03:31:42PM -0300, Alvaro Herrera wrote:
>> > Bruce Momjian wrote:
>> >
>> > > This sounds like the exact right patch.  However, since it has a lot
>> > > of
>> > > Windows-specific code, and we don't have any Windows experts, I am not
>> > > sure how this can be applied.
>> >
>> > Are you saying we will remove the Windows port?  That sounds awesome,
>> > thanks!  If you need help, I will volunteer on the spot, just LMK.
>>
>> :-)
>>
>> Well, I _am_ saying that historically patches that touch the innards of
>> the Windows API are rarely applied as we can't evaluate or maintain the
>> code.  I can probably come up with an example if you want.
>
> I think it is true to a great extent that Windows patches receive less
> attention, however in many cases the patch finally do get committed.
> I think the right thing for this patch is that Author should submit it to
> next CF, so that it could be tracked and reviewed, once it is reviewed
> by some one having Windows access, it should be taken care by
> Committer.
Adding it to the next CF would be a good first step. I got some access
to some 2k3 and 2k8 boxes, so I think that I could give it a shot.
--
Michael

Thank you. I have added it to next commitfest
<https://commitfest.postgresql.org/4/114/>.

On Thu, Jan 22, 2015 at 9:38 AM, Michael Paquier <michael.paquier@gmail.com>
wrote:

> On Thu, Jan 22, 2015 at 12:31 PM, Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> > On Thu, Jan 22, 2015 at 12:06 AM, Bruce Momjian <bruce@momjian.us>
> wrote:
> >>
> >> On Wed, Jan 21, 2015 at 03:31:42PM -0300, Alvaro Herrera wrote:
> >> > Bruce Momjian wrote:
> >> >
> >> > > This sounds like the exact right patch.  However, since it has a lot
> >> > > of
> >> > > Windows-specific code, and we don't have any Windows experts, I am
> not
> >> > > sure how this can be applied.
> >> >
> >> > Are you saying we will remove the Windows port?  That sounds awesome,
> >> > thanks!  If you need help, I will volunteer on the spot, just LMK.
> >>
> >> :-)
> >>
> >> Well, I _am_ saying that historically patches that touch the innards of
> >> the Windows API are rarely applied as we can't evaluate or maintain the
> >> code.  I can probably come up with an example if you want.
> >
> > I think it is true to a great extent that Windows patches receive less
> > attention, however in many cases the patch finally do get committed.
> > I think the right thing for this patch is that Author should submit it to
> > next CF, so that it could be tracked and reviewed, once it is reviewed
> > by some one having Windows access, it should be taken care by
> > Committer.
> Adding it to the next CF would be a good first step. I got some access
> to some 2k3 and 2k8 boxes, so I think that I could give it a shot.
> --
> Michael
>

On Thu, Jan 22, 2015 at 09:01:39AM +0530, Amit Kapila wrote:
> > The
> > Windows-specif code we do carry is required and was developed by people
> > that are no longer as involved.
> >
>
> I have seen many a times that once committer's (those who are not generally
> involved in Windows development) get reasonable confidence about patch
> and the review done for the same, they commit the patch.  It happens both
> for the patches where I was Author and where I was Reviewer, although I
> agree that it takes more time.

Here is Windows change to properly detach the server process that never
got implemented as no Windows expert developed or tested a patch, but
test code was posted:

    http://www.postgresql.org/message-id/flat/53759381.4000205@cubiclesoft.com#53759381.4000205@cubiclesoft.com

Here is another unapplied Windows patch for preventing Control-C from
closing the server:

    http://www.postgresql.org/message-id/flat/20140410214426.GI6917@momjian.us#20140410214426.GI6917@momjian.us

Again, I would love to say we handle Windows patches well, but we don't.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

On 2015-01-22 11:24:31 -0500, Bruce Momjian wrote:
> On Thu, Jan 22, 2015 at 09:01:39AM +0530, Amit Kapila wrote:
> > > The
> > > Windows-specif code we do carry is required and was developed by people
> > > that are no longer as involved.
> > >
> >
> > I have seen many a times that once committer's (those who are not generally
> > involved in Windows development) get reasonable confidence about patch
> > and the review done for the same, they commit the patch.  It happens both
> > for the patches where I was Author and where I was Reviewer, although I
> > agree that it takes more time.
>
> Here is Windows change to properly detach the server process that never
> got implemented as no Windows expert developed or tested a patch, but
> test code was posted:
>
>     http://www.postgresql.org/message-id/flat/53759381.4000205@cubiclesoft.com#53759381.4000205@cubiclesoft.com
>
> Here is another unapplied Windows patch for preventing Control-C from
> closing the server:
>
>     http://www.postgresql.org/message-id/flat/20140410214426.GI6917@momjian.us#20140410214426.GI6917@momjian.us
>
> Again, I would love to say we handle Windows patches well, but we don't.

I unfortunately don't the issue is that specific to windows.

Greetings,

Andres Freund

--
 Andres Freund                       http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On Thu, Jan 22, 2015 at 05:26:57PM +0100, Andres Freund wrote:
> > Again, I would love to say we handle Windows patches well, but we don't.
>
> I unfortunately don't the issue is that specific to windows.

Yes, any case where small number of people understand the problem is an
issue.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

On Thu, Jan 22, 2015 at 4:07 PM, Asif Naeem <anaeem.it@gmail.com> wrote:
> Thank you. I have added it to next commitfest.

Sorry for the late reply.

So I have been looking at both patches, and I would definitely be
useful to authorize the use of restricted tokens for both pg_upgrade
and pg_resetxlog. The patch for pg_resetxlog should be rebased on
latest HEAD because there is a small diff with get_restricted_token()
though.

The portion for initdb to make it use restricted tokens has been added
some time ago with fc9c20e, and the one of pg_ctl with a25cd81, both
of them in 2006, so instead of duplicating again the logic for
pg_upgrade and pg_resetxlog, shouldn't we create a common routine in
src/common and link to it all the frontend binaries that need it?
Hence, Asif, could you refactor first the existing code and make it
use a new API in libpqcommon? Let's say in
src/common/restricted_token.c or a better name than the one I just
gave out. Then, you could plug-in this new common routine with
pg_upgrade and pg_resetxlog in a second patch.
Regards,
--
Michael

On Fri, Jan 23, 2015 at 1:26 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2015-01-22 11:24:31 -0500, Bruce Momjian wrote:
>> On Thu, Jan 22, 2015 at 09:01:39AM +0530, Amit Kapila wrote:
>> > > The
>> > > Windows-specif code we do carry is required and was developed by people
>> > > that are no longer as involved.
>> > >
>> >
>> > I have seen many a times that once committer's (those who are not generally
>> > involved in Windows development) get reasonable confidence about patch
>> > and the review done for the same, they commit the patch.  It happens both
>> > for the patches where I was Author and where I was Reviewer, although I
>> > agree that it takes more time.
>>
>> Here is Windows change to properly detach the server process that never
>> got implemented as no Windows expert developed or tested a patch, but
>> test code was posted:
>>
>>       http://www.postgresql.org/message-id/flat/53759381.4000205@cubiclesoft.com#53759381.4000205@cubiclesoft.com
>>
>> Here is another unapplied Windows patch for preventing Control-C from
>> closing the server:
>>
>>       http://www.postgresql.org/message-id/flat/20140410214426.GI6917@momjian.us#20140410214426.GI6917@momjian.us
>>
>> Again, I would love to say we handle Windows patches well, but we don't.
>
> I unfortunately don't the issue is that specific to windows.

FWIW, I am putting those things in my never-ending TODO list. Thanks
for the reminder, Bruce.
--
Michael

On Wed, Mar 4, 2015 at 10:53 PM, Asif Naeem wrote:
> On Mon, Mar 2, 2015 at 9:42 AM, Michael Paquier wrote:
>>
>> The portion for initdb to make it use restricted tokens has been added
>> some time ago with fc9c20e, and the one of pg_ctl with a25cd81, both
>> of them in 2006, so instead of duplicating again the logic for
>> pg_upgrade and pg_resetxlog, shouldn't we create a common routine in
>> src/common and link to it all the frontend binaries that need it?
>> Hence, Asif, could you refactor first the existing code and make it
>> use a new API in libpqcommon?
>
> Sure. PFA patch, restricted token code is moved to
> src/common/restricted_token.c, Now it uses common routine for initdb,
> pg_upgrade and pg_resetxlog. Please do let me know if I missed something or
> more information is required. Thanks.

Thanks. This looks fine to me (tested on MinGW-32 and MSVC). I just
have one comment: get_restricted_token should use progname as
argument. This way you can just keep progname as a static variable for
each frontend and not play with constant variables in both code paths,
aka src/common and src/bin stuff.

Regards,
--
Michael

Asif Naeem wrote:
> Thank you Michael. Good suggestion, PFA updated patch, it don't have
> dependency on "progname" global variable any more and expecting this value
> as related function argument.

We have another copy of this in pg_ctl, which looks pretty much
identical except there's some additional stuff at the bottom.  Would it
make sense to add those additional "Job"-related things as a separate
routine, so that we can remove the duplicate copy of the "restricted
token" code, and have pg_ctl use get_restricted_process from src/common
instead?

There is a further copy of very similar stuff in pg_regress.c, but what
it does is slightly different; not sure how difficult it would be to
refactor that one too.

Is this a backpatchable bug fix, or are we considering this only for the
master branch?

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Mar 12, 2015 at 6:50 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Asif Naeem wrote:
>> Thank you Michael. Good suggestion, PFA updated patch, it don't have
>> dependency on "progname" global variable any more and expecting this value
>> as related function argument.
>
> We have another copy of this in pg_ctl, which looks pretty much
> identical except there's some additional stuff at the bottom.  Would it
> make sense to add those additional "Job"-related things as a separate
> routine, so that we can remove the duplicate copy of the "restricted
> token" code, and have pg_ctl use get_restricted_process from src/common
> instead?

Yes, looking at the code it makes sense.

> There is a further copy of very similar stuff in pg_regress.c, but what
> it does is slightly different; not sure how difficult it would be to
> refactor that one too.

In this part the only difference is a spawn of cmd /c, but I don't see
why it is useful to spawn a new command prompt here for this case, so
we could just drop this part. Looking at the log history, this has
been added since this code creation..

> Is this a backpatchable bug fix, or are we considering this only for the
> master branch?

It would be good to get that backpatched, that's something we really
miss now IMO. Now it modifies libpgcommon, so Windows packagers (me
being one) will certainly need to patch a bit stuff but that's a
one-line changer so it's not a big deal. And I imagine that this is
actually the reason why Asif reported that as a bug as well.
--
Michael

On Thu, Mar 12, 2015 at 11:23 PM, Michael Paquier wrote:
> On Thu, Mar 12, 2015 at 6:50 AM, Alvaro Herrera wrote:
> In this part the only difference is a spawn of cmd /c, but I don't see
> why it is useful to spawn a new command prompt here for this case, so
> we could just drop this part. Looking at the log history, this has
> been added since this code creation..

Taking back my words here. We definitely want to spawn a new process
in the case of pg_regress, so I think that the best thing to do would
be to pass the to-be-launched command to get_restrict_token(), and be
careful with WaitForSingleObject and GetExitCodeProcess() as there are
cases where we cannot wait for a process, so we are going to need a
control flag, or to let the callers of get_restricted_token() do the
wait themselves. I would think that the latter is better, additional
opinions being welcome.
--
Michael

Michael Paquier wrote:
> On Thu, Mar 12, 2015 at 11:23 PM, Michael Paquier wrote:
> > On Thu, Mar 12, 2015 at 6:50 AM, Alvaro Herrera wrote:
> > In this part the only difference is a spawn of cmd /c, but I don't see
> > why it is useful to spawn a new command prompt here for this case, so
> > we could just drop this part. Looking at the log history, this has
> > been added since this code creation..
>
> Taking back my words here. We definitely want to spawn a new process
> in the case of pg_regress, so I think that the best thing to do would
> be to pass the to-be-launched command to get_restrict_token(), and be
> careful with WaitForSingleObject and GetExitCodeProcess() as there are
> cases where we cannot wait for a process, so we are going to need a
> control flag, or to let the callers of get_restricted_token() do the
> wait themselves. I would think that the latter is better, additional
> opinions being welcome.

Maybe we want a specialized routine for pg_regress; maybe
get_restricted_token would call it with some args set to null or false
as appropriate.  My point is not to make the other callers of
get_restricted_token more complex.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Michael Paquier wrote:
> On Thu, Mar 12, 2015 at 6:50 AM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:

> > Is this a backpatchable bug fix, or are we considering this only for the
> > master branch?
>
> It would be good to get that backpatched, that's something we really
> miss now IMO. Now it modifies libpgcommon, so Windows packagers (me
> being one) will certainly need to patch a bit stuff but that's a
> one-line changer so it's not a big deal. And I imagine that this is
> actually the reason why Asif reported that as a bug as well.

I think it'd be better to patch only pg_upgrade in back branches, so
that there are no libpgcommon changes.  Seems that would make life
easier for packagers (See the \connect thread, where Robert opined that
it'd be better to duplicate some routines in back branches rather than
refactor libpq code and move the common code to pgcommon.  I didn't
completely agree with him at the time, but now that you mention
packagers pain, maybe he has a point.)

So let's do the refactoring in the master branch only, and duplicate
the code in back branches.  Nasty, but it seems the more robust
approach.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Mar 12, 2015 at 11:45 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Michael Paquier wrote:
>> On Thu, Mar 12, 2015 at 6:50 AM, Alvaro Herrera
>> <alvherre@2ndquadrant.com> wrote:
>
>> > Is this a backpatchable bug fix, or are we considering this only for the
>> > master branch?
>>
>> It would be good to get that backpatched, that's something we really
>> miss now IMO. Now it modifies libpgcommon, so Windows packagers (me
>> being one) will certainly need to patch a bit stuff but that's a
>> one-line changer so it's not a big deal. And I imagine that this is
>> actually the reason why Asif reported that as a bug as well.
>
> I think it'd be better to patch only pg_upgrade in back branches, so
> that there are no libpgcommon changes.  Seems that would make life
> easier for packagers (See the \connect thread, where Robert opined that
> it'd be better to duplicate some routines in back branches rather than
> refactor libpq code and move the common code to pgcommon.  I didn't
> completely agree with him at the time, but now that you mention
> packagers pain, maybe he has a point.)
>
> So let's do the refactoring in the master branch only, and duplicate
> the code in back branches.  Nasty, but it seems the more robust
> approach.

This plan sounds fine to me.
--
Michael

+1

On Fri, Mar 13, 2015 at 4:32 AM, Michael Paquier <michael.paquier@gmail.com>
wrote:

> On Thu, Mar 12, 2015 at 11:45 PM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
> > Michael Paquier wrote:
> >> On Thu, Mar 12, 2015 at 6:50 AM, Alvaro Herrera
> >> <alvherre@2ndquadrant.com> wrote:
> >
> >> > Is this a backpatchable bug fix, or are we considering this only for
> the
> >> > master branch?
> >>
> >> It would be good to get that backpatched, that's something we really
> >> miss now IMO. Now it modifies libpgcommon, so Windows packagers (me
> >> being one) will certainly need to patch a bit stuff but that's a
> >> one-line changer so it's not a big deal. And I imagine that this is
> >> actually the reason why Asif reported that as a bug as well.
> >
> > I think it'd be better to patch only pg_upgrade in back branches, so
> > that there are no libpgcommon changes.  Seems that would make life
> > easier for packagers (See the \connect thread, where Robert opined that
> > it'd be better to duplicate some routines in back branches rather than
> > refactor libpq code and move the common code to pgcommon.  I didn't
> > completely agree with him at the time, but now that you mention
> > packagers pain, maybe he has a point.)
> >
> > So let's do the refactoring in the master branch only, and duplicate
> > the code in back branches.  Nasty, but it seems the more robust
> > approach.
>
> This plan sounds fine to me.
> --
> Michael
>

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

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
>

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

I haven't followed this effort at all.  Are we on the same page about
backpatching, i.e. we want a minimal patch to apply to the back branches
(which would be 9.0 to 9.4 at this point, I think, right?); and a larger
patch that would apply to 9.5.

Now that I think about this, perhaps it'd be better to apply the same
patch to all branches, and once that's settled (i.e. buildfarmed until
we're sure it works) we refactor things in HEAD.

Speaking of buildfarming the patch -- how are we to actually test this?
As far as I understand, this code only matters when the server is being
run by an administrator account, which the buildfarm code doesn't do ...
So we're going to have to trust testers running stuff manually, right?

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Mar 19, 2015 at 7:04 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> I haven't followed this effort at all.  Are we on the same page about
> backpatching, i.e. we want a minimal patch to apply to the back branches
> (which would be 9.0 to 9.4 at this point, I think, right?); and a larger
> patch that would apply to 9.5.

Yes, that's the plan. What we have been arguing here is the refactoring patch.

> Now that I think about this, perhaps it'd be better to apply the same
> patch to all branches, and once that's settled (i.e. buildfarmed until
> we're sure it works) we refactor things in HEAD.

As a back-branch patch, it is possible to simply use the first version
that Asif has added upthread: it duplicates the restricted token stuff
directly in pg_upgrade and pg_resetxlog.

> Speaking of buildfarming the patch -- how are we to actually test this?
> As far as I understand, this code only matters when the server is being
> run by an administrator account, which the buildfarm code doesn't do ...

Well, I don't know. I am sure I could at least involve some internal
QE folks to get automated tests for this stuff, but the tests we do
are tightly integrated with our build infrastructure so this would
just stay internal, still I would get immediately reports if this
breaks and I could report and provide fixes easily. Having a solution
nicely integrated in the buildfarm scripts is something else though, a
build run being done only as one dedicated user.

> So we're going to have to trust testers running stuff manually, right?

I guess so.. Sorry I have no real solution to that.

Regards,
--
Michael

On Thu, Mar 19, 2015 at 6:42 AM, Asif Naeem wrote:
> 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.

After looking closely at those things, I agree with you: having some
code duplicated in two places instead of five is still a win... Now, I
think having this declaration of write_stderr in restricted_token.c is
confusing:
+#define write_stderr(fmt, ...) fprintf(stderr, fmt, __VA_ARGS__)
We could have bad surprises in the future if there is some work to
link pg_ctl with the stuff of restricted_token.c. Hence, could you
replace that with plain calls to fprintf(stderr, ...)? That's as well
what the other files of src/common are doing, so it would make the new
code more consistent with the rest.

[nitpicking]
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -30,6 +30,7 @@
 #endif

 #include "common/username.h"
+#include "common/restricted_token.h"

Alphabetical order of headers.
[/nitpicking]

Except those two small things, well I guess that's it for this patch.
The stuff for write_stderr may need a TODO item, but I think that even
with that we are going to finish by fixing the call to isatty that
looks indeed strange...

For the backpatching, the patches sent previously here (=>
http://www.postgresql.org/message-id/CAEB4t-OHNE95=n5U4ySsYkWipQsWeQuTBSJkaYJ63_1VzkzkhA@mail.gmail.com)
are fine IMO. They simply consist of a copy of what is done in
initdb.c. Now, perhaps we had better apply the patch duplicating the
logic to all branches, including HEAD, first, see what the buildfarm
says, and then finish wrapping up the refactoring patch.
Regards,
--
Michael

On Fri, Mar 20, 2015 at 11:14 PM, Asif Naeem <anaeem.it@gmail.com> wrote:
>> For the backpatching, the patches sent previously here (=>
>>
>> http://www.postgresql.org/message-id/CAEB4t-OHNE95=n5U4ySsYkWipQsWeQuTBSJkaYJ63_1VzkzkhA@mail.gmail.com)
>> are fine IMO. They simply consist of a copy of what is done in
>> initdb.c. Now, perhaps we had better apply the patch duplicating the
>> logic to all branches, including HEAD, first, see what the buildfarm
>> says, and then finish wrapping up the refactoring patch.
>
> PFA patch for older branches, v1 patch was not applying cleaning other than
> 94. Thanks.

Cool, thanks for all the patches!
Nothing to say about the patch on master, except this thing not fixed:
 #include "common/username.h"
+#include "common/restricted_token.h"
restricted_token.h should be declared before username.h.

In the back-branch patches, the format of the stderr messages is not
correct, and the suffix using progname is missing in pg_upgrade. For
example, this thing:
+               fprintf(stderr, _("could not open process token: error
code %lu\n"), GetLastError());
Should be rewritten like that for consistency with master and the
other utilities:
fprintf(stderr, _("%s: could not open process token: error code
%lu\n"), progname, GetLastError());
Perhaps now Bruce has a different opinion on the matter for pg_upgrade...

The 9.0 patch is missing as well, but I guess that the committer who
will pick up this series could get something up using the 9.1 patch...

I have also done some sanity checks down to 9.2 (got some issues when
building with ~9.1), and noticed no problems.
Regards,
--
Michael

On Tue, Mar 24, 2015 at 7:48 PM, Asif Naeem wrote:
> On Sat, Mar 21, 2015 at 5:09 PM, Michael Paquier wrote:
> Ah. Okey. I was not sure till which back branch it need to fixed. PFA patch
> for 90 branch.

OK. Thanks. This will surely help the committer who will pick up those
series of patches, particularly because it is for Windows.

>>
>> I have also done some sanity checks down to 9.2 (got some issues when
>> building with ~9.1), and noticed no problems.
>
>
> Thank you so much. I hope now there is nothing left from my side :).

I looked again at those patches and things are cool on my side, hence
I am switching it as "Ready for Committer".
Regards,
--
Michael