Re: Supporting huge pages on Windows

On Wed, Sep 28, 2016 at 2:26 AM, Tsunakawa, Takayuki <
tsunakawa(dot)takay(at)jp(dot)fujitsu(dot)com> wrote:

> > From: pgsql-hackers-owner(at)postgresql(dot)org
> > [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Robert Haas
> > On Sun, Sep 25, 2016 at 10:45 PM, Tsunakawa, Takayuki
> > <tsunakawa(dot)takay(at)jp(dot)fujitsu(dot)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?

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.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/