Re: Dynamic Shared Memory stuff

On Wed, Apr 9, 2014 at 7:41 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> Few Observations:
>
> 1. One new warning has been introduced in code.
> 1>src\backend\port\win32_shmem.c(295): warning C4013:
> 'dsm_set_control_handle' undefined; assuming extern returning int
> Attached patch fixes this warning.

OK, committed, after moving the header to the correct position in
alphabetical order.

> 2. On running test_shm_mq manually, the client side didn't get
> any problem (the results are as expected). However I am seeing
> below log on server:
> LOG:  registering background worker "test_shm_mq"
> LOG:  starting background worker process "test_shm_mq"
> LOG:  unrecognized win32 error code: 127
> LOG:  worker process: test_shm_mq (PID 2528) exited with exit code 1
> LOG:  unregistering background worker "test_shm_mq"
>
> I think below message in log is not expected:
> "LOG:  unrecognized win32 error code: 127"
>
> This has been started appearing after your commit related to DSM.
> I have debugged this issue and found that it comes from below part
> of code:
> dsm_impl_windows
> {
> ..
> else
> {
> hmap = OpenFileMapping(FILE_MAP_WRITE | FILE_MAP_READ,
>   FALSE, /* do not inherit the name */
>   name); /* name of mapping object */
> _dosmaperr(GetLastError());
> }
> }
>
> Here even though handle returned by OpenFileMapping() is a valid handle,
> but still GetLastError() return 127 (The specified procedure could not be
> found.).  Now the specs[1] of API says that if handle is non-NULL, then
> consider it success, so I am not sure whether we should bother about this
> error or not.  I have tried many ways (trying different parameters, search on
> net) to change this and related API's, but the same problem exists.  One
> strange thing is if I just call the API twice (I know this is not
> right, but just
> to experiment for finding some info), second time this error doesn't
> occur.
> The only difference in latest changes is that now the OpenFileMapping()
> is getting called by Child process rather than peer process (w.r.t process
> that has called CreateFileMapping), don't know why this should make
> such a difference.
>
> On net whatever examples I have seen for such usage, they call
> GetLastError() only if handle is invalid, we have called in above fashion
> just to keep code little bit simple.
>
> I am just not sure whether it is okay to rearrange the code and call
> GetLastError() only if returned handle is Invalid (NULL) or try to look
> for more info.

Well, I don't know either.  You wrote the Windows portion of this
code, so you'll have to decide what's best.  If the best practice in
this area is to only call GetLastError() if the handle isn't valid,
then that's probably what we should do, too.  But I can't speak to
what the best practice is.

> One question:
> 1. I have seen that initdb still creates pg_dynshmem, is it required
> after your latest changes?

It's only used now if dynamic_shared_memory_type = mmap.  I know
Andres was never a huge fan of the mmap implementation, so we could
rip that out and get rid of the directory, too, but I kind of liked
having it, and broke the tie in favor of myself.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company