Thread: Supporting huge pages on Windows
Hello, The attached patch implements huge_pages on Windows. I'll add this to the CommitFest. The performance improvement was about 2% with the following select-only pgbench. The scaling factor is 200, where the databasesize is roughly 3GB. I ran the benchmark on my Windows 10 PC with 6 CPU cores and 16GB of RAM. pgbench -c18 -j6 -T60 -S bench Before running pgbench, I used pg_prewarm to cache all pgbench tables and indexes (excluding the history table) in the 4GBshared buffers. The averages of running pgbench three times are: huge_pages=off: 70412 tps huge_pages=on : 72100 tps The purpose of pg_ctl.c modification is to retain "Lock pages in memory" Windows user right in postgres. That user rightis necessary for the large-page support. The current pg_ctl removes all privileges when spawning postgres, which isoverkill. The system administrator should be able to assign appropriate privileges to the PostgreSQL service account. Credit: This patch is based on Thomas Munro's one. Regards Takayuki Tsunakawa
Attachment
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? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
> 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'thave error processing or documentation. I added error handling, documentation, comments, and a little bit of structuralchange. The possibly biggest change, though it's only one-liner in pg_ctl.c, is additionally required. I failedto include it in the first patch. The attached patch includes that. Regards Takayuki Tsunakawa
Attachment
On Wed, Sep 28, 2016 at 1:26 PM, 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 itdidn't have error processing or documentation. I added error handling, documentation, comments, and a little bit of structuralchange. The possibly biggest change, though it's only one-liner in pg_ctl.c, is additionally required. I failedto include it in the first patch. The attached patch includes that. Right, my patch[1] was untested sketch code. I had heard complaints about poor performance with large shared_buffers on Windows, and then I bumped into some recommendations to turn large pages on for a couple of other RDBMSs if using large buffer pool. So I wrote that patch based on the documentation to try some time in the future if I ever got trapped in a room with Windows, but I posted it just in case the topic would interest other hackers. Thanks for picking it up! > huge_pages=off: 70412 tps > huge_pages=on : 72100 tps Hmm. I guess it could be noise or random code rearrangement effects. I saw your recent post[2] proposing to remove the sentence about the 512MB effective limit and I wondered why you didn't go to larger sizes with a larger database and more run time. But I will let others with more benchmarking experience comment on the best approach to investigate Windows shared_buffers performance. [1] https://www.postgresql.org/message-id/CAEepm=075-bgHi_VDt4SCAmt+o_+1XaRap2zh7XwfZvT294oHA@mail.gmail.com [2] https://www.postgresql.org/message-id/0A3221C70F24FB45833433255569204D1F5EE995@G01JPEXMBYT05 -- Thomas Munro http://www.enterprisedb.com
From: Thomas Munro [mailto:thomas.munro@enterprisedb.com] > > huge_pages=off: 70412 tps > > huge_pages=on : 72100 tps > > Hmm. I guess it could be noise or random code rearrangement effects. I'm not the difference was a random noise, because running multiple set of three runs of pgbench (huge_pages = on, off, on,off, on...) produced similar results. But I expected a bit greater improvement, say, +10%. There may be better benchmarkmodel where the large page stands out, but I think pgbench is not so bad because its random data access would causeTLB cache misses. > I saw your recent post[2] proposing to remove the sentence about the 512MB > effective limit and I wondered why you didn't go to larger sizes with a > larger database and more run time. But I will let others with more > benchmarking experience comment on the best approach to investigate Windows > shared_buffers performance. Yes, I could have gone to 8GB of shared_buffers because my PC has 16GB of RAM, but I felt the number of variations was sufficient. Anyway, positive comments on benchmarking would be appreciated. Regards Takayuki Tsunakawa
On Wed, Sep 28, 2016 at 7:32 PM, Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> wrote: > From: Thomas Munro [mailto:thomas.munro@enterprisedb.com] >> > huge_pages=off: 70412 tps >> > huge_pages=on : 72100 tps >> >> Hmm. I guess it could be noise or random code rearrangement effects. > > I'm not the difference was a random noise, because running multiple set of three runs of pgbench (huge_pages = on, off,on, off, on...) produced similar results. But I expected a bit greater improvement, say, +10%. There may be betterbenchmark model where the large page stands out, but I think pgbench is not so bad because its random data access wouldcause TLB cache misses. Your ~2.4% number is similar to what was reported for Linux with 4GB shared_buffers: https://www.postgresql.org/message-id/20130913234125.GC13697%40roobarb.crazydogs.org Later in that thread there was a report of a dramatic ~15% increase in "best result" TPS, but that was with 60GB of shared_buffers on a machine with 256GB of RAM: https://www.postgresql.org/message-id/20131024060313.GA21888%40toroid.org -- Thomas Munro http://www.enterprisedb.com
On 2016-10-11 09:57:48 +1300, Thomas Munro wrote: > Later in that thread there was a report of a dramatic ~15% increase in > "best result" TPS, but that was with 60GB of shared_buffers on a > machine with 256GB of RAM: > > https://www.postgresql.org/message-id/20131024060313.GA21888%40toroid.org FWIW, I've seen 2-3x increases with ~60GB of s_b. Andres
From: Thomas Munro [mailto:thomas.munro@enterprisedb.com] > Your ~2.4% number is similar to what was reported for Linux with 4GB > shared_buffers: > > https://www.postgresql.org/message-id/20130913234125.GC13697%40roobarb > .crazydogs.org I'm relieved to know that a similar figure was gained on Linux. Thanks for the info. > Later in that thread there was a report of a dramatic ~15% increase in "best > result" TPS, but that was with 60GB of shared_buffers on a machine with > 256GB of RAM: > > https://www.postgresql.org/message-id/20131024060313.GA21888%40toroid. > org From: Andres Freund [mailto:andres@anarazel.de] > FWIW, I've seen 2-3x increases with ~60GB of s_b. Wow, nice figures. It's unfortunate that I don't have such a big machine available at hand. Regards Takayuki Tsunakawa
Regards,
Hari Babu
Fujitsu Australia
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.
Attachment
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
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
Attachment
On Thu, Jan 5, 2017 at 4:12 PM, Tsunakawa, Takayuki
<tsunakawa.takay@jp.fujitsu.com> wrote:
> [win_large_pages_v4.patch]
Just a small suggestion about the wording in this patch:
+ 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>.
In the Microsoft documentation I've seen, the privilege's name is
always written as "Lock Pages in Memory" (note: "Pages" plural, and
with initial capital letters). It's quite hard to parse the sentence
otherwise! How about this?
Huge pages are known as large pages on Windows. To use them,
you need to assign the user right Lock Pages in Memory to the Windows user account that runs
<productname>PostgreSQL</productname>.
+ ereport(elevel,
+ (errmsg("could not enable Lock pages in memory user right: error
code %lu", GetLastError()),
+ errdetail("Failed system call was OpenProcessToken.")));
Same comment about capitalisation of the privilege name in this and
other error messages.
--
Thomas Munro
http://www.enterprisedb.com
From: Thomas Munro [mailto:thomas.munro@enterprisedb.com]
> In the Microsoft documentation I've seen, the privilege's name is always
> written as "Lock Pages in Memory" (note: "Pages" plural, and with initial
> capital letters). It's quite hard to parse the sentence otherwise! How
> about this?
>
> Huge pages are known as large pages on Windows. To use them, you
> need to
> assign the user right Lock Pages in Memory to the Windows user
> account
> that runs <productname>PostgreSQL</productname>.
>
> + ereport(elevel,
> + (errmsg("could not enable Lock pages in memory user right: error
> code %lu", GetLastError()),
> + errdetail("Failed system call was OpenProcessToken.")));
>
> Same comment about capitalisation of the privilege name in this and other
> error messages.
Thanks, your sentences are surely better. Fixed.
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
Attachment
On Thu, Jan 5, 2017 at 8:42 AM, Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> wrote: >> 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. > PGSharedMemoryReAttach is called after the startup of new process whereas pgwin32_ReserveSharedMemoryRegion is called before the new process could actually start. Basically, pgwin32_ReserveSharedMemoryRegion is used to reserve shared memory for each backend, so calling VirtualAlloc there should follow spec for huge pages. If you have some reason for not using, then it is not clear from your reply, can you try to explain in detail. I have tried to test v4 version of the patch and it is always failing in below error after call to AdjustTokenPrivileges: + if (GetLastError() != ERROR_SUCCESS) + { + 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."))); I have ensured that user used to run PostgreSQL has "Lock pages in memory" privilege/rights. I have followed msdn tips [1] to do that (restarted the m/c after assigning privilege). I am using Win7. Can you share the steps you have followed to test and your windows m/c details? + if (!LookupPrivilegeValue(NULL, SE_LOCK_MEMORY_NAME, &luid)) + { + CloseHandle(hToken); + ereport(elevel, + (errmsg("could not enable Lock pages in memory user right: error code %lu", GetLastError()), + errdetail("Failed system call was LookupPrivilegeValue."))); + return FALSE; + } The order of closing handle and ereport is different here than other places in the same function. If there is no specific reason for doing so, then keep the order consistent. [1] - https://msdn.microsoft.com/en-us/library/windows/desktop/ff961911(v=vs.85).aspx -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Hello, Amit, Magnus,
I'm sorry for my late reply. Yesterday was a national holiday in Japan.
From: pgsql-hackers-owner@postgresql.org
> [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Amit Kapila
> PGSharedMemoryReAttach is called after the startup of new process whereas
> pgwin32_ReserveSharedMemoryRegion is called before the new process could
> actually start. Basically, pgwin32_ReserveSharedMemoryRegion is used to
> reserve shared memory for each backend, so calling VirtualAlloc there should
> follow spec for huge pages. If you have some reason for not using, then
> it is not clear from your reply, can you try to explain in detail.
OK. The processing takes place in the following order:
1. postmaster calls CreateProcess() to start a child postgres in a suspended state.
2. postmaster calls VirtualAlloc(MEM_RESERVE) in pgwin32_ReserveSharedMemoryRegion() to reserve the virtual address
spacein the child to secure space for the shared memory. This call just affects the virtual address space and does not
allocatephysical memory. So the large page is still irrelevant.
3. postmaster resumes execution of the child postgres.
4. The child postgres calls VirtualFree(MEM_RESERVE) in PGSharedMemoryReAttach() to release the reserved virtual
addressspace. Here, the effect of VirtualAlloc() is invalidated anyway.
5. The child process calls MapViewOfFile() in PGSharedMemoryReAttach() to map the shared memory at the same address.
Hereafter,the large page option specified in CreateFileMapping() call is relevant.
> + if (!LookupPrivilegeValue(NULL, SE_LOCK_MEMORY_NAME, &luid)) {
> + CloseHandle(hToken); ereport(elevel, (errmsg("could not enable Lock
> + pages in memory user right: error
> code %lu", GetLastError()),
> + errdetail("Failed system call was LookupPrivilegeValue."))); return
> + FALSE; }
>
> The order of closing handle and ereport is different here than other places
> in the same function. If there is no specific reason for doing so, then
> keep the order consistent.
You are right, I modified the patch to call CloseHandle() after ereport() so that ereport() CloseHandle() wouldn't
changethe error code for ereport(). That's the order used in postmaster.c.
> I have tried to test v4 version of the patch and it is always failing in
> below error after call to AdjustTokenPrivileges:
>
> + if (GetLastError() != ERROR_SUCCESS)
> + {
> + 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.")));
>
> I have ensured that user used to run PostgreSQL has "Lock pages in memory"
> privilege/rights. I have followed msdn tips [1] to do that (restarted the
> m/c after assigning privilege). I am using Win7. Can you share the steps
> you have followed to test and your windows m/c details?
>
> [1] -
> https://msdn.microsoft.com/en-us/library/windows/desktop/ff961911(v=vs
> .85).aspx
I succeeded by following the same procedure using secpol.msc on Win10, running 64-bit PostgreSQL. I started PostgreSQL
asa Windows service because it's the normal way, with the service account being a regular Windows user account(i.e. not
LocalSystem).
But... I failed to start PostgreSQL by running "pg_ctl start" from a command prompt, receiving the same error message
asyou. On the other hand, I could start PostgreSQL on a command prompt with administrative privileges (right-click on
"Commandprompt" from the Start menu and select "Run as an administrator" in the menu. It seems that Windows removes
manyprivileges, including "Lock Pages in Memory", when starting the normal command prompt. As its evidence, you can
usethe attached priv.c to see what privileges are assigned and and enabled/disabled. Build it like "cl priv.c" and
justrun priv.exe on each command prompt. Those runs show different privileges.
Should I need to do something, e.g. explain in the document that the user should use the command prompt with
administrativeprivileges when he uses huge_pages?
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
Attachment
On Tue, Jan 10, 2017 at 8:49 AM, Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> wrote: > Hello, Amit, Magnus, > > I'm sorry for my late reply. Yesterday was a national holiday in Japan. > > > From: pgsql-hackers-owner@postgresql.org >> [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Amit Kapila >> PGSharedMemoryReAttach is called after the startup of new process whereas >> pgwin32_ReserveSharedMemoryRegion is called before the new process could >> actually start. Basically, pgwin32_ReserveSharedMemoryRegion is used to >> reserve shared memory for each backend, so calling VirtualAlloc there should >> follow spec for huge pages. If you have some reason for not using, then >> it is not clear from your reply, can you try to explain in detail. > > OK. The processing takes place in the following order: > > 1. postmaster calls CreateProcess() to start a child postgres in a suspended state. > 2. postmaster calls VirtualAlloc(MEM_RESERVE) in pgwin32_ReserveSharedMemoryRegion() to reserve the virtual address spacein the child to secure space for the shared memory. This call just affects the virtual address space and does not allocatephysical memory. So the large page is still irrelevant. Okay, today again reading VirtualAlloc specs, I could see that MEM_LARGE_PAGES is not not required to reserve the memory region. It is only required during allocation. > > I succeeded by following the same procedure using secpol.msc on Win10, running 64-bit PostgreSQL. I started PostgreSQLas a Windows service because it's the normal way, with the service account being a regular Windows user account(i.e.not LocalSystem). > > But... I failed to start PostgreSQL by running "pg_ctl start" from a command prompt, receiving the same error message asyou. On the other hand, I could start PostgreSQL on a command prompt with administrative privileges (right-click on "Commandprompt" from the Start menu and select "Run as an administrator" in the menu. Hmm. It doesn't work even on a command prompt with administrative privileges. It gives below error: waiting for server to start....2017-01-17 11:20:13.780 IST [4788] FATAL: could not create shared memory segment: error code 1450 2017-01-17 11:20:13.780 IST [4788] DETAIL: Failed system call was CreateFileMap ping(size=148897792, name=Global/PostgreSQL:E:/WorkSpace/PostgreSQL/master/Data) . 2017-01-17 11:20:13.780 IST [4788] LOG: database system is shut downstopped waiting pg_ctl: could not start server Examine the log output. Now, error code 1450 can occur due to insufficient system resources, so I have tried by increasing the size of shared memory (higher value of shared_buffers) without your patch and it works. This indicates some problem with the patch. > It seems that Windows removes many privileges, including "Lock Pages in Memory", when starting the normal command prompt. As its evidence, you can use the attached priv.c to see what privileges are assigned and and enabled/disabled. Buildit like "cl priv.c" and just run priv.exe on each command prompt. Those runs show different privileges. > This is bad. > Should I need to do something, e.g. explain in the document that the user should use the command prompt with administrativeprivileges when he uses huge_pages? > I think it is better to document in some way if we decide to go-ahead with the patch. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
From: Amit Kapila [mailto:amit.kapila16@gmail.com] > Hmm. It doesn't work even on a command prompt with administrative > privileges. It gives below error: > > waiting for server to start....2017-01-17 11:20:13.780 IST [4788] FATAL: > could not create shared memory segment: error code 1450 > 2017-01-17 11:20:13.780 IST [4788] DETAIL: Failed system call was > CreateFileMap ping(size=148897792, > name=Global/PostgreSQL:E:/WorkSpace/PostgreSQL/master/Data) > . > 2017-01-17 11:20:13.780 IST [4788] LOG: database system is shut down > stopped waiting > pg_ctl: could not start server > Examine the log output. > > > Now, error code 1450 can occur due to insufficient system resources, so > I have tried by increasing the size of shared memory (higher value of > shared_buffers) without your patch and it works. This indicates some > problem with the patch. Hmm, the large-page requires contiguous memory for each page, so this error could occur on a long-running system where thememory is heavily fragmented. For example, please see the following page and check the memory with RAMMap program referredthere. http://blog.dbi-services.com/large-pages-and-memory_target-on-windows/ BTW, is your OS or PostgreSQL 32-bit? > > It seems that Windows removes many privileges, including "Lock Pages > in Memory", when starting the normal command prompt. As its evidence, you > can use the attached priv.c to see what privileges are assigned and and > enabled/disabled. Build it like "cl priv.c" and just run priv.exe on each > command prompt. Those runs show different privileges. > > > > This is bad. > > > Should I need to do something, e.g. explain in the document that the user > should use the command prompt with administrative privileges when he uses > huge_pages? > > > > I think it is better to document in some way if we decide to go-ahead with > the patch. Sure, I added these sentences. + To start the database server on the command prompt as a standalone process, + not as a Windows service, run the command prompt as an administrator or + disable the User Access Control (UAC). When the UAC is enabled, the normal + command prompt revokes the user right Lock Pages in Memory. 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
Attachment
On Mon, Jan 30, 2017 at 7:16 AM, Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> wrote: > From: Amit Kapila [mailto:amit.kapila16@gmail.com] >> Hmm. It doesn't work even on a command prompt with administrative >> privileges. It gives below error: >> >> waiting for server to start....2017-01-17 11:20:13.780 IST [4788] FATAL: >> could not create shared memory segment: error code 1450 >> 2017-01-17 11:20:13.780 IST [4788] DETAIL: Failed system call was >> CreateFileMap ping(size=148897792, >> name=Global/PostgreSQL:E:/WorkSpace/PostgreSQL/master/Data) >> . >> 2017-01-17 11:20:13.780 IST [4788] LOG: database system is shut down >> stopped waiting >> pg_ctl: could not start server >> Examine the log output. >> >> >> Now, error code 1450 can occur due to insufficient system resources, so >> I have tried by increasing the size of shared memory (higher value of >> shared_buffers) without your patch and it works. This indicates some >> problem with the patch. > > Hmm, the large-page requires contiguous memory for each page, so this error could occur on a long-running system wherethe memory is heavily fragmented. For example, please see the following page and check the memory with RAMMap programreferred there. > I don't have RAMMap and it might take some time to investigate what is going on, but I think in such a case even if it works we should keep the default value of huge_pages as off on Windows. I request somebody else having access to Windows m/c to test this patch and if it works then we can move forward. > http://blog.dbi-services.com/large-pages-and-memory_target-on-windows/ > > BTW, is your OS or PostgreSQL 32-bit? > both 64-bit. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Magnus Hagander wrote:
> 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.
There are two reasons for doing things this way. One is that you reduce
the chances of a translator making a mistake with the function name (say
just a typo, or in egregious cases they may even translate the function
name). The other is that if you have many of these messages, you only
translate the generic part once instead of having the same message
a handful of times, exactly identical but for the function name.
So please do apply that kind of pattern wherever possible. We already
have the proposed error message, twice. No need for two more
occurrences of it.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Jan 30, 2017 at 10:46 AM, Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> wrote: > From: Amit Kapila [mailto:amit.kapila16@gmail.com] >> I think it is better to document in some way if we decide to go-ahead with >> the patch. > > Sure, I added these sentences. Patch has been moved to CF 2017-03. There is a recent new patch, and the discussion is moving on. -- Michael
From: Amit Kapila [mailto:amit.kapila16@gmail.com]
> > Hmm, the large-page requires contiguous memory for each page, so this
> error could occur on a long-running system where the memory is heavily
> fragmented. For example, please see the following page and check the memory
> with RAMMap program referred there.
> >
>
> I don't have RAMMap and it might take some time to investigate what is going
> on, but I think in such a case even if it works we should keep the default
> value of huge_pages as off on Windows. I request somebody else having
> access to Windows m/c to test this patch and if it works then we can move
> forward.
You are right. I modified the patch so that the code falls back to the non-huge page when CreateFileMapping() fails
dueto the shortage of large pages. That's what the Linux version does.
The other change is to parameterize the Win32 function names in the messages in EnableLockPagePrivileges(). This is to
avoidadding almost identical messages unnecessarily. I followed Alvaro's comment. I didn't touch the two existing
sitesthat embed Win32 function names. I'd like to leave it up to the committer to decide whether to change as well,
becausechanging them might make it a bit harder to apply some bug fixes to earlier releases.
FYI, I could reproduce the same error as Amit on 32-bit Win7, where the total RAM is 3.5 GB and available RAM is 2 GB.
Iused the attached largepage.c. Immediately after the system boot, I could only allocate 8 large pages. When I first
triedto allocate 32 large pages, the test program produced:
large page size = 2097152
allocating 32 large pages...
CreateFileMapping failed: error code = 1450
You can build the test program as follows:
cl largepage.c advapi32.lib
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
Attachment
Hi, I tried to test v8 version of patch. Firstly, I was able to start the postgresql server process with 'huge_pages' set to on. I had to follow the instructions given in MSDN[1] to enable lock pages in memory option and also had to start the postgresql server process as admin user. test=# show huge_pages;huge_pages ------------on (1 row) To start with, I ran the regression test-suite and didn't find any failures. But, then I am not sure if huge_pages are getting used or not. However, upon checking the settings for huge_pages and I found it as 'on'. I am assuming, if huge pages is not being used due to shortage of large pages, it should have fallen back to non-huge pages. I also ran the pgbench tests on read-only workload and here are the results I got. pgbench -c 4 -j 4 - T 600 bench huge_pages=on, TPS = 21120.768085 huge_pages=off, TPS = 20606.288995 [1] - https://msdn.microsoft.com/en-IN/library/ms190730.aspx -- With Regards, Ashutosh Sharma EnterpriseDB:http://www.enterprisedb.com On Thu, Feb 23, 2017 at 12:59 PM, Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> wrote: > From: Amit Kapila [mailto:amit.kapila16@gmail.com] >> > Hmm, the large-page requires contiguous memory for each page, so this >> error could occur on a long-running system where the memory is heavily >> fragmented. For example, please see the following page and check the memory >> with RAMMap program referred there. >> > >> >> I don't have RAMMap and it might take some time to investigate what is going >> on, but I think in such a case even if it works we should keep the default >> value of huge_pages as off on Windows. I request somebody else having >> access to Windows m/c to test this patch and if it works then we can move >> forward. > > You are right. I modified the patch so that the code falls back to the non-huge page when CreateFileMapping() fails dueto the shortage of large pages. That's what the Linux version does. > > The other change is to parameterize the Win32 function names in the messages in EnableLockPagePrivileges(). This is toavoid adding almost identical messages unnecessarily. I followed Alvaro's comment. I didn't touch the two existing sitesthat embed Win32 function names. I'd like to leave it up to the committer to decide whether to change as well, becausechanging them might make it a bit harder to apply some bug fixes to earlier releases. > > FYI, I could reproduce the same error as Amit on 32-bit Win7, where the total RAM is 3.5 GB and available RAM is 2 GB. I used the attached largepage.c. Immediately after the system boot, I could only allocate 8 large pages. When I firsttried to allocate 32 large pages, the test program produced: > > large page size = 2097152 > allocating 32 large pages... > CreateFileMapping failed: error code = 1450 > > You can build the test program as follows: > > cl largepage.c advapi32.lib > > 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 >
On Thu, Feb 23, 2017 at 8:29 PM, Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> wrote: > [win_large_pages_v8.patch] + Huge pages are known as large pages on Windows. To use them, you need to + assign the user right Lock Pages in Memory to the Windows user account + that runs <productname>PostgreSQL</productname>. + Huge pages are known as large pages on Windows. To use them, you need to + To start the database server on the command prompt as a standalone process, + not as a Windows service, run the command prompt as an administrator or + disable the User Access Control (UAC). When the UAC is enabled, the normal + command prompt revokes the user right Lock Pages in Memory. The line beginning 'Huge pages are known as...' has been accidentally duplicated. -- Thomas Munro http://www.enterprisedb.com
From: pgsql-hackers-owner@postgresql.org
> [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Ashutosh Sharma
> To start with, I ran the regression test-suite and didn't find any failures.
> But, then I am not sure if huge_pages are getting used or not. However,
> upon checking the settings for huge_pages and I found it as 'on'. I am
> assuming, if huge pages is not being used due to shortage of large pages,
> it should have fallen back to non-huge pages.
You are right, the server falls back to non-huge pages when the large pages run short.
> I also ran the pgbench tests on read-only workload and here are the results
> I got.
>
> pgbench -c 4 -j 4 - T 600 bench
>
> huge_pages=on, TPS = 21120.768085
> huge_pages=off, TPS = 20606.288995
Thanks. It's about 2% improvement, which is the same as what I got.
From: Thomas Munro [mailto:thomas.munro@enterprisedb.com]
> The line beginning 'Huge pages are known as...' has been accidentally
> duplicated.
Oops, how careless I was. Fixed. As Ashutosh referred, I added a very simple suggestion to use Windows Group Policy
tool.
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
Attachment
On 3/8/17 8:36 PM, Tsunakawa, Takayuki wrote: > From: pgsql-hackers-owner@postgresql.org >> [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Ashutosh Sharma >> To start with, I ran the regression test-suite and didn't find any failures. >> But, then I am not sure if huge_pages are getting used or not. However, >> upon checking the settings for huge_pages and I found it as 'on'. I am >> assuming, if huge pages is not being used due to shortage of large pages, >> it should have fallen back to non-huge pages. > > You are right, the server falls back to non-huge pages when the large pages run short. > >> I also ran the pgbench tests on read-only workload and here are the results >> I got. >> >> pgbench -c 4 -j 4 - T 600 bench >> >> huge_pages=on, TPS = 21120.768085 >> huge_pages=off, TPS = 20606.288995 > > Thanks. It's about 2% improvement, which is the same as what I got. > > > From: Thomas Munro [mailto:thomas.munro@enterprisedb.com] >> The line beginning 'Huge pages are known as...' has been accidentally >> duplicated. > > Oops, how careless I was. Fixed. As Ashutosh referred, I added a very simple suggestion to use Windows Group Policy tool. Amit, Magnus, you are signed up as reviewers for this patch. Do you know when you'll have a chance to take a look? Thanks, -- -David david@pgmasters.net
On Wed, Mar 22, 2017 at 12:07 AM, David Steele <david@pgmasters.net> wrote: > On 3/8/17 8:36 PM, Tsunakawa, Takayuki wrote: >> >> From: pgsql-hackers-owner@postgresql.org >>> >>> [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Ashutosh Sharma >>> To start with, I ran the regression test-suite and didn't find any >>> failures. >>> But, then I am not sure if huge_pages are getting used or not. However, >>> upon checking the settings for huge_pages and I found it as 'on'. I am >>> assuming, if huge pages is not being used due to shortage of large pages, >>> it should have fallen back to non-huge pages. >> >> >> You are right, the server falls back to non-huge pages when the large >> pages run short. >> >>> I also ran the pgbench tests on read-only workload and here are the >>> results >>> I got. >>> >>> pgbench -c 4 -j 4 - T 600 bench >>> >>> huge_pages=on, TPS = 21120.768085 >>> huge_pages=off, TPS = 20606.288995 >> >> >> Thanks. It's about 2% improvement, which is the same as what I got. >> >> >> From: Thomas Munro [mailto:thomas.munro@enterprisedb.com] >>> >>> The line beginning 'Huge pages are known as...' has been accidentally >>> duplicated. >> >> >> Oops, how careless I was. Fixed. As Ashutosh referred, I added a very >> simple suggestion to use Windows Group Policy tool. > > > Amit, Magnus, you are signed up as reviewers for this patch. Do you know > when you'll have a chance to take a look? > I have provided my feedback and I could not test it on my machine. I think Ashutosh Sharma has tested it. I can give it another look, but not sure if it helps. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Hi Ashutosh, On 3/22/17 8:52 AM, Amit Kapila wrote: > On Wed, Mar 22, 2017 at 12:07 AM, David Steele <david@pgmasters.net> wrote: >> >> Amit, Magnus, you are signed up as reviewers for this patch. Do you know >> when you'll have a chance to take a look? >> > > I have provided my feedback and I could not test it on my machine. I > think Ashutosh Sharma has tested it. I can give it another look, but > not sure if it helps. I know you are not signed up as a reviewer, but have you take a look at this patch? Thanks, -- -David david@pgmasters.net
Hi, On Fri, Mar 24, 2017 at 9:00 PM, David Steele <david@pgmasters.net> wrote: > Hi Ashutosh, > > On 3/22/17 8:52 AM, Amit Kapila wrote: >> >> On Wed, Mar 22, 2017 at 12:07 AM, David Steele <david@pgmasters.net> >> wrote: >>> >>> >>> Amit, Magnus, you are signed up as reviewers for this patch. Do you know >>> when you'll have a chance to take a look? >>> >> >> I have provided my feedback and I could not test it on my machine. I >> think Ashutosh Sharma has tested it. I can give it another look, but >> not sure if it helps. > > > I know you are not signed up as a reviewer, but have you take a look at this > patch? Well, I had a quick look into the patch just because I wanted the test it as I am having windows setup. But, I never looked into the patch from reviewer's perspective. The intention was to reverify the test results shared by Tsunawaka. -- With Regards, Ashutosh Sharma EnterpriseDB:http://www.enterprisedb.com
On 3/24/17 12:51 PM, Ashutosh Sharma wrote: > Hi, > > On Fri, Mar 24, 2017 at 9:00 PM, David Steele <david@pgmasters.net> wrote: >> Hi Ashutosh, >> >> On 3/22/17 8:52 AM, Amit Kapila wrote: >>> >>> On Wed, Mar 22, 2017 at 12:07 AM, David Steele <david@pgmasters.net> >>> wrote: >>>> >>>> >>>> Amit, Magnus, you are signed up as reviewers for this patch. Do you know >>>> when you'll have a chance to take a look? >>>> >>> >>> I have provided my feedback and I could not test it on my machine. I >>> think Ashutosh Sharma has tested it. I can give it another look, but >>> not sure if it helps. >> >> >> I know you are not signed up as a reviewer, but have you take a look at this >> patch? > > Well, I had a quick look into the patch just because I wanted the test > it as I am having windows setup. But, I never looked into the patch > from reviewer's perspective. The intention was to reverify the test > results shared by Tsunawaka. Thanks, Ashutosh. It's not clear to me what state this patch should be in. Is there more review that needs to be done or is it ready for a committer? -- -David david@pgmasters.net
From: pgsql-hackers-owner@postgresql.org > [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of David Steele > It's not clear to me what state this patch should be in. Is there more > review that needs to be done or is it ready for a committer? I would most appreciate it if Magnus could review the code, because he gave me the most critical comment that the value ofhuge_page variable should not be changed on the fly, and I did so. I hope that will be the final review. Regards Takayuki Tsunakawa
On Thu, Mar 9, 2017 at 7:06 AM, Tsunakawa, Takayuki
<tsunakawa.takay@jp.fujitsu.com> wrote:
> From: pgsql-hackers-owner@postgresql.org
>> [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Ashutosh Sharma
>> To start with, I ran the regression test-suite and didn't find any failures.
>> But, then I am not sure if huge_pages are getting used or not. However,
>> upon checking the settings for huge_pages and I found it as 'on'. I am
>> assuming, if huge pages is not being used due to shortage of large pages,
>> it should have fallen back to non-huge pages.
>
> You are right, the server falls back to non-huge pages when the large pages run short.
>
The latest patch looks good to me apart from one Debug message, so I
have marked it as Ready For Committer.
+ ereport(DEBUG1,
+ (errmsg("disabling huge pages")));
I think this should be similar to what we display in sysv_shmem.c as below:
elog(DEBUG1, "mmap(%zu) with MAP_HUGETLB failed, huge pages disabled: %m",
allocsize);
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
From: Amit Kapila [mailto:amit.kapila16@gmail.com]
> The latest patch looks good to me apart from one Debug message, so I have
> marked it as Ready For Committer.
Thank you so much!
> + ereport(DEBUG1,
> + (errmsg("disabling huge pages")));
>
> I think this should be similar to what we display in sysv_shmem.c as below:
>
> elog(DEBUG1, "mmap(%zu) with MAP_HUGETLB failed, huge pages disabled: %m",
> allocsize);
I understood you suggested this to make the reason clear for disabling huge pages. OK, done as follows.
+ elog(DEBUG1, "CreateFileMapping(%llu) with SEC_LARGE_PAGES failed "
+ "due to insufficient large pages, huge pages disabled",
+ size);
I hope this will be committed soon.
Regards
Takayuki Tsunakawa
Attachment
On Fri, Mar 31, 2017 at 8:05 AM, Tsunakawa, Takayuki
<tsunakawa.takay@jp.fujitsu.com> wrote:
> From: Amit Kapila [mailto:amit.kapila16@gmail.com]
>> The latest patch looks good to me apart from one Debug message, so I have
>> marked it as Ready For Committer.
>
> Thank you so much!
>
>
>> + ereport(DEBUG1,
>> + (errmsg("disabling huge pages")));
>>
>> I think this should be similar to what we display in sysv_shmem.c as below:
>>
>> elog(DEBUG1, "mmap(%zu) with MAP_HUGETLB failed, huge pages disabled: %m",
>> allocsize);
>
> I understood you suggested this to make the reason clear for disabling huge pages. OK, done as follows.
>
> + elog(DEBUG1, "CreateFileMapping(%llu) with SEC_LARGE_PAGES failed "
> + "due to insufficient large pages, huge pages disabled",
> + size);
>
You should use %zu instead of %llu to print Size type of variable.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
From: pgsql-hackers-owner@postgresql.org > [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Amit Kapila > You should use %zu instead of %llu to print Size type of variable. I did so at first, but it didn't work with Visual Studio 2010 at hand. It doesn't support %z which is added in C99. But "I" (capital i) instead of "ll" seems appropriate as the following page says. I chose this. https://en.wikipedia.org/wiki/Printf_format_string I For signed integer types, causes printf to expect ptrdiff_t-sized integer argument; for unsigned integer types, causesprintf to expect size_t-sized integer argument. Commonly found in Win32/Win64 platforms. Regards Takayuki Tsunakawa
Attachment
On Mon, Apr 3, 2017 at 1:12 PM, Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> wrote: > From: pgsql-hackers-owner@postgresql.org >> [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Amit Kapila >> You should use %zu instead of %llu to print Size type of variable. > > I did so at first, but it didn't work with Visual Studio 2010 at hand. It doesn't support %z which is added in C99. > > But "I" (capital i) instead of "ll" seems appropriate as the following page says. I chose this. > > https://en.wikipedia.org/wiki/Printf_format_string > > I For signed integer types, causes printf to expect ptrdiff_t-sized integer argument; for unsigned integer types, causesprintf to expect size_t-sized integer argument. Commonly found in Win32/Win64 platforms. + elog(DEBUG1, "CreateFileMapping(%Iu) with SEC_LARGE_PAGES failed " + "due to insufficient large pages, huge pages disabled", + size); Amit is right, you had better use %zu as we do in all the other places of the code dealing with Size variables that are printed. When compiling with Visual Studio, Postgres falls back to the implementation of sprintf in src/port/snprintf.c so that's not something to worry about. -- Michael
From: Michael Paquier [mailto:michael.paquier@gmail.com] > Amit is right, you had better use %zu as we do in all the other places of > the code dealing with Size variables that are printed. When compiling with > Visual Studio, Postgres falls back to the implementation of sprintf in > src/port/snprintf.c so that's not something to worry about. Thanks, fixed. Regards Takayuki Tsunakawa
Attachment
On 2017-04-03 04:56:45 +0000, Tsunakawa, Takayuki wrote:
> +/*
> + * 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: error code %lu", GetLastError()),
> + errdetail("Failed system call was %s.", "OpenProcessToken")));
> + return FALSE;
> + }
I don't think the errdetail is quite right - OpenProcessToken isn't
really a syscall, is it? But then it's a common pattern already in
wind32_shmem.c...
> + if (!LookupPrivilegeValue(NULL, SE_LOCK_MEMORY_NAME, &luid))
> + {
> + ereport(elevel,
> + (errmsg("could not enable Lock Pages in Memory user right: error code %lu", GetLastError()),
> + errdetail("Failed system call was %s.", "LookupPrivilegeValue")));
Other places in the file actually log the arguments to the function...
Wonder if we should quote "Lock Pages in Memory" or add dashes, to make
sure it's clear that that's the right?
> + 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;
Why don't we just add the relevant flag, instead of overwriting the
previous contents?
> diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
> index c63819b..2358ed0 100644
> --- a/src/bin/pg_ctl/pg_ctl.c
> +++ b/src/bin/pg_ctl/pg_ctl.c
> @@ -1708,11 +1708,6 @@ typedef BOOL (WINAPI * __SetInformationJobObject) (HANDLE, JOBOBJECTINFOCLASS, L
> typedef BOOL (WINAPI * __AssignProcessToJobObject) (HANDLE, HANDLE);
> typedef BOOL (WINAPI * __QueryInformationJobObject) (HANDLE, JOBOBJECTINFOCLASS, LPVOID, DWORD, LPDWORD);
>
> -/* Windows API define missing from some versions of MingW headers */
> -#ifndef DISABLE_MAX_PRIVILEGE
> -#define DISABLE_MAX_PRIVILEGE 0x1
> -#endif
> -
> /*
> * Create a restricted token, a job object sandbox, and execute the specified
> * process with it.
> @@ -1794,7 +1789,7 @@ CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo, bool as_ser
> }
>
> b = _CreateRestrictedToken(origToken,
> - DISABLE_MAX_PRIVILEGE,
> + 0,
> sizeof(dropSids) / sizeof(dropSids[0]),
> dropSids,
> 0, NULL,
Uh - isn't that a behavioural change? Was this discussed?
Greetings,
Andres Freund
From: pgsql-hackers-owner@postgresql.org
> [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Andres Freund
> I don't think the errdetail is quite right - OpenProcessToken isn't really
> a syscall, is it? But then it's a common pattern already in wind32_shmem.c...
Yes, "Win32 API function" would be correct, but I followed the existing code...
> > + errdetail("Failed system call was %s.",
> > +"LookupPrivilegeValue")));
>
> Other places in the file actually log the arguments to the function...
The only place is CreateFileMapping. Other places (DuplicateHandle and MapViewOfFileEx) don't log arguments. I guess
theoriginal developer thought that size and name arguments to CreateFileMapping() might be useful for troubleshooting.
> Wonder if we should quote "Lock Pages in Memory" or add dashes, to make
> sure it's clear that that's the right?
I saw several Microsoft pages, including a page someone introduced me here, and they didn't quote the user right. I'm
comfortablewith the current code, but I don't mind if the committer adds some quotation.
> > + flProtect = PAGE_READWRITE | SEC_COMMIT |
> SEC_LARGE_PAGES;
>
> Why don't we just add the relevant flag, instead of overwriting the previous
> contents?
I don't have strong opinion on this...
> Uh - isn't that a behavioural change? Was this discussed?
Yes, I described this in the first mail. Magnus asked about this later and I replied.
Regards
Takayuki Tsunakawa
On 2017-01-05 03:12:09 +0000, Tsunakawa, Takayuki wrote: > 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 Aren't like nearly all of them a concern? We gone from having some containment (not being to create users, shut the system down, ...), to none. I do however think there's a fair argument to be made that other platforms do not have a similar containment (no root, but sudo etc is still possible), and that the containment isn't super strong. Can't we, to reduce the size of the behavioural change, just use AdjustTokenPrivileges() to re-add the privileges we want? Regards, Andres
On 4 Apr. 2017 14:22, "Andres Freund" <andres@anarazel.de> wrote:
On 2017-01-05 03:12:09 +0000, Tsunakawa, Takayuki wrote:Aren't like nearly all of them a concern? We gone from having some
> 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
containment (not being to create users, shut the system down, ...), to
none. I do however think there's a fair argument to be made that other
platforms do not have a similar containment (no root, but sudo etc is
still possible), and that the containment isn't super strong.
TBH, anyone who cares about security and runs Win7 or Win2k8 or newer should be using virtual service accounts and managed service accounts.
Those are more like Unix service accounts. Notably they don't need a password, getting rid of some of the management pain that led us to abandon the 'postgres' system user on Windows.
Now that older platforms are EoL and even the oldest that added this feature are also near EoL or in extended maintenance, I think installers should switch to these by default instead of using NETWORK SERVICE.
Then the issue of priv dropping would be a lesser concern anyway.
From: Craig Ringer [mailto:craig.ringer@2ndquadrant.com] > TBH, anyone who cares about security and runs Win7 or Win2k8 or newer should > be using virtual service accounts and managed service accounts. > > https://technet.microsoft.com/en-us/library/dd548356 > > > Those are more like Unix service accounts. Notably they don't need a password, > getting rid of some of the management pain that led us to abandon the > 'postgres' system user on Windows. > > Now that older platforms are EoL and even the oldest that added this feature > are also near EoL or in extended maintenance, I think installers should > switch to these by default instead of using NETWORK SERVICE. > > Then the issue of priv dropping would be a lesser concern anyway. Good point! And I said earlier in this thread, I think managing privileges (adding/revoking privileges from the user account)is the DBA's or sysadmin's duty, and PG's removing all privileges feels overkill. OTOH, I tried again to leave the DISABLE_MAX_PRIVILEGE as is and add Lock Pages in Memory, using the attached pg_ctl.c. Please see EnableLockPagesPrivilege() and its call site. But pg_ctl -w start fails emitting the following message: error code 1300 waiting for server to start....FATAL: could not enable "Lock pages in memory" privilege HINT: Assign "Lock pages in memory" privilege to the Windows user account which runs PostgreSQL. LOG: database system is shut down stopped waiting pg_ctl: could not start server Examine the log output. error code 1300 is ERROR_NOT_ALL_ASSIGNED, which means AdjustTokenPrivileges() cound not enable Lock Pages in Memory privilege. It seems that the privilege cannot be enabled once it was removed with CreateRestrictedToken(DISABLE_MAX_PRIVILEGE). Regards Takayuki Tsunakawa
Attachment
On 5 April 2017 at 10:37, Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> wrote: > Good point! And I said earlier in this thread, I think managing privileges (adding/revoking privileges from the user account)is the DBA's or sysadmin's duty, and PG's removing all privileges feels overkill. I think it's a sensible alternative to refusing to run as a highly privileged role, which is what we used to do IIRC. > OTOH, I tried again to leave the DISABLE_MAX_PRIVILEGE as is and add Lock Pages in Memory, using the attached pg_ctl.c. Please see EnableLockPagesPrivilege() and its call site. But pg_ctl -w start fails emitting the following message: That won't work. You'd have to pass 0 to the flags of CreateRestrictedToken and instead supply a PrivilegesToDelete array. You'd probably GetTokenInformation and AND with a mask of ones you wanted to retain. -- Craig Ringer http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Remote DBA, Training &Services
From: Craig Ringer [mailto:craig.ringer@2ndquadrant.com] > On 5 April 2017 at 10:37, Tsunakawa, Takayuki > <tsunakawa.takay@jp.fujitsu.com> wrote: > > OTOH, I tried again to leave the DISABLE_MAX_PRIVILEGE as is and add Lock > Pages in Memory, using the attached pg_ctl.c. Please see > EnableLockPagesPrivilege() and its call site. But pg_ctl -w start fails > emitting the following message: > > That won't work. You'd have to pass 0 to the flags of CreateRestrictedToken > and instead supply a PrivilegesToDelete array. > You'd probably GetTokenInformation and AND with a mask of ones you wanted > to retain. Uh, that's inconvenient. We can't determine what privileges to delete, and we must be aware of new privileges added in thefuture version of Windows. Then, I have to say the last patch (v12) is the final answer. Regards Takayuki Tsunakawa
On 2017-04-05 04:25:41 +0000, Tsunakawa, Takayuki wrote: > From: Craig Ringer [mailto:craig.ringer@2ndquadrant.com] > > On 5 April 2017 at 10:37, Tsunakawa, Takayuki > > <tsunakawa.takay@jp.fujitsu.com> wrote: > > > > OTOH, I tried again to leave the DISABLE_MAX_PRIVILEGE as is and add Lock > > Pages in Memory, using the attached pg_ctl.c. Please see > > EnableLockPagesPrivilege() and its call site. But pg_ctl -w start fails > > emitting the following message: > > > > That won't work. You'd have to pass 0 to the flags of CreateRestrictedToken > > and instead supply a PrivilegesToDelete array. > > You'd probably GetTokenInformation and AND with a mask of ones you wanted > > to retain. > > Uh, that's inconvenient. We can't determine what privileges to delete, and we must be aware of new privileges added inthe future version of Windows. > > Then, I have to say the last patch (v12) is the final answer. As I asked before, why can't we delete all privs and add the explicitly needed once back (using AdjustTokenPrivileges)? - Andres
From: pgsql-hackers-owner@postgresql.org > [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Andres Freund > As I asked before, why can't we delete all privs and add the explicitly > needed once back (using AdjustTokenPrivileges)? I tried it with pg_ctl.c attached to an earlier mail today, i.e. delete all privs with CreateRestrictedToken(DISABLE_ALL_PRIVILEGE)and enable Lock Pages in Memory with AdjustTokenPrivileges(). But it didn'twork; AdjustTokenPrivileges() failed to enable the priv. It's probably that CreateRestrictedToken() deletes (unassigns?)the privs from the access token, so subsequent AdjustTokenPrivileges() can no longer enable the priv. Regards Takayuki Tsunakawa
On Wed, Apr 5, 2017 at 9:15 AM, Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> wrote:
At least that's my guess from reading the docs and trying to remember :)
From: pgsql-hackers-owner@postgresql.org
> [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Andres Freund
> As I asked before, why can't we delete all privs and add the explicitly
> needed once back (using AdjustTokenPrivileges)?
I tried it with pg_ctl.c attached to an earlier mail today, i.e. delete all privs with CreateRestrictedToken(DISABLE_ALL_PRIVILEGE) and enable Lock Pages in Memory with AdjustTokenPrivileges(). But it didn't work; AdjustTokenPrivileges() failed to enable the priv. It's probably that CreateRestrictedToken() deletes (unassigns?) the privs from the access token, so subsequent AdjustTokenPrivileges() can no longer enable the priv.
Once you have used CreateRestrictedToken(), you can no longer add *anything* to it. It's not just removed privileges, there's a special flag on the token that says it's restricted (can be checked with IsTokenRestricted()).
I think what you'd need to do is enumerate what privileges the user has *before* calling CreateRestrictedToken(), using GetTokenInformation(). And then pass those into PrivilegesToDelete (except for SeChangeNotifyPrivilege) in the call to CreateRestrictedToken(), instead of using DISABLE_MAX_PRIVILEGE. (and add the privilege needed for huge pages before you start that whole process -- that needs to be added in the token used *before* we create the restricted one).
At least that's my guess from reading the docs and trying to remember :)
On 2017-04-07 13:57:07 +0200, Magnus Hagander wrote: > On Wed, Apr 5, 2017 at 9:15 AM, Tsunakawa, Takayuki < > tsunakawa.takay@jp.fujitsu.com> wrote: > > > From: pgsql-hackers-owner@postgresql.org > > > [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Andres Freund > > > As I asked before, why can't we delete all privs and add the explicitly > > > needed once back (using AdjustTokenPrivileges)? > > > > I tried it with pg_ctl.c attached to an earlier mail today, i.e. delete > > all privs with CreateRestrictedToken(DISABLE_ALL_PRIVILEGE) and enable > > Lock Pages in Memory with AdjustTokenPrivileges(). But it didn't work; > > AdjustTokenPrivileges() failed to enable the priv. It's probably that > > CreateRestrictedToken() deletes (unassigns?) the privs from the access > > token, so subsequent AdjustTokenPrivileges() can no longer enable the priv. > > > > > Once you have used CreateRestrictedToken(), you can no longer add > *anything* to it. It's not just removed privileges, there's a special flag > on the token that says it's restricted (can be checked with > IsTokenRestricted()). :/ > I think what you'd need to do is enumerate what privileges the user has > *before* calling CreateRestrictedToken(), using GetTokenInformation(). And > then pass those into PrivilegesToDelete (except for > SeChangeNotifyPrivilege) in the call to CreateRestrictedToken(), instead of > using DISABLE_MAX_PRIVILEGE. (and add the privilege needed for huge pages > before you start that whole process -- that needs to be added in the token > used *before* we create the restricted one). > > At least that's my guess from reading the docs and trying to remember :) Yea, seems that way. Therefore I propose returning this patch with feedback. Andres
Hello, Magnus cc: Andres From: pgsql-hackers-owner@postgresql.org > [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Magnus Hagander > I think what you'd need to do is enumerate what privileges the user has > *before* calling CreateRestrictedToken(), using GetTokenInformation(). > And then pass those into PrivilegesToDelete (except for > SeChangeNotifyPrivilege) in the call to CreateRestrictedToken(), instead > of using DISABLE_MAX_PRIVILEGE. (and add the privilege needed for huge pages > before you start that whole process -- that needs to be added in the token > used *before* we create the restricted one). > > At least that's my guess from reading the docs and trying to remember :) Oh, I got it now. Thanks. The revised patch is attached. The only modified file is pg_ctl.c. The patch worked as expected. It is regrettable that I could not make it in time for PG 10, but I'd appreciate it if you could review and commit this patchearly in PG 11 while our memory is fresh. Thank you for your patience. I'll create an entry in the next CF soon. 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
Attachment
On Wed, Apr 12, 2017 at 7:08 PM, Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> wrote: > Oh, I got it now. Thanks. The revised patch is attached. The only modified file is pg_ctl.c. The patch worked as expected. > > It is regrettable that I could not make it in time for PG 10, but I'd appreciate it if you could review and commit thispatch early in PG 11 while our memory is fresh. Thank you for your patience. I'll create an entry in the next CF soon. Hi Takayuki-san, win_large_pages_v13.patch seems to be the latest version posted, but it no longer applies. Could you please post a rebased version? Thanks! -- Thomas Munro http://www.enterprisedb.com
On Thu, Aug 17, 2017 at 2:11 PM, Thomas Munro <thomas.munro@enterprisedb.com> wrote: > On Wed, Apr 12, 2017 at 7:08 PM, Tsunakawa, Takayuki > <tsunakawa.takay@jp.fujitsu.com> wrote: >> Oh, I got it now. Thanks. The revised patch is attached. The only modified file is pg_ctl.c. The patch worked as expected. >> >> It is regrettable that I could not make it in time for PG 10, but I'd appreciate it if you could review and commit thispatch early in PG 11 while our memory is fresh. Thank you for your patience. I'll create an entry in the next CF soon. > > win_large_pages_v13.patch seems to be the latest version posted, but > it no longer applies. Could you please post a rebased version? Since it only conflicts with c7b8998e because of pgindent whitespace movement, I applied it with "patch -p1 --ignore-whitespace" and created a new patch. See attached. -- Thomas Munro http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
Hi Thomas, Magnus From: pgsql-hackers-owner@postgresql.org > [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Thomas Munro > Since it only conflicts with c7b8998e because of pgindent whitespace > movement, I applied it with "patch -p1 --ignore-whitespace" and created > a new patch. See attached. Thanks, Thomas. I've added your name in the CF entry so that your name will also be listed on the release note, becausemy patch is originally based on your initial try. Please remove your name just in case you mind it. BTW, your auto-reviewerlooks very convenient. Thank you again for your great work. Magnus, it would be grateful if you could review and commit the patch while your memory is relatively fresh. I've been in a situation which keeps me from doing development recently, but I think I can gradually rejoin the communityactivity soon. 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
On Wed, Sep 13, 2017 at 7:11 AM, Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> wrote: > Hi Thomas, Magnus > > From: pgsql-hackers-owner@postgresql.org >> [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Thomas Munro >> Since it only conflicts with c7b8998e because of pgindent whitespace >> movement, I applied it with "patch -p1 --ignore-whitespace" and created >> a new patch. See attached. > > Thanks, Thomas. I've added your name in the CF entry so that your name will also be listed on the release note, becausemy patch is originally based on your initial try. Please remove your name just in case you mind it. BTW, your auto-reviewerlooks very convenient. Thank you again for your great work. > > Magnus, it would be grateful if you could review and commit the patch while your memory is relatively fresh. > > I've been in a situation which keeps me from doing development recently, but I think I can gradually rejoin the communityactivity soon. > I have once again tested the latest patch (v14 patch) on Windows and the results looked fine to me. Basically I have repeated the test cases which I had done earlier on v8 patch. For more details, on the tests that i have re-executed, please refer to - [1]. Thanks. [1]- https://www.postgresql.org/message-id/CAE9k0Pkz%2BtOiPmx2LrVePM7cZydTLNbQ6R3GqgeivurfsXyZ5w%40mail.gmail.com -- With Regards, Ashutosh Sharma EnterpriseDB:http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Sep 13, 2017 at 3:41 AM, Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com> wrote:
Hi Thomas, Magnus
From: pgsql-hackers-owner@postgresql.org
> [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Thomas Munro
> Since it only conflicts with c7b8998e because of pgindent whitespace
> movement, I applied it with "patch -p1 --ignore-whitespace" and created
> a new patch. See attached.
Thanks, Thomas. I've added your name in the CF entry so that your name will also be listed on the release note, because my patch is originally based on your initial try. Please remove your name just in case you mind it. BTW, your auto-reviewer looks very convenient. Thank you again for your great work.
Magnus, it would be grateful if you could review and commit the patch while your memory is relatively fresh.
I've been in a situation which keeps me from doing development recently, but I think I can gradually rejoin the community activity soon.
Hi!
It's my plan to get to this patch during this commitfest. I've been travelling for open and some 24/7 work so far, but hope to get CFing soon.
From: pgsql-hackers-owner@postgresql.org > [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Ashutosh Sharma > I have once again tested the latest patch (v14 patch) on Windows and the > results looked fine to me. Basically I have repeated the test cases which > I had done earlier on v8 patch. For more details, on the tests that i have > re-executed, please refer to - [1]. Thanks. Thanks so much. I'm relieved to know that the patch still works. 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
On Thu, Sep 14, 2017 at 12:32 AM, Magnus Hagander <magnus@hagander.net> wrote: > It's my plan to get to this patch during this commitfest. I've been > travelling for open and some 24/7 work so far, but hope to get CFing soon. I hope Tsunakawa-san doesn't mind me posting another rebased version of his patch. The last version conflicted with the change from SGML to XML that just landed in commit 3c49c6fa. -- Thomas Munro http://www.enterprisedb.com
Attachment
From: Thomas Munro [mailto:thomas.munro@enterprisedb.com] > I hope Tsunakawa-san doesn't mind me posting another rebased version of > his patch. The last version conflicted with the change from SGML to XML > that just landed in commit 3c49c6fa. Thank you very much for keeping it fresh. I hope the committer could have some time. Regards Takayuki Tsunakawa
On Fri, Dec 1, 2017 at 5:02 AM, Michael Paquier <michael.paquier@gmail.com> wrote:
On Fri, Nov 24, 2017 at 9:28 AM, Tsunakawa, Takayuki
<tsunakawa.takay@jp.fujitsu.com> wrote:
> From: Thomas Munro [mailto:thomas.munro@enterprisedb.com]
>> I hope Tsunakawa-san doesn't mind me posting another rebased version of
>> his patch. The last version conflicted with the change from SGML to XML
>> that just landed in commit 3c49c6fa.
>
> Thank you very much for keeping it fresh. I hope the committer could have some time.
I have moved this patch to next CF. Magnus, you are registered as a
reviewer of this patch. Are you planning to look at it and potentially
commit it?
Apologies for the delays here. Yes, I was and am.
I took a look at this patch again. I've made some small updates to comments etc. There was also from what I can tell one actual bug in the code -- a missing free() of delPrivs.
The debug message for ERROR_NO_SYSTEM_RESOURCES said it turned off huge pages because of not enough huge pages, but AIUI it can be because of other resources as well. So I dropped that specific.
However, reading through a few things -- we now use the API GetLargePageMinimum() unconditionally. This API appeared in Windows Vista and Windows 2003. That means that if we apply this patch, those are now our minimum versions of Windows. At least unless Microsoft backpatched things.
This is probably equally true of some other things we do, but those are runtime, and we would just give an error on old platforms. If I'm thinking right, then direct linking to GetLargePageMinimum() will simply make it impossible to start a PostgreSQL backend with or without huge pages on previous versions, because it will give a linker error.
I wonder if we need to do something similar to what we already do for CreateRestrictedToken() in pg_ctl.c for example. That one actually is done for compatibility with NT4/2000 -- CreateRestrictedToken was added in XP. So while we could consider getting rid of that workaround now, perhaps we need to put a similar one in place for this?
I don't have a Windows build box around ATM, or a Windows XP, so if somebody could try the attached version, build a postgres and see if it even starts on a Windows XP machine, that would be useful input!
If we can confirm/deny the situation around XP and decide what to do about it, I am now happy to commit the rest of this patch.
Attachment
Sat, Jan 20, 2018 at 2:33 PM, Magnus Hagander <magnus@hagander.net> wrote:
On Fri, Dec 1, 2017 at 5:02 AM, Michael Paquier <michael.paquier@gmail.com> wrote:On Fri, Nov 24, 2017 at 9:28 AM, Tsunakawa, Takayuki
<tsunakawa.takay@jp.fujitsu.com> wrote:
> From: Thomas Munro [mailto:thomas.munro@enterprisedb.com]
>> I hope Tsunakawa-san doesn't mind me posting another rebased version of
>> his patch. The last version conflicted with the change from SGML to XML
>> that just landed in commit 3c49c6fa.
>
> Thank you very much for keeping it fresh. I hope the committer could have some time.
I have moved this patch to next CF. Magnus, you are registered as a
reviewer of this patch. Are you planning to look at it and potentially
commit it?Apologies for the delays here. Yes, I was and am.I took a look at this patch again. I've made some small updates to comments etc. There was also from what I can tell one actual bug in the code -- a missing free() of delPrivs.The debug message for ERROR_NO_SYSTEM_RESOURCES said it turned off huge pages because of not enough huge pages, but AIUI it can be because of other resources as well. So I dropped that specific.However, reading through a few things -- we now use the API GetLargePageMinimum() unconditionally. This API appeared in Windows Vista and Windows 2003. That means that if we apply this patch, those are now our minimum versions of Windows. At least unless Microsoft backpatched things.This is probably equally true of some other things we do, but those are runtime, and we would just give an error on old platforms. If I'm thinking right, then direct linking to GetLargePageMinimum() will simply make it impossible to start a PostgreSQL backend with or without huge pages on previous versions, because it will give a linker error.I wonder if we need to do something similar to what we already do for CreateRestrictedToken() in pg_ctl.c for example. That one actually is done for compatibility with NT4/2000 -- CreateRestrictedToken was added in XP. So while we could consider getting rid of that workaround now, perhaps we need to put a similar one in place for this?I don't have a Windows build box around ATM, or a Windows XP, so if somebody could try the attached version, build a postgres and see if it even starts on a Windows XP machine, that would be useful input!If we can confirm/deny the situation around XP and decide what to do about it, I am now happy to commit the rest of this patch.
To add some more notes on this. Again, the API appears in Vista/2003. Windows Vista went EOL (out of extended support even) in April 2017, Windows 2003 did so in July 2015. Those are the versions that it's *in* -- obviously the versions without it are even older.
Our binaries only support Windows 2008 and up (at least the ones by EDB, which are the ones that have a supported-version matrix documented on our site).
We have traditionally supported older versions of Windows as long as people build from source. But perhaps I'm way overreading that and we should just bite the bullet, commit this patch, and declare those platforms as completely dead by PostgreSQL 11?
On Sun, Jan 21, 2018 at 01:42:13PM +0200, Magnus Hagander wrote: > We have traditionally supported older versions of Windows as long as people > build from source. But perhaps I'm way overreading that and we should just > bite the bullet, commit this patch, and declare those platforms as > completely dead by PostgreSQL 11? Yeah, I'd like to think that this is a good idea for HEAD. Microsoft is pushing hard to have all its users move to newer versions like 10 anyway. -- Michael
Attachment
On Sun, Jan 21, 2018 at 2:30 PM, Michael Paquier <michael.paquier@gmail.com> wrote:
On Sun, Jan 21, 2018 at 01:42:13PM +0200, Magnus Hagander wrote:
> We have traditionally supported older versions of Windows as long as people
> build from source. But perhaps I'm way overreading that and we should just
> bite the bullet, commit this patch, and declare those platforms as
> completely dead by PostgreSQL 11?
Yeah, I'd like to think that this is a good idea for HEAD. Microsoft is
pushing hard to have all its users move to newer versions like 10
anyway.
I got myself a working build env now so I can at least verify it builds, which it does.
With that, I'm pushing this. Let's see what the buildfarm thinks of it. And if others end up complaining about the platform drop, but I doubt that.
Once again, apologies for the delay, and thanks for the patch!
Magnus Hagander <magnus@hagander.net> writes:
> With that, I'm pushing this. Let's see what the buildfarm thinks of it. And
> if others end up complaining about the platform drop, but I doubt that.
frogmouth:
pg_shmem.c: In function 'PGSharedMemoryCreate':
pg_shmem.c:205:3: warning: implicit declaration of function 'GetLargePageMinimum'
pg_shmem.c:222:38: error: 'SEC_LARGE_PAGES' undeclared (first use in this function)
pg_shmem.c:222:38: note: each undeclared identifier is reported only once for each function it appears in
make[3]: *** [pg_shmem.o] Error 1
so you were right to guess that this functionality isn't in XP.
I wonder whether this could be band-aided around by using "#ifdef
SEC_LARGE_PAGES" to protect the new code. I have no particular desire to
spend effort on supporting old Windows versions, but if there's someone
out there who does, they could be asked to look into that.
regards, tom lane
On Sun, Jan 21, 2018 at 6:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Magnus Hagander <magnus@hagander.net> writes:
> With that, I'm pushing this. Let's see what the buildfarm thinks of it. And
> if others end up complaining about the platform drop, but I doubt that.
frogmouth:
pg_shmem.c: In function 'PGSharedMemoryCreate':
pg_shmem.c:205:3: warning: implicit declaration of function 'GetLargePageMinimum'
pg_shmem.c:222:38: error: 'SEC_LARGE_PAGES' undeclared (first use in this function)
pg_shmem.c:222:38: note: each undeclared identifier is reported only once for each function it appears in
make[3]: *** [pg_shmem.o] Error 1
so you were right to guess that this functionality isn't in XP.
I wonder whether this could be band-aided around by using "#ifdef
SEC_LARGE_PAGES" to protect the new code. I have no particular desire to
spend effort on supporting old Windows versions, but if there's someone
out there who does, they could be asked to look into that.
I think that is not actually XP, in this case it's a case of the SDK being too old. Which *might* happen on mingw on a modern platform as well.
The question is do we care enough. Or do we just declare XP as unsupported, in which case frogmouth should stop building master.
I think the second one is OK, as long as it's only things that are as old as XP. But I think we have to wait for a modre modern mingw (jacana?) to complete before we can be sure.
Hi, On 2018-01-21 13:42:13 +0200, Magnus Hagander wrote: > To add some more notes on this. Again, the API appears in Vista/2003. > Windows Vista went EOL (out of extended support even) in April 2017, > Windows 2003 did so in July 2015. Those are the versions that it's *in* -- > obviously the versions without it are even older. > > Our binaries only support Windows 2008 and up (at least the ones by EDB, > which are the ones that have a supported-version matrix documented on our > site). > > We have traditionally supported older versions of Windows as long as people > build from source. But perhaps I'm way overreading that and we should just > bite the bullet, commit this patch, and declare those platforms as > completely dead by PostgreSQL 11? Yea, I think it's beyond time that we declare some old windows versions dead. There's enough weird behaviour in supported versions of windows (especially its socket API) that I really don't want to support more than necessary. And supporting versions that've been out of date for a while seems more than unnecessary. Greetings, Andres Freund
On 01/21/2018 01:02 PM, Andres Freund wrote: > Hi, > > On 2018-01-21 13:42:13 +0200, Magnus Hagander wrote: >> To add some more notes on this. Again, the API appears in Vista/2003. >> Windows Vista went EOL (out of extended support even) in April 2017, >> Windows 2003 did so in July 2015. Those are the versions that it's *in* -- >> obviously the versions without it are even older. >> >> Our binaries only support Windows 2008 and up (at least the ones by EDB, >> which are the ones that have a supported-version matrix documented on our >> site). >> >> We have traditionally supported older versions of Windows as long as people >> build from source. But perhaps I'm way overreading that and we should just >> bite the bullet, commit this patch, and declare those platforms as >> completely dead by PostgreSQL 11? > Yea, I think it's beyond time that we declare some old windows versions > dead. There's enough weird behaviour in supported versions of windows > (especially its socket API) that I really don't want to support more > than necessary. And supporting versions that've been out of date for a > while seems more than unnecessary. > I'll be quite happy to retire the XP machine running brolga, currawong and frogmouth, if that's the consensus. XP is now long out of support. OTOH I have personal experience of it running in many potentially critical situations, still (hospitals, for example). I can, if people want, keep the machine running just building the back branches. I should probably look at setting up a modern 32-bit replacement (say Windows 10 Pro-32). cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Jan 22, 2018 at 3:45 AM, Magnus Hagander <magnus@hagander.net> wrote: > I got myself a working build env now so I can at least verify it builds, > which it does. > > With that, I'm pushing this. Let's see what the buildfarm thinks of it. And > if others end up complaining about the platform drop, but I doubt that. > > Once again, apologies for the delay, and thanks for the patch! + To start the database server on the command prompt as a standalone process, + not as a Windows service, the command prompt must be run as an administrator + User Access Control (UAC) must be disabled. When the UAC is enabled, the normal + command prompt revokes the user right Lock Pages in Memory when started. Is that first sentence missing the word "and" after "administrator"? -- Thomas Munro http://www.enterprisedb.com
On Mon, Jan 22, 2018 at 12:13 AM, Andrew Dunstan <andrew.dunstan@2ndquadrant.com> wrote:
On 01/21/2018 01:02 PM, Andres Freund wrote:
> Hi,
>
> On 2018-01-21 13:42:13 +0200, Magnus Hagander wrote:
>> To add some more notes on this. Again, the API appears in Vista/2003.
>> Windows Vista went EOL (out of extended support even) in April 2017,
>> Windows 2003 did so in July 2015. Those are the versions that it's *in* --
>> obviously the versions without it are even older.
>>
>> Our binaries only support Windows 2008 and up (at least the ones by EDB,
>> which are the ones that have a supported-version matrix documented on our
>> site).
>>
>> We have traditionally supported older versions of Windows as long as people
>> build from source. But perhaps I'm way overreading that and we should just
>> bite the bullet, commit this patch, and declare those platforms as
>> completely dead by PostgreSQL 11?
> Yea, I think it's beyond time that we declare some old windows versions
> dead. There's enough weird behaviour in supported versions of windows
> (especially its socket API) that I really don't want to support more
> than necessary. And supporting versions that've been out of date for a
> while seems more than unnecessary.
>
I'll be quite happy to retire the XP machine running brolga, currawong
and frogmouth, if that's the consensus. XP is now long out of support.
OTOH I have personal experience of it running in many potentially
critical situations, still (hospitals, for example).
But do they really run PostgreSQL 11 (or 10..) on that? In my experience they usually run an old business application on it only. That is a problem in itself of course, but that is not our problem in this case :)
I can, if people
want, keep the machine running just building the back branches.
That's what I suggest we do. Removing the builds of back branches would be the equivalent of de-supporting it on a still supported branch, and I don't like that idea. But removing the master branch means we desupport in 11, which I think is the right thing to do.
I should probably look at setting up a modern 32-bit replacement (say
Windows 10 Pro-32).
Unless we want to desupport 32-bit Windows completely. But unless we have an actual reason to do so, I think we shouldn't. So yeah if you can get a box like that up and running, that'd be much welcome.
On Mon, Jan 22, 2018 at 4:24 AM, Thomas Munro <thomas.munro@enterprisedb.com> wrote:
On Mon, Jan 22, 2018 at 3:45 AM, Magnus Hagander <magnus@hagander.net> wrote:
> I got myself a working build env now so I can at least verify it builds,
> which it does.
>
> With that, I'm pushing this. Let's see what the buildfarm thinks of it. And
> if others end up complaining about the platform drop, but I doubt that.
>
> Once again, apologies for the delay, and thanks for the patch!
+ To start the database server on the command prompt as a
standalone process,
+ not as a Windows service, the command prompt must be run as
an administrator
+ User Access Control (UAC) must be disabled. When the UAC is
enabled, the normal
+ command prompt revokes the user right Lock Pages in Memory
when started.
Is that first sentence missing the word "and" after "administrator"?
Nope.
But it is missing the word "or".
Fixed, thanks for the spot!
On 01/22/2018 04:16 AM, Magnus Hagander wrote: > > > > > I'll be quite happy to retire the XP machine running brolga, currawong > and frogmouth, if that's the consensus. XP is now long out of support. > OTOH I have personal experience of it running in many potentially > critical situations, still (hospitals, for example). > > > But do they really run PostgreSQL 11 (or 10..) on that? In my > experience they usually run an old business application on it only. > That is a problem in itself of course, but that is not our problem in > this case :) > > > > I can, if people > want, keep the machine running just building the back branches. > > > That's what I suggest we do. Removing the builds of back branches > would be the equivalent of de-supporting it on a still supported > branch, and I don't like that idea. But removing the master branch > means we desupport in 11, which I think is the right thing to do. OK, I have left the machine running but these three animals will no longer build 11, only the back branches. > > > > > I should probably look at setting up a modern 32-bit replacement (say > Windows 10 Pro-32). > > > Unless we want to desupport 32-bit Windows completely. But unless we > have an actual reason to do so, I think we shouldn't. So yeah if you > can get a box like that up and running, that'd be much welcome. It's probably going to have to wait a couple of months, and at least a couple of weeks. It's worth noting that the last Windows Server edition that supported 342bit architectures was WS2008. That's quite old now. I wonder how long they will continue to support it in the consumer-grade Windows versions. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services