Re: Re: [HACKERS] Windows service is not starting sothere’s message in log: FATAL: "could not createshared memory segment “Global/PostgreSQL.851401618”: Permissiondenied” - Mailing list pgsql-hackers

Sorry, forgot to close the valid handle on return.



> > 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.
>
> It seems somewhat strange. Looking into the create operation in
> dms_impl_posix(), it should return false but not emit error when
> the shared memory object already exists.
>
> 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.
>
> On the other hand, in the case of GetLastError() ==
> ERROR_ACCESS_DENIED, which occurs between postmasters exexuted as
> service, it can reasonablly be assumed that the segment is
> already created by someone else. I think this is no problem
> because postgres processes don't need to access someone else's
> segments.
>
> Finally, the valid fix would be that make it return false if
> GetLastError() == ERROR_ALREADY_EXISTS or ERROR_ACCESS_DENIED.
>
> The patch attached will fix *both of* the problems.
>
>
> regards,
>
>
> At Fri, 16 Oct 2015 09:02:41 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in
<CAA4eK1+pjtDouF-Lc9y0UgFDmWHnaf4KwiM1Y3Anq0wZ1gwsAA@mail.gmail.com>
> > > 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.  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).

--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/storage/ipc/dsm_impl.c b/src/backend/storage/ipc/dsm_impl.c
index 921f029..8ef9898 100644
--- a/src/backend/storage/ipc/dsm_impl.c
+++ b/src/backend/storage/ipc/dsm_impl.c
@@ -671,6 +671,7 @@ dsm_impl_windows(dsm_op op, dsm_handle handle, Size request_size,
     {
         DWORD        size_high;
         DWORD        size_low;
+        DWORD        errcode;

         /* Shifts >= the width of the type are undefined. */
 #ifdef _WIN64
@@ -686,27 +687,31 @@ dsm_impl_windows(dsm_op op, dsm_handle handle, Size request_size,
                                  size_high,        /* Upper 32 bits of size */
                                  size_low,        /* Lower 32 bits of size */
                                  name);
+
+        /*
+         * If the mapping already exists, CreateFileMapping returns a valid
+         * handle but sets ERROR_ALREADY_EXISTS. Return false for the case.
+         * CreateFileMapping may return ERROR_ACCESS_DENIED for the cases when
+         * running as a service. It also tells that the mapping is already
+         * created by someone else, so return false.
+         */
+        errcode = GetLastError();
+        if (errcode == ERROR_ALREADY_EXISTS || errcode == ERROR_ACCESS_DENIED)
+        {
+            if (hmap)
+                CloseHandle(hmap);
+          return false;
+        }
+
         if (!hmap)
         {
-            _dosmaperr(GetLastError());
+            _dosmaperr(errcode);
             ereport(elevel,
                     (errcode_for_dynamic_shared_memory(),
                   errmsg("could not create shared memory segment \"%s\": %m",
                          name)));
             return false;
         }
-        _dosmaperr(GetLastError());
-        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;
-        }
     }
     else
     {