Re: [HACKERS] Supporting huge pages on Windows
| От | Magnus Hagander |
|---|---|
| Тема | Re: [HACKERS] Supporting huge pages on Windows |
| Дата | |
| Msg-id | CABUevEz8ui4yCVM8xNce_dO4-A7NJXKaUjJy1rdHy=0RD2Wkzw@mail.gmail.com обсуждение исходный текст |
| Ответ на | Re: Supporting huge pages on Windows ("Tsunakawa, Takayuki" <tsunakawa.takay@jp.fujitsu.com>) |
| Ответы |
Re: [HACKERS] Supporting huge pages on Windows
(Amit Kapila <amit.kapila16@gmail.com>)
Re: [HACKERS] Supporting huge pages on Windows (Alvaro Herrera <alvherre@2ndquadrant.com>) |
| Список | pgsql-hackers |
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?
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.
Вложения
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Magnus HaganderДата:
Сообщение: Re: [HACKERS] port of INSTALL file generation to XSLT