Re: [HACKERS] Re: [HACKERS] Re: [HACKERS] Windows service is not starting so there’s message in log: FATAL: "could not create shared memory segment “Global/PostgreSQL.851401618”: Permission denied”
Robert Haas writes: > Yeah, random() is the wrong thing. It should use PostmasterRandom(). > Fixed to do that instead. I am not very happy about this patch; have you considered the security implications of what you just did? If you haven't, I'll tell you: you just made the postmaster's selection of "random" cancel keys and password salts a lot more predictable. Formerly, the srandom() seed for those depended on both the postmaster start time and the time of the first connection request, but this change removes the first connection request from the equation. If you know the postmaster start time --- which we will happily tell any asker --- it will not take too many trials to find the seed that's in use. I'd be the first to agree that this point is inadequately documented in the code, but PostmasterRandom should be reserved for its existing security-related uses, not exposed to the world for (ahem) random other uses. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [HACKERS] Re: [HACKERS] Windows service is not starting so there’s message in log: FATAL: "could not create shared memory segment “Global/PostgreSQL.851401618”: Permission denied”
On Thu, Oct 15, 2015 at 11:32 PM, Amit Kapila wrote: > On Thu, Oct 15, 2015 at 8:35 PM, Dmitry Vasilyev > wrote: >> >> I think that function dsm_impl_windows() with EACCES error should not >> do ereport() with FATAL level. It works, but it is likely to make an >> infinite loop if the user will receive EACCES error. >> > > Currently we are using error level as ERROR for creating dsm during > postmaster startup which is not right and rather we should use error > level as LOG. Can you please try with the attached patch and see > if the issue is fixed for you. > > Another some what related point is currently we are using random() > function to ensure a unique name for dsm and it seems to me that > it is always going to generate same number on first invocation (at least > thats what happening on windows) due to which you are seeing the > error. Yeah, random() is the wrong thing. It should use PostmasterRandom(). Fixed to do that instead. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [HACKERS] Re: [HACKERS] Windows service is not starting so there’s message in log: FATAL: "could not create shared memory segment “Global/PostgreSQL.851401618”: Permission denied”
Amit Kapila writes: > On Sat, Oct 17, 2015 at 12:07 AM, Robert Haas wrote: >> Maybe we need to be using PostmasterRandom() rather than random() for >> the control segment name. > +1. Though I think it is better to investigate the actual cause before > doing this. BackendRun() deliberately prevents that from working. And it also sets srandom() to a new value for each subprocess, so that AFAICS this idea would be a net negative. If you are seeing duplicate key values getting selected, the problem is elsewhere. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [HACKERS] Re: [HACKERS] Windows service is not starting so there’s message in log: FATAL: "could not create shared memory segment “Global/PostgreSQL.851401618”: Permission denied”
On Sat, Oct 17, 2015 at 12:07 AM, Robert Haas wrote: > > On Thu, Oct 15, 2015 at 11:32 PM, Amit Kapila wrote: > > Another some what related point is currently we are using random() > > function to ensure a unique name for dsm and it seems to me that > > it is always going to generate same number on first invocation (at least > > thats what happening on windows) due to which you are seeing the > > error. Another options could be to append current pid or data directory > > path as we are doing in win32_shmem.c. I think this could be an > > optimization which can be done in addition to the fix attached (we can > > do this as a separate patch as well, if we agreed to do anything). > > Maybe we need to be using PostmasterRandom() rather than random() for > the control segment name. > +1. Though I think it is better to investigate the actual cause before doing this. > But regardless, this patch isn't the right fix. > dsm_impl_op(DSM_OP_CREATE, ...) is supposed to return false in the > event of a segment-already-exists condition, without ereporting. > Thats right, but in this case, it seems from what is reported, that we are hitting Access Denied error which on retry seems to disappear (which ideally shouldn't happen). It's not clear to me why that is happening. By reading code, it appears that previously when we get such errors for main shared memory, we replaced the usage of 'Global\PostreSQL:..' to 'Global/PostgreSQL:.. (refer GetSharedMemName()), but in case of dsm we have already taken care of same, so not sure what else could be reason for Access Denied error. Dmitry, can we try to see what is the exact value of GetLastError() when it fails (one way is to check the logs at DEBUG5 level, _dosmaperr() will print that value or you can modify the code to see it.) Which Windows version you are using? With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
[HACKERS] Re: [HACKERS] Re: [HACKERS] Windows service is not starting so there’s message in log: FATAL: "could not create shared memory segment “Global/PostgreSQL.851401618”: Permission denied”
On Thu, Oct 15, 2015 at 11:32 PM, Amit Kapila wrote: > Another some what related point is currently we are using random() > function to ensure a unique name for dsm and it seems to me that > it is always going to generate same number on first invocation (at least > thats what happening on windows) due to which you are seeing the > error. Another options could be to append current pid or data directory > path as we are doing in win32_shmem.c. I think this could be an > optimization which can be done in addition to the fix attached (we can > do this as a separate patch as well, if we agreed to do anything). Maybe we need to be using PostmasterRandom() rather than random() for the control segment name. But regardless, this patch isn't the right fix. dsm_impl_op(DSM_OP_CREATE, ...) is supposed to return false in the event of a segment-already-exists condition, without ereporting. If it hits any OTHER error, then it should ereport(). In the Windows implementation, the code that caters to this is here: if (errno == EEXIST) { /* * On Windows, when the segment already exists, a handle for the * existing segment is returned. We must close it before * returning. We don't do _dosmaperr here, so errno won't be * modified. */ CloseHandle(hmap); return false; } Kyotaro Horiguchi's analysis seems to me to be going in about the right direction. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [HACKERS] Re: [HACKERS] Windows service is not starting so there’s message in log: FATAL: "could not create shared memory segment “Global/PostgreSQL.851401618”: Permission denied”
On Fri, Oct 16, 2015 at 12:16 PM, Kyotaro HORIGUCHI < horiguchi.kyot...@lab.ntt.co.jp> wrote: > This is wrong, current code does well for this case. I should > broke the code during investigating the problem. > > > > So, to make the windows version behave as the same, > > > dsm_impl_windows should return false if GetLastError() == > > > ERROR_ALREADY_EXISTS regardless of hmap is valid. The current > > > behavior is wrong because two or more postmaster *share* the same > > > DSM segment instead of having their own segments. > > > > The patch attached will fix *both of* the problems. > > So, the patch fixes only the "Permission denied" case. > Why do you think it is bad to display even log for "Permission denied" case? It seems all other implementations does the same and I think it is useful information and we should log it. Don't you think the patch sent by me is good enough to fix the reported issue? With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com