> [] On Behalf Of Magnus Hagander
> For the pg_ctl changes, we're going from removing all privilieges from the
> token, to removing none. Are there any other privileges that we should be
> worried about? I think you may be correct in that it's overkill to do it,
> but I think we need some more specifics to decide that.

This page lists the privileges.  Is there anyhing you are concerned about?

> Also, what happens with privileges that were granted to the groups that
> were removed? Are they retained or lost?

Are you referring to the privileges of Administrators and PowerUsers that 
pg_ctl removes?  They are lost.  The Windows user account who actually runs 
PostgreSQL needs SeLockMemory privilege.

> Should we perhaps consider having pg_ctl instead *disable* all the
> privileges (rather than removing them), using AdjustTokenPrivileges? As
> a middle ground?

Sorry, I may misunderstand what you are suggesting, but AdjustTokenPrivilege() 
cannot enable the privilege which is not assigned to the user.  Anyway, I think 
it's the user's responsibility (and freedom) to assign desired privileges, and 
pg_ctl's disabling all privileges is overkill.

> +                                errdetail("Failed system call was %s,
> error code %lu", "LookupPrivilegeValue", GetLastError())));
> this seems to be a new pattern of code -- for other similar cases it just
> writes LookupPrivilegeValue inside the patch itself. I'm guessing the idea
> was for translatability, but I think it's better we stick to the existing
> pattern.

OK, modified.

> When AdjustTokenPrivileges() returns, you explicitly check for
> ERROR_NOT_ALL_ASSIGNED, which is good. But we should probably also
> explicitly check for ERROR_SUCCESS for that case. Right now that's the only
> two possible options that can be returned, but in a future version other
> result codes could be added and we'd miss them. Basically, "there should
> be an else branch".

OK, modified.

> Is there a reason the error messages for AdjustTokenPrivileges() returning
> false and ERROR_NOT_ALL_ASSIGNED is different?

As mentioned in the following page, the error cause is clearly defined.  So, I 
thought it'd be better to give a specific hint message to help users 
troubleshoot the error.

The token does not have one or more of the privileges specified in the NewState 
parameter. The function may succeed with this error value even if no privileges 
were adjusted. The PreviousState parameter indicates the privileges that were 

> There are three repeated blocks of
> +       if (huge_pages == HUGE_PAGES_ON || huge_pages == HUGE_PAGES_TRY)
> It threw me off in the initial reading, until I realized the upper levels
> of them can change the value of huge_pages.

OK, I like your code.

> I don't think changing the global variable huge_pages like that is a very
> good idea.
Yes, actually, I was afraid that it might be controversial to change the GUC 
value.  But I thought it may be better for "SHOW huge_pages" to reflect whether 
the huge_pages feature is effective.  Otherwise, users don't know about that.  
For example, wal_buffers behaves similarly; if it's set to -1 (default), "SHOW 
wal_buffers" displays the actual wal buffer size, not -1.  What do you think?  
Surely, the Linux code for huge_pages doesn't do that.  I'm OK with either.

From: Amit Kapila []
> Your version of the patch looks better than the previous one.  Don't you
> need to consider MEM_LARGE_PAGES in VirtualAllocEx call (refer
> pgwin32_ReserveSharedMemoryRegion)?  At least that is what is mentioned
> in MSDN [1].  Another point worth considering is that currently for
> VirtualAllocEx() we use PAGE_READWRITE as flProtect value, shouldn't it
> be same as used in CreateFileMapping() by patch.
> [1] -
> .85).aspx

No, it's not necessary.  Please see PGSharedMemoryReAttach(), where 
VirtualFree() is called to free the reserved address space and then call 
MapViewOfFile() to allocate the already created shared memory to that area.

Takayuki Tsunakawa

Attachment: win_large_pages_v4.patch
Description: win_large_pages_v4.patch

Sent via pgsql-hackers mailing list (
To make changes to your subscription:

Reply via email to