Re: [HACKERS] Supporting huge pages on Windows

On Sat, Dec 31, 2016 at 7:04 PM, Magnus Hagander <magnus@hagander.net> wrote:
>
>
> On Wed, Sep 28, 2016 at 2:26 AM, Tsunakawa, Takayuki
> <tsunakawa.takay@jp.fujitsu.com> wrote:
>>
>> > From: pgsql-hackers-owner@postgresql.org
>> > [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Robert Haas
>> > On Sun, Sep 25, 2016 at 10:45 PM, Tsunakawa, Takayuki
>> > <tsunakawa.takay@jp.fujitsu.com> wrote:
>> > > Credit: This patch is based on Thomas Munro's one.
>> >
>> > How are they different?
>>
>> As Thomas mentioned, his patch (only win32_shmem.c) might not have been
>> able to compile (though I didn't try.)  And it didn't have error processing
>> or documentation.  I added error handling, documentation, comments, and a
>> little bit of structural change.  The possibly biggest change, though it's
>> only one-liner in pg_ctl.c, is additionally required.  I failed to include
>> it in the first patch.  The attached patch includes that.
>>
>
>
> 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.
>
> Also, what happens with privileges that were granted to the groups that were
> removed? Are they retained or lost?
>
> Should we perhaps consider having pg_ctl instead *disable* all the
> privileges (rather than removing them), using AdjustTokenPrivileges? As a
> middle ground?
>

Sounds like a good idea.

>
>
>
> Taking a look at the actual code here, and a few small nitpicks:
>
> +                                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.
>
>
> 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".
>
>
> Is there a reason the error messages for AdjustTokenPrivileges() returning
> false and ERROR_NOT_ALL_ASSIGNED is different?
>
>
> 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.
>
> I don't think changing the global variable huge_pages like that is a very
> good idea. Wouldn't something like the attached be cleaner (not tested)? At
> least I find that one easier to read.
>

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


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