On Wed, Sep 28, 2016 at 2:26 AM, Tsunakawa, Takayuki <
tsunakawa.ta...@jp.fujitsu.com> wrote:
> > From: pgsql-hackers-ow...@postgresql.org
> > [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Robert Haas
> > On Sun, Sep 25, 2016 at 10:45 PM, Tsunakawa, Takayuki
> > <tsunakawa.ta...@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.
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 8d7b3bf..e1c3c66 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1335,17 +1335,23 @@ include_dir 'conf.d'
</para>
<para>
- At present, this feature is supported only on Linux. The setting is
- ignored on other systems when set to <literal>try</literal>.
+ At present, this feature is supported only on Linux and Windows. The
+ setting is ignored on other systems when set to <literal>try</literal>.
</para>
<para>
The use of huge pages results in smaller page tables and less CPU time
- spent on memory management, increasing performance. For more details,
+ spent on memory management, increasing performance. For more details on Linux,
see <xref linkend="linux-huge-pages">.
</para>
<para>
+ This feature uses the large-page support on Windows. To use the large-page
+ support, you need to assign Lock page in memory user right to the Windows
+ user account which runs <productname>PostgreSQL</productname>.
+ </para>
+
+ <para>
With <varname>huge_pages</varname> set to <literal>try</literal>,
the server will try to use huge pages, but fall back to using
normal allocation if that fails. With <literal>on</literal>, failure
diff --git a/src/backend/port/win32_shmem.c b/src/backend/port/win32_shmem.c
index 0ff2c7e..88b195a 100644
--- a/src/backend/port/win32_shmem.c
+++ b/src/backend/port/win32_shmem.c
@@ -21,6 +21,7 @@ HANDLE UsedShmemSegID = INVALID_HANDLE_VALUE;
void *UsedShmemSegAddr = NULL;
static Size UsedShmemSegSize = 0;
+static bool EnableLockPagesPrivilege(int elevel);
static void pgwin32_SharedMemoryDelete(int status, Datum shmId);
/*
@@ -103,6 +104,61 @@ PGSharedMemoryIsInUse(unsigned long id1, unsigned long id2)
return true;
}
+/*
+ * EnableLockPagesPrivilege
+ *
+ * Try to acquire SeLockMemoryPrivilege so we can use large pages.
+ */
+static bool
+EnableLockPagesPrivilege(int elevel)
+{
+ HANDLE hToken;
+ TOKEN_PRIVILEGES tp;
+ LUID luid;
+
+ if (!OpenProcessToken(GetCurrentProcess(), TOKEN_ADJUST_PRIVILEGES | TOKEN_QUERY, &hToken))
+ {
+ ereport(elevel,
+ (errmsg("could not enable Lock pages in memory user right"),
+ errdetail("Failed system call was %s, error code %lu", "OpenProcessToken", GetLastError())));
+ return FALSE;
+ }
+
+ if (!LookupPrivilegeValue(NULL, SE_LOCK_MEMORY_NAME, &luid))
+ {
+ CloseHandle(hToken);
+ ereport(elevel,
+ (errmsg("could not enable Lock pages in memory user right"),
+ errdetail("Failed system call was %s, error code %lu", "LookupPrivilegeValue", GetLastError())));
+ return FALSE;
+ }
+ tp.PrivilegeCount = 1;
+ tp.Privileges[0].Luid = luid;
+ tp.Privileges[0].Attributes = SE_PRIVILEGE_ENABLED;
+
+ if (!AdjustTokenPrivileges(hToken, FALSE, &tp, 0, NULL, NULL))
+ {
+ ereport(elevel,
+ (errmsg("could not enable Lock pages in memory user right"),
+ errdetail("Failed system call was %s, error code %lu", "AdjustTokenPrivileges", GetLastError())));
+ CloseHandle(hToken);
+ return FALSE;
+ }
+
+ if (GetLastError() == ERROR_NOT_ALL_ASSIGNED)
+ {
+ ereport(elevel,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("could not enable Lock pages in memory user right"),
+ errhint("Assign Lock pages in memory user right to the Windows user account which runs PostgreSQL.")));
+ CloseHandle(hToken);
+ return FALSE;
+ }
+
+ CloseHandle(hToken);
+
+ return TRUE;
+}
/*
* PGSharedMemoryCreate
@@ -127,11 +183,8 @@ PGSharedMemoryCreate(Size size, bool makePrivate, int port,
int i;
DWORD size_high;
DWORD size_low;
-
- if (huge_pages == HUGE_PAGES_ON)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("huge pages not supported on this platform")));
+ SIZE_T largePageSize = 0;
+ DWORD flProtect = PAGE_READWRITE;
/* Room for a header? */
Assert(size > MAXALIGN(sizeof(PGShmemHeader)));
@@ -140,6 +193,34 @@ PGSharedMemoryCreate(Size size, bool makePrivate, int port,
UsedShmemSegAddr = NULL;
+ if (huge_pages == HUGE_PAGES_ON || huge_pages == HUGE_PAGES_TRY)
+ {
+ /* Does the processor support large pages? */
+ largePageSize = GetLargePageMinimum();
+ if (largePageSize == 0)
+ {
+ ereport(huge_pages == HUGE_PAGES_ON ? FATAL : DEBUG1,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("the processor does not support large pages")));
+ ereport(DEBUG1,
+ (errmsg("disabling huge pages")));
+ }
+ else if (!EnableLockPagesPrivilege(huge_pages == HUGE_PAGES_ON ? FATAL : DEBUG1))
+ {
+ ereport(DEBUG1,
+ (errmsg("disabling huge pages")));
+ }
+ else
+ {
+ /* Huge pages available and privilege enabled, so turn on */
+ flProtect = PAGE_READWRITE | SEC_COMMIT | SEC_LARGE_PAGES;
+
+ /* Round size up as appropriate. */
+ if (size % largePageSize != 0)
+ size += largePageSize - (size % largePageSize);
+ }
+ }
+
#ifdef _WIN64
size_high = size >> 32;
#else
@@ -163,7 +244,7 @@ PGSharedMemoryCreate(Size size, bool makePrivate, int port,
hmap = CreateFileMapping(INVALID_HANDLE_VALUE, /* Use the pagefile */
NULL, /* Default security attrs */
- PAGE_READWRITE, /* Memory is Read/Write */
+ flProtect,
size_high, /* Size Upper 32 Bits */
size_low, /* Size Lower 32 bits */
szShareMem);
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 946ba9e..9659000 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -3796,7 +3796,7 @@ static struct config_enum ConfigureNamesEnum[] =
{
{"huge_pages", PGC_POSTMASTER, RESOURCES_MEM,
- gettext_noop("Use of huge pages on Linux."),
+ gettext_noop("Use of huge pages on Linux/Windows."),
NULL
},
&huge_pages,
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
