Re: [HACKERS] Supporting huge pages on Windows - Mailing list pgsql-hackers

From: pgsql-hackers-owner@postgresql.org
> [mailto:pgsql-hackers-owner@postgresql.org] 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?

https://msdn.microsoft.com/ja-jp/library/windows/desktop/bb530716(v=vs.85).aspx



> 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
useraccount 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
assignedto the user.  Anyway, I think it's the user's responsibility (and freedom) to assign desired privileges, and
pg_ctl'sdisabling 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
specifichint message to help users troubleshoot the error.
 


https://msdn.microsoft.com/ja-jp/library/windows/desktop/aa375202(v=vs.85).aspx

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


> 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
"SHOWhuge_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 [mailto:amit.kapila16@gmail.com]
> 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] -
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa366720(v=vs
> .85).aspx

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

Regards
Takayuki Tsunakawa



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers